feature: stricter permission fallback mechanism for isGranted (#461)

* refactor: dao signature validation * chore: maintained changelog * test: test default behaviour when no permission is granted * refactor: removed unused ERC1271Mock contract * chore: maintained changelog * chore: maintained changelog * refactor: deprecation of variables and functions * chore: bump protocol version to v1.4.0 * chore: maintained changelog * refactor: use removed instead of depreacted * chore: bump package versions * fix: wrong error name * fix: wrong version number in changelog * test: fixed broken test * revert: do not export flags that are not needed in other files currently * Apply suggestions from code review Co-authored-by: Mathias Scherer <[email protected]> * chore: maintain changelog * refactor: improved confusing and broken tests * docs: minor comment improvement * refactor: rename removed variable * docs: improve comment * test: check for correct value instead non-reverting call * test: move factory creation into beforeEach * test: a specific and a generic condition are granted at the same time * refactor: permission manager refactor: simplifications * style: naming and comments * test: added and refactored tests of isGranted * docs: improve NatSpec * chore: maintained changelog * docs: updated docs * docs: improved condition order explanation * docs: improved natspec * refactor: naming, comments, and bracketing * docs: improved comment * fix: adapt test to the new behaviour * docs: improved comment * docs: apply suggestions from code review Co-authored-by: Mathias Scherer <[email protected]> * typo: removed redundant comment signs --------- Co-authored-by: Mathias Scherer <[email protected]>
aragon · Sep 21, 2023 · c359ed0 · c359ed0
1 parent 2a10844
commit c359ed0
Show file tree

Hide file tree

Showing 9 changed files with 445 additions and 99 deletions.
diff --git a/packages/contracts/CHANGELOG.md b/packages/contracts/CHANGELOG.md
@@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 
+- Refactored the fallback in the `isGranted` function in `PermissionManager` to make conditions mutually exclusive: Specific conditions answering `false` do not fall back to generic caller conditions (`_who: ANY_ADDR`) or generic target conditions (`_where: ANY_ADDR`).
 - Renamed the `signatureValidator` variable in `DAO` to `__removed0`.
 - Use the DAOs permission manager functionality to validate signatures.
 

diff --git a/packages/contracts/docs/osx/01-how-it-works/01-core/02-permissions/index.md b/packages/contracts/docs/osx/01-how-it-works/01-core/02-permissions/index.md
@@ -133,6 +133,11 @@ Granting a permission with `_where: ANY_ADDR` to a condition has the effect that
 Imagine, for example, that many instances of the `Service` contract exist, and a user should have the permission to use all of them. By granting the `USE_PERMISSION_ID` with `_where: ANY_ADDR`, to some user `_who: userAddr`, the user has access to all of them. If this should not be possible anymore, you can later revoke the permission.
 
 However, some restrictions apply. For security reasons, Aragon OSx does not allow you to use both, `_where: ANY_ADDR` and `_who: ANY_ADDR` in the same permission. Furthermore, the permission IDs of [permissions native to the `DAO` Contract](#permissions-native-to-the-dao-contract) cannot be used.
+Moreover, if a condition is set, we return its `isGranted` result and do not fall back to a more generic one. The condition checks occur in the following order
+
+1. Condition with specific `_who` and specific `where`.
+2. Condition with generic `_who: ANY_ADDR` and specific `_where`.
+3. Condition with specific `_where` and generic `_who: ANY_ADDR`.
 
 ### Permissions Native to the `DAO` Contract
 

diff --git a/packages/contracts/src/core/dao/DAO.sol b/packages/contracts/src/core/dao/DAO.sol
@@ -233,7 +233,7 @@ contract DAO is
         bytes32 _permissionId,
         bytes memory _data
     ) external view override returns (bool) {
-        return isGranted(_where, _who, _permissionId, _data);
+        return isGranted({_where: _where, _who: _who, _permissionId: _permissionId, _data: _data});
     }
 
     /// @inheritdoc IDAO

diff --git a/packages/contracts/src/core/dao/IDAO.sol b/packages/contracts/src/core/dao/IDAO.sol
@@ -116,7 +116,7 @@ interface IDAO {
     /// @notice Checks whether a signature is valid for a provided hash according to [ERC-1271](https://eips.ethereum.org/EIPS/eip-1271).
     /// @param _hash The hash of the data to be signed.
     /// @param _signature The signature byte array associated with `_hash`.
-    /// @return Returns the `bytes4` magic value `0x1626ba7e` if the signature is valid.
+    /// @return Returns the `bytes4` magic value `0x1626ba7e` if the signature is valid and `0xffffffff` if not.
     function isValidSignature(bytes32 _hash, bytes memory _signature) external returns (bytes4);
 
     /// @notice Registers an ERC standard having a callback by registering its [ERC-165](https://eips.ethereum.org/EIPS/eip-165) interface ID and callback function signature.

diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol
@@ -106,7 +106,7 @@ abstract contract PermissionManager is Initializable {
     /// @dev The initial owner is granted the `ROOT_PERMISSION_ID` permission.
     /// @param _initialOwner The initial owner of the permission manager.
     function __PermissionManager_init(address _initialOwner) internal onlyInitializing {
-        _initializePermissionManager(_initialOwner);
+        _initializePermissionManager({_initialOwner: _initialOwner});
     }
 
     /// @notice Grants permission to an address to call methods in a contract guarded by an auth modifier with the specified permission identifier.
@@ -120,7 +120,7 @@ abstract contract PermissionManager is Initializable {
         address _who,
         bytes32 _permissionId
     ) external virtual auth(ROOT_PERMISSION_ID) {
-        _grant(_where, _who, _permissionId);
+        _grant({_where: _where, _who: _who, _permissionId: _permissionId});
     }
 
     /// @notice Grants permission to an address to call methods in a target contract guarded by an auth modifier with the specified permission identifier if the referenced condition permits it.
@@ -136,7 +136,12 @@ abstract contract PermissionManager is Initializable {
         bytes32 _permissionId,
         IPermissionCondition _condition
     ) external virtual auth(ROOT_PERMISSION_ID) {
-        _grantWithCondition(_where, _who, _permissionId, _condition);
+        _grantWithCondition({
+            _where: _where,
+            _who: _who,
+            _permissionId: _permissionId,
+            _condition: _condition
+        });
     }
 
     /// @notice Revokes permission from an address to call methods in a target contract guarded by an auth modifier with the specified permission identifier.
@@ -150,7 +155,7 @@ abstract contract PermissionManager is Initializable {
         address _who,
         bytes32 _permissionId
     ) external virtual auth(ROOT_PERMISSION_ID) {
-        _revoke(_where, _who, _permissionId);
+        _revoke({_where: _where, _who: _who, _permissionId: _permissionId});
     }
 
     /// @notice Applies an array of permission operations on a single target contracts `_where`.
@@ -164,9 +169,9 @@ abstract contract PermissionManager is Initializable {
             PermissionLib.SingleTargetPermission memory item = items[i];
 
             if (item.operation == PermissionLib.Operation.Grant) {
-                _grant(_where, item.who, item.permissionId);
+                _grant({_where: _where, _who: item.who, _permissionId: item.permissionId});
             } else if (item.operation == PermissionLib.Operation.Revoke) {
-                _revoke(_where, item.who, item.permissionId);
+                _revoke({_where: _where, _who: item.who, _permissionId: item.permissionId});
             } else if (item.operation == PermissionLib.Operation.GrantWithCondition) {
                 revert GrantWithConditionNotSupported();
             }
@@ -186,16 +191,16 @@ abstract contract PermissionManager is Initializable {
             PermissionLib.MultiTargetPermission memory item = _items[i];
 
             if (item.operation == PermissionLib.Operation.Grant) {
-                _grant(item.where, item.who, item.permissionId);
+                _grant({_where: item.where, _who: item.who, _permissionId: item.permissionId});
             } else if (item.operation == PermissionLib.Operation.Revoke) {
-                _revoke(item.where, item.who, item.permissionId);
+                _revoke({_where: item.where, _who: item.who, _permissionId: item.permissionId});
             } else if (item.operation == PermissionLib.Operation.GrantWithCondition) {
-                _grantWithCondition(
-                    item.where,
-                    item.who,
-                    item.permissionId,
-                    IPermissionCondition(item.condition)
-                );
+                _grantWithCondition({
+                    _where: item.where,
+                    _who: item.who,
+                    _permissionId: item.permissionId,
+                    _condition: IPermissionCondition(item.condition)
+                });
             }
 
             unchecked {
@@ -204,28 +209,125 @@ abstract contract PermissionManager is Initializable {
         }
     }
 
-    /// @notice Checks if an address has permission on a contract via a permission identifier and considers if `ANY_ADDRESS` was used in the granting process.
+    /// @notice Checks if the caller address has permission on the target contract via a permission identifier and relays the answer to a condition contract if this was declared during the granting process.
     /// @param _where The address of the target contract for which `_who` receives permission.
     /// @param _who The address (EOA or contract) for which the permission is checked.
     /// @param _permissionId The permission identifier.
-    /// @param _data The optional data passed to the `PermissionCondition` registered.
+    /// @param _data Optional data to be passed to the set `PermissionCondition`.
     /// @return Returns true if `_who` has the permissions on the target contract via the specified permission identifier.
     function isGranted(
         address _where,
         address _who,
         bytes32 _permissionId,
         bytes memory _data
     ) public view virtual returns (bool) {
-        return
-            _isGranted(_where, _who, _permissionId, _data) || // check if `_who` has permission for `_permissionId` on `_where`
-            _isGranted(_where, ANY_ADDR, _permissionId, _data) || // check if anyone has permission for `_permissionId` on `_where`
-            _isGranted(ANY_ADDR, _who, _permissionId, _data); // check if `_who` has permission for `_permissionI` on any contract
+        // Specific caller (`_who`) and target (`_where`) permission check
+        {
+            // This permission may have been granted directly via the `grant` function or with a condition via the `grantWithCondition` function.
+            address specificCallerTargetPermission = permissionsHashed[
+                permissionHash({_where: _where, _who: _who, _permissionId: _permissionId})
+            ];
+
+            // If the permission was granted directly, return `true`.
+            if (specificCallerTargetPermission == ALLOW_FLAG) return true;
+
+            // If the permission was granted with a condition, check the condition and return the result.
+            if (specificCallerTargetPermission != UNSET_FLAG) {
+                return
+                    _checkCondition({
+                        _condition: specificCallerTargetPermission,
+                        _where: _where,
+                        _who: _who,
+                        _permissionId: _permissionId,
+                        _data: _data
+                    });
+            }
+
+            // If this permission is not set, continue.
+        }
+
+        // Generic caller (`_who: ANY_ADDR`) condition check
+        {
+            // This permission can only be granted in conjunction with a condition via the `grantWithCondition` function.
+            address genericCallerPermission = permissionsHashed[
+                permissionHash({_where: _where, _who: ANY_ADDR, _permissionId: _permissionId})
+            ];
+
+            // If the permission was granted with a condition, check the condition and return the result.
+            if (genericCallerPermission != UNSET_FLAG) {
+                return
+                    _checkCondition({
+                        _condition: genericCallerPermission,
+                        _where: _where,
+                        _who: _who,
+                        _permissionId: _permissionId,
+                        _data: _data
+                    });
+            }
+            // If this permission is not set, continue.
+        }
+
+        // Generic target (`_where: ANY_ADDR`) condition check
+        {
+            // This permission can only be granted in conjunction with a condition via the `grantWithCondition` function.
+            address genericTargetPermission = permissionsHashed[
+                permissionHash({_where: ANY_ADDR, _who: _who, _permissionId: _permissionId})
+            ];
+
+            // If the permission was granted with a condition, check the condition and return the result.
+            if (genericTargetPermission != UNSET_FLAG) {
+                return
+                    _checkCondition({
+                        _condition: genericTargetPermission,
+                        _where: _where,
+                        _who: _who,
+                        _permissionId: _permissionId,
+                        _data: _data
+                    });
+            }
+            // If this permission is not set, continue.
+        }
+
+        // No specific or generic permission applies to the `_who`, `_where`, `_permissionId`, so we return `false`.
+        return false;
+    }
+
+    /// @notice Relays the question if caller address has permission on target contract via a permission identifier to a condition contract.
+    /// @notice Checks a condition contract by doing an external call via try/catch.
+    /// @param _condition The condition contract that is called.
+    /// @param _where The address of the target contract for which `_who` receives permission.
+    /// @param _who The address (EOA or contract) owning the permission.
+    /// @param _permissionId The permission identifier.
+    /// @param _data Optional data to be passed to a referenced `PermissionCondition`.
+    /// @return Returns `true` if a caller (`_who`) has the permissions on the contract (`_where`) via the specified permission identifier.
+    /// @dev If the external call fails, we return `false`.
+    function _checkCondition(
+        address _condition,
+        address _where,
+        address _who,
+        bytes32 _permissionId,
+        bytes memory _data
+    ) internal view virtual returns (bool) {
+        // Try-catch to skip failures
+        try
+            IPermissionCondition(_condition).isGranted({
+                _where: _where,
+                _who: _who,
+                _permissionId: _permissionId,
+                _data: _data
+            })
+        returns (bool result) {
+            if (result) {
+                return true;
+            }
+        } catch {}
+        return false;
     }
 
     /// @notice Grants the `ROOT_PERMISSION_ID` permission to the initial owner during initialization of the permission manager.
     /// @param _initialOwner The initial owner of the permission manager.
     function _initializePermissionManager(address _initialOwner) internal {
-        _grant(address(this), _initialOwner, ROOT_PERMISSION_ID);
+        _grant({_where: address(this), _who: _initialOwner, _permissionId: ROOT_PERMISSION_ID});
     }
 
     /// @notice This method is used in the external `grant` method of the permission manager.
@@ -238,15 +340,25 @@ abstract contract PermissionManager is Initializable {
             revert PermissionsForAnyAddressDisallowed();
         }
 
-        bytes32 permHash = permissionHash(_where, _who, _permissionId);
+        bytes32 permHash = permissionHash({
+            _where: _where,
+            _who: _who,
+            _permissionId: _permissionId
+        });
 
         address currentFlag = permissionsHashed[permHash];
 
         // Means permHash is not currently set.
         if (currentFlag == UNSET_FLAG) {
             permissionsHashed[permHash] = ALLOW_FLAG;
 
-            emit Granted(_permissionId, msg.sender, _where, _who, ALLOW_FLAG);
+            emit Granted({
+                permissionId: _permissionId,
+                here: msg.sender,
+                where: _where,
+                who: _who,
+                condition: ALLOW_FLAG
+            });
         }
     }
 
@@ -289,15 +401,25 @@ abstract contract PermissionManager is Initializable {
             }
         }
 
-        bytes32 permHash = permissionHash(_where, _who, _permissionId);
+        bytes32 permHash = permissionHash({
+            _where: _where,
+            _who: _who,
+            _permissionId: _permissionId
+        });
 
         address currentCondition = permissionsHashed[permHash];
 
         // Means permHash is not currently set.
         if (currentCondition == UNSET_FLAG) {
             permissionsHashed[permHash] = conditionAddr;
 
-            emit Granted(_permissionId, msg.sender, _where, _who, conditionAddr);
+            emit Granted({
+                permissionId: _permissionId,
+                here: msg.sender,
+                where: _where,
+                who: _who,
+                condition: conditionAddr
+            });
         } else if (currentCondition != conditionAddr) {
             // Revert if `permHash` is already granted, but uses a different condition.
             // If we don't revert, we either should:
@@ -320,48 +442,18 @@ abstract contract PermissionManager is Initializable {
     /// @param _permissionId The permission identifier.
     /// @dev Note, that revoking permissions with `_who` or `_where` equal to `ANY_ADDR` does not revoke other permissions with specific `_who` and `_where` addresses that might have been granted in parallel.
     function _revoke(address _where, address _who, bytes32 _permissionId) internal virtual {
-        bytes32 permHash = permissionHash(_where, _who, _permissionId);
+        bytes32 permHash = permissionHash({
+            _where: _where,
+            _who: _who,
+            _permissionId: _permissionId
+        });
         if (permissionsHashed[permHash] != UNSET_FLAG) {
             permissionsHashed[permHash] = UNSET_FLAG;
 
-            emit Revoked(_permissionId, msg.sender, _where, _who);
+            emit Revoked({permissionId: _permissionId, here: msg.sender, where: _where, who: _who});
         }
     }
 
-    /// @notice Checks if a caller is granted permissions on a target contract via a permission identifier and redirects the approval to a `PermissionCondition` if this was specified in the setup.
-    /// @param _where The address of the target contract for which `_who` receives permission.
-    /// @param _who The address (EOA or contract) owning the permission.
-    /// @param _permissionId The permission identifier.
-    /// @param _data The optional data passed to the `PermissionCondition` registered.
-    /// @return Returns true if `_who` has the permissions on the contract via the specified permissionId identifier.
-    function _isGranted(
-        address _where,
-        address _who,
-        bytes32 _permissionId,
-        bytes memory _data
-    ) internal view virtual returns (bool) {
-        address accessFlagOrCondition = permissionsHashed[
-            permissionHash(_where, _who, _permissionId)
-        ];
-
-        if (accessFlagOrCondition == UNSET_FLAG) return false;
-        if (accessFlagOrCondition == ALLOW_FLAG) return true;
-
-        // Since it's not a flag, assume it's a PermissionCondition and try-catch to skip failures
-        try
-            IPermissionCondition(accessFlagOrCondition).isGranted(
-                _where,
-                _who,
-                _permissionId,
-                _data
-            )
-        returns (bool allowed) {
-            if (allowed) return true;
-        } catch {}
-
-        return false;
-    }
-
     /// @notice A private function to be used to check permissions on the permission manager contract (`address(this)`) itself.
     /// @param _permissionId The permission identifier required to call the method this modifier is applied to.
     function _auth(bytes32 _permissionId) internal view virtual {

diff --git a/packages/contracts/src/test/permission/PermissionManagerTest.sol b/packages/contracts/src/test/permission/PermissionManagerTest.sol
@@ -41,7 +41,7 @@ contract PermissionManagerTest is PermissionManager {
         bytes32 _permissionId,
         bytes memory _data
     ) public view returns (bool) {
-        return _isGranted(_where, _who, _permissionId, _data);
+        return isGranted(_where, _who, _permissionId, _data);
     }
 
     function isPermissionRestrictedForAnyAddr(

diff --git a/packages/contracts/test/core/dao/dao.ts b/packages/contracts/test/core/dao/dao.ts
@@ -1388,11 +1388,11 @@ describe('DAO', function () {
           ).to.equal(VALID_ERC1271_SIGNATURE);
         });
 
-        it('returns valid if only the generic condition is met', async () => {
+        it('returns invalid if the specific condition is not met although the generic condition is met (no fallback)', async () => {
           await specificMockCondition.setAnswer(false);
           expect(
             await dao.connect(caller).isValidSignature(hash, signature)
-          ).to.equal(VALID_ERC1271_SIGNATURE);
+          ).to.equal(INVALID_ERC1271_SIGNATURE);
         });
 
         it('returns invalid if both conditions are not met', async () => {