From 983ba612eb0e8ddded6210efe348c614c198ced2 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 25 Sep 2023 17:21:23 -0500 Subject: [PATCH 01/10] Add named return parameters and _checkSelector function --- contracts/access/manager/AccessManager.sol | 47 ++++++++++++---------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 47dc47f739a..7a4aa500329 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -130,7 +130,7 @@ contract AccessManager is Context, Multicall, IAccessManager { * is backward compatible. Some contracts may thus ignore the second return argument. In that case they will fail * to identify the indirect workflow, and will consider calls that require a delay to be forbidden. */ - function canCall(address caller, address target, bytes4 selector) public view virtual returns (bool, uint32) { + function canCall(address caller, address target, bytes4 selector) public view virtual returns (bool immediate, uint32 delay) { if (isTargetClosed(target)) { return (false, 0); } else if (caller == address(this)) { @@ -139,8 +139,9 @@ contract AccessManager is Context, Multicall, IAccessManager { return (_isExecuting(target, selector), 0); } else { uint64 roleId = getTargetFunctionRole(target, selector); - (bool isMember, uint32 currentDelay) = hasRole(roleId, caller); - return isMember ? (currentDelay == 0, currentDelay) : (false, 0); + bool inRole; + (inRole, delay) = hasRole(roleId, caller); + return inRole ? (delay == 0, delay) : (false, 0); } } @@ -220,11 +221,11 @@ contract AccessManager is Context, Multicall, IAccessManager { * [2] Pending execution delay for the account. * [3] Timestamp at which the pending execution delay will become active. 0 means no delay update is scheduled. */ - function getAccess(uint64 roleId, address account) public view virtual returns (uint48, uint32, uint32, uint48) { + function getAccess(uint64 roleId, address account) public view virtual returns (uint48 since, uint32 currentDelay, uint32 pendingDelay, uint48 effect) { Access storage access = _roles[roleId].members[account]; - uint48 since = access.since; - (uint32 currentDelay, uint32 pendingDelay, uint48 effect) = access.delay.getFull(); + since = access.since; + (currentDelay, pendingDelay, effect) = access.delay.getFull(); return (since, currentDelay, pendingDelay, effect); } @@ -233,7 +234,7 @@ contract AccessManager is Context, Multicall, IAccessManager { * @dev Check if a given account currently had the permission level corresponding to a given role. Note that this * permission might be associated with a delay. {getAccess} can provide more details. */ - function hasRole(uint64 roleId, address account) public view virtual returns (bool, uint32) { + function hasRole(uint64 roleId, address account) public view virtual returns (bool isMember, uint32 executionDelay) { if (roleId == PUBLIC_ROLE) { return (true, 0); } else { @@ -439,10 +440,6 @@ contract AccessManager is Context, Multicall, IAccessManager { * Emits a {RoleGrantDelayChanged} event */ function _setGrantDelay(uint64 roleId, uint32 newDelay) internal virtual { - if (roleId == PUBLIC_ROLE) { - revert AccessManagerLockedRole(roleId); - } - uint48 effect; (_roles[roleId].grantDelay, effect) = _roles[roleId].grantDelay.withUpdate(newDelay, minSetback()); @@ -578,7 +575,7 @@ contract AccessManager is Context, Multicall, IAccessManager { // if call is not authorized, or if requested timing is too soon if ((!immediate && setback == 0) || (when > 0 && when < minWhen)) { - revert AccessManagerUnauthorizedCall(caller, target, bytes4(data[0:4])); + revert AccessManagerUnauthorizedCall(caller, target, _checkSelector(data)); } // Reuse variable due to stack too deep @@ -631,7 +628,7 @@ contract AccessManager is Context, Multicall, IAccessManager { // If caller is not authorised, revert if (!immediate && setback == 0) { - revert AccessManagerUnauthorizedCall(caller, target, bytes4(data)); + revert AccessManagerUnauthorizedCall(caller, target, _checkSelector(data)); } // If caller is authorised, check operation was scheduled early enough @@ -644,7 +641,7 @@ contract AccessManager is Context, Multicall, IAccessManager { // Mark the target and selector as authorised bytes32 executionIdBefore = _executionId; - _executionId = _hashExecutionId(target, bytes4(data)); + _executionId = _hashExecutionId(target, _checkSelector(data)); // Perform call Address.functionCallWithValue(target, data, msg.value); @@ -707,7 +704,7 @@ contract AccessManager is Context, Multicall, IAccessManager { */ function cancel(address caller, address target, bytes calldata data) public virtual returns (uint32) { address msgsender = _msgSender(); - bytes4 selector = bytes4(data[0:4]); + bytes4 selector = _checkSelector(data); bytes32 operationId = hashOperation(caller, target, data); if (_schedules[operationId].timepoint == 0) { @@ -779,8 +776,8 @@ contract AccessManager is Context, Multicall, IAccessManager { * - uint64: which role is this operation restricted to * - uint32: minimum delay to enforce for that operation (on top of the admin's execution delay) */ - function _getAdminRestrictions(bytes calldata data) private view returns (bool, uint64, uint32) { - bytes4 selector = bytes4(data); + function _getAdminRestrictions(bytes calldata data) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { + bytes4 selector = _checkSelector(data); if (data.length < 4) { return (false, 0, 0); @@ -813,8 +810,7 @@ contract AccessManager is Context, Multicall, IAccessManager { if (selector == this.grantRole.selector || selector == this.revokeRole.selector) { // First argument is a roleId. uint64 roleId = abi.decode(data[0x04:0x24], (uint64)); - uint64 roleAdminId = getRoleAdmin(roleId); - return (true, roleAdminId, 0); + return (true, getRoleAdmin(roleId), 0); } return (false, 0, 0); @@ -831,11 +827,11 @@ contract AccessManager is Context, Multicall, IAccessManager { * If immediate is true, the delay can be disregarded and the operation can be immediately executed. * If immediate is false, the operation can be executed if and only if delay is greater than 0. */ - function _canCallExtended(address caller, address target, bytes calldata data) private view returns (bool, uint32) { + function _canCallExtended(address caller, address target, bytes calldata data) private view returns (bool immediate, uint32 delay) { if (target == address(this)) { return _canCallSelf(caller, data); } else { - bytes4 selector = bytes4(data); + bytes4 selector = _checkSelector(data); return canCall(caller, target, selector); } } @@ -847,7 +843,7 @@ contract AccessManager is Context, Multicall, IAccessManager { if (caller == address(this)) { // 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(address(this), bytes4(data)), 0); + return (_isExecuting(address(this), _checkSelector(data)), 0); } (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data); @@ -878,4 +874,11 @@ contract AccessManager is Context, Multicall, IAccessManager { function _isExpired(uint48 timepoint) private view returns (bool) { return timepoint + expiration() <= Time.timestamp(); } + + /** + * @dev Extracts the selector from calldata. Panics if data is not at least 4 bytes + */ + function _checkSelector(bytes calldata data) private pure returns (bytes4) { + return bytes4(data[0:4]); + } } From b48e92528829cc0408a61c54302b922ac2254785 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Mon, 25 Sep 2023 17:29:02 -0500 Subject: [PATCH 02/10] Lint --- contracts/access/manager/AccessManager.sol | 28 +++++++++++++++++----- 1 file changed, 22 insertions(+), 6 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 7a4aa500329..a4db9200fe4 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -130,7 +130,11 @@ contract AccessManager is Context, Multicall, IAccessManager { * is backward compatible. Some contracts may thus ignore the second return argument. In that case they will fail * to identify the indirect workflow, and will consider calls that require a delay to be forbidden. */ - function canCall(address caller, address target, bytes4 selector) public view virtual returns (bool immediate, uint32 delay) { + function canCall( + address caller, + address target, + bytes4 selector + ) public view virtual returns (bool immediate, uint32 delay) { if (isTargetClosed(target)) { return (false, 0); } else if (caller == address(this)) { @@ -221,10 +225,13 @@ contract AccessManager is Context, Multicall, IAccessManager { * [2] Pending execution delay for the account. * [3] Timestamp at which the pending execution delay will become active. 0 means no delay update is scheduled. */ - function getAccess(uint64 roleId, address account) public view virtual returns (uint48 since, uint32 currentDelay, uint32 pendingDelay, uint48 effect) { + function getAccess( + uint64 roleId, + address account + ) public view virtual returns (uint48 since, uint32 currentDelay, uint32 pendingDelay, uint48 effect) { Access storage access = _roles[roleId].members[account]; - since = access.since; + since = access.since; (currentDelay, pendingDelay, effect) = access.delay.getFull(); return (since, currentDelay, pendingDelay, effect); @@ -234,7 +241,10 @@ contract AccessManager is Context, Multicall, IAccessManager { * @dev Check if a given account currently had the permission level corresponding to a given role. Note that this * permission might be associated with a delay. {getAccess} can provide more details. */ - function hasRole(uint64 roleId, address account) public view virtual returns (bool isMember, uint32 executionDelay) { + function hasRole( + uint64 roleId, + address account + ) public view virtual returns (bool isMember, uint32 executionDelay) { if (roleId == PUBLIC_ROLE) { return (true, 0); } else { @@ -776,7 +786,9 @@ contract AccessManager is Context, Multicall, IAccessManager { * - uint64: which role is this operation restricted to * - uint32: minimum delay to enforce for that operation (on top of the admin's execution delay) */ - function _getAdminRestrictions(bytes calldata data) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { + function _getAdminRestrictions( + bytes calldata data + ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { bytes4 selector = _checkSelector(data); if (data.length < 4) { @@ -827,7 +839,11 @@ contract AccessManager is Context, Multicall, IAccessManager { * If immediate is true, the delay can be disregarded and the operation can be immediately executed. * If immediate is false, the operation can be executed if and only if delay is greater than 0. */ - function _canCallExtended(address caller, address target, bytes calldata data) private view returns (bool immediate, uint32 delay) { + function _canCallExtended( + address caller, + address target, + bytes calldata data + ) private view returns (bool immediate, uint32 delay) { if (target == address(this)) { return _canCallSelf(caller, data); } else { From 280f29b21593b6b9b114131fa950fc281b1a458f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Tue, 26 Sep 2023 16:33:21 -0500 Subject: [PATCH 03/10] Avoid panicking in view functions --- contracts/access/manager/AccessManager.sol | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index a4db9200fe4..a195a1acdd2 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -789,12 +789,12 @@ contract AccessManager is Context, Multicall, IAccessManager { function _getAdminRestrictions( bytes calldata data ) private view returns (bool restricted, uint64 roleAdminId, uint32 executionDelay) { - bytes4 selector = _checkSelector(data); - if (data.length < 4) { return (false, 0, 0); } + bytes4 selector = _checkSelector(data); + // Restricted to ADMIN with no delay beside any execution delay the caller may have if ( selector == this.labelRole.selector || @@ -847,8 +847,7 @@ contract AccessManager is Context, Multicall, IAccessManager { if (target == address(this)) { return _canCallSelf(caller, data); } else { - bytes4 selector = _checkSelector(data); - return canCall(caller, target, selector); + return data.length < 4 ? (false, 0) : canCall(caller, target, _checkSelector(data)); } } @@ -856,6 +855,10 @@ contract AccessManager is Context, Multicall, IAccessManager { * @dev A version of {canCall} that checks for admin restrictions in this contract. */ function _canCallSelf(address caller, bytes calldata data) private view returns (bool immediate, uint32 delay) { + if (data.length < 4) { + return (false, 0); + } + if (caller == address(this)) { // 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. From 40692f24b5d0cebd652db4260810738a5fc3cace Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 27 Sep 2023 13:38:56 -0500 Subject: [PATCH 04/10] Revert _setGrantDelay PUBLIC_ROLE case --- contracts/access/manager/AccessManager.sol | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index a195a1acdd2..5c0b3234a90 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -450,6 +450,10 @@ contract AccessManager is Context, Multicall, IAccessManager { * Emits a {RoleGrantDelayChanged} event */ function _setGrantDelay(uint64 roleId, uint32 newDelay) internal virtual { + if (roleId == PUBLIC_ROLE) { + revert AccessManagerLockedRole(roleId); + } + uint48 effect; (_roles[roleId].grantDelay, effect) = _roles[roleId].grantDelay.withUpdate(newDelay, minSetback()); From 3dac6f48cf9646fefbada842c3f584bdc495460b Mon Sep 17 00:00:00 2001 From: ernestognw Date: Wed, 27 Sep 2023 14:02:08 -0500 Subject: [PATCH 05/10] Revert canCall hasRole path return --- contracts/access/manager/AccessManager.sol | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 5c0b3234a90..31ef86c3f11 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -143,9 +143,8 @@ contract AccessManager is Context, Multicall, IAccessManager { return (_isExecuting(target, selector), 0); } else { uint64 roleId = getTargetFunctionRole(target, selector); - bool inRole; - (inRole, delay) = hasRole(roleId, caller); - return inRole ? (delay == 0, delay) : (false, 0); + (bool isMember, uint32 currentDelay) = hasRole(roleId, caller); + return isMember ? (currentDelay == 0, currentDelay) : (false, 0); } } From 7e0fa74beee8c960353608ad6bdebc42d1e6327f Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 28 Sep 2023 12:49:07 -0500 Subject: [PATCH 06/10] Make AccessManaged _checkCanCall panic on short calldarta --- contracts/access/manager/AccessManaged.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index c207c5e5183..bcd9c3c4a73 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -99,14 +99,15 @@ abstract contract AccessManaged is Context, IAccessManaged { } /** - * @dev Reverts if the caller is not allowed to call the function identified by a selector. + * @dev Reverts if the caller is not allowed to call the function identified by a selector. Panics if the calldata + * is less than 4 bytes long. */ function _checkCanCall(address caller, bytes calldata data) internal virtual { (bool immediate, uint32 delay) = AuthorityUtils.canCallWithDelay( authority(), caller, address(this), - bytes4(data) + bytes4(data[0:4]) ); if (!immediate) { if (delay > 0) { From 89c50f900c318ad798e0fdffcfbdc1e589e94ff5 Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 28 Sep 2023 13:02:37 -0500 Subject: [PATCH 07/10] Ignore short calldatas in propose() --- .../extensions/GovernorTimelockAccess.sol | 3 +++ contracts/mocks/AccessManagedTarget.sol | 5 +++++ .../extensions/GovernorTimelockAccess.test.js | 15 ++++++++++++++- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/contracts/governance/extensions/GovernorTimelockAccess.sol b/contracts/governance/extensions/GovernorTimelockAccess.sol index 08e295c743b..0c6dbeab217 100644 --- a/contracts/governance/extensions/GovernorTimelockAccess.sol +++ b/contracts/governance/extensions/GovernorTimelockAccess.sol @@ -191,6 +191,9 @@ abstract contract GovernorTimelockAccess is Governor { plan.length = SafeCast.toUint16(targets.length); for (uint256 i = 0; i < targets.length; ++i) { + if (calldatas[i].length < 4) { + continue; + } address target = targets[i]; bytes4 selector = bytes4(calldatas[i]); (bool immediate, uint32 delay) = AuthorityUtils.canCallWithDelay( diff --git a/contracts/mocks/AccessManagedTarget.sol b/contracts/mocks/AccessManagedTarget.sol index 305c989f635..0f7c7a193a4 100644 --- a/contracts/mocks/AccessManagedTarget.sol +++ b/contracts/mocks/AccessManagedTarget.sol @@ -7,6 +7,7 @@ import {AccessManaged} from "../access/manager/AccessManaged.sol"; abstract contract AccessManagedTarget is AccessManaged { event CalledRestricted(address caller); event CalledUnrestricted(address caller); + event CalledFallback(address caller); function fnRestricted() public restricted { emit CalledRestricted(msg.sender); @@ -15,4 +16,8 @@ abstract contract AccessManagedTarget is AccessManaged { function fnUnrestricted() public { emit CalledUnrestricted(msg.sender); } + + fallback() external { + emit CalledFallback(msg.sender); + } } diff --git a/test/governance/extensions/GovernorTimelockAccess.test.js b/test/governance/extensions/GovernorTimelockAccess.test.js index 59ddf6dac61..9734a2f5c41 100644 --- a/test/governance/extensions/GovernorTimelockAccess.test.js +++ b/test/governance/extensions/GovernorTimelockAccess.test.js @@ -84,6 +84,18 @@ contract('GovernorTimelockAccess', function (accounts) { this.unrestricted.operation.target, this.unrestricted.operation.data, ); + + this.fallback = {}; + this.fallback.operation = { + target: this.receiver.address, + value: '0', + data: '0x1234', + }; + this.fallback.operationId = hashOperation( + this.mock.address, + this.fallback.operation.target, + this.fallback.operation.data, + ); }); it('accepts ether transfers', async function () { @@ -180,7 +192,7 @@ contract('GovernorTimelockAccess', function (accounts) { await this.manager.grantRole(roleId, this.mock.address, managerDelay, { from: admin }); this.proposal = await this.helper.setProposal( - [this.restricted.operation, this.unrestricted.operation], + [this.restricted.operation, this.unrestricted.operation, this.fallback.operation], 'descr', ); @@ -209,6 +221,7 @@ contract('GovernorTimelockAccess', function (accounts) { }); await expectEvent.inTransaction(txExecute.tx, this.receiver, 'CalledRestricted'); await expectEvent.inTransaction(txExecute.tx, this.receiver, 'CalledUnrestricted'); + await expectEvent.inTransaction(txExecute.tx, this.receiver, 'CalledFallback'); }); describe('cancel', function () { From 96960e1fe6b6cfe8216997bed11fcc260775cdad Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 28 Sep 2023 14:20:03 -0500 Subject: [PATCH 08/10] Add note --- contracts/access/manager/AccessManaged.sol | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index bcd9c3c4a73..73d25315dab 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -41,17 +41,15 @@ abstract contract AccessManaged is Context, IAccessManaged { * function at the bottom of the call stack, and not the function where the modifier is visible in the source code. * ==== * - * [NOTE] + * [WARNING] * ==== - * Selector collisions are mitigated by scoping permissions per contract, but some edge cases must be considered: + * Avoid adding this modifier to the https://docs.soliditylang.org/en/v0.8.20/contracts.html#receive-ether-function[`receive()`] + * function or the https://docs.soliditylang.org/en/v0.8.20/contracts.html#fallback-function[`fallback()`]. These + * functions are the only execution paths where a function selector cannot be unambiguosly determined from the calldata + * since the selector defaults to `0x00000000` in the `receive()` function and similary in the `fallback()` function + * if no calldata is provided. (See {_checkCanCall}). * - * * If the https://docs.soliditylang.org/en/v0.8.20/contracts.html#receive-ether-function[`receive()`] function - * is restricted, any other function with a `0x00000000` selector will share permissions with `receive()`. - * * Similarly, if there's no `receive()` function but a `fallback()` instead, the fallback might be called with - * empty `calldata`, sharing the `0x00000000` selector permissions as well. - * * For any other selector, if the restricted function is set on an upgradeable contract, an upgrade may remove - * the restricted function and replace it with a new method whose selector replaces the last one, keeping the - * previous permissions. + * The `receive()` function will always panic whereas the `fallback()` may panic depending on the calldata length. * ==== */ modifier restricted() { From bc24be65094e3654599a682412705c1768c295fe Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 28 Sep 2023 14:26:19 -0500 Subject: [PATCH 09/10] Lint --- contracts/access/manager/AccessManaged.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 73d25315dab..fc18779e706 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -49,7 +49,7 @@ abstract contract AccessManaged is Context, IAccessManaged { * since the selector defaults to `0x00000000` in the `receive()` function and similary in the `fallback()` function * if no calldata is provided. (See {_checkCanCall}). * - * The `receive()` function will always panic whereas the `fallback()` may panic depending on the calldata length. + * The `receive()` function will always panic whereas the `fallback()` may panic depending on the calldata length. * ==== */ modifier restricted() { From 44f5b13a6eee5bcad5e46c84954449e553cc42ba Mon Sep 17 00:00:00 2001 From: ernestognw Date: Thu, 28 Sep 2023 14:29:16 -0500 Subject: [PATCH 10/10] Codespell --- contracts/access/manager/AccessManaged.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index fc18779e706..cbf9e0810bb 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -46,7 +46,7 @@ abstract contract AccessManaged is Context, IAccessManaged { * Avoid adding this modifier to the https://docs.soliditylang.org/en/v0.8.20/contracts.html#receive-ether-function[`receive()`] * function or the https://docs.soliditylang.org/en/v0.8.20/contracts.html#fallback-function[`fallback()`]. These * functions are the only execution paths where a function selector cannot be unambiguosly determined from the calldata - * since the selector defaults to `0x00000000` in the `receive()` function and similary in the `fallback()` function + * since the selector defaults to `0x00000000` in the `receive()` function and similarly in the `fallback()` function * if no calldata is provided. (See {_checkCanCall}). * * The `receive()` function will always panic whereas the `fallback()` may panic depending on the calldata length.