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.

Sign up for GitHub

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
merged 10 commits into from
Sep 28, 2023
Merged

Add named return parameters and _checkSelector function to AccessManager #4624

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
983ba61
Add named return parameters and _checkSelector function
ernestognw Sep 25, 2023
b48e925
Lint
ernestognw Sep 25, 2023
280f29b
Avoid panicking in view functions
ernestognw Sep 26, 2023
40692f2
Revert _setGrantDelay PUBLIC_ROLE case
ernestognw Sep 27, 2023
3dac6f4
Revert canCall hasRole path return
ernestognw Sep 27, 2023
7e0fa74
Make AccessManaged _checkCanCall panic on short calldarta
ernestognw Sep 28, 2023
89c50f9
Ignore short calldatas in propose()
ernestognw Sep 28, 2023
96960e1
Add note
ernestognw Sep 28, 2023
bc24be6
Lint
ernestognw Sep 28, 2023
44f5b13
Codespell
ernestognw Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 41 additions & 22 deletions contracts/access/manager/AccessManager.sol
View file
Open in desktop
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 All @@ -139,8 +143,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
return (_isExecuting(target, selector), 0);
} else {
uint64 roleId = getTargetFunctionRole(target, selector);
(bool isMember, uint32 currentDelay) = hasRole(roleId, caller);
return isMember ? (currentDelay == 0, currentDelay) : (false, 0);
bool inRole;
(inRole, delay) = hasRole(roleId, caller);
return inRole ? (delay == 0, delay) : (false, 0);
}
}

Expand Down Expand Up @@ -220,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);
}
Expand All @@ -233,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 {
Expand Down Expand Up @@ -439,10 +450,6 @@ contract AccessManager is Context, Multicall, IAccessManager {
* Emits a {RoleGrantDelayChanged} event
*/
function _setGrantDelay(uint64 roleId, uint32 newDelay) internal virtual {
if (roleId == PUBLIC_ROLE) {
revert AccessManagerLockedRole(roleId);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self and others: this is removed because the public role is not grantable according to the logic of _grantRole.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the public role is indeed not grantable, does it make sens to set the grant delay ? IMO it send the wrong signal that you can set a delay, and emit the corresponding event, when you know the delay will never be enforced.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps documentation is fine for the case of granting a delay to the PUBLIC_ROLE, I wouldn't add an extra check for a case that is low likely to happen

Copy link
Contributor

@frangio frangio Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO it send the wrong signal that you can set a delay, and emit the corresponding event, when you know the delay will never be enforced

This is an interesting point, but this is the grant delay for a role that every account automatically has. Observing RoleGrantDelayChanged for such a role should provide no information. I can't think of any incorrect conclusion someone could draw from this event.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any incorrect conclusion someone could draw from this event.

I can imagine someone thinking that new user (if that means anything) are in the grant role with a delay. For example that when a create a new seedphrase/private key (or when I create a new account smart contract) I'll get in the public group after a delay.

Sure that makes no sens, but I'm pretty sure some people out these will believe that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in _setRoleAdmin and _setRoleGuardian we don't allow setting the admin/guardian for the PUBLIC_ROLE. I'm not sure why that would be forbiden, but setting the grantDelay would be authorized.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a sync we agreed to keep the initial PUBLIC_ROLE check in _setGrantDelay and make no changes to other functions. I'm reverting this change and pushing an update.


uint48 effect;
(_roles[roleId].grantDelay, effect) = _roles[roleId].grantDelay.withUpdate(newDelay, minSetback());

Expand Down Expand Up @@ -578,7 +585,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 +638,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 +651,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 +714,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,8 +786,10 @@ 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) {
bytes4 selector = _checkSelector(data);

if (data.length < 4) {
return (false, 0, 0);
Expand Down Expand Up @@ -813,8 +822,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,11 +839,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);
bytes4 selector = _checkSelector(data);
return canCall(caller, target, selector);
}
}
Expand All @@ -847,7 +859,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
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);
Expand Down Expand Up @@ -878,4 +890,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]);
}
}