From 736091afc4c5f1cb88c4f766e713bbbbb70fa83e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 8 Aug 2023 01:21:46 +0200 Subject: [PATCH] Refactor restriction mechanism in AccessManager to enable enforce executionDelay (#4518) Co-authored-by: Francisco Giordano --- .github/workflows/checks.yml | 2 + contracts/access/manager/AccessManager.sol | 160 ++++++++++++--------- 2 files changed, 93 insertions(+), 69 deletions(-) diff --git a/.github/workflows/checks.yml b/.github/workflows/checks.yml index 122d39564..41719b142 100644 --- a/.github/workflows/checks.yml +++ b/.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 }} diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index d3b65b0e2..6ecad9921 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/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(); }