Fix bypassing target admin delay using execute (#6388)

Co-authored-by: ernestognw <ernestognw@gmail.com>

Author: Amxx

.changeset/legal-cameras-sin.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`AccessManager`: treat `setAuthority` differently in `canCall` to prevent bypassing the `updateAuthority` security using an `execute`.

contracts/access/manager/AccessManager.sol

@@ -148,6 +148,11 @@ contract AccessManager is Context, Multicall, IAccessManager {
             // 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(target, selector), 0);
+        } else if (selector == IAccessManaged.setAuthority.selector) {
+            (bool isAdmin, uint32 executionDelay) = hasRole(ADMIN_ROLE, caller);
+            uint32 adminDelay = getTargetAdminDelay(target);
+            uint32 setAuthorityDelay = uint32(Math.max(executionDelay, adminDelay));
+            return isAdmin ? (setAuthorityDelay == 0, setAuthorityDelay) : (false, 0);
         } else {
             uint64 roleId = getTargetFunctionRole(target, selector);
             (bool isMember, uint32 currentDelay) = hasRole(roleId, caller);
@@ -388,6 +393,11 @@ contract AccessManager is Context, Multicall, IAccessManager {
      * Emits a {TargetFunctionRoleUpdated} event.
      */
     function _setTargetFunctionRole(address target, bytes4 selector, uint64 roleId) internal virtual {
+        if (selector == IAccessManaged.setAuthority.selector) {
+            // Prevent updating authority using an execute call, instead only allow it through updateAuthority to
+            // ensure the proper delay and admin restrictions are applied.
+            revert AccessManagerLockedFunction(selector);
+        }
         _targets[target].allowedRoles[selector] = roleId;
         emit TargetFunctionRoleUpdated(target, selector, roleId);
     }

contracts/access/manager/IAccessManager.sol

@@ -80,6 +80,7 @@ interface IAccessManager {
     error AccessManagerNotReady(bytes32 operationId);
     error AccessManagerExpired(bytes32 operationId);
     error AccessManagerLockedRole(uint64 roleId);
+    error AccessManagerLockedFunction(bytes4 selector);
     error AccessManagerBadConfirmation();
     error AccessManagerUnauthorizedAccount(address msgsender, uint64 roleId);
     error AccessManagerUnauthorizedCall(address caller, address target, bytes4 selector);

test/access/manager/AccessManager.behavior.js

@@ -57,20 +57,15 @@ function shouldBehaveLikeDelayedAdminOperation() {
  */
 function shouldBehaveLikeNotDelayedAdminOperation() {
   const getAccessPath = LIKE_COMMON_GET_ACCESS;
-
-  function testScheduleOperation(mineDelay) {
-    return function self() {
-      self.mineDelay = mineDelay;
-      beforeEach('set execution delay', async function () {
-        this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation
-      });
-      testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE);
-    };
-  }
-
+  testAsDelayedOperation.mineDelay = true;
   getAccessPath.requiredRoleIsGranted.roleGrantingIsDelayed.callerHasAnExecutionDelay.afterGrantDelay =
-    testScheduleOperation(true);
-  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = testScheduleOperation(false);
+    testAsDelayedOperation;
+  getAccessPath.requiredRoleIsGranted.roleGrantingIsNotDelayed.callerHasAnExecutionDelay = function () {
+    beforeEach('set execution delay', async function () {
+      this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation
+    });
+    testAsSchedulableOperation(LIKE_COMMON_SCHEDULABLE);
+  };
 
   beforeEach('set target as manager', function () {
     this.target = this.manager;

test/access/manager/AccessManager.predicate.js

@@ -275,7 +275,7 @@ function testAsDelayedOperation() {
     describe('when operation delay is greater than execution delay', function () {
       beforeEach('set operation delay', async function () {
         this.operationDelay = this.executionDelay + time.duration.hours(1);
-        await this.manager.$_setTargetAdminDelay(this.target, this.operationDelay);
+        await this.manager.$_setTargetAdminDelay(this.operationDelayTarget ?? this.target, this.operationDelay);
         this.scheduleIn = this.operationDelay; // For testAsSchedulableOperation
       });
 
@@ -285,7 +285,7 @@ function testAsDelayedOperation() {
     describe('when operation delay is shorter than execution delay', function () {
       beforeEach('set operation delay', async function () {
         this.operationDelay = this.executionDelay - time.duration.hours(1);
-        await this.manager.$_setTargetAdminDelay(this.target, this.operationDelay);
+        await this.manager.$_setTargetAdminDelay(this.operationDelayTarget ?? this.target, this.operationDelay);
         this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation
       });
 
@@ -296,7 +296,7 @@ function testAsDelayedOperation() {
   describe('without operation delay', function () {
     beforeEach('set operation delay', async function () {
       this.operationDelay = 0n;
-      await this.manager.$_setTargetAdminDelay(this.target, this.operationDelay);
+      await this.manager.$_setTargetAdminDelay(this.operationDelayTarget ?? this.target, this.operationDelay);
       this.scheduleIn = this.executionDelay; // For testAsSchedulableOperation
     });
 

test/access/manager/AccessManager.test.js

@@ -1063,6 +1063,7 @@ describe('AccessManager', function () {
 
         describe('restrictions', function () {
           beforeEach('set method and args', function () {
+            this.operationDelayTarget = this.newManagedTarget;
             this.calldata = this.manager.interface.encodeFunctionData('updateAuthority(address,address)', [
               this.newManagedTarget.target,
               this.newAuthority.target,
@@ -1086,6 +1087,7 @@ describe('AccessManager', function () {
       describe('#setTargetClosed', function () {
         describe('restrictions', function () {
           beforeEach('set method and args', function () {
+            this.operationDelayTarget = this.other;
             const args = [this.other.address, true];
             const method = this.manager.interface.getFunction('setTargetClosed(address,bool)');
             this.calldata = this.manager.interface.encodeFunctionData(method, args);
@@ -1124,6 +1126,7 @@ describe('AccessManager', function () {
       describe('#setTargetFunctionRole', function () {
         describe('restrictions', function () {
           beforeEach('set method and args', function () {
+            this.operationDelayTarget = this.other;
             const args = [this.other.address, ['0x12345678'], 443342];
             const method = this.manager.interface.getFunction('setTargetFunctionRole(address,bytes4[],uint64)');
             this.calldata = this.manager.interface.encodeFunctionData(method, args);
@@ -1162,6 +1165,15 @@ describe('AccessManager', function () {
             );
           }
         });
+
+        it('reverts setting a function role for the setAuthority selector', async function () {
+          const { selector } = this.target.interface.getFunction('setAuthority');
+          await expect(
+            this.manager.connect(this.admin).$_setTargetFunctionRole(this.target, selector, this.roles.ADMIN.id),
+          )
+            .to.be.revertedWithCustomError(this.manager, 'AccessManagerLockedFunction')
+            .withArgs(selector);
+        });
       });
 
       describe('role admin operations', function () {
@@ -2240,6 +2252,41 @@ describe('AccessManager', function () {
         .to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled')
         .withArgs(operationId);
     });
+
+    it('can execute a setAuthority call when no target admin delay is set', async function () {
+      const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]);
+
+      await expect(this.manager.getTargetAdminDelay(this.target)).to.eventually.equal(0n);
+
+      await expect(
+        this.manager
+          .connect(this.admin)
+          .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])),
+      )
+        .to.emit(this.target, 'AuthorityUpdated')
+        .withArgs(newAuthority);
+    });
+
+    it('cannot execute a setAuthority call when a target admin delay is set', async function () {
+      const newAuthority = await ethers.deployContract('$AccessManager', [this.admin]);
+
+      await this.manager.$_setTargetAdminDelay(this.target, 10n);
+      await time.increaseBy.timestamp(5n * 86400n, true); // minSetBack is 5 days
+
+      await expect(this.manager.getTargetAdminDelay(this.target)).to.eventually.equal(10n);
+
+      // cannot update directly - there is a delay
+      await expect(
+        this.manager.connect(this.admin).updateAuthority(this.target, newAuthority),
+      ).to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled');
+
+      // cannot bypass via execute either - targetAdminDelay is enforced for setAuthority
+      await expect(
+        this.manager
+          .connect(this.admin)
+          .execute(this.target, this.target.interface.encodeFunctionData('setAuthority', [newAuthority.target])),
+      ).to.be.revertedWithCustomError(this.manager, 'AccessManagerNotScheduled');
+    });
   });
 
   describe('#consumeScheduledOp', function () {