NoBey/openzeppelin-contracts
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Rename AccessManager.relay to execute (#4578)

Browse Source 
Co-authored-by: Francisco Giordano <fg@frang.io>
This commit is contained in:
Hadrien Croubois
2023-09-07 10:08:45 +02:00
committed by GitHub
parent 25c416d01c
commit a05a529049
4 changed files with 53 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

32
contracts/access/manager/AccessManager.sol
View File

@ -40,7 +40,7 @@ import {Time} from "../../utils/types/Time.sol";
* *
* NOTE: Systems that implement other access control mechanisms (for example using {Ownable}) can be paired with an * NOTE: Systems that implement other access control mechanisms (for example using {Ownable}) can be paired with an
* {AccessManager} by transferring permissions (ownership in the case of {Ownable}) directly to the {AccessManager}. * {AccessManager} by transferring permissions (ownership in the case of {Ownable}) directly to the {AccessManager}.
* Users will be able to interact with these contracts through the {relay} function, following the access rules * Users will be able to interact with these contracts through the {execute} function, following the access rules
* registered in the {AccessManager}. Keep in mind that in that context, the msg.sender seen by restricted functions * registered in the {AccessManager}. Keep in mind that in that context, the msg.sender seen by restricted functions
* will be {AccessManager} itself. * will be {AccessManager} itself.
* *
@ -62,7 +62,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
// Timepoint at which the user gets the permission. If this is either 0, or in the future, the group permission // Timepoint at which the user gets the permission. If this is either 0, or in the future, the group permission
// is not available. // is not available.
uint48 since; uint48 since;
// delay for execution. Only applies to restricted() / relay() calls. This does not restrict access to // delay for execution. Only applies to restricted() / execute() calls. This does not restrict access to
// functions that use the `onlyGroup` modifier. // functions that use the `onlyGroup` modifier.
Time.Delay delay; Time.Delay delay;
} }
@ -92,7 +92,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
mapping(bytes32 operationId => Schedule) private _schedules; mapping(bytes32 operationId => Schedule) private _schedules;
// This should be transient storage when supported by the EVM. // This should be transient storage when supported by the EVM.
bytes32 private _relayIdentifier; bytes32 private _executionId;
/** /**
* @dev Check that the caller is authorized to perform the operation, following the restrictions encoded in * @dev Check that the caller is authorized to perform the operation, following the restrictions encoded in
@ -116,7 +116,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
/** /**
* @dev Check if an address (`caller`) is authorised to call a given function on a given contract directly (with * @dev 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} * no restriction). Additionally, it returns the delay needed to perform the call indirectly through the {schedule}
* & {relay} workflow. * & {execute} workflow.
* *
* This function is usually called by the targeted contract to control immediate execution of restricted functions. * This function is usually called by the targeted contract to control immediate execution of restricted functions.
* Therefore we only return true is the call can be performed without any delay. If the call is subject to a delay, * Therefore we only return true is the call can be performed without any delay. If the call is subject to a delay,
@ -133,9 +133,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
if (isTargetClosed(target)) { if (isTargetClosed(target)) {
return (false, 0); return (false, 0);
} else if (caller == address(this)) { } else if (caller == address(this)) {
// Caller is AccessManager => call was relayed. In that case the relay already checked permissions. We // Caller is AccessManager, this means the call was sent through {execute} and it already checked
// verify that the call "identifier", which is set during the relay call, is correct. // permissions. We verify that the call "identifier", which is set during {execute}, is correct.
return (_relayIdentifier == _hashRelayIdentifier(target, selector), 0); return (_executionId == _hashExecutionId(target, selector), 0);
} else { } else {
uint64 groupId = getTargetFunctionGroup(target, selector); uint64 groupId = getTargetFunctionGroup(target, selector);
(bool inGroup, uint32 currentDelay) = hasGroup(groupId, caller); (bool inGroup, uint32 currentDelay) = hasGroup(groupId, caller);
@ -555,7 +555,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
* *
* Returns the `operationId` that was scheduled. Since this value is a hash of the parameters, it can reoccur when * Returns the `operationId` that was scheduled. Since this value is a hash of the parameters, it can reoccur when
* the same parameters are used; if this is relevant, the returned `nonce` can be used to uniquely identify this * the same parameters are used; if this is relevant, the returned `nonce` can be used to uniquely identify this
* scheduled operation from other occurrences of the same `operationId` in invocations of {relay} and {cancel}. * scheduled operation from other occurrences of the same `operationId` in invocations of {execute} and {cancel}.
* *
* Emits a {OperationScheduled} event. * Emits a {OperationScheduled} event.
*/ */
@ -604,7 +604,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
* @dev Execute a function that is delay restricted, provided it was properly scheduled beforehand, or the * @dev Execute a function that is delay restricted, provided it was properly scheduled beforehand, or the
* execution delay is 0. * execution delay is 0.
* *
* Returns the nonce that identifies the previously scheduled operation that is relayed, or 0 if the * Returns the nonce that identifies the previously scheduled operation that is executed, or 0 if the
* operation wasn't previously scheduled (if the caller doesn't have an execution delay). * operation wasn't previously scheduled (if the caller doesn't have an execution delay).
* *
* Emits an {OperationExecuted} event only if the call was scheduled and delayed. * Emits an {OperationExecuted} event only if the call was scheduled and delayed.
@ -612,7 +612,7 @@ contract AccessManager is Context, Multicall, IAccessManager {
// Reentrancy is not an issue because permissions are checked on msg.sender. Additionally, // Reentrancy is not an issue because permissions are checked on msg.sender. Additionally,
// _consumeScheduledOp guarantees a scheduled operation is only executed once. // _consumeScheduledOp guarantees a scheduled operation is only executed once.
// slither-disable-next-line reentrancy-no-eth // slither-disable-next-line reentrancy-no-eth
function relay(address target, bytes calldata data) public payable virtual returns (uint32) { function execute(address target, bytes calldata data) public payable virtual returns (uint32) {
address caller = _msgSender(); address caller = _msgSender();
// Fetch restrictions that apply to the caller on the targeted function // Fetch restrictions that apply to the caller on the targeted function
@ -632,14 +632,14 @@ contract AccessManager is Context, Multicall, IAccessManager {
} }
// Mark the target and selector as authorised // Mark the target and selector as authorised
bytes32 relayIdentifierBefore = _relayIdentifier; bytes32 executionIdBefore = _executionId;
_relayIdentifier = _hashRelayIdentifier(target, bytes4(data)); _executionId = _hashExecutionId(target, bytes4(data));
// Perform call // Perform call
Address.functionCallWithValue(target, data, msg.value); Address.functionCallWithValue(target, data, msg.value);
// Reset relay identifier // Reset execute identifier
_relayIdentifier = relayIdentifierBefore; _executionId = executionIdBefore;
return nonce; return nonce;
} }
@ -725,9 +725,9 @@ contract AccessManager is Context, Multicall, IAccessManager {
} }
/** /**
* @dev Hashing function for relay protection * @dev Hashing function for execute protection
*/ */
function _hashRelayIdentifier(address target, bytes4 selector) private pure returns (bytes32) { function _hashExecutionId(address target, bytes4 selector) private pure returns (bytes32) {
return keccak256(abi.encode(target, selector)); return keccak256(abi.encode(target, selector));
} }

