Refactor restriction mechanism in AccessManager to enable enforce executionDelay (#4518)

Co-authored-by: Francisco Giordano <fg@frang.io>

.github/workflows/checks.yml

@@ -42,6 +42,7 @@ jobs:
         run: npm run test:generation
       - name: Compare gas costs
         uses: ./.github/actions/gas-compare
+        if: github.base_ref == 'master'
         with:
           token: ${{ github.token }}
 
@@ -63,6 +64,7 @@ jobs:
         run: npm run test:inheritance
       - name: Check storage layout
         uses: ./.github/actions/storage-layout
+        if: github.base_ref == 'master'
         with:
           token: ${{ github.token }}
 

contracts/access/manager/AccessManager.sol

@@ -7,6 +7,7 @@ import {IAccessManaged} from "./IAccessManaged.sol";
 import {Address} from "../../utils/Address.sol";
 import {Context} from "../../utils/Context.sol";
 import {Multicall} from "../../utils/Multicall.sol";
+import {Math} from "../../utils/math/Math.sol";
 import {Time} from "../../utils/types/Time.sol";
 
 /**
@@ -95,20 +96,11 @@ contract AccessManager is Context, Multicall, IAccessManager {
     bytes32 private _relayIdentifier;
 
     /**
-     * @dev Check that the caller has a given permission level (`groupId`). Note that this does NOT consider execution
-     * delays that may be associated to that group.
+     * @dev Check that the caller is authorized to perform the operation, following the restrictions encoded in
+     * {_getAdminRestrictions}.
      */
-    modifier onlyGroup(uint64 groupId) {
-        _checkGroup(groupId);
-        _;
-    }
-
-    /**
-     * @dev Check that the caller is an admin and that the top-level function currently executing has been scheduled
-     * sufficiently ahead of time, if necessary according to admin delays.
-     */
-    modifier withFamilyDelay(uint64 familyId) {
-        _checkFamilyDelay(familyId);
+    modifier onlyAuthorized() {
+        _checkAuthorized();
         _;
     }
 
@@ -145,7 +137,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
         } else {
             uint64 groupId = getFamilyFunctionGroup(familyId, selector);
             (bool inGroup, uint32 currentDelay) = hasGroup(groupId, caller);
-            return (inGroup && currentDelay == 0, currentDelay);
+            return inGroup ? (currentDelay == 0, currentDelay) : (false, 0);
         }
     }
 
@@ -250,7 +242,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupLabel} event.
      */
