Enhanced Access Control: Removal of Global Permissions, Direct Permission Setting, and External Contract Calls by IP Owners

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);
 
@@ -164,21 +146,22 @@ contract AccessController is IAccessController, AccessManagedUpgradeable, UUPSUp
         // 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 (
+            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 +177,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 +213,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));
     }
 

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

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);

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);
     }
 

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);
+    }
 }