From b1932bffed38c0704c2ac74a513824d0488d1695 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Tue, 17 Sep 2024 12:30:50 +0400 Subject: [PATCH 1/3] fix more tests and cleaner --- .../src/core/permission/PermissionManager.sol | 36 +- .../core/permission/permission-manager.ts | 754 +++++++++--------- .../test/framework/dao/dao-factory.ts | 2 +- 3 files changed, 390 insertions(+), 402 deletions(-) diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol index 711f0d290..456177b58 100644 --- a/packages/contracts/src/core/permission/PermissionManager.sol +++ b/packages/contracts/src/core/permission/PermissionManager.sol @@ -321,7 +321,7 @@ abstract contract PermissionManager is Initializable { bytes32 _permissionIdOrSelector, address _owner, uint256 _flags - ) external { + ) public { if (_flags == 0) { revert FlagCanNotBeZero(); } @@ -369,7 +369,7 @@ abstract contract PermissionManager is Initializable { /// @param _where The address of the target contract for which `_who` receives permission. /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. /// @param _flags The flags as uint256 to remove specific rights the calling owner does have. (only revoke? only grant? both?) - function removeOwner(address _where, bytes32 _permissionIdOrSelector, uint256 _flags) external { + function removeOwner(address _where, bytes32 _permissionIdOrSelector, uint256 _flags) public { if (_flags == 0) { revert FlagCanNotBeZero(); } @@ -414,6 +414,38 @@ abstract contract PermissionManager is Initializable { return _isPermissionFrozen(permission); } + /// @notice Function to retrieve the owner and delegate flags of an `_owner` on a permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @return The counts of how many owners are on a permission and whether permission has been created or not yet. + function getPermissionData( + address _where, + bytes32 _permissionIdOrSelector + ) public view returns (bool, uint64, uint64) { + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + return (permission.created, permission.grantCounter, permission.revokeCounter); + } + + /// @notice Function to retrieve the owner and delegate flags of an `_owner` on a permission. + /// @param _where The address of the target contract for which `_who` receives permission. + /// @param _permissionIdOrSelector The permission hash or function selector used for this permission. + /// @param _owner The address for which to return the current flags. + /// @return The owner and delegate flags. + function getFlags( + address _where, + bytes32 _permissionIdOrSelector, + address _owner + ) public view returns (uint256, uint256) { + Permission storage permission = permissions[ + permissionHash(_where, _permissionIdOrSelector) + ]; + + return (permission.owners[_owner], permission.delegations[_owner]); + } + /// @notice Grants permission to an address to call methods in a contract guarded by an auth modifier with the specified permission identifier. /// @dev Requires the `ROOT_PERMISSION_ID` permission. /// @param _where The address of the target contract for which `_who` receives permission. diff --git a/packages/contracts/test/core/permission/permission-manager.ts b/packages/contracts/test/core/permission/permission-manager.ts index 748e18d9c..e7eb3c40f 100644 --- a/packages/contracts/test/core/permission/permission-manager.ts +++ b/packages/contracts/test/core/permission/permission-manager.ts @@ -39,7 +39,16 @@ interface SingleTargetPermission { permissionId: string; } -describe('Core: PermissionManager', function () { +const GRANT_OWNER_FLAG = 2; +const REVOKE_OWNER_FLAG = 4; +const FULL_OWNER_FLAG = 6; +const FREEZE_ADDRESS = '0x0000000000000000000000000000000000000001'; + +const someWhere = '0xb794F5eA0ba39494cE839613fffBA74279579268'; +const somePermissionId = + '0x0000000000000000000000000000000000000000000000000000000012345678'; + +describe.only('Core: PermissionManager', function () { let pm: PermissionManagerTest; let signers: SignerWithAddress[]; let ownerSigner: SignerWithAddress; @@ -92,75 +101,126 @@ describe('Core: PermissionManager', function () { it('should add an owner with all the flags passed', async () => { await expect( - pm.addOwner(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 6) + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) ) .to.emit(pm, 'OwnerAdded') - .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 6); + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ); }); - it('should revert with FlagCanNotBeZero if a zero flag is passed', async () => { + it('should revert if a zero flag is passed', async () => { await expect( pm.addOwner(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 0) ).to.be.revertedWithCustomError(pm, 'FlagCanNotBeZero'); }); - it('should revert with ZeroAddress if _owner is address(0)', async () => { + it('should revert if an _owner passed is address(0)', async () => { await expect( pm.addOwner( pm.address, ADMIN_PERMISSION_ID, - '0x0000000000000000000000000000000000000000', - 6 + ethers.constants.AddressZero, + FULL_OWNER_FLAG ) ).to.be.revertedWithCustomError(pm, 'ZeroAddress'); }); - it('should revert with ZeroAddress if _where is address(0)', async () => { + it('should revert if _where passed is address(0)', async () => { await expect( pm.addOwner( - '0x0000000000000000000000000000000000000000', + ethers.constants.AddressZero, ADMIN_PERMISSION_ID, otherSigner.address, - 6 + FULL_OWNER_FLAG ) ).to.be.revertedWithCustomError(pm, 'ZeroAddress'); }); - it('should revert with PermissionFrozen if the permission is frozen', async () => { + it('should revert if the permission is frozen', async () => { + const freezeAddr = FREEZE_ADDRESS; + await pm.addOwner( pm.address, ADMIN_PERMISSION_ID, - '0x0000000000000000000000000000000000000001', - 6 + freezeAddr, + FULL_OWNER_FLAG ); - await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 6); + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, FULL_OWNER_FLAG); await expect( - pm.addOwner(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 6) + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ) ) .to.be.revertedWithCustomError(pm, 'PermissionFrozen') .withArgs(pm.address, ADMIN_PERMISSION_ID); }); - it('should revert with UnauthorizedOwner if the caller doesnt have ownership for revoke', async () => { - await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 4); + it('should revert with UnauthorizedOwner if the caller does not have ownership for revoke', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); + + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG); await expect( - pm.addOwner(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 4) + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + someAddress, + REVOKE_OWNER_FLAG + ) ) .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') - .withArgs(ownerSigner.address, 2, 4); + .withArgs(ownerSigner.address, GRANT_OWNER_FLAG, REVOKE_OWNER_FLAG); + + // Note that it should still have grant owner capability. + await expect( + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + someAddress, + GRANT_OWNER_FLAG + ) + ).to.not.be.reverted; }); - it('should revert with UnauthorizedOwner if the caller doesnt have ownership for grant', async () => { - await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 2); + it('should revert with UnauthorizedOwner if the caller does not have ownership for grant', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); + + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); await expect( - pm.addOwner(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 2) + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + someAddress, + GRANT_OWNER_FLAG + ) ) .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') - .withArgs(ownerSigner.address, 4, 2); + .withArgs(ownerSigner.address, REVOKE_OWNER_FLAG, GRANT_OWNER_FLAG); + + // Note that it should still have revoke owner capability. + await expect( + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + someAddress, + REVOKE_OWNER_FLAG + ) + ).to.not.be.reverted; }); }); @@ -180,18 +240,20 @@ describe('Core: PermissionManager', function () { .withArgs(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, 1); });*/ - it('should revert with FlagCanNotBeZero if a zero flag is passed', async () => { + it('should revert if a zero flag is passed', async () => { await expect( pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 0) ).to.be.revertedWithCustomError(pm, 'FlagCanNotBeZero'); }); - it('should revert with InvalidFlagsForRemovalPassed if flags are tried to be removed that do not exist', async () => { - await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 2); + it('should revert if flags that dont exist are removed', async () => { + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); - await expect(pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 2)) + await expect( + pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG) + ) .to.be.revertedWithCustomError(pm, 'InvalidFlagsForRemovalPassed') - .withArgs(4, 2); + .withArgs(REVOKE_OWNER_FLAG, GRANT_OWNER_FLAG); }); }); @@ -199,89 +261,93 @@ describe('Core: PermissionManager', function () { it('should only be callable by the ROOT user', async () => { await expect( pm.createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', + pm.address, + ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ) ) .to.emit(pm, 'PermissionCreated') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', - otherSigner.address, - ['0xb794F5eA0ba39494cE839613fffBA74279579268'] - ); + .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, []); await expect( pm .connect(otherSigner) .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', + pm.address, + ethers.utils.id('TEST_PERMISSION_1'), otherSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') .withArgs( pm.address, otherSigner.address, - '0x815fe80e4b37c8582a3b773d1d7071f983eacfd56b5965db654f3087c25ada33' + DAO_PERMISSIONS.ROOT_PERMISSION_ID ); }); - it('should revert with PermissionAlreadyCreated in case the permission is already existing', async () => { + it('should revert in case the permission is already existing', async () => { + await pm.createPermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + [] + ); + await expect( pm.createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', + pm.address, + ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ) - ) - .to.emit(pm, 'PermissionCreated') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', - otherSigner.address, - ['0xb794F5eA0ba39494cE839613fffBA74279579268'] - ); + ).to.be.revertedWithCustomError(pm, 'PermissionAlreadyCreated'); + }); + it('should create a permission and call grant for all whos passed', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); await expect( pm.createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', + pm.address, + ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [someAddress] ) - ).to.be.revertedWithCustomError(pm, 'PermissionAlreadyCreated'); + ) + .to.emit(pm, 'PermissionCreated') + .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, [ + someAddress, + ]); + + expect( + await pm.isGranted(pm.address, someAddress, ADMIN_PERMISSION_ID, '0x') + ).to.be.true; }); - it('should create a permission and call grant for all whos passed', async () => { + it('should create a permission with an owner of full capability', async () => { + const someAddress = await ethers.Wallet.createRandom().getAddress(); await expect( pm.createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', + pm.address, + ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [someAddress] ) ) .to.emit(pm, 'PermissionCreated') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234', - otherSigner.address, - ['0xb794F5eA0ba39494cE839613fffBA74279579268'] - ); + .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, [ + someAddress, + ]); - const flag = await pm.getAuthPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000000001234' - ); + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([FULL_OWNER_FLAG, 0]); - expect(flag).to.equal('0x0000000000000000000000000000000000000002'); + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 1]); }); }); @@ -291,11 +357,11 @@ describe('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ); }); - it('should revert with FlagCanNotBeZero if a zero flag is passed', async () => { + it('should revert if a zero flag is passed', async () => { await expect( pm.delegatePermission( pm.address, @@ -306,28 +372,28 @@ describe('Core: PermissionManager', function () { ).to.be.revertedWithCustomError(pm, 'FlagCanNotBeZero'); }); - it('should revert with PermissionFrozen if the permission is actually frozen', async () => { + it('should revert if the permission is actually frozen', async () => { await pm.addOwner( pm.address, ADMIN_PERMISSION_ID, - '0x0000000000000000000000000000000000000001', - 6 + FREEZE_ADDRESS, + FULL_OWNER_FLAG ); - await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 6); + await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, FULL_OWNER_FLAG); await expect( pm.delegatePermission( pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - 6 + FULL_OWNER_FLAG ) ) .to.be.revertedWithCustomError(pm, 'PermissionFrozen') .withArgs(pm.address, ADMIN_PERMISSION_ID); }); - it('should revert with UnauthorizedOwner if the caller isnt the owner of the permission he wants to delegate', async () => { + it('should revert if the caller is not the owner of the permission he/she wants to delegate', async () => { await expect( pm .connect(otherSigner) @@ -335,24 +401,33 @@ describe('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - 6 + FULL_OWNER_FLAG ) ) .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') - .withArgs(otherSigner.address, 0, 6); + .withArgs(otherSigner.address, 0, FULL_OWNER_FLAG); }); - it('should emit PermissionDelegated when the permission got delegated successfully', async () => { + it('should emit PermissionDelegated and correctly store the flags', async () => { await expect( pm.delegatePermission( pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - 6 + FULL_OWNER_FLAG ) ) .to.emit(pm, 'PermissionDelegated') - .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 6); + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + FULL_OWNER_FLAG + ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([0, FULL_OWNER_FLAG]); }); }); @@ -362,18 +437,18 @@ describe('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ); await pm.delegatePermission( pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - 6 + FULL_OWNER_FLAG ); }); - it('should revert with FlagCanNotBeZero when a zero flag is passed', async () => { + it('should revert when a zero flag is passed', async () => { await expect( pm.undelegatePermission( pm.address, @@ -384,7 +459,7 @@ describe('Core: PermissionManager', function () { ).to.be.revertedWithCustomError(pm, 'FlagCanNotBeZero'); }); - it('should revert with UnauthorizedOwner if the caller isnt the owner of the permission he wants to undelegate', async () => { + it('should revert if the caller is not the owner of the permission he/she wants to undelegate', async () => { await expect( pm .connect(otherSigner) @@ -392,24 +467,33 @@ describe('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - 6 + FULL_OWNER_FLAG ) ) .to.be.revertedWithCustomError(pm, 'UnauthorizedOwner') - .withArgs(otherSigner.address, 0, 6); + .withArgs(otherSigner.address, 0, FULL_OWNER_FLAG); }); - it('should emit PermissionUndelegated when the permissiion got undelegated successfully', async () => { + it('should emit PermissionUndelegated', async () => { await expect( pm.undelegatePermission( pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ) ) .to.emit(pm, 'PermissionUndelegated') - .withArgs(pm.address, ADMIN_PERMISSION_ID, otherSigner.address, 2); + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([0, GRANT_OWNER_FLAG]); }); }); @@ -457,6 +541,27 @@ describe('Core: PermissionManager', function () { ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); }); + it('reverts if special `APPLY_TARGET_PERMISSION_ID` is granted to non-allowed address', async () => { + const allowedApplyTargetMethodGrantee = + await pm.applyTargetMethodGrantee(); + + await expect( + pm.grant(pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID) + ) + .to.be.revertedWithCustomError( + pm, + 'IncorrectApplyTargetMethodGranteeSet' + ) + .withArgs(allowedApplyTargetMethodGrantee); + + // It should succeed if it's allowed. + await pm.setApplyTargetMethodGrantee(otherSigner.address); + + await expect( + pm.grant(pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID) + ).to.not.be.reverted; + }); + it('should emit Granted', async () => { await expect( pm.grant(pm.address, otherSigner.address, ADMIN_PERMISSION_ID) @@ -502,103 +607,69 @@ describe('Core: PermissionManager', function () { it('should revert if the permission is frozen', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ ownerSigner.address, - [ownerSigner.address] - ); + ]); await pm .connect(ownerSigner) .addOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - '0x0000000000000000000000000000000000000001', - 2 + someWhere, + somePermissionId, + FREEZE_ADDRESS, + GRANT_OWNER_FLAG ); await pm .connect(ownerSigner) - .removeOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - 6 - ); + .removeOwner(someWhere, somePermissionId, FULL_OWNER_FLAG); + -0xb794f5ea0ba39494ce839613fffba74279579268 + + 0xb794f5ea0ba39494ce839613fffba74279579268; await expect( pm .connect(ownerSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .grant(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'PermissionFrozen') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, somePermissionId); }); - it('should revert if the caller doesnt have the grant flag set', async () => { + it('should revert if the caller does not have the grant flag set', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await expect( pm .connect(otherSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .grant(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); it('should allow the root user as fallback if the permission is created but no grant owners are existing anymore', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - otherSigner.address, - [ownerSigner.address] - ); + .createPermission(someWhere, somePermissionId, otherSigner.address, [ + ownerSigner.address, + ]); await pm .connect(otherSigner) - .removeOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - 2 - ); + .removeOwner(someWhere, somePermissionId, GRANT_OWNER_FLAG); await expect( pm .connect(ownerSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .grant(someWhere, otherSigner.address, somePermissionId) ).to.emit(pm, 'Granted'); }); - it('should allow the root user as fallback if the permission isnt created', async () => { + it('should allow the root user as fallback if the permission is not created', async () => { await expect( pm .connect(ownerSigner) @@ -613,92 +684,66 @@ describe('Core: PermissionManager', function () { it('should allow the delegatee to call grant once', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ ownerSigner.address, - [ownerSigner.address] - ); + ]); await pm .connect(ownerSigner) .delegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 2 + GRANT_OWNER_FLAG ); await expect( pm .connect(otherSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .grant(someWhere, otherSigner.address, somePermissionId) ).to.emit(pm, 'Granted'); await expect( pm .connect(otherSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .grant(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); it('should revert if undelegate got called before grantWithCondition got called by the delegatee', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await pm .connect(ownerSigner) .delegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await pm .connect(ownerSigner) .undelegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await expect( pm .connect(otherSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .grant(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); }); @@ -893,100 +938,76 @@ describe('Core: PermissionManager', function () { it('should revert if the permission is frozen', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ ownerSigner.address, - [ownerSigner.address] - ); + ]); await pm .connect(ownerSigner) .addOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - '0x0000000000000000000000000000000000000001', - 2 + someWhere, + somePermissionId, + FREEZE_ADDRESS, + GRANT_OWNER_FLAG ); await pm .connect(ownerSigner) - .removeOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - 6 - ); + .removeOwner(someWhere, somePermissionId, FULL_OWNER_FLAG); await expect( pm .connect(ownerSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ) .to.be.revertedWithCustomError(pm, 'PermissionFrozen') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, somePermissionId); }); it('should revert if the caller doesnt have the grantWithCondition flag set', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await expect( pm .connect(otherSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); it('should allow the root user as fallback if the permission is created but no grantWithCondition owners are existing anymore', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - otherSigner.address, - [ownerSigner.address] - ); + .createPermission(someWhere, somePermissionId, otherSigner.address, [ + ownerSigner.address, + ]); await pm .connect(otherSigner) - .removeOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - 2 - ); + .removeOwner(someWhere, somePermissionId, GRANT_OWNER_FLAG); await expect( pm .connect(ownerSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ).to.emit(pm, 'Granted'); @@ -997,9 +1018,9 @@ describe('Core: PermissionManager', function () { pm .connect(ownerSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ).to.emit(pm, 'Granted'); @@ -1008,29 +1029,26 @@ describe('Core: PermissionManager', function () { it('should allow the delegatee to call grantWithCondition once', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ ownerSigner.address, - [ownerSigner.address] - ); + ]); await pm .connect(ownerSigner) .delegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 2 + GRANT_OWNER_FLAG ); await expect( pm .connect(otherSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ).to.emit(pm, 'Granted'); @@ -1039,64 +1057,53 @@ describe('Core: PermissionManager', function () { pm .connect(otherSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); it('should revert if undelegate got called before grantWithCondition got called by the delegatee', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await pm .connect(ownerSigner) .delegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await pm .connect(ownerSigner) .undelegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await expect( pm .connect(otherSigner) .grantWithCondition( - '0xb794f5ea0ba39494ce839613fffba74279579268', + someWhere, otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678', + somePermissionId, conditionMock.address ) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); }); @@ -1162,211 +1169,141 @@ describe('Core: PermissionManager', function () { it('should revert if the permission is frozen', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ ownerSigner.address, - [ownerSigner.address] - ); + ]); await pm .connect(ownerSigner) .addOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - '0x0000000000000000000000000000000000000001', - 2 + someWhere, + somePermissionId, + FREEZE_ADDRESS, + GRANT_OWNER_FLAG ); await pm .connect(ownerSigner) - .removeOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - 6 - ); + .removeOwner(someWhere, somePermissionId, FULL_OWNER_FLAG); await expect( pm .connect(ownerSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'PermissionFrozen') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, somePermissionId); }); it('should revert if the caller doesnt have the revoke flag set', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await expect( pm .connect(otherSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); it('should allow the root user as fallback if the permission is created but no revoke owners are existing anymore', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + .createPermission(someWhere, somePermissionId, otherSigner.address, [ otherSigner.address, - [otherSigner.address] - ); + ]); await pm .connect(otherSigner) - .removeOwner( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - 4 - ); + .removeOwner(someWhere, somePermissionId, REVOKE_OWNER_FLAG); await expect( pm .connect(ownerSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ).to.emit(pm, 'Revoked'); }); it('should allow the root user as fallback if the permission isnt created', async () => { await pm .connect(ownerSigner) - .grant( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .grant(someWhere, otherSigner.address, somePermissionId); await expect( pm .connect(ownerSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ).to.emit(pm, 'Revoked'); }); it('should allow the delegatee to call revoke once', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await pm .connect(ownerSigner) .delegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await expect( pm .connect(otherSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ).to.emit(pm, 'Revoked'); await expect( pm .connect(otherSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); it('should revert if undelegate got called before revoke got called by the delegatee', async () => { await pm .connect(ownerSigner) - .createPermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', - ownerSigner.address, - [otherSigner.address] - ); + .createPermission(someWhere, somePermissionId, ownerSigner.address, [ + otherSigner.address, + ]); await pm .connect(ownerSigner) .delegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await pm .connect(ownerSigner) .undelegatePermission( - '0xb794f5ea0ba39494ce839613fffba74279579268', - '0x0000000000000000000000000000000000000000000000000000000012345678', + someWhere, + somePermissionId, otherSigner.address, - 4 + REVOKE_OWNER_FLAG ); await expect( pm .connect(otherSigner) - .revoke( - '0xb794f5ea0ba39494ce839613fffba74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ) + .revoke(someWhere, otherSigner.address, somePermissionId) ) .to.be.revertedWithCustomError(pm, 'Unauthorized') - .withArgs( - '0xb794F5eA0ba39494cE839613fffBA74279579268', - otherSigner.address, - '0x0000000000000000000000000000000000000000000000000000000012345678' - ); + .withArgs(someWhere, otherSigner.address, somePermissionId); }); }); @@ -1414,8 +1351,11 @@ describe('Core: PermissionManager', function () { ownerSigner.address, ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794F5eA0ba39494cE839613fffBA74279579268'] + [someWhere] ); + + await pm.setApplyTargetMethodGrantee(otherSigner.address); + await pm.grant( pm.address, otherSigner.address, @@ -1446,8 +1386,11 @@ describe('Core: PermissionManager', function () { ownerSigner.address, ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794F5eA0ba39494cE839613fffBA74279579268'] + [someWhere] ); + + await pm.setApplyTargetMethodGrantee(ownerSigner.address); + await pm.grant( pm.address, ownerSigner.address, @@ -1474,6 +1417,8 @@ describe('Core: PermissionManager', function () { permissionId: ADMIN_PERMISSION_ID, }; + await pm.setApplyTargetMethodGrantee(otherSigner.address); + await pm.grant( pm.address, otherSigner.address, @@ -1620,16 +1565,18 @@ describe('Core: PermissionManager', function () { ownerSigner.address, ADMIN_PERMISSION_ID, otherSigner.address, - ['0xb794F5eA0ba39494cE839613fffBA74279579268'] + [someWhere] ); + await pm.setApplyTargetMethodGrantee(otherSigner.address); + await pm.grant( pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID ); - await pm.setAllowedContract(otherSigner.address); + await pm.setApplyTargetMethodGrantee(otherSigner.address); await pm .connect(otherSigner) @@ -1657,6 +1604,9 @@ describe('Core: PermissionManager', function () { otherSigner.address, ['0xb794F5eA0ba39494cE839613fffBA74279579268'] ); + + await pm.setApplyTargetMethodGrantee(ownerSigner.address); + await pm.grant( pm.address, ownerSigner.address, @@ -1683,11 +1633,14 @@ describe('Core: PermissionManager', function () { permissionId: ADMIN_PERMISSION_ID, }; + await pm.setApplyTargetMethodGrantee(otherSigner.address); + await pm.grant( pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID ); + await pm.applySingleTargetPermissions(otherSigner.address, [bulkItem]); const permission = await pm.getAuthPermission( @@ -1752,16 +1705,19 @@ describe('Core: PermissionManager', function () { [otherSigner.address] ); + const permissionId1 = ethers.utils.id('TEST_PERMISSION_1'); + const permissionId2 = ethers.utils.id('TEST_PERMISSION_2'); + const bulkItems: SingleTargetPermission[] = [ { operation: Operation.Revoke, who: otherSigner.address, - permissionId: ADMIN_PERMISSION_ID, + permissionId: permissionId1, }, { operation: Operation.Grant, who: ownerSigner.address, - permissionId: APPLY_TARGET_PERMISSION_ID, + permissionId: permissionId2, }, ]; @@ -1770,14 +1726,14 @@ describe('Core: PermissionManager', function () { await pm.getAuthPermission( pm.address, otherSigner.address, - ADMIN_PERMISSION_ID + permissionId1 ) ).to.be.equal(UNSET_FLAG); expect( await pm.getAuthPermission( pm.address, ownerSigner.address, - APPLY_TARGET_PERMISSION_ID + permissionId2 ) ).to.be.equal(ALLOW_FLAG); }); diff --git a/packages/contracts/test/framework/dao/dao-factory.ts b/packages/contracts/test/framework/dao/dao-factory.ts index cf86b2826..707e218ee 100644 --- a/packages/contracts/test/framework/dao/dao-factory.ts +++ b/packages/contracts/test/framework/dao/dao-factory.ts @@ -108,7 +108,7 @@ async function getAnticipatedAddress(from: string) { return anticipatedAddress; } -describe.only('DAOFactory: ', function () { +describe('DAOFactory: ', function () { let daoFactory: DAOFactory; let managingDao: any; From 53c5def7b9eb1ddf810b7cb7d44138b7f40edd96 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Tue, 17 Sep 2024 13:29:55 +0400 Subject: [PATCH 2/3] test fixes + permissionFrozen for multi target + more fixes --- .../src/core/permission/PermissionManager.sol | 32 +- .../core/permission/permission-manager.ts | 323 +++++++++++++++++- 2 files changed, 342 insertions(+), 13 deletions(-) diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol index 456177b58..eb27a57d5 100644 --- a/packages/contracts/src/core/permission/PermissionManager.sol +++ b/packages/contracts/src/core/permission/PermissionManager.sol @@ -272,7 +272,14 @@ abstract contract PermissionManager is Initializable { revert UnauthorizedOwner(msg.sender, permission.owners[msg.sender], _flags); } - uint256 newFlags = permission.delegations[_delegatee] | _flags; + uint256 currentFlags = permission.delegations[_delegatee]; + + // If the same flags that a `delegatee` already holds is added, return early. + if (currentFlags == _flags) { + return; + } + + uint256 newFlags = currentFlags | _flags; permission.delegations[_delegatee] = newFlags; emit PermissionDelegated(_where, _permissionIdOrSelector, _delegatee, newFlags); @@ -344,6 +351,11 @@ abstract contract PermissionManager is Initializable { uint256 currentFlags = permission.owners[_owner]; + // If the same flags that an `owner` already holds is added, return early. + if (currentFlags == _flags) { + return; + } + if (_owner != address(1)) { if ( _checkFlags(_flags, GRANT_OWNER_FLAG) && @@ -380,7 +392,7 @@ abstract contract PermissionManager is Initializable { uint256 currentFlags = permission.owners[msg.sender]; - // Check if the removal flags have more bit set as the owner currently has + // Check if the removal flags have more bit set as the owner currently has. if (!_checkFlags(currentFlags, _flags)) { revert InvalidFlagsForRemovalPassed(currentFlags, _flags); } @@ -528,6 +540,10 @@ abstract contract PermissionManager is Initializable { PermissionLib.SingleTargetPermission memory item = _items[i]; Permission storage permission = permissions[permissionHash(_where, item.permissionId)]; + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(_where, item.permissionId); + } + if (!_checkOwner(permission, msg.sender, item.operation, isRoot_)) { revert Unauthorized(_where, item.who, item.permissionId); } @@ -565,6 +581,10 @@ abstract contract PermissionManager is Initializable { permissionHash(item.where, item.permissionId) ]; + if (_isPermissionFrozen(permission)) { + revert PermissionFrozen(item.where, item.permissionId); + } + if (!_checkOwner(permission, msg.sender, item.operation, isRoot_)) { revert Unauthorized(item.where, item.who, item.permissionId); } @@ -790,6 +810,14 @@ abstract contract PermissionManager is Initializable { } } + // Make sure that this special permission is only granted + // to the address allowed by ROOT. + if (_permissionId == APPLY_TARGET_PERMISSION_ID) { + if (applyTargetMethodGrantee == address(0) || applyTargetMethodGrantee != _who) { + revert IncorrectApplyTargetMethodGranteeSet(applyTargetMethodGrantee); + } + } + bytes32 permHash = permissionHash({ _where: _where, _who: _who, diff --git a/packages/contracts/test/core/permission/permission-manager.ts b/packages/contracts/test/core/permission/permission-manager.ts index e7eb3c40f..d5e438c29 100644 --- a/packages/contracts/test/core/permission/permission-manager.ts +++ b/packages/contracts/test/core/permission/permission-manager.ts @@ -95,7 +95,7 @@ describe.only('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ); }); @@ -115,6 +115,91 @@ describe.only('Core: PermissionManager', function () { otherSigner.address, FULL_OWNER_FLAG ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([FULL_OWNER_FLAG, 0]); + }); + + it('should add an owner with only specific flags passed', async () => { + await expect( + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ) + ) + .to.emit(pm, 'OwnerAdded') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([GRANT_OWNER_FLAG, 0]); + + await expect( + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG + ) + ) + .to.emit(pm, 'OwnerAdded') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG + ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([FULL_OWNER_FLAG, 0]); + }); + + it('should correctly increase owner counters', async () => { + const newOwner = otherSigner.address; + + await pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + newOwner, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 1]); + + // If the same flag that `newOwner` already holds is added, + // it shouldn't increase counts and neither emit the event + await expect( + pm.addOwner(pm.address, ADMIN_PERMISSION_ID, newOwner, GRANT_OWNER_FLAG) + ).to.not.emit(pm, 'OwnerAdded'); + + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 1]); + + // Add the same owner but with revoke which should increase revoke counter only. + await expect( + pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + newOwner, + REVOKE_OWNER_FLAG + ) + ).to.emit(pm, 'OwnerAdded'); + + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 2]); }); it('should revert if a zero flag is passed', async () => { @@ -169,7 +254,7 @@ describe.only('Core: PermissionManager', function () { .withArgs(pm.address, ADMIN_PERMISSION_ID); }); - it('should revert with UnauthorizedOwner if the caller does not have ownership for revoke', async () => { + it('should revert if the caller does not have ownership for revoke', async () => { const someAddress = await ethers.Wallet.createRandom().getAddress(); await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG); @@ -196,7 +281,7 @@ describe.only('Core: PermissionManager', function () { ).to.not.be.reverted; }); - it('should revert with UnauthorizedOwner if the caller does not have ownership for grant', async () => { + it('should revert if the caller does not have ownership for grant', async () => { const someAddress = await ethers.Wallet.createRandom().getAddress(); await pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); @@ -230,15 +315,48 @@ describe.only('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, - ['0xb794f5ea0ba39494ce839613fffba74279579268'] + [] ); }); - /*it('should remove the passed flags from the owner', async () => { - await expect(pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, 6)) - .to.emit(pm.address, 'OwnerRemoved') - .withArgs(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, 1); - });*/ + it('should remove the FULL_OWNER_FLAGS from the owner', async () => { + await expect( + pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, FULL_OWNER_FLAG) + ) + .to.emit(pm, 'OwnerRemoved') + .withArgs(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, 0); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address) + ).to.deep.equal([0, 0]); + }); + + it('should remove the specific flag from the owner', async () => { + await expect( + pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG) + ) + .to.emit(pm, 'OwnerRemoved') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + ownerSigner.address, + REVOKE_OWNER_FLAG + ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address) + ).to.deep.equal([REVOKE_OWNER_FLAG, 0]); + + await expect( + pm.removeOwner(pm.address, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG) + ) + .to.emit(pm, 'OwnerRemoved') + .withArgs(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address, 0); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, ownerSigner.address) + ).to.deep.equal([0, 0]); + }); it('should revert if a zero flag is passed', async () => { await expect( @@ -255,6 +373,41 @@ describe.only('Core: PermissionManager', function () { .to.be.revertedWithCustomError(pm, 'InvalidFlagsForRemovalPassed') .withArgs(REVOKE_OWNER_FLAG, GRANT_OWNER_FLAG); }); + + it('should correctly decrease owner counters', async () => { + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 1]); + + const newOwner = otherSigner.address; + + await pm.addOwner( + pm.address, + ADMIN_PERMISSION_ID, + newOwner, + FULL_OWNER_FLAG + ); + + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 2, 2]); + + await pm + .connect(otherSigner) + .removeOwner(pm.address, ADMIN_PERMISSION_ID, GRANT_OWNER_FLAG); + + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 2]); + + await pm + .connect(otherSigner) + .removeOwner(pm.address, ADMIN_PERMISSION_ID, REVOKE_OWNER_FLAG); + + expect( + await pm.getPermissionData(pm.address, ADMIN_PERMISSION_ID) + ).to.deep.equal([true, 1, 1]); + }); }); describe('createPermission', () => { @@ -414,7 +567,27 @@ describe.only('Core: PermissionManager', function () { pm.address, ADMIN_PERMISSION_ID, otherSigner.address, - FULL_OWNER_FLAG + GRANT_OWNER_FLAG + ) + ) + .to.emit(pm, 'PermissionDelegated') + .withArgs( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + expect( + await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) + ).to.deep.equal([0, GRANT_OWNER_FLAG]); + + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG ) ) .to.emit(pm, 'PermissionDelegated') @@ -429,6 +602,33 @@ describe.only('Core: PermissionManager', function () { await pm.getFlags(pm.address, ADMIN_PERMISSION_ID, otherSigner.address) ).to.deep.equal([0, FULL_OWNER_FLAG]); }); + + it('should not emit PermissionDelegated if the same flags are added', async () => { + await pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ); + + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + GRANT_OWNER_FLAG + ) + ).to.not.emit(pm, 'PermissionDelegated'); + + await expect( + pm.delegatePermission( + pm.address, + ADMIN_PERMISSION_ID, + otherSigner.address, + REVOKE_OWNER_FLAG + ) + ).to.emit(pm, 'PermissionDelegated'); + }); }); describe('undelegatePermission', () => { @@ -474,7 +674,7 @@ describe.only('Core: PermissionManager', function () { .withArgs(otherSigner.address, 0, FULL_OWNER_FLAG); }); - it('should emit PermissionUndelegated', async () => { + it('should emit PermissionUndelegated and correctly update the flags', async () => { await expect( pm.undelegatePermission( pm.address, @@ -783,6 +983,37 @@ describe.only('Core: PermissionManager', function () { .withArgs(nonConditionContract.address); }); + it('reverts if special `APPLY_TARGET_PERMISSION_ID` is granted to non-allowed address', async () => { + const allowedApplyTargetMethodGrantee = + await pm.applyTargetMethodGrantee(); + + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + APPLY_TARGET_PERMISSION_ID, + conditionMock.address + ) + ) + .to.be.revertedWithCustomError( + pm, + 'IncorrectApplyTargetMethodGranteeSet' + ) + .withArgs(allowedApplyTargetMethodGrantee); + + // It should succeed if it's allowed. + await pm.setApplyTargetMethodGrantee(otherSigner.address); + + await expect( + pm.grantWithCondition( + pm.address, + otherSigner.address, + APPLY_TARGET_PERMISSION_ID, + conditionMock.address + ) + ).to.not.be.reverted; + }); + it('should add permission', async () => { await pm.grantWithCondition( pm.address, @@ -1522,6 +1753,42 @@ describe.only('Core: PermissionManager', function () { .to.be.revertedWithCustomError(pm, 'Unauthorized') .withArgs(pm.address, otherSigner.address, APPLY_TARGET_PERMISSION_ID); }); + + it('should revert if at least one of the permission is frozen', async () => { + const permissionId1 = ethers.utils.id('TEST_PERMISSION_1'); + const permissionId2 = ethers.utils.id('TEST_PERMISSION_2'); + + const where = someWhere; + + const bulkItems: MultiTargetPermission[] = [ + { + operation: Operation.Grant, + where: where, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId1, + }, + { + operation: Operation.Grant, + where: where, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId2, + }, + ]; + + // Let's freeze one of the permission. + await pm.createPermission(where, permissionId2, ownerSigner.address, []); + await pm.addOwner(where, permissionId2, FREEZE_ADDRESS, FULL_OWNER_FLAG); + await pm.removeOwner(where, permissionId2, FULL_OWNER_FLAG); + + // make sure that permission is really frozen. + expect(await pm.isPermissionFrozen(where, permissionId2)).to.be.true; + + await expect(pm.applyMultiTargetPermissions(bulkItems)) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(where, permissionId2); + }); }); describe('bulk on single target', () => { @@ -1684,6 +1951,40 @@ describe.only('Core: PermissionManager', function () { } }); + it('should revert if at least one of the permission is frozen', async () => { + const permissionId1 = ethers.utils.id('TEST_PERMISSION_1'); + const permissionId2 = ethers.utils.id('TEST_PERMISSION_2'); + + const where = someWhere; + + const bulkItems: MultiTargetPermission[] = [ + { + operation: Operation.Grant, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId1, + }, + { + operation: Operation.Grant, + who: otherSigner.address, + condition: addressZero, + permissionId: permissionId2, + }, + ]; + + // Let's freeze one of the permission. + await pm.createPermission(where, permissionId2, ownerSigner.address, []); + await pm.addOwner(where, permissionId2, FREEZE_ADDRESS, FULL_OWNER_FLAG); + await pm.removeOwner(where, permissionId2, FULL_OWNER_FLAG); + + // make sure that permission is really frozen. + expect(await pm.isPermissionFrozen(where, permissionId2)).to.be.true; + + await expect(pm.applySingleTargetPermissions(where, bulkItems)) + .to.be.revertedWithCustomError(pm, 'PermissionFrozen') + .withArgs(where, permissionId2); + }); + it('reverts for `Operation.GrantWithCondition` ', async () => { const bulkItems: SingleTargetPermission[] = [ { From 7a35db5c3d7539e1cfc0647c8d6c360521597ae3 Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Tue, 17 Sep 2024 15:23:27 +0400 Subject: [PATCH 3/3] improve natspec --- .../src/core/permission/PermissionManager.sol | 16 +++++++++++----- .../contracts/test/framework/dao/dao-factory.ts | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol index eb27a57d5..4334583e6 100644 --- a/packages/contracts/src/core/permission/PermissionManager.sol +++ b/packages/contracts/src/core/permission/PermissionManager.sol @@ -43,12 +43,18 @@ abstract contract PermissionManager is Initializable { /// @notice A mapping storing permissions as hashes (i.e., `permissionHash(where, who, permissionId)`) and their status encoded by an address (unset, allowed, or redirecting to a `PermissionCondition`). mapping(bytes32 => address) internal permissionsHashed; + /// @notice A struct containing the information for a permission. + /// @param delegations Owners can delegate the permission so delegatees can only grant it one time only. + /// @param owners The current owners of the permission with their own specific flags capabilities. + /// @param created Whether the permission has been created or not - used in consumer contracts(such as DAO.sol) to make decisions whether permission exists or not. + /// @param grantCounter How many owners(that have grant capabilities) are currently set for a permission. + /// @param revokeCounter How many owners(that have revoke capabilities) are currently set for a permission. struct Permission { - mapping(address => uint256) delegations; // Owners can delegate the permission so delegatees can only grant it one time only. - mapping(address => uint256) owners; // The current owners of the permission with their own specific flags capabilities. - bool created; // Whether the permission has been created or not - used in consumer contracts(such as DAO.sol) to make decisions whether permission exists or not. - uint64 grantCounter; // How many owners(that have grant capabilities) are currently set for a permission. - uint64 revokeCounter; // How many owners(that have revoke capabilities) are currently set for a permission. + mapping(address => uint256) delegations; + mapping(address => uint256) owners; + bool created; + uint64 grantCounter; + uint64 revokeCounter; } /// @notice A mapping storing owners and delegations of each permission. diff --git a/packages/contracts/test/framework/dao/dao-factory.ts b/packages/contracts/test/framework/dao/dao-factory.ts index 707e218ee..cf86b2826 100644 --- a/packages/contracts/test/framework/dao/dao-factory.ts +++ b/packages/contracts/test/framework/dao/dao-factory.ts @@ -108,7 +108,7 @@ async function getAnticipatedAddress(from: string) { return anticipatedAddress; } -describe('DAOFactory: ', function () { +describe.only('DAOFactory: ', function () { let daoFactory: DAOFactory; let managingDao: any;