diff --git a/.gitignore b/.gitignore index 85198aaa..30a74b08 100644 --- a/.gitignore +++ b/.gitignore @@ -12,3 +12,4 @@ docs/ # Dotenv file .env +.gas-snapshot \ No newline at end of file diff --git a/src/access/AccessControl/AccessControlFacet.sol b/src/access/AccessControl/AccessControlFacet.sol index a80a6afa..7a34bc28 100644 --- a/src/access/AccessControl/AccessControlFacet.sol +++ b/src/access/AccessControl/AccessControlFacet.sol @@ -79,6 +79,24 @@ contract AccessControlFacet { return s.adminRole[_role]; } + /// @notice Sets the admin role for a role. + /// @param _role The role to set the admin for. + /// @param _adminRole The new admin role to set. + /// @dev Emits a {RoleAdminChanged} event. + /// @custom:error AccessControlUnauthorizedAccount If the caller is not the current admin of the role. + function setRoleAdmin(bytes32 _role, bytes32 _adminRole) external { + AccessControlStorage storage s = getStorage(); + bytes32 previousAdminRole = s.adminRole[_role]; + + // Check if the caller is the current admin of the role. + if (!s.hasRole[msg.sender][previousAdminRole]) { + revert AccessControlUnauthorizedAccount(msg.sender, previousAdminRole); + } + + s.adminRole[_role] = _adminRole; + emit RoleAdminChanged(_role, previousAdminRole, _adminRole); + } + /// @notice Grants a role to an account. /// @param _role The role to grant. /// @param _account The account to grant the role to. @@ -121,6 +139,56 @@ contract AccessControlFacet { } } + /// @notice Grants a role to multiple accounts in a single transaction. + /// @param _role The role to grant. + /// @param _accounts The accounts to grant the role to. + /// @dev Emits a {RoleGranted} event for each newly granted account. + /// @custom:error AccessControlUnauthorizedAccount If the caller is not the admin of the role. + function grantRoleBatch(bytes32 _role, address[] calldata _accounts) external { + AccessControlStorage storage s = getStorage(); + bytes32 adminRole = s.adminRole[_role]; + + // Check if the caller is the admin of the role. + if (!s.hasRole[msg.sender][adminRole]) { + revert AccessControlUnauthorizedAccount(msg.sender, adminRole); + } + + uint256 length = _accounts.length; + for (uint256 i = 0; i < length; i++) { + address account = _accounts[i]; + bool _hasRole = s.hasRole[account][_role]; + if (!_hasRole) { + s.hasRole[account][_role] = true; + emit RoleGranted(_role, account, msg.sender); + } + } + } + + /// @notice Revokes a role from multiple accounts in a single transaction. + /// @param _role The role to revoke. + /// @param _accounts The accounts to revoke the role from. + /// @dev Emits a {RoleRevoked} event for each account the role is revoked from. + /// @custom:error AccessControlUnauthorizedAccount If the caller is not the admin of the role. + function revokeRoleBatch(bytes32 _role, address[] calldata _accounts) external { + AccessControlStorage storage s = getStorage(); + bytes32 adminRole = s.adminRole[_role]; + + // Check if the caller is the admin of the role. + if (!s.hasRole[msg.sender][adminRole]) { + revert AccessControlUnauthorizedAccount(msg.sender, adminRole); + } + + uint256 length = _accounts.length; + for (uint256 i = 0; i < length; i++) { + address account = _accounts[i]; + bool _hasRole = s.hasRole[account][_role]; + if (_hasRole) { + s.hasRole[account][_role] = false; + emit RoleRevoked(_role, account, msg.sender); + } + } + } + /// @notice Renounces a role from the caller. /// @param _role The role to renounce. /// @param _account The account to renounce the role from. diff --git a/test/access/AccessControl/AccessControlFacet.t.sol b/test/access/AccessControl/AccessControlFacet.t.sol index eaddc131..f00075b5 100644 --- a/test/access/AccessControl/AccessControlFacet.t.sol +++ b/test/access/AccessControl/AccessControlFacet.t.sol @@ -120,6 +120,203 @@ contract AccessControlFacetTest is Test { assertEq(accessControl.getRoleAdmin(MINTER_ROLE), PAUSER_ROLE); } + // ============================================ + // setRoleAdmin Tests (external) + // ============================================ + + function test_SetRoleAdmin_SucceedsWhenCallerIsCurrentAdmin() public { + // DEFAULT_ADMIN_ROLE is current admin for new roles + vm.expectEmit(true, true, true, true); + emit RoleAdminChanged(MINTER_ROLE, DEFAULT_ADMIN_ROLE, PAUSER_ROLE); + + vm.prank(ADMIN); + accessControl.setRoleAdmin(MINTER_ROLE, PAUSER_ROLE); + + assertEq(accessControl.getRoleAdmin(MINTER_ROLE), PAUSER_ROLE); + } + + function test_RevertWhen_SetRoleAdmin_CallerIsNotCurrentAdmin() public { + // Set current admin to PAUSER_ROLE + accessControl.forceSetRoleAdmin(MINTER_ROLE, PAUSER_ROLE); + + // ADMIN has DEFAULT_ADMIN_ROLE but not PAUSER_ROLE + vm.expectRevert( + abi.encodeWithSelector(AccessControlFacet.AccessControlUnauthorizedAccount.selector, ADMIN, PAUSER_ROLE) + ); + vm.prank(ADMIN); + accessControl.setRoleAdmin(MINTER_ROLE, UPGRADER_ROLE); + } + + // ============================================ + // Batch Grant/Revoke Tests + // ============================================ + + function test_GrantRoleBatch_SucceedsAndEmitsPerNewGrant() public { + address[] memory accounts = new address[](3); + accounts[0] = ALICE; + accounts[1] = BOB; + accounts[2] = CHARLIE; + + vm.startPrank(ADMIN); + // Expect three RoleGranted events + vm.expectEmit(true, true, true, true); + emit RoleGranted(MINTER_ROLE, ALICE, ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleGranted(MINTER_ROLE, BOB, ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleGranted(MINTER_ROLE, CHARLIE, ADMIN); + accessControl.grantRoleBatch(MINTER_ROLE, accounts); + vm.stopPrank(); + + assertTrue(accessControl.hasRole(MINTER_ROLE, ALICE)); + assertTrue(accessControl.hasRole(MINTER_ROLE, BOB)); + assertTrue(accessControl.hasRole(MINTER_ROLE, CHARLIE)); + } + + function test_GrantRoleBatch_SkipsAlreadyGrantedWithoutExtraEvents() public { + // Pre-grant ALICE + vm.prank(ADMIN); + accessControl.grantRole(MINTER_ROLE, ALICE); + + address[] memory accounts = new address[](3); + accounts[0] = ALICE; // already granted + accounts[1] = BOB; + accounts[2] = CHARLIE; + + vm.startPrank(ADMIN); + // Expect only two events for new grants + vm.expectEmit(true, true, true, true); + emit RoleGranted(MINTER_ROLE, BOB, ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleGranted(MINTER_ROLE, CHARLIE, ADMIN); + accessControl.grantRoleBatch(MINTER_ROLE, accounts); + vm.stopPrank(); + + assertTrue(accessControl.hasRole(MINTER_ROLE, ALICE)); + assertTrue(accessControl.hasRole(MINTER_ROLE, BOB)); + assertTrue(accessControl.hasRole(MINTER_ROLE, CHARLIE)); + } + + function test_RevertWhen_GrantRoleBatch_CallerIsNotAdmin() public { + address[] memory accounts = new address[](2); + accounts[0] = ALICE; + accounts[1] = BOB; + + vm.expectRevert( + abi.encodeWithSelector( + AccessControlFacet.AccessControlUnauthorizedAccount.selector, ALICE, DEFAULT_ADMIN_ROLE + ) + ); + vm.prank(ALICE); + accessControl.grantRoleBatch(MINTER_ROLE, accounts); + } + + function test_RevokeRoleBatch_SucceedsAndEmitsPerRevocation() public { + // Setup grants + vm.startPrank(ADMIN); + accessControl.grantRole(MINTER_ROLE, ALICE); + accessControl.grantRole(MINTER_ROLE, BOB); + accessControl.grantRole(MINTER_ROLE, CHARLIE); + vm.stopPrank(); + + address[] memory accounts = new address[](3); + accounts[0] = ALICE; + accounts[1] = BOB; + accounts[2] = CHARLIE; + + vm.startPrank(ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleRevoked(MINTER_ROLE, ALICE, ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleRevoked(MINTER_ROLE, BOB, ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleRevoked(MINTER_ROLE, CHARLIE, ADMIN); + accessControl.revokeRoleBatch(MINTER_ROLE, accounts); + vm.stopPrank(); + + assertFalse(accessControl.hasRole(MINTER_ROLE, ALICE)); + assertFalse(accessControl.hasRole(MINTER_ROLE, BOB)); + assertFalse(accessControl.hasRole(MINTER_ROLE, CHARLIE)); + } + + function test_RevokeRoleBatch_SkipsNotGrantedWithoutExtraEvents() public { + // Only ALICE has the role + vm.prank(ADMIN); + accessControl.grantRole(MINTER_ROLE, ALICE); + + address[] memory accounts = new address[](3); + accounts[0] = ALICE; // granted + accounts[1] = BOB; // not granted + accounts[2] = CHARLIE; // not granted + + vm.startPrank(ADMIN); + vm.expectEmit(true, true, true, true); + emit RoleRevoked(MINTER_ROLE, ALICE, ADMIN); + accessControl.revokeRoleBatch(MINTER_ROLE, accounts); + vm.stopPrank(); + + assertFalse(accessControl.hasRole(MINTER_ROLE, ALICE)); + assertFalse(accessControl.hasRole(MINTER_ROLE, BOB)); + assertFalse(accessControl.hasRole(MINTER_ROLE, CHARLIE)); + } + + function test_RevertWhen_RevokeRoleBatch_CallerIsNotAdmin() public { + // Setup grants + vm.prank(ADMIN); + accessControl.grantRole(MINTER_ROLE, ALICE); + + address[] memory accounts = new address[](1); + accounts[0] = ALICE; + + vm.expectRevert( + abi.encodeWithSelector( + AccessControlFacet.AccessControlUnauthorizedAccount.selector, BOB, DEFAULT_ADMIN_ROLE + ) + ); + vm.prank(BOB); + accessControl.revokeRoleBatch(MINTER_ROLE, accounts); + } + + function test_GrantRoleBatch_SucceedsWithEmptyArray() public { + address[] memory accounts = new address[](0); + + // Should just succeed with no reverts and no events + vm.prank(ADMIN); + accessControl.grantRoleBatch(MINTER_ROLE, accounts); + } + + function test_RevokeRoleBatch_SucceedsWithEmptyArray() public { + address[] memory accounts = new address[](0); + + // Should just succeed with no reverts and no events + vm.prank(ADMIN); + accessControl.revokeRoleBatch(MINTER_ROLE, accounts); + } + + function test_DelegatedAdminCanExerciseAdminPowers() public { + // === Arrange === + vm.startPrank(ADMIN); + accessControl.grantRole(MINTER_ROLE, BOB); + accessControl.grantRole(PAUSER_ROLE, ALICE); + accessControl.setRoleAdmin(MINTER_ROLE, PAUSER_ROLE); + vm.stopPrank(); + + // Assert the setup is correct before acting + assertTrue(accessControl.hasRole(MINTER_ROLE, BOB)); + assertTrue(accessControl.hasRole(PAUSER_ROLE, ALICE)); + + // === Act === + // Expect the event to be emitted + vm.expectEmit(true, true, true, true); + emit RoleRevoked(MINTER_ROLE, BOB, ALICE); + + vm.prank(ALICE); + accessControl.revokeRole(MINTER_ROLE, BOB); + + // === Assert === + assertFalse(accessControl.hasRole(MINTER_ROLE, BOB)); + } + function test_GetRoleAdmin_DefaultAdminRoleAdminIsItself() public view { assertEq(accessControl.getRoleAdmin(DEFAULT_ADMIN_ROLE), DEFAULT_ADMIN_ROLE); }