From 562330f7c7178f60ced942a53cd75e6900f9187d Mon Sep 17 00:00:00 2001 From: Giorgi Lagidze Date: Tue, 29 Oct 2024 18:25:31 +0400 Subject: [PATCH] condition restrictions --- .../src/core/permission/PermissionManager.sol | 17 +++++- packages/contracts/test/core/dao/dao.ts | 2 +- .../core/permission/permission-manager.ts | 52 +++++++++++++------ 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/packages/contracts/src/core/permission/PermissionManager.sol b/packages/contracts/src/core/permission/PermissionManager.sol index 22f36a8a1..f2e2ad97e 100644 --- a/packages/contracts/src/core/permission/PermissionManager.sol +++ b/packages/contracts/src/core/permission/PermissionManager.sol @@ -191,6 +191,12 @@ abstract contract PermissionManager is Initializable { PermissionLib.MultiTargetPermission memory item = _items[i]; if (item.operation == PermissionLib.Operation.Grant) { + // Ensure a non-zero condition isn't passed, as `_grant` can't handle conditions. + // This avoids the false impression that a conditional grant occurred, + // since the transaction would still succeed without conditions. + if (item.condition != address(0)) { + revert GrantWithConditionNotSupported(); + } _grant({_where: item.where, _who: item.who, _permissionId: item.permissionId}); } else if (item.operation == PermissionLib.Operation.Revoke) { _revoke({_where: item.where, _who: item.who, _permissionId: item.permissionId}); @@ -336,10 +342,19 @@ abstract contract PermissionManager is Initializable { /// @param _permissionId The permission identifier. /// @dev Note, that granting permissions with `_who` or `_where` equal to `ANY_ADDR` does not replace other permissions with specific `_who` and `_where` addresses that exist in parallel. function _grant(address _where, address _who, bytes32 _permissionId) internal virtual { - if (_where == ANY_ADDR || _who == ANY_ADDR) { + if (_where == ANY_ADDR) { revert PermissionsForAnyAddressDisallowed(); } + if (_who == ANY_ADDR) { + if ( + _permissionId == ROOT_PERMISSION_ID || + isPermissionRestrictedForAnyAddr(_permissionId) + ) { + revert PermissionsForAnyAddressDisallowed(); + } + } + bytes32 permHash = permissionHash({ _where: _where, _who: _who, diff --git a/packages/contracts/test/core/dao/dao.ts b/packages/contracts/test/core/dao/dao.ts index 79db98b8d..d334747b3 100644 --- a/packages/contracts/test/core/dao/dao.ts +++ b/packages/contracts/test/core/dao/dao.ts @@ -85,7 +85,7 @@ const EVENTS = { export const VALID_ERC1271_SIGNATURE = '0x1626ba7e'; export const INVALID_ERC1271_SIGNATURE = '0xffffffff'; -describe.only('DAO', function () { +describe('DAO', function () { let signers: SignerWithAddress[]; let ownerAddress: string; let dao: DAO; diff --git a/packages/contracts/test/core/permission/permission-manager.ts b/packages/contracts/test/core/permission/permission-manager.ts index b62456972..6d1577c4e 100644 --- a/packages/contracts/test/core/permission/permission-manager.ts +++ b/packages/contracts/test/core/permission/permission-manager.ts @@ -94,28 +94,24 @@ describe('Core: PermissionManager', function () { ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); }); - it('reverts if permissionId is restricted and `_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => { - for (let i = 0; i < RESTRICTED_PERMISSIONS_FOR_ANY_ADDR.length; i++) { - await expect( - pm.grant(pm.address, ANY_ADDR, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i]) - ).to.be.revertedWithCustomError( - pm, - 'PermissionsForAnyAddressDisallowed' - ); - await expect( - pm.grant(ANY_ADDR, pm.address, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[i]) - ).to.be.revertedWithCustomError( - pm, - 'PermissionsForAnyAddressDisallowed' - ); - } + it('reverts if permissionId is restricted and `_who == ANY_ADDR`', async () => { + await expect( + pm.grant(pm.address, ANY_ADDR, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[0]) + ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); }); - it('reverts if permissionId is not restricted and`_who == ANY_ADDR` or `_where == ANY_ADDR`', async () => { + it('succeeds if permissionId is not restricted and `_who == ANY_ADDR`', async () => { + await expect(pm.grant(pm.address, ANY_ADDR, ADMIN_PERMISSION_ID)).to.not + .be.reverted; + }); + + it('reverts if permissionId is restricted and `_where == ANY_ADDR`', async () => { await expect( - pm.grant(pm.address, ANY_ADDR, ADMIN_PERMISSION_ID) + pm.grant(ANY_ADDR, pm.address, RESTRICTED_PERMISSIONS_FOR_ANY_ADDR[0]) ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); + }); + it('reverts if permissionId is not restricted and `_where == ANY_ADDR`', async () => { await expect( pm.grant(ANY_ADDR, pm.address, ADMIN_PERMISSION_ID) ).to.be.revertedWithCustomError(pm, 'PermissionsForAnyAddressDisallowed'); @@ -504,6 +500,28 @@ describe('Core: PermissionManager', function () { } }); + it('should revert if non-zero condition is used with `grant` operation type', async () => { + const signers = await ethers.getSigners(); + + const conditionMock = await new PermissionConditionMock__factory( + signers[0] + ).deploy(); + + const bulkItems: MultiTargetPermission[] = [ + { + operation: Operation.Grant, + where: signers[1].address, + who: signers[0].address, + condition: conditionMock.address, + permissionId: ADMIN_PERMISSION_ID, + }, + ]; + + await expect( + pm.applyMultiTargetPermissions(bulkItems) + ).to.be.revertedWithCustomError(pm, 'GrantWithConditionNotSupported'); + }); + it('should grant with condition', async () => { const signers = await ethers.getSigners();