Update docs

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

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

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

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(

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

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:
-
-* 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.
+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}).
+
+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.
 

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