Extend onlyAuthorized to support extra functions in AccessManager (#5014)

Co-authored-by: Hadrien Croubois <[email protected]>

Author: ernestognw

.changeset/light-news-listen.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`AccessManager`: Allow the `onlyAuthorized` modifier to restrict functions added to the manager.

certora/diff/access_manager_AccessManager.sol.patch

@@ -64,8 +64,8 @@
       */
      function _getAdminRestrictions(
          bytes calldata data
--    ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) {
-+    ) internal view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { // private → internal for FV
+-    ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) {
++    ) internal view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) { // private → internal for FV
          if (data.length < 4) {
              return (false, 0, 0);
          }

contracts/access/manager/AccessManager.sol

@@ -412,9 +412,6 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * Emits a {TargetClosed} event.
      */
     function _setTargetClosed(address target, bool closed) internal virtual {
-        if (target == address(this)) {
-            revert AccessManagerLockedAccount(target);
-        }
         _targets[target].closed = closed;
         emit TargetClosed(target, closed);
     }
@@ -586,7 +583,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
 
     // ================================================= ADMIN LOGIC ==================================================
     /**
-     * @dev Check if the current call is authorized according to admin logic.
+     * @dev Check if the current call is authorized according to admin and roles logic.
+     *
+     * WARNING: Carefully review the considerations of {AccessManaged-restricted} since they apply to this modifier.
      */
     function _checkAuthorized() private {
         address caller = _msgSender();
@@ -611,7 +610,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      */
     function _getAdminRestrictions(
         bytes calldata data
-    ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) {
+    ) private view returns (bool adminRestricted, uint64 roleAdminId, uint32 executionDelay) {
         if (data.length < 4) {
             return (false, 0, 0);
         }
@@ -648,7 +647,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
             return (true, getRoleAdmin(roleId), 0);
         }
 
-        return (false, 0, 0);
+        return (false, getTargetFunctionRole(address(this), selector), 0);
     }
 
     // =================================================== HELPERS ====================================================
@@ -673,7 +672,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
     }
 
     /**
-     * @dev A version of {canCall} that checks for admin restrictions in this contract.
+     * @dev A version of {canCall} that checks for restrictions in this contract.
      */
     function _canCallSelf(address caller, bytes calldata data) private view returns (bool immediate, uint32 delay) {
         if (data.length < 4) {
@@ -686,8 +685,10 @@ contract AccessManager is Context, Multicall, IAccessManager {
             return (_isExecuting(address(this), _checkSelector(data)), 0);
         }
 
-        (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
-        if (!enabled) {
+        (bool adminRestricted, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
+
+        // isTragetClosed apply to non-admin-restricted function
+        if (!adminRestricted && isTargetClosed(address(this))) {
             return (false, 0);
         }
 

contracts/access/manager/IAccessManager.sol

@@ -82,7 +82,6 @@ interface IAccessManager {
     error AccessManagerNotScheduled(bytes32 operationId);
     error AccessManagerNotReady(bytes32 operationId);
     error AccessManagerExpired(bytes32 operationId);
-    error AccessManagerLockedAccount(address account);
     error AccessManagerLockedRole(uint64 roleId);
     error AccessManagerBadConfirmation();
     error AccessManagerUnauthorizedAccount(address msgsender, uint64 roleId);
@@ -108,7 +107,7 @@ interface IAccessManager {
      * is backward compatible. Some contracts may thus ignore the second return argument. In that case they will fail
      * to identify the indirect workflow, and will consider calls that require a delay to be forbidden.
      *
-     * NOTE: This function does not report the permissions of this manager itself. These are defined by the
+     * NOTE: This function does not report the permissions of the admin functions in the manager itself. These are defined by the
      * {AccessManager} documentation.
      */
     function canCall(
@@ -134,6 +133,8 @@ interface IAccessManager {
 
     /**
      * @dev Get whether the contract is closed disabling any access. Otherwise role permissions are applied.
+     *
+     * NOTE: When the manager itself is closed, admin functions are still accessible to avoid locking the contract.
      */
     function isTargetClosed(address target) external view returns (bool);
 
@@ -308,6 +309,8 @@ interface IAccessManager {
     /**
      * @dev Set the closed flag for a contract.
      *
+     * Closing the manager itself won't disable access to admin methods to avoid locking the contract.
+     *
      * Requirements:
      *
      * - the caller must be a global admin

contracts/mocks/AccessManagerMock.sol

@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: MIT
+
+pragma solidity ^0.8.20;
+
+import {AccessManager} from "../access/manager/AccessManager.sol";
+import {StorageSlot} from "../utils/StorageSlot.sol";
+
+contract AccessManagerMock is AccessManager {
+    event CalledRestricted(address caller);
+    event CalledUnrestricted(address caller);
+
+    constructor(address initialAdmin) AccessManager(initialAdmin) {}
+
+    function fnRestricted() public onlyAuthorized {
+        emit CalledRestricted(msg.sender);
+    }
+
+    function fnUnrestricted() public {
+        emit CalledUnrestricted(msg.sender);
+    }
+}

test/access/manager/AccessManager.behavior.js

@@ -193,9 +193,65 @@ function shouldBehaveLikeAManagedRestrictedOperation() {
   });
 }
 
+/**
+ * @requires this.{target,manager,roles,calldata,role}
+ */
+function shouldBehaveLikeASelfRestrictedOperation() {
+  function revertUnauthorized() {
+    it('reverts as AccessManagerUnauthorizedAccount', async function () {
+      await expect(this.caller.sendTransaction({ to: this.target, data: this.calldata }))
+        .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedAccount')
+        .withArgs(this.caller, this.role?.id ?? 0n);
+    });
+  }
+
+  const getAccessPath = LIKE_COMMON_GET_ACCESS;
+
+  function testScheduleOperation(mineDelay) {
+    return function self() {
+      self.mineDelay = mineDelay;
+      beforeEach('sets execution delay', async function () {
+        this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation
+      });
+      testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE);
+    };
+  }
+
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay =
+    testScheduleOperation(true);
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = testScheduleOperation(false);
+
+  beforeEach('set target as manager', function () {
+    this.target = this.manager;
+  });
+
+  const isExecutingPath = LIKE_COMMON_IS_EXECUTING;
+  isExecutingPath.notExecuting = revertUnauthorized;
+
+  testAsCanCall({
+    closed: revertUnauthorized,
+    open: {
+      callerIsTheManager: isExecutingPath,
+      callerIsNotTheManager: {
+        publicRoleIsRequired() {
+          it('succeeds called directly', async function () {
+            await this.caller.sendTransaction({ to: this.target, data: this.calldata });
+          });
+
+          it('succeeds via execute', async function () {
+            await this.manager.connect(this.caller).execute(this.target, this.calldata);
+          });
+        },
+        specificRoleIsRequired: getAccessPath,
+      },
+    },
+  });
+}
+
 module.exports = {
   shouldBehaveLikeDelayedAdminOperation,
   shouldBehaveLikeNotDelayedAdminOperation,
   shouldBehaveLikeRoleAdminOperation,
   shouldBehaveLikeAManagedRestrictedOperation,
+  shouldBehaveLikeASelfRestrictedOperation,
 };

test/access/manager/AccessManager.test.js

@@ -23,6 +23,7 @@ const {
   shouldBehaveLikeNotDelayedAdminOperation,
   shouldBehaveLikeRoleAdminOperation,
   shouldBehaveLikeAManagedRestrictedOperation,
+  shouldBehaveLikeASelfRestrictedOperation,
 } = require('./AccessManager.behavior');
 
 const {
@@ -48,7 +49,7 @@ async function fixture() {
   roles.SOME.members = [member];
   roles.PUBLIC.members = [admin, roleAdmin, roleGuardian, member, user, other];
 
-  const manager = await ethers.deployContract('$AccessManager', [admin]);
+  const manager = await ethers.deployContract('$AccessManagerMock', [admin]);
   const target = await ethers.deployContract('$AccessManagedTarget', [manager]);
 
   for (const { id: roleId, admin, guardian, members } of Object.values(roles)) {
@@ -1105,10 +1106,18 @@ describe('AccessManager', function () {
           expect(await this.manager.isTargetClosed(this.target)).to.be.false;
         });
 
-        it('reverts if closing the manager', async function () {
-          await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, true))
-            .to.be.revertedWithCustomError(this.manager, 'AccessManagerLockedAccount')
-            .withArgs(this.manager);
+        describe('when the target is the manager', async function () {
+          it('closes and opens the manager', async function () {
+            await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, true))
+              .to.emit(this.manager, 'TargetClosed')
+              .withArgs(this.manager, true);
+            expect(await this.manager.isTargetClosed(this.manager)).to.be.true;
+
+            await expect(this.manager.connect(this.admin).setTargetClosed(this.manager, false))
+              .to.emit(this.manager, 'TargetClosed')
+              .withArgs(this.manager, false);
+            expect(await this.manager.isTargetClosed(this.manager)).to.be.false;
+          });
         });
       });
 
@@ -1670,18 +1679,74 @@ describe('AccessManager', function () {
     });
   });
 
+  describe('access managed self operations', function () {
+    describe('when calling a restricted target function', function () {
+      const method = 'fnRestricted()';
+
+      beforeEach('set required role', async function () {
+        this.role = { id: 785913n };
+        await this.manager.$_setTargetFunctionRole(
+          this.manager,
+          this.manager[method].getFragment().selector,
+          this.role.id,
+        );
+      });
+
+      describe('restrictions', function () {
+        beforeEach('set method and args', function () {
+          this.caller = this.user;
+          this.calldata = this.manager.interface.encodeFunctionData(method, []);
+        });
+
+        shouldBehaveLikeASelfRestrictedOperation();
+      });
+
+      it('succeeds called by a role member', async function () {
+        await this.manager.$_grantRole(this.role.id, this.user, 0, 0);
+
+        await expect(this.manager.connect(this.user)[method]())
+          .to.emit(this.manager, 'CalledRestricted')
+          .withArgs(this.user);
+      });
+    });
+
+    describe('when calling a non-restricted target function', function () {
+      const method = 'fnUnrestricted()';
+
+      beforeEach('set required role', async function () {
+        this.role = { id: 879435n };
+        await this.manager.$_setTargetFunctionRole(
+          this.manager,
+          this.manager[method].getFragment().selector,
+          this.role.id,
+        );
+      });
+
+      it('succeeds called by anyone', async function () {
+        await expect(this.manager.connect(this.user)[method]())
+          .to.emit(this.manager, 'CalledUnrestricted')
+          .withArgs(this.user);
+      });
+    });
+  });
+
   describe('access managed target operations', function () {
     describe('when calling a restricted target function', function () {
-      beforeEach('set required role', function () {
-        this.method = this.target.fnRestricted.getFragment();
+      const method = 'fnRestricted()';
+
+      beforeEach('set required role', async function () {
         this.role = { id: 3597243n };
-        this.manager.$_setTargetFunctionRole(this.target, this.method.selector, this.role.id);
+        await this.manager.$_setTargetFunctionRole(
+          this.target,
+          this.target[method].getFragment().selector,
+          this.role.id,
+        );
       });
 
       describe('restrictions', function () {
         beforeEach('set method and args', function () {
-          this.calldata = this.target.interface.encodeFunctionData(this.method, []);
           this.caller = this.user;
+          this.calldata = this.target.interface.encodeFunctionData(method, []);
         });
 
         shouldBehaveLikeAManagedRestrictedOperation();
@@ -1690,11 +1755,7 @@ describe('AccessManager', function () {
       it('succeeds called by a role member', async function () {
         await this.manager.$_grantRole(this.role.id, this.user, 0, 0);
 
-        await expect(
-          this.target.connect(this.user)[this.method.selector]({
-            data: this.calldata,
-          }),
-        )
+        await expect(this.target.connect(this.user)[method]())
           .to.emit(this.target, 'CalledRestricted')
           .withArgs(this.user);
       });
@@ -1713,11 +1774,7 @@ describe('AccessManager', function () {
       });
 
       it('succeeds called by anyone', async function () {
-        await expect(
-          this.target.connect(this.user)[method]({
-            data: this.calldata,
-          }),
-        )
+        await expect(this.target.connect(this.user)[method]())
           .to.emit(this.target, 'CalledUnrestricted')
           .withArgs(this.user);
       });