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,7 +586,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 +613,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 +650,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 +675,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 +688,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/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 () {