Add a _canCancel function to improve granularity and allow roleAdminId to cancel the grant and revoke functions (#6573)

Co-authored-by: James Toussaint <33313130+james-toussaint@users.noreply.github.com>
Co-authored-by: ernestognw <ernestognw@gmail.com>

Author: 3 people

.changeset/famous-roses-clean.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': minor
+---
+
+`AccessManager`: Allow a role admin to cancel grant and revoke operations.

contracts/access/manager/AccessManager.sol

@@ -541,13 +541,8 @@ contract AccessManager is Context, Multicall, IAccessManager {
         bytes32 operationId = hashOperation(caller, target, data);
         if (_schedules[operationId].timepoint == 0) {
             revert AccessManagerNotScheduled(operationId);
-        } else if (caller != msgsender) {
-            // calls can only be canceled by the account that scheduled them, a global admin, or by a guardian of the required role.
-            (bool isAdmin, ) = hasRole(ADMIN_ROLE, msgsender);
-            (bool isGuardian, ) = hasRole(getRoleGuardian(getTargetFunctionRole(target, selector)), msgsender);
-            if (!isAdmin && !isGuardian) {
-                revert AccessManagerUnauthorizedCancel(msgsender, caller, target, selector);
-            }
+        } else if (!_canCancel(caller, target, data)) {
+            revert AccessManagerUnauthorizedCancel(msgsender, caller, target, selector);
         }
 
         delete _schedules[operationId].timepoint; // reset the timepoint, keep the nonce
@@ -721,6 +716,37 @@ contract AccessManager is Context, Multicall, IAccessManager {
         return (delay == 0, delay);
     }
 
+    /**
+     * @dev Returns true if a scheduled operation can be canceled by the caller.
+     */
+    function _canCancel(address caller, address target, bytes calldata data) internal view virtual returns (bool) {
+        address msgsender = _msgSender();
+
+        // caller can cancel if they are the msg.sender of the scheduled operation
+        if (caller == msgsender) {
+            return true;
+        }
+
+        // admins can cancel any operation, and guardians of the target function's role can cancel it
+        (bool isAdmin, ) = hasRole(ADMIN_ROLE, msgsender);
+        (bool isGuardian, ) = hasRole(getRoleGuardian(getTargetFunctionRole(target, _checkSelector(data))), msgsender);
+        if (isAdmin || isGuardian) {
+            return true;
+        }
+
+        // if the target is this AccessManager and the call matches an admin-restricted function, allow members
+        // of the admin role returned by _getAdminRestrictions to cancel. ADMIN_ROLE was already checked above.
+        if (target == address(this)) {
+            (bool adminRestricted, uint64 roleId, ) = _getAdminRestrictions(data);
+            if (adminRestricted && roleId != ADMIN_ROLE) {
+                (bool inRole, ) = hasRole(roleId, msgsender);
+                return inRole;
+            }
+        }
+
+        return false;
+    }
+
     /**
      * @dev Returns true if a call with `target` and `selector` is being executed via {executed}.
      */

test/access/manager/AccessManager.test.js

@@ -2392,6 +2392,48 @@ describe('AccessManager', function () {
             });
           });
 
+          [
+            { name: 'grant', method: 'grantRole(uint64,address,uint32)' },
+            { name: 'revoke', method: 'revokeRole(uint64,address)' },
+          ].forEach(({ name, method }) => {
+            describe(`when caller is a role admin (${name})`, function () {
+              beforeEach('reschedule as a role admin call targeting the manager', async function () {
+                this.method = this.manager.interface.getFunction(method);
+                this.caller = this.roles.SOME_ADMIN.members[0];
+                await this.manager.$_grantRole(this.roles.SOME_ADMIN.id, this.caller, 0, 1); // nonzero execution delay
+                await this.manager.$_grantRole(this.roles.SOME_ADMIN.id, this.other, 0, 1); // nonzero execution delay
+                this.calldata = this.manager.interface.encodeFunctionData(
+                  this.method,
+                  name == 'grant'
+                    ? [this.roles.SOME.id, ethers.ZeroAddress, 0]
+                    : [this.roles.SOME.id, ethers.ZeroAddress],
+                );
+                const { operationId, schedule } = await prepareOperation(this.manager, {
+                  caller: this.caller,
+                  target: this.manager,
+                  calldata: this.calldata,
+                  delay: this.scheduleIn,
+                });
+                this.operationId = operationId;
+                await schedule();
+              });
+
+              it('another member of the role admin succeeds', async function () {
+                await expect(this.manager.connect(this.other).cancel(this.caller, this.manager, this.calldata))
+                  .to.emit(this.manager, 'OperationCanceled')
+                  .withArgs(this.operationId, 1n);
+                expect(await this.manager.getSchedule(this.operationId)).to.equal('0');
+              });
+
+              it('a member of the granted role but not its admin reverts', async function () {
+                const roleMember = this.roles.SOME.members[0];
+                await expect(this.manager.connect(roleMember).cancel(this.caller, this.manager, this.calldata))
+                  .to.be.revertedWithCustomError(this.manager, 'AccessManagerUnauthorizedCancel')
+                  .withArgs(roleMember, this.caller, this.manager, this.method.selector);
+              });
+            });
+          });
+
           describe('when caller is any other account', function () {
             it('reverts as AccessManagerUnauthorizedCancel', async function () {
               await expect(this.manager.connect(this.other).cancel(this.caller, this.target, this.calldata))