Skip to content

Commit

Permalink
Enhanced Access Control: Removal of Global Permissions, Direct Permis…
Browse files Browse the repository at this point in the history 
…sion Setting, and External Contract Calls by IP Owners (#89)

* initial impl

* support owner set permission directly

* add more comments
  • Loading branch information
@kingster-will
kingster-will authored Apr 14, 2024
1 parent dc40dae commit 99a23ba
Show file tree
Hide file tree
Showing 5 changed files with 130 additions and 116 deletions.
52 changes: 15 additions & 37 deletions contracts/access/AccessController.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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);

Expand All @@ -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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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));
}

Expand Down
8 changes: 0 additions & 8 deletions contracts/interfaces/access/IAccessController.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion contracts/lib/Errors.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
13 changes: 11 additions & 2 deletions test/foundry/access/AccessControlled.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
171 changes: 103 additions & 68 deletions test/foundry/access/AccessController.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
}

0 comments on commit 99a23ba

Please sign in to comment.