From 85822663f2f087d09ce06ffa91c068797d1af747 Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Thu, 28 Sep 2023 20:09:44 +0000 Subject: [PATCH] Update docs --- README.md | 8 ++- contracts/access/manager/AccessManaged.sol | 21 +++---- contracts/access/manager/AccessManager.sol | 61 +++++++++++++------ .../extensions/GovernorTimelockAccess.sol | 3 + contracts/mocks/AccessManagedTarget.sol | 5 ++ docs/modules/api/pages/access.adoc | 25 ++++---- .../extensions/GovernorTimelockAccess.test.js | 15 ++++- 7 files changed, 92 insertions(+), 46 deletions(-) diff --git a/README.md b/README.md index 53c29e5f8..549891e3f 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -> **Note** +> [!NOTE] > Version 5.0 is currently in release candidate period. Bug bounty rewards are boosted 50% until the release. > [See more details on Immunefi.](https://immunefi.com/bounty/openzeppelin/) @@ -35,9 +35,11 @@ $ npm install @openzeppelin/contracts #### Foundry (git) -> **Warning** When installing via git, it is a common error to use the `master` branch. This is a development branch that should be avoided in favor of tagged releases. The release process involves security measures that the `master` branch does not guarantee. +> [!WARNING] +> When installing via git, it is a common error to use the `master` branch. This is a development branch that should be avoided in favor of tagged releases. The release process involves security measures that the `master` branch does not guarantee. -> **Warning** Foundry installs the latest version initially, but subsequent `forge update` commands will use the `master` branch. +> [!WARNING] +> Foundry installs the latest version initially, but subsequent `forge update` commands will use the `master` branch. ``` $ forge install OpenZeppelin/openzeppelin-contracts diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index 6d10d6aa3..53e95dead 100644 --- a/contracts/access/manager/AccessManaged.sol +++ b/contracts/access/manager/AccessManaged.sol @@ -42,17 +42,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 similarly 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() { @@ -100,14 +98,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) { diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 3ae74764a..505e6a022 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -131,7 +131,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, 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)) { @@ -221,11 +225,14 @@ 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); } @@ -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, uint32) { + function hasRole( + uint64 roleId, + address account + ) public view virtual returns (bool isMember, uint32 executionDelay) { if (roleId == PUBLIC_ROLE) { return (true, 0); } else { @@ -579,7 +589,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 @@ -632,7 +642,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 @@ -645,7 +655,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); @@ -708,7 +718,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) { @@ -780,13 +790,15 @@ 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) { 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 || @@ -814,8 +826,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); @@ -832,12 +843,15 @@ 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); - return canCall(caller, target, selector); + return data.length < 4 ? (false, 0) : canCall(caller, target, _checkSelector(data)); } } @@ -845,10 +859,14 @@ 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. - return (_isExecuting(address(this), bytes4(data)), 0); + return (_isExecuting(address(this), _checkSelector(data)), 0); } (bool enabled, uint64 roleId, uint32 operationDelay) = _getAdminRestrictions(data); @@ -879,4 +897,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]); + } } diff --git a/contracts/governance/extensions/GovernorTimelockAccess.sol b/contracts/governance/extensions/GovernorTimelockAccess.sol index 007e0c6f9..b81f8e0f6 100644 --- a/contracts/governance/extensions/GovernorTimelockAccess.sol +++ b/contracts/governance/extensions/GovernorTimelockAccess.sol @@ -192,6 +192,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 305c989f6..0f7c7a193 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/docs/modules/api/pages/access.adoc b/docs/modules/api/pages/access.adoc index f96488ea7..ada16f0a2 100644 --- a/docs/modules/api/pages/access.adoc +++ b/docs/modules/api/pages/access.adoc @@ -2064,7 +2064,7 @@ Check that the caller is authorized to perform the operation, following the rest [.contract-item] [[AccessManager-canCall-address-address-bytes4-]] -==== `[.contract-item-name]#++canCall++#++(address caller, address target, bytes4 selector) → bool, uint32++` [.item-kind]#public# +==== `[.contract-item-name]#++canCall++#++(address caller, address target, bytes4 selector) → bool immediate, uint32 delay++` [.item-kind]#public# Check if an address (`caller`) is authorised to call a given function on a given contract directly (with no restriction). Additionally, it returns the delay needed to perform the call indirectly through the {schedule} @@ -2140,7 +2140,7 @@ a call to {setGrantDelay}. Changes to this value, including effect timepoint are [.contract-item] [[AccessManager-getAccess-uint64-address-]] -==== `[.contract-item-name]#++getAccess++#++(uint64 roleId, address account) → uint48, uint32, uint32, uint48++` [.item-kind]#public# +==== `[.contract-item-name]#++getAccess++#++(uint64 roleId, address account) → uint48 since, uint32 currentDelay, uint32 pendingDelay, uint48 effect++` [.item-kind]#public# Get the access details for a given account for a given role. These details include the timepoint at which membership becomes active, and the delay applied to all operation by this user that requires this permission @@ -2154,7 +2154,7 @@ Returns: [.contract-item] [[AccessManager-hasRole-uint64-address-]] -==== `[.contract-item-name]#++hasRole++#++(uint64 roleId, address account) → bool, uint32++` [.item-kind]#public# +==== `[.contract-item-name]#++hasRole++#++(uint64 roleId, address account) → bool isMember, uint32 executionDelay++` [.item-kind]#public# 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. @@ -2528,17 +2528,15 @@ implications! This is because the permissions are determined by the function tha 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 similarly 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. ==== [.contract-item] @@ -2578,5 +2576,6 @@ permissions set by the current authority. [[AccessManaged-_checkCanCall-address-bytes-]] ==== `[.contract-item-name]#++_checkCanCall++#++(address caller, bytes data)++` [.item-kind]#internal# -Reverts if the caller is not allowed to call the function identified by a selector. +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. diff --git a/test/governance/extensions/GovernorTimelockAccess.test.js b/test/governance/extensions/GovernorTimelockAccess.test.js index 59ddf6dac..9734a2f5c 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 () {