2
contracts/access/manager/IAccessManager.sol
View File

@ -102,7 +102,7 @@ interface IAccessManager {
function schedule(address target, bytes calldata data, uint48 when) external returns (bytes32, uint32); function schedule(address target, bytes calldata data, uint48 when) external returns (bytes32, uint32);
function relay(address target, bytes calldata data) external payable returns (uint32); function execute(address target, bytes calldata data) external payable returns (uint32);
function cancel(address caller, address target, bytes calldata data) external returns (uint32); function cancel(address caller, address target, bytes calldata data) external returns (uint32);

22
contracts/governance/extensions/GovernorTimelockAccess.sol
View File

@ -19,7 +19,7 @@ import {Time} from "../../utils/types/Time.sol";
* This extension allows the governor to hold and use its own assets and permissions, unlike {GovernorTimelockControl} * This extension allows the governor to hold and use its own assets and permissions, unlike {GovernorTimelockControl}
* and {GovernorTimelockCompound}, where the timelock is a separate contract that must be the one to hold assets and * and {GovernorTimelockCompound}, where the timelock is a separate contract that must be the one to hold assets and
* permissions. Operations that are delay-restricted by the manager, however, will be executed through the * permissions. Operations that are delay-restricted by the manager, however, will be executed through the
* {AccessManager-relay} function. * {AccessManager-execute} function.
* *
* Note that some operations may be cancelable in the {AccessManager} by the admin or a set of guardians, depending on * Note that some operations may be cancelable in the {AccessManager} by the admin or a set of guardians, depending on
* the restricted operation being invoked. Since proposals are atomic, the cancellation by a guardian of a single * the restricted operation being invoked. Since proposals are atomic, the cancellation by a guardian of a single
@ -27,15 +27,15 @@ import {Time} from "../../utils/types/Time.sol";
*/ */
abstract contract GovernorTimelockAccess is Governor { abstract contract GovernorTimelockAccess is Governor {
// An execution plan is produced at the moment a proposal is created, in order to fix at that point the exact // An execution plan is produced at the moment a proposal is created, in order to fix at that point the exact
// execution semantics of the proposal, namely whether a call will go through {AccessManager-relay}. // execution semantics of the proposal, namely whether a call will go through {AccessManager-execute}.
struct ExecutionPlan { struct ExecutionPlan {
uint16 length; uint16 length;
uint32 delay; uint32 delay;
// We use mappings instead of arrays because it allows us to pack values in storage more tightly without storing // We use mappings instead of arrays because it allows us to pack values in storage more tightly without
// the length redundantly. // storing the length redundantly.
// We pack 8 operations' data in each bucket. Each uint32 value is set to 1 upon proposal creation if it has to // We pack 8 operations' data in each bucket. Each uint32 value is set to 1 upon proposal creation if it has
// be scheduled and relayed through the manager. Upon queuing, the value is set to nonce + 1, where the nonce is // to be scheduled and executed through the manager. Upon queuing, the value is set to nonce + 1, where the
// that which we get back from the manager when scheduling the operation. // nonce is that which we get back from the manager when scheduling the operation.
mapping(uint256 operationBucket => uint32[8]) managerData; mapping(uint256 operationBucket => uint32[8]) managerData;
} }
@ -175,7 +175,7 @@ abstract contract GovernorTimelockAccess is Governor {
} }
/** /**
* @dev Mechanism to execute a proposal, potentially going through {AccessManager-relay} for delayed operations. * @dev Mechanism to execute a proposal, potentially going through {AccessManager-execute} for delayed operations.
*/ */
function _executeOperations( function _executeOperations(
uint256 proposalId, uint256 proposalId,
@ -194,9 +194,9 @@ abstract contract GovernorTimelockAccess is Governor {
for (uint256 i = 0; i < targets.length; ++i) { for (uint256 i = 0; i < targets.length; ++i) {
(bool delayed, uint32 nonce) = _getManagerData(plan, i); (bool delayed, uint32 nonce) = _getManagerData(plan, i);
if (delayed) { if (delayed) {
uint32 relayedNonce = _manager.relay{value: values[i]}(targets[i], calldatas[i]); uint32 executedNonce = _manager.execute{value: values[i]}(targets[i], calldatas[i]);
if (relayedNonce != nonce) { if (executedNonce != nonce) {
revert GovernorMismatchedNonce(proposalId, nonce, relayedNonce); revert GovernorMismatchedNonce(proposalId, nonce, executedNonce);
} }
} else { } else {
(bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]); (bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]);

50
test/access/manager/AccessManager.test.js
View File

@ -555,7 +555,7 @@ contract('AccessManager', function (accounts) {
); );
this.direct = (opts = {}) => this.target.fnRestricted({ from: user, ...opts }); this.direct = (opts = {}) => this.target.fnRestricted({ from: user, ...opts });
this.schedule = (opts = {}) => this.manager.schedule(...this.call, 0, { from: user, ...opts }); this.schedule = (opts = {}) => this.manager.schedule(...this.call, 0, { from: user, ...opts });
this.relay = (opts = {}) => this.manager.relay(...this.call, { from: user, ...opts }); this.execute = (opts = {}) => this.manager.execute(...this.call, { from: user, ...opts });
this.cancel = (opts = {}) => this.manager.cancel(user, ...this.call, { from: user, ...opts }); this.cancel = (opts = {}) => this.manager.cancel(user, ...this.call, { from: user, ...opts });
}); });
@ -711,20 +711,20 @@ contract('AccessManager', function (accounts) {
} }
}); });
it('Calling indirectly: only relay', async function () { it('Calling indirectly: only execute', async function () {
// relay without schedule // execute without schedule
if (directSuccess) { if (directSuccess) {
const { receipt, tx } = await this.relay(); const { receipt, tx } = await this.execute();
expectEvent.notEmitted(receipt, 'OperationExecuted', { operationId: this.opId }); expectEvent.notEmitted(receipt, 'OperationExecuted', { operationId: this.opId });
await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address }); await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address });
} else if (indirectSuccess) { } else if (indirectSuccess) {
await expectRevertCustomError(this.relay(), 'AccessManagerNotScheduled', [this.opId]); await expectRevertCustomError(this.execute(), 'AccessManagerNotScheduled', [this.opId]);
} else { } else {
await expectRevertCustomError(this.relay(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); await expectRevertCustomError(this.execute(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
} }
}); });
it('Calling indirectly: schedule and relay', async function () { it('Calling indirectly: schedule and execute', async function () {
if (directSuccess || indirectSuccess) { if (directSuccess || indirectSuccess) {
const { receipt } = await this.schedule(); const { receipt } = await this.schedule();
const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN); const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
@ -743,23 +743,23 @@ contract('AccessManager', function (accounts) {
// execute without wait // execute without wait
if (directSuccess) { if (directSuccess) {
const { receipt, tx } = await this.relay(); const { receipt, tx } = await this.execute();
await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address }); await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address });
if (delay && fnGroup !== GROUPS.PUBLIC) { if (delay && fnGroup !== GROUPS.PUBLIC) {
expectEvent(receipt, 'OperationExecuted', { operationId: this.opId }); expectEvent(receipt, 'OperationExecuted', { operationId: this.opId });
expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0'); expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0');
} }
} else if (indirectSuccess) { } else if (indirectSuccess) {
await expectRevertCustomError(this.relay(), 'AccessManagerNotReady', [this.opId]); await expectRevertCustomError(this.execute(), 'AccessManagerNotReady', [this.opId]);
} else { } else {
await expectRevertCustomError(this.relay(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); await expectRevertCustomError(this.execute(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
} }
} else { } else {
await expectRevertCustomError(this.schedule(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); await expectRevertCustomError(this.schedule(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
} }
}); });
it('Calling indirectly: schedule wait and relay', async function () { it('Calling indirectly: schedule wait and execute', async function () {
if (directSuccess || indirectSuccess) { if (directSuccess || indirectSuccess) {
const { receipt } = await this.schedule(); const { receipt } = await this.schedule();
const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN); const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN);
@ -781,14 +781,14 @@ contract('AccessManager', function (accounts) {
// execute without wait // execute without wait
if (directSuccess || indirectSuccess) { if (directSuccess || indirectSuccess) {
const { receipt, tx } = await this.relay(); const { receipt, tx } = await this.execute();
await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address }); await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address });
if (delay && fnGroup !== GROUPS.PUBLIC) { if (delay && fnGroup !== GROUPS.PUBLIC) {
expectEvent(receipt, 'OperationExecuted', { operationId: this.opId }); expectEvent(receipt, 'OperationExecuted', { operationId: this.opId });
expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0'); expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0');
} }
} else { } else {
await expectRevertCustomError(this.relay(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); await expectRevertCustomError(this.execute(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
} }
} else { } else {
await expectRevertCustomError(this.schedule(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); await expectRevertCustomError(this.schedule(), 'AccessManagerUnauthorizedCall', [user, ...this.call]);
@ -883,7 +883,7 @@ contract('AccessManager', function (accounts) {
await this.manager.$_grantGroup(GROUPS.SOME, user, 0, executeDelay); await this.manager.$_grantGroup(GROUPS.SOME, user, 0, executeDelay);
}); });
it('Checking canCall when caller is the manager depend on the _relayIdentifier', async function () { it('Checking canCall when caller is the manager depend on the _executionId', async function () {
const result = await this.manager.canCall(this.manager.address, this.target.address, '0x00000000'); const result = await this.manager.canCall(this.manager.address, this.target.address, '0x00000000');
expect(result[0]).to.be.false; expect(result[0]).to.be.false;
expect(result[1]).to.be.bignumber.equal('0'); expect(result[1]).to.be.bignumber.equal('0');
@ -900,13 +900,13 @@ contract('AccessManager', function (accounts) {
await time.increaseTo(timestamp.add(executeDelay).subn(2)); await time.increaseTo(timestamp.add(executeDelay).subn(2));
// too early // too early
await expectRevertCustomError(this.relay(), 'AccessManagerNotReady', [this.opId]); await expectRevertCustomError(this.execute(), 'AccessManagerNotReady', [this.opId]);
// the revert happened one second before the execution delay expired // the revert happened one second before the execution delay expired
expect(await time.latest()).to.be.bignumber.equal(timestamp.add(executeDelay).subn(1)); expect(await time.latest()).to.be.bignumber.equal(timestamp.add(executeDelay).subn(1));
// ok // ok
await this.relay(); await this.execute();
// the success happened when the delay was reached (earliest possible) // the success happened when the delay was reached (earliest possible)
expect(await time.latest()).to.be.bignumber.equal(timestamp.add(executeDelay)); expect(await time.latest()).to.be.bignumber.equal(timestamp.add(executeDelay));
@ -928,10 +928,10 @@ contract('AccessManager', function (accounts) {
await expectRevertCustomError(this.cancel(), 'AccessManagerNotScheduled', [this.opId]); await expectRevertCustomError(this.cancel(), 'AccessManagerNotScheduled', [this.opId]);
}); });
it('Cannot cancel an operation that is not already relayed', async function () { it('Cannot cancel an operation that is already executed', async function () {
await this.schedule(); await this.schedule();
await time.increase(executeDelay); await time.increase(executeDelay);
await this.relay(); await this.execute();
await expectRevertCustomError(this.cancel(), 'AccessManagerNotScheduled', [this.opId]); await expectRevertCustomError(this.cancel(), 'AccessManagerNotScheduled', [this.opId]);
}); });
@ -973,7 +973,7 @@ contract('AccessManager', function (accounts) {
it('Can re-schedule after execution', async function () { it('Can re-schedule after execution', async function () {
await this.schedule(); await this.schedule();
await time.increase(executeDelay); await time.increase(executeDelay);
await this.relay(); await this.execute();
// reschedule // reschedule
const { receipt } = await this.schedule(); const { receipt } = await this.schedule();
@ -1026,7 +1026,7 @@ contract('AccessManager', function (accounts) {
it('relayed call (with group): reverts', async function () { it('relayed call (with group): reverts', async function () {
await expectRevertCustomError( await expectRevertCustomError(
this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: user }), this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: user }),
'AccessManagerUnauthorizedCall', 'AccessManagerUnauthorizedCall',
[user, this.ownable.address, selector('$_checkOwner()')], [user, this.ownable.address, selector('$_checkOwner()')],
); );
@ -1034,7 +1034,7 @@ contract('AccessManager', function (accounts) {
it('relayed call (without group): reverts', async function () { it('relayed call (without group): reverts', async function () {
await expectRevertCustomError( await expectRevertCustomError(
this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: other }), this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: other }),
'AccessManagerUnauthorizedCall', 'AccessManagerUnauthorizedCall',
[other, this.ownable.address, selector('$_checkOwner()')], [other, this.ownable.address, selector('$_checkOwner()')],
); );
@ -1054,12 +1054,12 @@ contract('AccessManager', function (accounts) {
}); });
it('relayed call (with group): success', async function () { it('relayed call (with group): success', async function () {
await this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: user }); await this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: user });
}); });
it('relayed call (without group): reverts', async function () { it('relayed call (without group): reverts', async function () {
await expectRevertCustomError( await expectRevertCustomError(
this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: other }), this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: other }),
'AccessManagerUnauthorizedCall', 'AccessManagerUnauthorizedCall',
[other, this.ownable.address, selector('$_checkOwner()')], [other, this.ownable.address, selector('$_checkOwner()')],
); );
@ -1078,11 +1078,11 @@ contract('AccessManager', function (accounts) {
}); });
it('relayed call (with group): success', async function () { it('relayed call (with group): success', async function () {
await this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: user }); await this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: user });
}); });
it('relayed call (without group): success', async function () { it('relayed call (without group): success', async function () {
await this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: other }); await this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: other });
}); });
}); });
}); });