diff --git a/packages/contracts/CHANGELOG.md b/packages/contracts/CHANGELOG.md index 4fe77298d..7183d9691 100644 --- 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 index 2090809fc..96ca1d1ae 100644 --- 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 index 727612e99..8dcc24b89 100644 --- 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 index 194a5a5e1..ac1329e2a 100644 --- 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 index b3eee78d2..f39721e64 100644 --- 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,11 +209,11 @@ 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, @@ -216,16 +221,113 @@ abstract contract PermissionManager is Initializable { 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,7 +340,11 @@ 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]; @@ -246,7 +352,13 @@ abstract contract PermissionManager is Initializable { 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,7 +401,11 @@ abstract contract PermissionManager is Initializable { } } - bytes32 permHash = permissionHash(_where, _who, _permissionId); + bytes32 permHash = permissionHash({ + _where: _where, + _who: _who, + _permissionId: _permissionId + }); address currentCondition = permissionsHashed[permHash]; @@ -297,7 +413,13 @@ abstract contract PermissionManager is Initializable { 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 index 029a27cc7..250b80910 100644 --- 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 index 63b128cc6..95c36f226 100644 --- 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 () => { diff --git a/packages/contracts/test/core/permission/permission-manager.ts b/packages/contracts/test/core/permission/permission-manager.ts index 2034a8a9e..7a5df98c3 100644 --- a/packages/contracts/test/core/permission/permission-manager.ts +++ b/packages/contracts/test/core/permission/permission-manager.ts @@ -9,7 +9,6 @@ import { PermissionConditionMock__factory, TestPlugin__factory, } from '../../../typechain'; -import {DeployTestPermissionCondition} from '../../test-utils/conditions'; import {OZ_ERRORS} from '../../test-utils/error'; import {Operation} from '../../../utils/types'; @@ -728,7 +727,7 @@ describe('Core: PermissionManager', function () { }); describe('isGranted', () => { - it('should return true if the permission is granted to the user', async () => { + it('returns `true` if the permission is granted to the user', async () => { await pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID); const isGranted = await pm.callStatic.isGranted( pm.address, @@ -739,7 +738,7 @@ describe('Core: PermissionManager', function () { expect(isGranted).to.be.equal(true); }); - it('should return false if the permissions is not granted to the user', async () => { + it('returns `false` if the permission is not granted to the user', async () => { const isGranted = await pm.callStatic.isGranted( pm.address, otherSigner.address, @@ -749,35 +748,112 @@ describe('Core: PermissionManager', function () { expect(isGranted).to.be.equal(false); }); - it('should return true for permissions granted to any address on a specific target contract using the `ANY_ADDR` flag', async () => { - const anyAddr = await pm.getAnyAddr(); - const condition = await DeployTestPermissionCondition(); + it('returns `true` if a condition is set for a specific caller and target answering `true`', async () => { + const condition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); await pm.grantWithCondition( pm.address, - anyAddr, + ownerSigner.address, ADMIN_PERMISSION_ID, condition.address ); + await condition.setAnswer(true); + + expect( + await pm.isGranted( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.true; + }); + + it('returns `true` if a condition is set for a generic caller answering `true`', async () => { + const condition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); await pm.grantWithCondition( - anyAddr, - pm.address, - ADMIN_PERMISSION_ID, - condition.address - ); - const isGranted_1 = await pm.callStatic.isGranted( pm.address, - anyAddr, + ANY_ADDR, ADMIN_PERMISSION_ID, condition.address ); - const isGranted_2 = await pm.callStatic.isGranted( - pm.address, - anyAddr, + await condition.setAnswer(true); + + // Check `ownerSigner.address` as a caller `_who` + expect( + await pm.isGranted( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.true; + + // Check `otherSigner.address` as a caller `_who` + expect( + await pm.isGranted( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.true; + + // Check that `false` is returned if `address(0)` is the target `_where`. + expect( + await pm.isGranted( + ethers.constants.AddressZero, + ownerSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.false; + }); + + it('returns `true` if a condition is set for a generic target answering `true`', async () => { + const condition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + await pm.grantWithCondition( + ANY_ADDR, + ownerSigner.address, ADMIN_PERMISSION_ID, condition.address ); - expect(isGranted_1).to.be.equal(true); - expect(isGranted_2).to.be.equal(true); + await condition.setAnswer(true); + + // Check `pm.address` as a target `_where` + expect( + await pm.isGranted( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.true; + + // Check `address(0)` as a target `_where` + expect( + await pm.isGranted( + ethers.constants.AddressZero, + ownerSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.true; + + // Check that `false` is returned if `otherSigner is the caller `_who`. + expect( + await pm.isGranted( + ethers.constants.AddressZero, + otherSigner.address, + ADMIN_PERMISSION_ID, + condition.address + ) + ).to.be.false; }); it('should be callable by anyone', async () => { @@ -791,13 +867,198 @@ describe('Core: PermissionManager', function () { ); expect(isGranted).to.be.equal(false); }); + + it('does not fall back to a generic caller or target condition if a specific condition is set already answering `false`', async () => { + const specificCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + const genericCallerCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + const genericTargetCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + + // Grant with a specific condition that will answer false + await pm.grantWithCondition( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + specificCondition.address + ); + await specificCondition.setAnswer(false); + + // Grant with a generic caller condition that will answer true + await pm.grantWithCondition( + pm.address, + ANY_ADDR, + ADMIN_PERMISSION_ID, + genericCallerCondition.address + ); + await genericCallerCondition.setAnswer(true); + + // Grant with a generic target condition that will answer true + await pm.grantWithCondition( + ANY_ADDR, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ); + await genericCallerCondition.setAnswer(true); + + // Check that `isGranted` returns false for `ownerSigner` to whom the specific condition was granted. + expect( + await pm.isGranted( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.false; + + // Check that `ownerSigner` is still granted access to other contracts (e.g., `address(0)`) through the `genericTargetCondition` condition. + expect( + await pm.isGranted( + ethers.constants.AddressZero, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.true; + }); + + it('does not fall back to a generic target condition if a generic caller condition is set already answering `false`', async () => { + const genericCallerCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + const genericTargetCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + + // Grant with a generic caller condition that will answer false. + await pm.grantWithCondition( + pm.address, + ANY_ADDR, + ADMIN_PERMISSION_ID, + genericCallerCondition.address + ); + await genericCallerCondition.setAnswer(false); + + // Grant with a generic target condition that will answer true. + await pm.grantWithCondition( + ANY_ADDR, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ); + await genericTargetCondition.setAnswer(true); + + // Check that `isGranted` returns false for `ANY_ADDR` (here, we check only two addresses, `ownerSigner` and `otherSigner`). + expect( + await pm.isGranted( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.false; + expect( + await pm.isGranted( + pm.address, + otherSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.false; + + // Check that `ownerSigner` is granted access to other contracts (e.g., `address(0)`) via the `genericTargetCondition` condition. + expect( + await pm.isGranted( + ethers.constants.AddressZero, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.true; + + // Check that `otherSigner` is not granted access to other contracts (e.g., `address(0)`) via the `genericTargetCondition` condition. + expect( + await pm.isGranted( + ethers.constants.AddressZero, + otherSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.false; + }); + + it('does not fall back to a generic caller or target condition if a specific condition is set already answering `false`', async () => { + const specificCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + const genericCallerCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + const genericTargetCondition = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + + // Grant with a specific condition that will answer false + await pm.grantWithCondition( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + specificCondition.address + ); + await specificCondition.setAnswer(false); + + // Grant with a generic caller condition that will answer true + await pm.grantWithCondition( + pm.address, + ANY_ADDR, + ADMIN_PERMISSION_ID, + genericCallerCondition.address + ); + await genericCallerCondition.setAnswer(true); + + // Grant with a generic target condition that will answer true + await pm.grantWithCondition( + ANY_ADDR, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ); + await genericCallerCondition.setAnswer(true); + + // Check that `isGranted` returns false for `ownerSigner` to whom the specific condition was granted. + expect( + await pm.isGranted( + pm.address, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.false; + + // Check that `ownerSigner` is still granted access to other contracts (e.g., `address(0)`) through the `genericTargetCondition` condition. + expect( + await pm.isGranted( + ethers.constants.AddressZero, + ownerSigner.address, + ADMIN_PERMISSION_ID, + genericTargetCondition.address + ) + ).to.be.true; + }); }); describe('_hasPermission', () => { let permissionCondition: PermissionConditionMock; beforeEach(async () => { - permissionCondition = await DeployTestPermissionCondition(); + permissionCondition = await new PermissionConditionMock__factory( + ownerSigner + ).deploy(); }); it('should call IPermissionCondition.isGranted', async () => { diff --git a/packages/contracts/test/test-utils/conditions.ts b/packages/contracts/test/test-utils/conditions.ts deleted file mode 100644 index 17cc58971..000000000 --- a/packages/contracts/test/test-utils/conditions.ts +++ /dev/null @@ -1,13 +0,0 @@ -import {ethers} from 'hardhat'; - -import { - PermissionConditionMock, - PermissionConditionMock__factory, -} from '../../typechain'; - -export async function DeployTestPermissionCondition(): Promise { - const signers = await ethers.getSigners(); - const aclConditionFactory = new PermissionConditionMock__factory(signers[0]); - const permissionCondition = await aclConditionFactory.deploy(); - return permissionCondition; -}