Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add named return parameters and _checkSelector function to AccessManager #4624

Merged
61 changes: 43 additions & 18 deletions contracts/access/manager/AccessManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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);
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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 ||
Expand Down Expand Up @@ -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);
Expand All @@ -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)) {
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
// 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);
Expand Down Expand Up @@ -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) {
ernestognw marked this conversation as resolved.
Show resolved Hide resolved
return bytes4(data[0:4]);
}
}
Loading