From 99a23baa983b9cfc5b4d021a921bf0e995b43081 Mon Sep 17 00:00:00 2001 From: kingster-will <83567446+kingster-will@users.noreply.github.com> Date: Sun, 14 Apr 2024 09:20:15 -0700 Subject: [PATCH] Enhanced Access Control: Removal of Global Permissions, Direct Permission Setting, and External Contract Calls by IP Owners (#89) * initial impl * support owner set permission directly * add more comments --- contracts/access/AccessController.sol | 52 ++---- .../interfaces/access/IAccessController.sol | 8 - contracts/lib/Errors.sol | 2 +- test/foundry/access/AccessControlled.t.sol | 13 +- test/foundry/access/AccessController.t.sol | 171 +++++++++++------- 5 files changed, 130 insertions(+), 116 deletions(-) diff --git a/contracts/access/AccessController.sol b/contracts/access/AccessController.sol index 5ba0b29e..4945a524 100644 --- a/contracts/access/AccessController.sol +++ b/contracts/access/AccessController.sol @@ -92,24 +92,6 @@ contract AccessController is IAccessController, AccessManagedUpgradeable, UUPSUp } } - /// @notice Sets the permission for all IPAccounts - /// @dev Enforced to be only callable by the protocol admin in governance. - /// @param signer The address that can call `to` on behalf of the IP account - /// @param to The address that can be called by the `signer` (currently only modules can be `to`) - /// @param func The function selector of `to` that can be called by the `signer` on behalf of the `ipAccount` - /// @param permission The new permission level - function setGlobalPermission(address signer, address to, bytes4 func, uint8 permission) external restricted { - if (signer == address(0)) { - revert Errors.AccessController__SignerIsZeroAddress(); - } - // permission must be one of ABSTAIN, ALLOW, DENY - if (permission > 2) { - revert Errors.AccessController__PermissionIsNotValid(); - } - _setPermission(address(0), signer, to, func, permission); - emit PermissionSet(address(0), address(0), signer, to, func, permission); - } - /// @notice Sets the permission for a specific function call /// @dev Each policy is represented as a mapping from an IP account address to a signer address to a recipient /// address to a function selector to a permission level. The permission level can be 0 (ABSTAIN), 1 (ALLOW), or @@ -141,8 +123,8 @@ contract AccessController is IAccessController, AccessManagedUpgradeable, UUPSUp if (permission > 2) { revert Errors.AccessController__PermissionIsNotValid(); } - if (!IModuleRegistry($.moduleRegistry).isRegistered(msg.sender) && ipAccount != msg.sender) { - revert Errors.AccessController__CallerIsNotIPAccount(); + if (ipAccount != msg.sender && IIPAccount(payable(ipAccount)).owner() != msg.sender) { + revert Errors.AccessController__CallerIsNotIPAccountOrOwner(); } _setPermission(ipAccount, signer, to, func, permission); @@ -160,25 +142,27 @@ contract AccessController is IAccessController, AccessManagedUpgradeable, UUPSUp /// @param func The function selector of `to` that can be called by the `signer` on behalf of the `ipAccount` // solhint-disable code-complexity function checkPermission(address ipAccount, address signer, address to, bytes4 func) external view { - // The ipAccount is restricted to interact exclusively with registered modules. - // This includes initiating calls to these modules and receiving calls from them. - // Additionally, it can modify Permissions settings. AccessControllerStorage storage $ = _getAccessControllerStorage(); - if ( - to != address(this) && - !IModuleRegistry($.moduleRegistry).isRegistered(to) && - !IModuleRegistry($.moduleRegistry).isRegistered(signer) - ) { - revert Errors.AccessController__BothCallerAndRecipientAreNotRegisteredModule(signer, to); - } // Must be a valid IPAccount if (!IIPAccountRegistry($.ipAccountRegistry).isIpAccount(ipAccount)) { revert Errors.AccessController__IPAccountIsNotValid(ipAccount); } - // Owner can call all functions of all modules + // Owner can call any contracts either registered module or unregistered/external contracts if (IIPAccount(payable(ipAccount)).owner() == signer) { return; } + + // If the caller (signer) is not the Owner, IPAccount is limited to interactions with only registered modules. + // These interactions can be either initiating calls to these modules or receiving calls from them. + // The IP account can also modify its own Permissions settings. + if ( + to != address(this) && + !IModuleRegistry($.moduleRegistry).isRegistered(to) && + !IModuleRegistry($.moduleRegistry).isRegistered(signer) + ) { + revert Errors.AccessController__BothCallerAndRecipientAreNotRegisteredModule(signer, to); + } + uint functionPermission = getPermission(ipAccount, signer, to, func); // Specific function permission overrides wildcard/general permission if (functionPermission == AccessPermission.ALLOW) { @@ -194,9 +178,6 @@ contract AccessController is IAccessController, AccessManagedUpgradeable, UUPSUp } // If module level permission is ABSTAIN, check transaction signer level permission if (modulePermission == AccessPermission.ABSTAIN) { - if (getPermission(address(0), signer, to, func) == AccessPermission.ALLOW) { - return; - } // Pass if the ipAccount allow the signer can call all functions of all modules // Otherwise, revert if (getPermission(ipAccount, signer, address(0), bytes4(0)) == AccessPermission.ALLOW) { @@ -233,9 +214,6 @@ contract AccessController is IAccessController, AccessManagedUpgradeable, UUPSUp address to, bytes4 func ) internal view returns (bytes32) { - if (ipAccount == address(0)) { - return keccak256(abi.encode(address(0), address(0), signer, to, func)); - } return keccak256(abi.encode(IIPAccount(payable(ipAccount)).owner(), ipAccount, signer, to, func)); } diff --git a/contracts/interfaces/access/IAccessController.sol b/contracts/interfaces/access/IAccessController.sol index 3bed3c7d..d0a64216 100644 --- a/contracts/interfaces/access/IAccessController.sol +++ b/contracts/interfaces/access/IAccessController.sol @@ -24,14 +24,6 @@ interface IAccessController { /// @param permissions An array of `Permission` structs, each representing the permission to be set. function setBatchPermissions(AccessPermission.Permission[] memory permissions) external; - /// @notice Sets the permission for all IPAccounts - /// @dev Enforced to be only callable by the protocol admin in governance. - /// @param signer The address that can call `to` on behalf of the IP account - /// @param to The address that can be called by the `signer` (currently only modules can be `to`) - /// @param func The function selector of `to` that can be called by the `signer` on behalf of the `ipAccount` - /// @param permission The new permission level - function setGlobalPermission(address signer, address to, bytes4 func, uint8 permission) external; - /// @notice Sets the permission for a specific function call /// @dev Each policy is represented as a mapping from an IP account address to a signer address to a recipient /// address to a function selector to a permission level. The permission level can be 0 (ABSTAIN), 1 (ALLOW), or diff --git a/contracts/lib/Errors.sol b/contracts/lib/Errors.sol index cb559878..538f50ad 100644 --- a/contracts/lib/Errors.sol +++ b/contracts/lib/Errors.sol @@ -332,7 +332,7 @@ library Errors { error AccessController__IPAccountIsZeroAddress(); error AccessController__IPAccountIsNotValid(address ipAccount); error AccessController__SignerIsZeroAddress(); - error AccessController__CallerIsNotIPAccount(); + error AccessController__CallerIsNotIPAccountOrOwner(); error AccessController__PermissionIsNotValid(); error AccessController__BothCallerAndRecipientAreNotRegisteredModule(address signer, address to); error AccessController__PermissionDenied(address ipAccount, address signer, address to, bytes4 func); diff --git a/test/foundry/access/AccessControlled.t.sol b/test/foundry/access/AccessControlled.t.sol index 2e5fc575..144f086d 100644 --- a/test/foundry/access/AccessControlled.t.sol +++ b/test/foundry/access/AccessControlled.t.sol @@ -163,14 +163,23 @@ contract AccessControlledTest is BaseTest { address(moduleRegistry), "NonRegisteredMockAccessControlledModule" ); + address signer = vm.addr(2); + vm.prank(owner); + accessController.setPermission( + address(ipAccount), + signer, + address(nonRegisteredModule), + nonRegisteredModule.ipAccountOrPermissionFunction.selector, + AccessPermission.ALLOW + ); vm.expectRevert( abi.encodeWithSelector( Errors.AccessController__BothCallerAndRecipientAreNotRegisteredModule.selector, - owner, + signer, address(nonRegisteredModule) ) ); - vm.prank(owner); + vm.prank(signer); nonRegisteredModule.ipAccountOrPermissionFunction(address(ipAccount), "test", true); } diff --git a/test/foundry/access/AccessController.t.sol b/test/foundry/access/AccessController.t.sol index 25fb6301..75e37308 100644 --- a/test/foundry/access/AccessController.t.sol +++ b/test/foundry/access/AccessController.t.sol @@ -138,8 +138,7 @@ contract AccessControllerTest is BaseTest { AccessPermission.ALLOW ); - vm.prank(owner); // not calling from ipAccount - vm.expectRevert(Errors.AccessController__CallerIsNotIPAccount.selector); + vm.prank(owner); // directly call by owner accessController.setPermission( address(ipAccount), signer, @@ -864,71 +863,6 @@ contract AccessControllerTest is BaseTest { mockOrchestratorModule.workflowFailure(payable(address(ipAccount))); } - function test_AccessController_OrchestratorModuleWithGlobalPermission() public { - vm.startPrank(u.admin); - MockOrchestratorModule mockOrchestratorModule = new MockOrchestratorModule( - address(ipAccountRegistry), - address(moduleRegistry) - ); - moduleRegistry.registerModule("MockOrchestratorModule", address(mockOrchestratorModule)); - - MockModule module1WithPermission = new MockModule( - address(ipAccountRegistry), - address(moduleRegistry), - "Module1WithPermission" - ); - moduleRegistry.registerModule("Module1WithPermission", address(module1WithPermission)); - - MockModule module2WithPermission = new MockModule( - address(ipAccountRegistry), - address(moduleRegistry), - "Module2WithPermission" - ); - moduleRegistry.registerModule("Module2WithPermission", address(module2WithPermission)); - vm.stopPrank(); - - vm.prank(u.admin); - accessController.setGlobalPermission( - address(mockOrchestratorModule), - address(module1WithPermission), - mockModule.executeSuccessfully.selector, - AccessPermission.ALLOW - ); - - vm.prank(u.admin); - accessController.setGlobalPermission( - address(mockOrchestratorModule), - address(module2WithPermission), - mockModule.executeNoReturn.selector, - AccessPermission.ALLOW - ); - - vm.prank(owner); - mockOrchestratorModule.workflowPass(payable(address(ipAccount))); - } - - function test_AccessController_revert_setGlobalPermissionWithInvalidPermission() public { - vm.prank(u.admin); - vm.expectRevert(Errors.AccessController__PermissionIsNotValid.selector); - accessController.setGlobalPermission( - address(mockModule), - address(mockModule), - mockModule.executeNoReturn.selector, - 3 - ); - } - - function test_AccessController_revert_setGlobalPermissionWithZeroSignerAddress() public { - vm.prank(u.admin); - vm.expectRevert(abi.encodeWithSelector(Errors.AccessController__SignerIsZeroAddress.selector)); - accessController.setGlobalPermission( - address(0), - address(mockModule), - mockModule.executeNoReturn.selector, - AccessPermission.ALLOW - ); - } - function test_AccessController_ipAccountOwnerSetBatchPermissions() public { address signer = vm.addr(2); @@ -1237,7 +1171,7 @@ contract AccessControllerTest is BaseTest { permission: AccessPermission.ALLOW }); - vm.expectRevert(Errors.AccessController__CallerIsNotIPAccount.selector); + vm.expectRevert(Errors.AccessController__CallerIsNotIPAccountOrOwner.selector); accessController.setBatchPermissions(permissionList); } @@ -1596,4 +1530,105 @@ contract AccessControllerTest is BaseTest { tokenWithdrawalModule.withdrawERC20(payable(address(ipAccount)), address(mock20), 1e18); assertEq(mock20.balanceOf(owner), 1e18); } + + function test_AccessController_OwnerSetPermission() public { + address signer = vm.addr(2); + vm.prank(owner); + accessController.setPermission( + address(ipAccount), + signer, + address(mockModule), + mockModule.executeSuccessfully.selector, + AccessPermission.ALLOW + ); + assertEq( + accessController.getPermission( + address(ipAccount), + signer, + address(mockModule), + mockModule.executeSuccessfully.selector + ), + AccessPermission.ALLOW + ); + } + + function test_AccessController_OwnerSetPermissionBatch() public { + address signer = vm.addr(2); + AccessPermission.Permission[] memory permissionList = new AccessPermission.Permission[](3); + permissionList[0] = AccessPermission.Permission({ + ipAccount: address(ipAccount), + signer: signer, + to: address(mockModule), + func: mockModule.executeSuccessfully.selector, + permission: AccessPermission.ALLOW + }); + permissionList[1] = AccessPermission.Permission({ + ipAccount: address(ipAccount), + signer: signer, + to: address(mockModule), + func: mockModule.executeNoReturn.selector, + permission: AccessPermission.DENY + }); + permissionList[2] = AccessPermission.Permission({ + ipAccount: address(ipAccount), + signer: signer, + to: address(mockModule), + func: mockModule.executeRevert.selector, + permission: AccessPermission.ALLOW + }); + + vm.prank(owner); + accessController.setBatchPermissions(permissionList); + assertEq( + accessController.getPermission( + address(ipAccount), + signer, + address(mockModule), + mockModule.executeSuccessfully.selector + ), + AccessPermission.ALLOW + ); + + assertEq( + accessController.getPermission( + address(ipAccount), + signer, + address(mockModule), + mockModule.executeNoReturn.selector + ), + AccessPermission.DENY + ); + + assertEq( + accessController.getPermission( + address(ipAccount), + signer, + address(mockModule), + mockModule.executeRevert.selector + ), + AccessPermission.ALLOW + ); + } + + function test_AccessController_OwnerCallExternalContracts() public { + vm.startPrank(owner); + bytes memory result = ipAccount.execute( + address(mockNFT), + 0, + abi.encodeWithSignature("mint(address)", address(owner)) + ); + assertEq(abi.decode(result, (uint256)), 1); + + ipAccount.execute( + address(mockNFT), + 0, + abi.encodeWithSignature("transferFrom(address,address,uint256)", address(owner), address(ipAccount), 1) + ); + result = ipAccount.execute( + address(mockNFT), + 0, + abi.encodeWithSignature("balanceOf(address)", address(ipAccount)) + ); + assertEq(abi.decode(result, (uint256)), 1); + } }