ERC-165 support for PermissionConditions

packages/contracts/CHANGELOG.md

@@ -9,15 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
+- Added `PermissionConditionBase` to have ERC-165 support for `IPermissionCondition` implementations.
 - Inherit `ProtocolVersion` and `ERC165` in `DAOFactory`.
 - Inherit `ProtocolVersion` in `DAO`.
 - Added a `nonReentrant` modifier to the `execute` function in the `DAO` contract.
 - Added `allowFailureMap` to `IDAO.Executed` event.
 
 ### Changed
 
-- Revert with an error if the `grantWithCondition` function in `PermissionManager` is called with a condition that is not a contract.
-- Revert with an error if the `applySingleTargetPermissions` function in `PermissionManager` is called with `PermissionLib.Operation.GrantWithCondition`.
+- Revert with errors (`ConditionNotAContract`, `ConditionInterfacNotSupported`) if the `grantWithCondition` function in `PermissionManager` is called with a condition address that is not a `IPermissionCondition` implementation.
+- Revert with an error (`GrantWithConditionNotSupported`) if the `applySingleTargetPermissions` function in `PermissionManager` is called with `PermissionLib.Operation.GrantWithCondition`.
 - Fixed logic bug in the `TokenVoting` and `AddresslistVoting` implementations that caused the `createProposal` function to emit the unvalidated `_startDate` and `_endDate` input arguments (that both can be zero) in the `ProposalCreated` event instead of the validated ones.
 - Changed the `createProposal` functions in `Multisig` to allow creating proposals when the `_msgSender()` is listed in the current block.
 - Changed the `createProposal` functions in `AddresslistVoting` to allow creating proposals when the `_msgSender()` is listed in the current block.

packages/contracts/docs/osx/01-how-it-works/01-core/02-permissions/01-conditions.md

@@ -5,7 +5,7 @@ title: Conditions
 ## Permission Conditions
 
 Permission conditions relay the decision if an authorized call is permitted to another contract.
-This contract must implement the `IPermissionCondition` interface.
+This contract must inherit from `PermissionConditionBase` and implement the `IPermissionCondition` interface.
 
 ```solidity title="@aragon/osx/core/permission/IPermissionCondition.sol"
 interface IPermissionCondition {
@@ -84,7 +84,7 @@ Let’s imagine that we want to make sure that `_amount` is not more than `1 ETH
 We can realize this requirement by deploying a `ParameterConstraintCondition` condition.
 
 ```solidity title="ParameterConstraintCondition.sol"
-contract ParameterConstraintCondition is IPermissionCondition {
+contract ParameterConstraintCondition is PermissionConditionBase {
 	uint256 internal maxValue;
 
 	constructor(uint256 _maxValue) {
@@ -112,7 +112,7 @@ Now, after granting the `SEND_COINS_PERMISSION_ID` permission to `_where` and `_
 In another use-case, we might want to make sure that the `sendCoins` can only be called after a certain date. This would look as following:
 
 ```solidity title="TimeCondition.sol"
-contract TimeCondition is IPermissionCondition {
+contract TimeCondition is PermissionConditionBase {
   uint256 internal date;
 
   constructor(uint256 _date) {
@@ -143,7 +143,7 @@ interface IProofOfHumanity {
   function isRegistered(address _submissionID) external view returns (bool);
 }
 
-contract ProofOfHumanityCondition is IPermissionCondition {
+contract ProofOfHumanityCondition is PermissionConditionBase {
   IProofOfHumanity internal registry;
 
   constructor(IProofOfHumanity _registry) {
@@ -173,7 +173,7 @@ In another use-case, we might want to make sure that the `sendCoins` function ca
 ```solidity title="PriceOracleCondition.sol"
 import '@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol';
 
