diff --git a/contracts/access/manager/AccessManager.sol b/contracts/access/manager/AccessManager.sol index 5261b1dd9..30e77f126 100644 --- a/contracts/access/manager/AccessManager.sol +++ b/contracts/access/manager/AccessManager.sol @@ -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 * {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 * 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 // is not available. 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. Time.Delay delay; } @@ -92,7 +92,7 @@ contract AccessManager is Context, Multicall, IAccessManager { mapping(bytes32 operationId => Schedule) private _schedules; // 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 @@ -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 * 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. * 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)) { return (false, 0); } else if (caller == address(this)) { - // Caller is AccessManager => call was relayed. In that case the relay already checked permissions. We - // verify that the call "identifier", which is set during the relay call, is correct. - return (_relayIdentifier == _hashRelayIdentifier(target, selector), 0); + // 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 (_executionId == _hashExecutionId(target, selector), 0); } else { uint64 groupId = getTargetFunctionGroup(target, selector); (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 * 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. */ @@ -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 * 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). * * 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, // _consumeScheduledOp guarantees a scheduled operation is only executed once. // 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(); // 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 - bytes32 relayIdentifierBefore = _relayIdentifier; - _relayIdentifier = _hashRelayIdentifier(target, bytes4(data)); + bytes32 executionIdBefore = _executionId; + _executionId = _hashExecutionId(target, bytes4(data)); // Perform call Address.functionCallWithValue(target, data, msg.value); - // Reset relay identifier - _relayIdentifier = relayIdentifierBefore; + // Reset execute identifier + _executionId = executionIdBefore; 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)); } diff --git a/contracts/access/manager/IAccessManager.sol b/contracts/access/manager/IAccessManager.sol index 5906e9b23..af6b2f3f0 100644 --- a/contracts/access/manager/IAccessManager.sol +++ b/contracts/access/manager/IAccessManager.sol @@ -102,7 +102,7 @@ interface IAccessManager { 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); diff --git a/contracts/governance/extensions/GovernorTimelockAccess.sol b/contracts/governance/extensions/GovernorTimelockAccess.sol index 6ebb3f1d6..63e547b83 100644 --- a/contracts/governance/extensions/GovernorTimelockAccess.sol +++ b/contracts/governance/extensions/GovernorTimelockAccess.sol @@ -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} * 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 - * {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 * 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 { // 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 { uint16 length; uint32 delay; - // We use mappings instead of arrays because it allows us to pack values in storage more tightly without 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 - // be scheduled and relayed through the manager. Upon queuing, the value is set to nonce + 1, where the nonce is - // that which we get back from the manager when scheduling the operation. + // We use mappings instead of arrays because it allows us to pack values in storage more tightly without + // 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 be scheduled and executed through the manager. Upon queuing, the value is set to nonce + 1, where the + // nonce is that which we get back from the manager when scheduling the operation. 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( uint256 proposalId, @@ -194,9 +194,9 @@ abstract contract GovernorTimelockAccess is Governor { for (uint256 i = 0; i < targets.length; ++i) { (bool delayed, uint32 nonce) = _getManagerData(plan, i); if (delayed) { - uint32 relayedNonce = _manager.relay{value: values[i]}(targets[i], calldatas[i]); - if (relayedNonce != nonce) { - revert GovernorMismatchedNonce(proposalId, nonce, relayedNonce); + uint32 executedNonce = _manager.execute{value: values[i]}(targets[i], calldatas[i]); + if (executedNonce != nonce) { + revert GovernorMismatchedNonce(proposalId, nonce, executedNonce); } } else { (bool success, bytes memory returndata) = targets[i].call{value: values[i]}(calldatas[i]); diff --git a/test/access/manager/AccessManager.test.js b/test/access/manager/AccessManager.test.js index fc80c13ed..92ae830b5 100644 --- a/test/access/manager/AccessManager.test.js +++ b/test/access/manager/AccessManager.test.js @@ -555,7 +555,7 @@ contract('AccessManager', function (accounts) { ); this.direct = (opts = {}) => this.target.fnRestricted({ 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 }); }); @@ -711,20 +711,20 @@ contract('AccessManager', function (accounts) { } }); - it('Calling indirectly: only relay', async function () { - // relay without schedule + it('Calling indirectly: only execute', async function () { + // execute without schedule if (directSuccess) { - const { receipt, tx } = await this.relay(); + const { receipt, tx } = await this.execute(); expectEvent.notEmitted(receipt, 'OperationExecuted', { operationId: this.opId }); await expectEvent.inTransaction(tx, this.target, 'CalledRestricted', { caller: this.manager.address }); } else if (indirectSuccess) { - await expectRevertCustomError(this.relay(), 'AccessManagerNotScheduled', [this.opId]); + await expectRevertCustomError(this.execute(), 'AccessManagerNotScheduled', [this.opId]); } 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) { const { receipt } = await this.schedule(); const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN); @@ -743,23 +743,23 @@ contract('AccessManager', function (accounts) { // execute without wait 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 }); if (delay && fnGroup !== GROUPS.PUBLIC) { expectEvent(receipt, 'OperationExecuted', { operationId: this.opId }); expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0'); } } else if (indirectSuccess) { - await expectRevertCustomError(this.relay(), 'AccessManagerNotReady', [this.opId]); + await expectRevertCustomError(this.execute(), 'AccessManagerNotReady', [this.opId]); } else { - await expectRevertCustomError(this.relay(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); + await expectRevertCustomError(this.execute(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); } } else { 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) { const { receipt } = await this.schedule(); const timestamp = await clockFromReceipt.timestamp(receipt).then(web3.utils.toBN); @@ -781,14 +781,14 @@ contract('AccessManager', function (accounts) { // execute without wait 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 }); if (delay && fnGroup !== GROUPS.PUBLIC) { expectEvent(receipt, 'OperationExecuted', { operationId: this.opId }); expect(await this.manager.getSchedule(this.opId)).to.be.bignumber.equal('0'); } } else { - await expectRevertCustomError(this.relay(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); + await expectRevertCustomError(this.execute(), 'AccessManagerUnauthorizedCall', [user, ...this.call]); } } else { 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); }); - 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'); expect(result[0]).to.be.false; expect(result[1]).to.be.bignumber.equal('0'); @@ -900,13 +900,13 @@ contract('AccessManager', function (accounts) { await time.increaseTo(timestamp.add(executeDelay).subn(2)); // 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 expect(await time.latest()).to.be.bignumber.equal(timestamp.add(executeDelay).subn(1)); // ok - await this.relay(); + await this.execute(); // the success happened when the delay was reached (earliest possible) 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]); }); - 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 time.increase(executeDelay); - await this.relay(); + await this.execute(); await expectRevertCustomError(this.cancel(), 'AccessManagerNotScheduled', [this.opId]); }); @@ -973,7 +973,7 @@ contract('AccessManager', function (accounts) { it('Can re-schedule after execution', async function () { await this.schedule(); await time.increase(executeDelay); - await this.relay(); + await this.execute(); // reschedule const { receipt } = await this.schedule(); @@ -1026,7 +1026,7 @@ contract('AccessManager', function (accounts) { it('relayed call (with group): reverts', async function () { await expectRevertCustomError( - this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: user }), + this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: user }), 'AccessManagerUnauthorizedCall', [user, this.ownable.address, selector('$_checkOwner()')], ); @@ -1034,7 +1034,7 @@ contract('AccessManager', function (accounts) { it('relayed call (without group): reverts', async function () { await expectRevertCustomError( - this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: other }), + this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: other }), 'AccessManagerUnauthorizedCall', [other, this.ownable.address, selector('$_checkOwner()')], ); @@ -1054,12 +1054,12 @@ contract('AccessManager', function (accounts) { }); 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 () { await expectRevertCustomError( - this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: other }), + this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: other }), 'AccessManagerUnauthorizedCall', [other, this.ownable.address, selector('$_checkOwner()')], ); @@ -1078,11 +1078,11 @@ contract('AccessManager', function (accounts) { }); 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 () { - await this.manager.relay(this.ownable.address, selector('$_checkOwner()'), { from: other }); + await this.manager.execute(this.ownable.address, selector('$_checkOwner()'), { from: other }); }); }); });