Add named return parameters and _checkSelector function to AccessManager

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,23 +842,30 @@ 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));
         }
     }
 
     /**
      * @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]);
+    }
 }