-contract PriceOracleCondition is IPermissionCondition {
+contract PriceOracleCondition is PermissionConditionBase {
   AggregatorV3Interface internal priceFeed;
 
   // Network: Goerli

packages/contracts/docs/osx/01-how-it-works/01-core/02-permissions/index.md

@@ -97,14 +97,14 @@ Exceptions are, again, the [DAO creation](../../02-framework/01-dao-creation/ind
 
 #### Granting Permission with Conditions
 
-Aragon OSx supports relaying the authorization of a function call to another contract inheriting from the `IPermissionCondition` interface. This works by granting the permission with the `grantWithCondition` function
+Aragon OSx supports relaying the authorization of a function call to another contract inheriting from the `PermissionConditionBase` interface. This works by granting the permission with the `grantWithCondition` function
 
 ```solidity title="@aragon/osx/core/permission/PermissionManager.sol"
 function grantWithCondition(
   address _where,
   address _who,
   bytes32 _permissionId,
-  IPermissionCondition _condition
+  PermissionConditionBase _condition
 ) external auth(_where, ROOT_PERMISSION_ID) {}
 ```
 

packages/contracts/src/core/permission/PermissionConditionBase.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: AGPL-3.0-or-later
+
+pragma solidity ^0.8.8;
+
+import {ERC165Upgradeable} from "@openzeppelin/contracts-upgradeable/utils/introspection/ERC165Upgradeable.sol";
+
+import {IPermissionCondition} from "./IPermissionCondition.sol";
+
+/// @title PermissionConditionBase
+/// @author Aragon Association - 2021-2023
+/// @notice An abstract contract to inherit from and to be be implemented to support more customary permissions depending on on- or off-chain state, e.g., by querying token ownershop or a secondary condition, respectively.
+abstract contract PermissionConditionBase is ERC165Upgradeable, IPermissionCondition {
+    /// @notice Checks if an interface is supported by this or its parent contract.
+    /// @param _interfaceId The ID of the interface.
+    /// @return Returns `true` if the interface is supported.
+    function supportsInterface(bytes4 _interfaceId) public view override returns (bool) {
+        return
+            _interfaceId == type(IPermissionCondition).interfaceId ||
+            super.supportsInterface(_interfaceId);
+    }
+}

packages/contracts/src/core/permission/PermissionManager.sol

@@ -5,7 +5,7 @@ pragma solidity ^0.8.8;
 import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
 
-import "./IPermissionCondition.sol";
+import "./PermissionConditionBase.sol";
 import "./PermissionLib.sol";
 
 /// @title PermissionManager
@@ -50,9 +50,13 @@ abstract contract PermissionManager is Initializable {
         address newCondition
     );
 
-    /// @notice Thrown if an address is not a condition.
-    /// @param condition The address that is not a contract..
-    error ConditionInvalid(IPermissionCondition condition);
+    /// @notice Thrown if a condition address is not a contract.
+    /// @param condition The address that is not a contract.
+    error ConditionNotAContract(PermissionConditionBase condition);
+
+    /// @notice Thrown if a condition contract does not support the `IPermissionCondition` interface.
+    /// @param condition The address that is not a contract.
+    error ConditionInterfacNotSupported(PermissionConditionBase condition);
 
     /// @notice Thrown for `ROOT_PERMISSION_ID` or `EXECUTE_PERMISSION_ID` permission grants where `who` or `where` is `ANY_ADDR`.
 
@@ -69,13 +73,13 @@ abstract contract PermissionManager is Initializable {
     /// @param here The address of the context in which the permission is granted.
     /// @param where The address of the target contract for which `_who` receives permission.
     /// @param who The address (EOA or contract) receiving the permission.
-    /// @param condition The address `ALLOW_FLAG` for regular permissions or, alternatively, the `PermissionCondition` to be used.
+    /// @param condition The address `ALLOW_FLAG` for regular permissions or, alternatively, the `PermissionConditionBase` contract implementation to be used.
     event Granted(
         bytes32 indexed permissionId,
         address indexed here,
         address where,
         address indexed who,
-        IPermissionCondition condition
+        address condition
     );
 
     /// @notice Emitted when a permission `permission` is revoked in the context `here` from the address `_who` for the contract `_where`.
@@ -129,7 +133,7 @@ abstract contract PermissionManager is Initializable {
         address _where,
         address _who,
         bytes32 _permissionId,
-        IPermissionCondition _condition
+        PermissionConditionBase _condition
     ) external virtual auth(ROOT_PERMISSION_ID) {
         _grantWithCondition(_where, _who, _permissionId, _condition);
     }
@@ -189,7 +193,7 @@ abstract contract PermissionManager is Initializable {
                     item.where,
                     item.who,
                     item.permissionId,
-                    IPermissionCondition(item.condition)
+                    PermissionConditionBase(item.condition)
                 );
             }
 
@@ -241,7 +245,7 @@ abstract contract PermissionManager is Initializable {
         if (currentFlag == UNSET_FLAG) {
             permissionsHashed[permHash] = ALLOW_FLAG;
 
-            emit Granted(_permissionId, msg.sender, _where, _who, IPermissionCondition(ALLOW_FLAG));
+            emit Granted(_permissionId, msg.sender, _where, _who, ALLOW_FLAG);
         }
     }
 
@@ -255,10 +259,14 @@ abstract contract PermissionManager is Initializable {
         address _where,
         address _who,
         bytes32 _permissionId,
-        IPermissionCondition _condition
+        PermissionConditionBase _condition
     ) internal virtual {
         if (!address(_condition).isContract()) {
-            revert ConditionInvalid(_condition);
+            revert ConditionNotAContract(_condition);
+        }
+
+        if (!_condition.supportsInterface(type(IPermissionCondition).interfaceId)) {
+            revert ConditionInterfacNotSupported(_condition);
         }
 
         if (_where == ANY_ADDR && _who == ANY_ADDR) {
@@ -283,7 +291,7 @@ abstract contract PermissionManager is Initializable {
         if (currentCondition == UNSET_FLAG) {
             permissionsHashed[permHash] = newCondition;
 
-            emit Granted(_permissionId, msg.sender, _where, _who, _condition);
+            emit Granted(_permissionId, msg.sender, _where, _who, newCondition);
         } else if (currentCondition != newCondition) {
             // Revert if `permHash` is already granted, but uses a different condition.
             // If we don't revert, we either should:
@@ -335,7 +343,7 @@ abstract contract PermissionManager is Initializable {
 
         // Since it's not a flag, assume it's a PermissionCondition and try-catch to skip failures
         try
-            IPermissionCondition(accessFlagOrCondition).isGranted(
+            PermissionConditionBase(accessFlagOrCondition).isGranted(
                 _where,
                 _who,
                 _permissionId,

packages/contracts/src/test/permission/ParameterScopingPermissionConditionTest.sol

@@ -2,10 +2,10 @@
 
 pragma solidity 0.8.17;
 
-import {IPermissionCondition} from "../../core/permission/IPermissionCondition.sol";
+import {PermissionConditionBase} from "../../core/permission/PermissionConditionBase.sol";
 import {TestPlugin} from "../plugin/PluginTest.sol";
 
-contract TestParameterScopingPermissionCondition is IPermissionCondition {
+contract TestParameterScopingPermissionCondition is PermissionConditionBase {
     bytes4 public constant ADD_PERMISSIONED_SELECTOR = TestPlugin.addPermissioned.selector;
 
     function getSelector(bytes memory _data) public pure returns (bytes4 sig) {

packages/contracts/src/test/permission/PermissionConditionMock.sol

@@ -2,9 +2,9 @@
 
 pragma solidity 0.8.17;
 
-import "../../core/permission/IPermissionCondition.sol";
+import "../../core/permission/PermissionConditionBase.sol";
 
-contract PermissionConditionMock is IPermissionCondition {
+contract PermissionConditionMock is PermissionConditionBase {
     bool internal _hasPermissionsResult = true;
 
     function isGranted(

packages/contracts/src/test/plugin/SharedPluginTest.sol

@@ -4,13 +4,13 @@ pragma solidity 0.8.17;
 
 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 
-import {IPermissionCondition} from "../../core/permission/IPermissionCondition.sol";
+import {PermissionConditionBase} from "../../core/permission/PermissionConditionBase.sol";
 import {PluginUUPSUpgradeable} from "../../core/plugin/PluginUUPSUpgradeable.sol";
 import {DaoUnauthorized} from "../../core/utils/auth.sol";
 import {IDAO} from "../../core/dao/IDAO.sol";
 
 /// @notice A test Plugin that manages permission to internal objects by associating their IDs with specific DAOs. Only the DAO for which the object was created has the permission to perform ID-gated actions on them.
-/// @dev This is realized by asking a `IPermissionCondition` that must be authorized in the DAO's permission manager.
+/// @dev This is realized by asking a `PermissionConditionBase` that must be authorized in the DAO's permission manager.
 contract TestSharedPlugin is PluginUUPSUpgradeable {
     bytes32 public constant ID_GATED_ACTION_PERMISSION_ID = keccak256("ID_GATED_ACTION_PERMISSION");
 
@@ -51,15 +51,15 @@ contract TestSharedPlugin is PluginUUPSUpgradeable {
     }
 
     /// @notice Executes something if the `id` parameter is authorized by the DAO associated through `ownedIds`.
-    ///         This is done by asking a `IPermissionCondition` that must be authorized in the DAO's permission manager via `grantWithCondition` and the `ID_GATED_ACTION_PERMISSION_ID`.
+    ///         This is done by asking a `PermissionConditionBase` that must be authorized in the DAO's permission manager via `grantWithCondition` and the `ID_GATED_ACTION_PERMISSION_ID`.
     /// @param _id The ID that is associated with a specific DAO
     function idGatedAction(uint256 _id) external sharedAuth(_id, ID_GATED_ACTION_PERMISSION_ID) {
         // do something
     }
 }
 
 /// @notice The condition associated with `TestSharedPlugin`
-contract TestIdGatingCondition is IPermissionCondition {
+contract TestIdGatingCondition is PermissionConditionBase {
     uint256 public allowedId;
 
     constructor(uint256 _id) {

packages/contracts/test/core/permission/permission-manager.ts

@@ -7,6 +7,7 @@ import {
   PermissionConditionMock,
   PermissionManagerTest__factory,
   PermissionConditionMock__factory,
+  TestPlugin__factory,
 } from '../../../typechain';
 import {DeployTestPermissionCondition} from '../../test-utils/conditions';
 import {OZ_ERRORS} from '../../test-utils/error';
@@ -182,10 +183,27 @@ describe('Core: PermissionManager', function () {
           ethers.constants.AddressZero
         )
       )
-        .to.be.revertedWithCustomError(pm, 'ConditionInvalid')
+        .to.be.revertedWithCustomError(pm, 'ConditionNotAContract')
         .withArgs(ethers.constants.AddressZero);
     });
 
+    it('reverts if the condition contract does not support `IPermissionConditon`', async () => {
+      const nonConditionContract = await new TestPlugin__factory(
+        signers[0]
+      ).deploy();
+
+      await expect(
+        pm.grantWithCondition(
+          pm.address,
+          otherSigner.address,
+          ADMIN_PERMISSION_ID,
+          nonConditionContract.address
+        )
+      )
+        .to.be.revertedWithCustomError(pm, 'ConditionInterfacNotSupported')
+        .withArgs(nonConditionContract.address);
+    });
+
     it('should add permission', async () => {
       await pm.grantWithCondition(
         pm.address,