diff --git a/contracts/access/manager/AccessManaged.sol b/contracts/access/manager/AccessManaged.sol index c207c5e5183..cbf9e0810bb 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 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() { @@ -99,14 +97,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 47dc47f739a..31ef86c3f11 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, 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)) { @@ -220,11 +224,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); } @@ -233,7 +240,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 { @@ -578,7 +588,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 +641,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 +654,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 +717,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,13 +789,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 || @@ -813,8 +825,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,12 +842,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)); } } @@ -844,10 +858,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); @@ -878,4 +896,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 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 () {