-    function labelGroup(uint64 groupId, string calldata label) public virtual onlyGroup(ADMIN_GROUP) {
+    function labelGroup(uint64 groupId, string calldata label) public virtual onlyAuthorized {
         if (groupId == ADMIN_GROUP || groupId == PUBLIC_GROUP) {
             revert AccessManagerLockedGroup(groupId);
         }
@@ -270,11 +262,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupGranted} event
      */
-    function grantGroup(
-        uint64 groupId,
-        address account,
-        uint32 executionDelay
-    ) public virtual onlyGroup(getGroupAdmin(groupId)) {
+    function grantGroup(uint64 groupId, address account, uint32 executionDelay) public virtual onlyAuthorized {
         _grantGroup(groupId, account, getGroupGrantDelay(groupId), executionDelay);
     }
 
@@ -287,7 +275,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupRevoked} event
      */
-    function revokeGroup(uint64 groupId, address account) public virtual onlyGroup(getGroupAdmin(groupId)) {
+    function revokeGroup(uint64 groupId, address account) public virtual onlyAuthorized {
         _revokeGroup(groupId, account);
     }
 
@@ -319,11 +307,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupExecutionDelayUpdated} event
      */
-    function setExecuteDelay(
-        uint64 groupId,
-        address account,
-        uint32 newDelay
-    ) public virtual onlyGroup(getGroupAdmin(groupId)) {
+    function setExecuteDelay(uint64 groupId, address account, uint32 newDelay) public virtual onlyAuthorized {
         _setExecuteDelay(groupId, account, newDelay);
     }
 
@@ -336,7 +320,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupAdminChanged} event
      */
-    function setGroupAdmin(uint64 groupId, uint64 admin) public virtual onlyGroup(ADMIN_GROUP) {
+    function setGroupAdmin(uint64 groupId, uint64 admin) public virtual onlyAuthorized {
         _setGroupAdmin(groupId, admin);
     }
 
@@ -349,7 +333,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupGuardianChanged} event
      */
-    function setGroupGuardian(uint64 groupId, uint64 guardian) public virtual onlyGroup(ADMIN_GROUP) {
+    function setGroupGuardian(uint64 groupId, uint64 guardian) public virtual onlyAuthorized {
         _setGroupGuardian(groupId, guardian);
     }
 
@@ -362,7 +346,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {GroupGrantDelayChanged} event
      */
-    function setGrantDelay(uint64 groupId, uint32 newDelay) public virtual onlyGroup(ADMIN_GROUP) {
+    function setGrantDelay(uint64 groupId, uint32 newDelay) public virtual onlyAuthorized {
         _setGrantDelay(groupId, newDelay);
     }
 
@@ -482,7 +466,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
         uint64 familyId,
         bytes4[] calldata selectors,
         uint64 groupId
-    ) public virtual onlyGroup(ADMIN_GROUP) withFamilyDelay(familyId) {
+    ) public virtual onlyAuthorized {
         for (uint256 i = 0; i < selectors.length; ++i) {
             _setFamilyFunctionGroup(familyId, selectors[i], groupId);
         }
@@ -508,7 +492,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {FunctionAllowedGroupUpdated} event per selector
      */
-    function setFamilyAdminDelay(uint64 familyId, uint32 newDelay) public virtual onlyGroup(ADMIN_GROUP) {
+    function setFamilyAdminDelay(uint64 familyId, uint32 newDelay) public virtual onlyAuthorized {
         _setFamilyAdminDelay(familyId, newDelay);
     }
 
@@ -544,10 +528,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {ContractFamilyUpdated} event.
      */
-    function setContractFamily(
-        address target,
-        uint64 familyId
-    ) public virtual onlyGroup(ADMIN_GROUP) withFamilyDelay(_getContractFamilyId(target)) {
+    function setContractFamily(address target, uint64 familyId) public virtual onlyAuthorized {
         _setContractFamily(target, familyId);
     }
 
@@ -570,7 +551,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits a {ContractClosed} event.
      */
-    function setContractClosed(address target, bool closed) public virtual onlyGroup(ADMIN_GROUP) {
+    function setContractClosed(address target, bool closed) public virtual onlyAuthorized {
         _setContractClosed(target, closed);
     }
 
@@ -636,6 +617,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * Emits an {OperationExecuted} event only if the call was scheduled and delayed.
      */
+    // Reentrancy is not an issue because permissions are checked on msg.sender. Additionally,
+    // _consumeScheduledOp guarantees a scheduled operation is only executed once.
+    // slither-disable-next-line reentrancy-no-eth
     function relay(address target, bytes calldata data) public payable virtual {
         address caller = _msgSender();
 
@@ -716,11 +700,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
             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 group.
+            (uint64 familyId, ) = getContractFamily(target);
             (bool isAdmin, ) = hasGroup(ADMIN_GROUP, msgsender);
-            (bool isGuardian, ) = hasGroup(
-                getGroupGuardian(getFamilyFunctionGroup(_getContractFamilyId(target), selector)),
-                msgsender
-            );
+            (bool isGuardian, ) = hasGroup(getGroupGuardian(getFamilyFunctionGroup(familyId, selector)), msgsender);
             if (!isAdmin && !isGuardian) {
                 revert AccessManagerCannotCancel(msgsender, caller, target, selector);
             }
@@ -753,56 +735,96 @@ contract AccessManager is Context, Multicall, IAccessManager {
      *
      * - the caller must be a global admin
      */
-    function updateAuthority(
-        address target,
-        address newAuthority
-    ) public virtual onlyGroup(ADMIN_GROUP) withFamilyDelay(_getContractFamilyId(target)) {
+    function updateAuthority(address target, address newAuthority) public virtual onlyAuthorized {
         IAccessManaged(target).setAuthority(newAuthority);
     }
 
-    // =================================================== HELPERS ====================================================
-    function _checkGroup(uint64 groupId) internal view virtual {
-        address account = _msgSender();
-        (bool inGroup, ) = hasGroup(groupId, account);
-        if (!inGroup) {
-            revert AccessManagerUnauthorizedAccount(account, groupId);
-        }
-    }
-
-    function _checkFamilyDelay(uint64 familyId) internal virtual {
-        uint32 delay = getFamilyAdminDelay(familyId);
-        if (delay > 0) {
-            _consumeScheduledOp(_hashOperation(_msgSender(), address(this), _msgData()));
+    // ================================================= ADMIN LOGIC ==================================================
+    /**
+     * @dev Check if the current call is authorized according to admin logic.
+     */
+    function _checkAuthorized() private {
+        address caller = _msgSender();
+        (bool allowed, uint32 delay) = _canCallExtended(caller, address(this), _msgData());
+        if (!allowed) {
+            if (delay == 0) {
+                (, uint64 requiredGroup, ) = _getAdminRestrictions(_msgData());
+                revert AccessManagerUnauthorizedAccount(caller, requiredGroup);
+            } else {
+                _consumeScheduledOp(_hashOperation(caller, address(this), _msgData()));
+            }
         }
     }
 
-    function _getContractFamilyId(address target) private view returns (uint64 familyId) {
-        (familyId, ) = getContractFamily(target);
-    }
-
-    function _parseFamilyOperation(bytes calldata data) private view returns (bool, uint64) {
+    /**
+     * @dev Get the admin restrictions of a given function call based on the function and arguments involved.
+     */
+    function _getAdminRestrictions(bytes calldata data) private view returns (bool, uint64, uint32) {
         bytes4 selector = bytes4(data);
+
         if (selector == this.updateAuthority.selector || selector == this.setContractFamily.selector) {
-            return (true, _getContractFamilyId(abi.decode(data[0x04:0x24], (address))));
+            // First argument is a target. Restricted to ADMIN with the family delay corresponding to the target's family
+            address target = abi.decode(data[0x04:0x24], (address));
+            (uint64 familyId, ) = getContractFamily(target);
+            uint32 delay = getFamilyAdminDelay(familyId);
+            return (true, ADMIN_GROUP, delay);
         } else if (selector == this.setFamilyFunctionGroup.selector) {
-            return (true, abi.decode(data[0x04:0x24], (uint64)));
+            // First argument is a family. Restricted to ADMIN with the family delay corresponding to the family
+            uint64 familyId = abi.decode(data[0x04:0x24], (uint64));
+            uint32 delay = getFamilyAdminDelay(familyId);
+            return (true, ADMIN_GROUP, delay);
+        } else if (
+            selector == this.labelGroup.selector ||
+            selector == this.setGroupAdmin.selector ||
+            selector == this.setGroupGuardian.selector ||
+            selector == this.setGrantDelay.selector ||
+            selector == this.setFamilyAdminDelay.selector ||
+            selector == this.setContractClosed.selector
+        ) {
+            // Restricted to ADMIN with no delay beside any execution delay the caller may have
+            return (true, ADMIN_GROUP, 0);
+        } else if (
+            selector == this.grantGroup.selector ||
+            selector == this.revokeGroup.selector ||
+            selector == this.setExecuteDelay.selector
+        ) {
+            // First argument is a groupId. Restricted to that group's admin with no delay beside any execution delay the caller may have.
+            uint64 groupId = abi.decode(data[0x04:0x24], (uint64));
+            uint64 groupAdminId = getGroupAdmin(groupId);
+            return (true, groupAdminId, 0);
         } else {
-            return (false, 0);
+            return (false, 0, 0);
         }
     }
 
+    // =================================================== HELPERS ====================================================
+    /**
+     * @dev An extended version of {canCall} for internal use that considers restrictions for admin functions.
+     */
     function _canCallExtended(address caller, address target, bytes calldata data) private view returns (bool, uint32) {
         if (target == address(this)) {
-            (bool isFamilyOperation, uint64 familyId) = _parseFamilyOperation(data);
-            uint32 delay = getFamilyAdminDelay(familyId);
-            (bool inGroup, ) = hasGroup(ADMIN_GROUP, caller);
-            return (inGroup && isFamilyOperation && delay == 0, delay);
+            (bool enabled, uint64 groupId, uint32 operationDelay) = _getAdminRestrictions(data);
+            if (!enabled) {
+                return (false, 0);
+            }
+
+            (bool inGroup, uint32 executionDelay) = hasGroup(groupId, caller);
+            if (!inGroup) {
+                return (false, 0);
+            }
+
+            // downcast is safe because both options are uint32
+            uint32 delay = uint32(Math.max(operationDelay, executionDelay));
+            return (delay == 0, delay);
         } else {
             bytes4 selector = bytes4(data);
             return canCall(caller, target, selector);
         }
     }
 
+    /**
+     * @dev Returns true if a schedule timepoint is past its expiration deadline.
+     */
     function _isExpired(uint48 timepoint) private view returns (bool) {
         return timepoint + expiration() <= Time.timestamp();
     }