Add named return parameters and `_checkSelector` function to AccessManager (#4624)

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) {

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]);
+    }
 }

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(

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);
+    }
 }

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 () {