From 66aa6e95928e7a18d3dcc694cdc7ecbe4e564778 Mon Sep 17 00:00:00 2001 From: Yash Patil Date: Fri, 22 Nov 2024 16:31:03 -0500 Subject: [PATCH] feat: admin 2-step --- .../interfaces/IPermissionController.sol | 50 +++++- .../permissions/PermissionController.sol | 56 +++++- .../PermissionControllerStorage.sol | 2 + src/test/unit/PermissionControllerUnit.t.sol | 167 ++++++++++++++---- 4 files changed, 232 insertions(+), 43 deletions(-) diff --git a/src/contracts/interfaces/IPermissionController.sol b/src/contracts/interfaces/IPermissionController.sol index 3ad7ce3d8..5d4be806d 100644 --- a/src/contracts/interfaces/IPermissionController.sol +++ b/src/contracts/interfaces/IPermissionController.sol @@ -4,8 +4,6 @@ pragma solidity ^0.8.27; interface IPermissionControllerErrors { /// @notice Thrown when the caller is not the admin error NotAdmin(); - /// @notice Thrown when an admin is set to the zero address - error AdminAlreadySet(); /// @notice Thrown when the admin to remove is not an admin error AdminNotSet(); /// @notice Thrown when an appointee is already set for the account's function @@ -14,6 +12,12 @@ interface IPermissionControllerErrors { error AppointeeNotSet(); /// @notice Thrown when the account attempts to remove the only admin error CannotHaveZeroAdmins(); + /// @notice Thrown when an admin is already set + error AdminAlreadySet(); + /// @notice Thrown when an admin is not pending + error AdminNotPending(); + /// @notice Thrown when an admin is already pending + error AdminAlreadyPending(); } interface IPermissionControllerEvents { @@ -23,6 +27,12 @@ interface IPermissionControllerEvents { /// @notice Emitted when an appointee is revoked event AppointeeRemoved(address indexed account, address indexed appointee, address target, bytes4 selector); + /// @notice Emitted when an admin is set as pending for an account + event PendingAdminAdded(address indexed account, address admin); + + /// @notice Emitted when an admin is removed as pending for an account + event PendingAdminRemoved(address indexed account, address admin); + /// @notice Emitted when an admin is set for a given account event AdminSet(address indexed account, address admin); @@ -32,12 +42,29 @@ interface IPermissionControllerEvents { interface IPermissionController is IPermissionControllerErrors, IPermissionControllerEvents { /** - * @notice Set the admin of an account - * @param account to set admin for + * @notice Sets a pending admin of an account + * @param account to set pending admin for * @param admin to set * @dev Multiple admins can be set for an account */ - function addAdmin(address account, address admin) external; + function addPendingAdmin(address account, address admin) external; + + /** + * @notice Removes a pending admin of an account + * @param account to remove pending admin for + * @param admin to remove + * @dev Only the admin of the account can remove a pending admin + */ + function removePendingAdmin(address account, address admin) external; + + /** + * @notice Accepts the admin role of an account + * @param account to accept admin for + * @dev Only a pending admin for the account can become an admin + */ + function acceptAdmin( + address account + ) external; /** * @notice Remove an admin of an account @@ -74,6 +101,11 @@ interface IPermissionController is IPermissionControllerErrors, IPermissionContr */ function isAdmin(address account, address caller) external view returns (bool); + /** + * @notice Checks if the `pendingAdmin` is a pending admin of the `account` + */ + function isPendingAdmin(address account, address pendingAdmin) external view returns (bool); + /** * @notice Get the admins of an account * @param account The account to get the admin of @@ -83,6 +115,14 @@ interface IPermissionController is IPermissionControllerErrors, IPermissionContr address account ) external view returns (address[] memory); + /** + * @notice Get the pending admins of an account + * @param account The account to get the pending admin of + */ + function getPendingAdmins( + address account + ) external view returns (address[] memory); + /** * @notice Checks if the given caller has permissions to call the fucntion * @param account to check diff --git a/src/contracts/permissions/PermissionController.sol b/src/contracts/permissions/PermissionController.sol index b433d91cc..c927c05f7 100644 --- a/src/contracts/permissions/PermissionController.sol +++ b/src/contracts/permissions/PermissionController.sol @@ -32,13 +32,45 @@ contract PermissionController is Initializable, PermissionControllerStorage { */ /// @inheritdoc IPermissionController - function addAdmin(address account, address admin) external onlyAdmin(account) { + function addPendingAdmin(address account, address admin) external onlyAdmin(account) { + AccountPermissions storage permissions = _permissions[account]; + + // Revert if the admin is already set + require(!permissions.admins.contains(admin), AdminAlreadySet()); + + // Add the admin to the account's pending admins + // If the admin is already pending, the add will fail + require(permissions.pendingAdmins.add(admin), AdminAlreadyPending()); + + emit PendingAdminAdded(account, admin); + } + + /// @inheritdoc IPermissionController + function removePendingAdmin(address account, address admin) external onlyAdmin(account) { + EnumerableSet.AddressSet storage pendingAdmins = _permissions[account].pendingAdmins; + + // Remove the admin from the account's pending admins + // Revert if the admin is not pending + require(pendingAdmins.remove(admin), AdminNotPending()); + + emit PendingAdminRemoved(account, admin); + } + + /// @inheritdoc IPermissionController + function acceptAdmin( + address account + ) external { + AccountPermissions storage permissions = _permissions[account]; + + // Remove the admin from the pending list + // Revert if the admin is not pending + require(permissions.pendingAdmins.remove(msg.sender), AdminNotPending()); + // Add the admin to the account's admins - // If the admin is already set, the set will fail - EnumerableSet.AddressSet storage admins = _permissions[account].admins; - require(admins.add(admin), AdminAlreadySet()); + // Not wrapped in a require since it must be the case the admin is not one + permissions.admins.add(msg.sender); - emit AdminSet(account, admin); + emit AdminSet(account, msg.sender); } /// @inheritdoc IPermissionController @@ -110,7 +142,7 @@ contract PermissionController is Initializable, PermissionControllerStorage { } /// @dev Decodes the target and selector from a single bytes32 value - /// @dev Encoded Format: [160 bits target][32 bits selector][64 bits padding], + /// @dev Encoded Format: [160 bits target][32 bits selector][64 bits padding], function _decodeTargetSelector( bytes32 targetSelector ) internal pure returns (address, bytes4) { @@ -139,6 +171,11 @@ contract PermissionController is Initializable, PermissionControllerStorage { } } + /// @inheritdoc IPermissionController + function isPendingAdmin(address account, address pendingAdmin) external view returns (bool) { + return _permissions[account].pendingAdmins.contains(pendingAdmin); + } + /// @inheritdoc IPermissionController function getAdmins( address account @@ -152,6 +189,13 @@ contract PermissionController is Initializable, PermissionControllerStorage { } } + /// @inheritdoc IPermissionController + function getPendingAdmins( + address account + ) external view returns (address[] memory) { + return _permissions[account].pendingAdmins.values(); + } + /// @inheritdoc IPermissionController function canCall(address account, address caller, address target, bytes4 selector) external view returns (bool) { return isAdmin(account, caller) diff --git a/src/contracts/permissions/PermissionControllerStorage.sol b/src/contracts/permissions/PermissionControllerStorage.sol index 9ee91bade..771987c78 100644 --- a/src/contracts/permissions/PermissionControllerStorage.sol +++ b/src/contracts/permissions/PermissionControllerStorage.sol @@ -10,6 +10,8 @@ abstract contract PermissionControllerStorage is IPermissionController { using EnumerableSet for EnumerableSet.AddressSet; struct AccountPermissions { + /// @notice The pending admins of the account + EnumerableSet.AddressSet pendingAdmins; /// @notice The admins of the account EnumerableSet.AddressSet admins; /// @notice Mapping from an appointee to the list of encoded target & selectors diff --git a/src/test/unit/PermissionControllerUnit.t.sol b/src/test/unit/PermissionControllerUnit.t.sol index ca9fa7a0f..43088a4a3 100644 --- a/src/test/unit/PermissionControllerUnit.t.sol +++ b/src/test/unit/PermissionControllerUnit.t.sol @@ -30,13 +30,7 @@ contract PermissionControllerUnitTests is EigenLayerUnitTestSetup, IPermissionCo } } -contract PermissionControllerUnitTests_SetAdmin is PermissionControllerUnitTests { - modifier initializeAdmin() { - cheats.prank(account); - permissionController.addAdmin(account, admin); - _; - } - +contract PermissionControllerUnitTests_AddPendingAdmin is PermissionControllerUnitTests { function testFuzz_getAdmin_notSet(address account) public view filterFuzzedAddressInputs(account) { address[] memory admins = permissionController.getAdmins(account); assertEq(admins[0], account, "Account is not admin"); @@ -46,49 +40,148 @@ contract PermissionControllerUnitTests_SetAdmin is PermissionControllerUnitTests /// @notice Tests the setAdmin function when it has not been initialized function test_revert_caller_not_account() public { cheats.expectRevert(IPermissionControllerErrors.NotAdmin.selector); - permissionController.addAdmin(account, admin); + permissionController.addPendingAdmin(account, admin); } - function test_setAdmin_initialSet() public { + function test_revert_adminPending() public { + // Set admin to be pending + cheats.startPrank(account); + permissionController.addPendingAdmin(account, admin); + + // Expect revert + cheats.expectRevert(IPermissionControllerErrors.AdminAlreadyPending.selector); + permissionController.addPendingAdmin(account, admin); + } + + function test_addPendingAdmin_initialSet() public { // Expect emit cheats.expectEmit(true, true, true, true); - emit AdminSet(account, admin); + emit PendingAdminAdded(account, admin); cheats.prank(account); - permissionController.addAdmin(account, admin); + permissionController.addPendingAdmin(account, admin); // Check storage - address[] memory admins = permissionController.getAdmins(account); - assertEq(admins[0], admin, "Admin not set correctly"); - assertTrue(permissionController.isAdmin(account, admin), "Admin not set correctly"); - assertTrue(permissionController.canCall(account, admin, address(0), bytes4(0)), "Admin cannot call"); + address[] memory pendingAdmins = permissionController.getPendingAdmins(account); + assertEq(pendingAdmins[0], admin, "Admin not set to pending"); + assertTrue(permissionController.isPendingAdmin(account, admin), "Pending admin not set correctly"); + assertFalse(permissionController.isAdmin(account, admin), "Pending admin not set correctly"); } +} - function test_revert_invalidAdmin_alreadySet() public initializeAdmin { - cheats.prank(admin); - cheats.expectRevert(IPermissionControllerErrors.AdminAlreadySet.selector); - permissionController.addAdmin(account, admin); - } +contract PermissionControllerUnitTests_RemovePendingAdmin is PermissionControllerUnitTests { + function setUp() virtual public override { + // Setup + PermissionControllerUnitTests.setUp(); - function test_revert_caller_not_admin() public initializeAdmin { + // Set admin to be pending cheats.prank(account); + permissionController.addPendingAdmin(account, admin); + } + + function test_revert_notAdmin() public { cheats.expectRevert(IPermissionControllerErrors.NotAdmin.selector); - permissionController.addAdmin(account, admin2); + permissionController.removePendingAdmin(account, admin); + } + + function test_revert_notPending() public { + // Expect revert + cheats.prank(account); + cheats.expectRevert(IPermissionControllerErrors.AdminNotPending.selector); + permissionController.removePendingAdmin(account, admin2); + } + + function test_removePendingAdmin() public { + // Expect emit + cheats.expectEmit(true, true, true, true); + emit PendingAdminRemoved(account, admin); + + cheats.prank(account); + permissionController.removePendingAdmin(account, admin); + + // Check storage + address[] memory pendingAdmins = permissionController.getPendingAdmins(account); + assertEq(pendingAdmins.length, 0, "Pending admin not removed"); + } +} + +contract PermissionControllerUnitTests_AcceptAdmin is PermissionControllerUnitTests { + function setUp() virtual public override { + // Setup + PermissionControllerUnitTests.setUp(); + + // Set admin + cheats.prank(account); + permissionController.addPendingAdmin(account, admin); } - function test_addAdditionalAdmin() public initializeAdmin { + function test_revert_adminNotPending() public { + cheats.prank(admin2); + cheats.expectRevert(IPermissionControllerErrors.AdminNotPending.selector); + permissionController.acceptAdmin(account); + } + + function test_acceptAdmin() public { // Expect emit cheats.expectEmit(true, true, true, true); - emit AdminSet(account, admin2); + emit AdminSet(account, admin); + + cheats.prank(admin); + permissionController.acceptAdmin(account); + + // Check storage + address[] memory admins = permissionController.getAdmins(account); + address[] memory pendingAdmins = permissionController.getPendingAdmins(account); + assertEq(pendingAdmins.length, 0, "Pending admin not removed"); + assertEq(admins.length, 1, "Additional admin not added"); + assertFalse(permissionController.isAdmin(account, account), "Account should no longer be admin"); + assertTrue(permissionController.isAdmin(account, admin), "Admin not set"); + assertTrue(permissionController.canCall(account, admin, address(0), bytes4(0)), "Admin cannot call"); + } + + function test_addMultipleAdmin_fromAccount() public { + // Add another pending admin + cheats.prank(account); + permissionController.addPendingAdmin(account, admin2); + // Assert pending admins length + address[] memory pendingAdmins = permissionController.getPendingAdmins(account); + assertEq(pendingAdmins.length, 2, "Additional pending admin not added"); + + // Accept admins cheats.prank(admin); - permissionController.addAdmin(account, admin2); + permissionController.acceptAdmin(account); + cheats.prank(admin2); + permissionController.acceptAdmin(account); + + // Check storage + address[] memory admins = permissionController.getAdmins(account); + assertEq(admins.length, 2, "Additional admin not added"); + assertTrue(permissionController.isAdmin(account, admin), "Old admin should still be admin"); + assertTrue(permissionController.isAdmin(account, admin2), "New admin not set correctly"); + assertTrue(permissionController.canCall(account, admin, address(0), bytes4(0)), "Admin cannot call"); + assertTrue(permissionController.canCall(account, admin2, address(0), bytes4(0)), "Admin cannot call"); + } + + function test_addMultipleAdmin_newAdminAdds() public { + // Accept admin + cheats.startPrank(admin); + permissionController.acceptAdmin(account); + + // Add another pending admin + permissionController.addPendingAdmin(account, admin2); + cheats.stopPrank(); + + // Accept admins + cheats.prank(admin2); + permissionController.acceptAdmin(account); // Check storage address[] memory admins = permissionController.getAdmins(account); assertEq(admins.length, 2, "Additional admin not added"); assertTrue(permissionController.isAdmin(account, admin), "Old admin should still be admin"); assertTrue(permissionController.isAdmin(account, admin2), "New admin not set correctly"); + assertTrue(permissionController.canCall(account, admin, address(0), bytes4(0)), "Admin cannot call"); assertTrue(permissionController.canCall(account, admin2, address(0), bytes4(0)), "Admin cannot call"); } } @@ -99,10 +192,16 @@ contract PermissionControllerUnitTests_RemoveAdmin is PermissionControllerUnitTe PermissionControllerUnitTests.setUp(); // Set admins - cheats.prank(account); - permissionController.addAdmin(account, admin); + cheats.startPrank(account); + permissionController.addPendingAdmin(account, admin); + permissionController.addPendingAdmin(account, admin2); + cheats.stopPrank(); + + // Accept admins cheats.prank(admin); - permissionController.addAdmin(account, admin2); + permissionController.acceptAdmin(account); + cheats.prank(admin2); + permissionController.acceptAdmin(account); } function test_revert_notAdmin() public { @@ -152,9 +251,11 @@ contract PermissionControllerUnitTests_SetAppointee is PermissionControllerUnitT // Setup PermissionControllerUnitTests.setUp(); - // Set admin + // Set & accept admin cheats.prank(account); - permissionController.addAdmin(account, admin); + permissionController.addPendingAdmin(account, admin); + cheats.prank(admin); + permissionController.acceptAdmin(account); } function test_revert_notAdmin() public { @@ -235,9 +336,11 @@ contract PermissionControllerUnitTests_RemoveAppointee is PermissionControllerUn // Setup PermissionControllerUnitTests.setUp(); - // Set admin + // Set & accept admin cheats.prank(account); - permissionController.addAdmin(account, admin); + permissionController.addPendingAdmin(account, admin); + cheats.prank(admin); + permissionController.acceptAdmin(account); // Set appointees cheats.startPrank(admin);