Extend onlyAuthorized to support extra functions in AccessManager

.changeset/light-news-listen.md

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

contracts/access/manager/AccessManager.sol

@@ -586,14 +586,15 @@ 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();
-        (bool immediate, uint32 delay) = _canCallSelf(caller, _msgData());
+        (bool immediate, uint64 requiredRole, uint32 delay) = _canCallSelf(caller, _msgData());
         if (!immediate) {
             if (delay == 0) {
-                (, uint64 requiredRole, ) = _getAdminRestrictions(_msgData());
                 revert AccessManagerUnauthorizedAccount(caller, requiredRole);
             } else {
                 _consumeScheduledOp(hashOperation(caller, address(this), _msgData()));
@@ -666,39 +667,49 @@ contract AccessManager is Context, Multicall, IAccessManager {
         bytes calldata data
     ) private view returns (bool immediate, uint32 delay) {
         if (target == address(this)) {
-            return _canCallSelf(caller, data);
+            (immediate, , delay) = _canCallSelf(caller, data);
+            return (immediate, delay);
         } else {
             return data.length < 4 ? (false, 0) : canCall(caller, target, _checkSelector(data));
         }
     }
 
     /**
-     * @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) {
+    function _canCallSelf(
+        address caller,
+        bytes calldata data
+    ) private view returns (bool immediate, uint64 requiredRole, uint32 delay) {
         if (data.length < 4) {
-            return (false, 0);
+            return (false, 0, 0);
         }
 
+        bytes4 selector = _checkSelector(data);
+
+        (bool isAdminRestriction, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
+
         if (caller == address(this)) {
             // Caller is AccessManager, this means the call was sent through {execute} and it already checked
             // permissions. We verify that the call "identifier", which is set during {execute}, is correct.
-            return (_isExecuting(address(this), _checkSelector(data)), 0);
+            return (_isExecuting(address(this), selector), roleId, 0);
         }
 
-        (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data);
-        if (!enabled) {
-            return (false, 0);
+        if (!isAdminRestriction) {
+            if (isTargetClosed(address(this))) {
+                return (false, roleId, 0);
+            }
+            roleId = getTargetFunctionRole(address(this), selector);
         }
 
         (bool inRole, uint32 executionDelay) = hasRole(roleId, caller);
         if (!inRole) {
-            return (false, 0);
+            return (false, roleId, 0);
         }
 
         // downcast is safe because both options are uint32
         delay = uint32(Math.max(operationDelay, executionDelay));
-        return (delay == 0, delay);
+        return (delay == 0, roleId, delay);
     }
 
     /**

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,62 @@ function shouldBehaveLikeAManagedRestrictedOperation() {
   });
 }
 
+/**
+ * @requires this.{manager,roles,calldata,role}
+ */
+function shouldBehaveLikeASelfRestrictedOperation() {
+  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 = function () {
+    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, 0); // There's no way to know the required role of a function executed by the manager since the selector will be that of "execute"
+    });
+  };
+
+  testAsRestrictedOperation({
+    callerIsTheManager: isExecutingPath,
+    callerIsNotTheManager() {
+      testAsHasRole({
+        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)) {
@@ -1670,6 +1671,60 @@ describe('AccessManager', function () {
     });
   });
 
+  describe('access managed self operations', function () {
+    describe('when calling a restricted target function', function () {
+      beforeEach('set required role', function () {
+        this.method = this.target.fnRestricted.getFragment();
+        this.role = { id: 785913n };
+        this.manager.$_setTargetFunctionRole(this.target, this.method.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;
+        });
+
+        shouldBehaveLikeASelfRestrictedOperation();
+      });
+
+      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,
+          }),
+        )
+          .to.emit(this.target, '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.target,
+          this.target[method].getFragment().selector,
+          this.role.id,
+        );
+      });
+
+      it('succeeds called by anyone', async function () {
+        await expect(
+          this.target.connect(this.user)[method]({
+            data: this.calldata,
+          }),
+        )
+          .to.emit(this.target, 'CalledUnrestricted')
+          .withArgs(this.user);
+      });
+    });
+  });
+
   describe('access managed target operations', function () {
     describe('when calling a restricted target function', function () {
       beforeEach('set required role', function () {