diff --git a/contracts/account/modules/ERC7579DelayedExecutor.sol b/contracts/account/modules/ERC7579DelayedExecutor.sol index bcd72b49..ad92062d 100644 --- a/contracts/account/modules/ERC7579DelayedExecutor.sol +++ b/contracts/account/modules/ERC7579DelayedExecutor.sol @@ -80,6 +80,9 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { /// @dev Emitted when the expiration delay is updated. event ERC7579ExecutorExpirationUpdated(address indexed account, uint32 newExpiration); + /// @dev The module is not installed on the account. + error ERC7579ExecutorModuleNotInstalled(); + /** * @dev The current state of a operation is not the expected. The `expectedStates` is a bitmap with the * bits enabled for each OperationState enum position counting from right to left. See {_encodeStateBitmap}. @@ -122,72 +125,106 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { return OperationState.Ready; } - /// @dev See {ERC7579Executor-canExecute}. Allows anyone to execute an operation if it's `Ready`. - function canExecute( + /** + * @dev See {ERC7579Executor-_validateExecution}. Validates that the operation is in `Ready` state. + * + * Derived contracts can override this function to add additional validation logic. + * + * Example extension: + * + * ```solidity + * function _validateExecution( + * address account, + * Mode mode, + * bytes calldata executionCalldata, + * bytes32 salt + * ) internal view virtual override { + * bool conditionMet = ...; // custom logic to check condition + * require(conditionMet, ERC7579ExecutorConditionNotMet()); + * super._validateExecution(account, mode, executionCalldata, salt); + * } + *``` + */ + function _validateExecution( address account, Mode mode, bytes calldata executionCalldata, bytes32 salt - ) public view virtual override returns (bool) { - return - state(account, mode, executionCalldata, salt) == OperationState.Ready || - super.canExecute(account, mode, executionCalldata, salt); + ) internal view virtual override { + bytes32 id = hashOperation(account, mode, executionCalldata, salt); + _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready)); + + super._validateExecution(account, mode, executionCalldata, salt); } /** - * @dev Whether the caller is authorized to cancel operations. - * By default, this checks if the caller is the account itself. Derived contracts can - * override this to implement custom authorization logic. + * @dev Validates cancellation. This base implementation validates the caller + * is authorized to cancel and that the operation is in a valid state. + * + * Derived contracts can override this function to add additional validation logic. * * Example extension: * * ```solidity - * function canCancel( + * function _validateCancel( * address account, * Mode mode, * bytes calldata executionCalldata, * bytes32 salt - * ) public view virtual returns (bool) { - * bool isAuthorized = ...; // custom logic to check authorization - * return isAuthorized || super.canCancel(account, mode, executionCalldata, salt); + * ) internal view virtual override { + * bool conditionMet = ...; // custom logic to check additional condition + * require(conditionMet, ERC7579ExecutorConditionNotMet()); + * super._validateCancel(account, mode, executionCalldata, salt); * } *``` */ - function canCancel( + function _validateCancel( address account, - Mode /* mode */, - bytes calldata /* executionCalldata */, - bytes32 /* salt */ - ) public view virtual returns (bool) { - return account == msg.sender; + Mode mode, + bytes calldata executionCalldata, + bytes32 salt + ) internal view virtual { + bytes32 id = hashOperation(account, mode, executionCalldata, salt); + bytes32 allowedStates = _encodeStateBitmap(OperationState.Scheduled) | _encodeStateBitmap(OperationState.Ready); + _validateStateBitmap(id, allowedStates); + + require(msg.sender == account, ERC7579ExecutorUnauthorizedCancellation()); } /** - * @dev Whether the caller is authorized to schedule operations. - * By default, this checks if the caller is the account itself. Derived contracts can - * override this to implement custom authorization logic. + * @dev Validates scheduling. This base implementation validates the caller + * is authorized to schedule, the module is installed, and the operation was + * not scheduled before. + * + * Can be overridden by derived contracts to implement custom validation logic. * * Example extension: * * ```solidity - * function canSchedule( + * function _validateSchedule( * address account, * Mode mode, * bytes calldata executionCalldata, * bytes32 salt - * ) public view virtual returns (bool) { - * bool isAuthorized = ...; // custom logic to check authorization - * return isAuthorized || super.canSchedule(account, mode, executionCalldata, salt); + * ) internal view virtual override { + * bool conditionMet = ...; // custom logic to check condition + * require(conditionMet, ERC7579ExecutorConditionNotMet()); + * super._validateSchedule(account, mode, executionCalldata, salt); * } *``` */ - function canSchedule( + function _validateSchedule( address account, - Mode /* mode */, - bytes calldata /* executionCalldata */, - bytes32 /* salt */ - ) public view virtual returns (bool) { - return account == msg.sender; + Mode mode, + bytes calldata executionCalldata, + bytes32 salt + ) internal view virtual { + require(_config[account].installed, ERC7579ExecutorModuleNotInstalled()); + + bytes32 id = hashOperation(account, mode, executionCalldata, salt); + _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown)); + + require(msg.sender == account, ERC7579ExecutorUnauthorizedSchedule()); } /// @dev Minimum delay after which {setDelay} takes effect. @@ -293,22 +330,19 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { /** * @dev Schedules an operation to be executed after the account's delay period (see {getDelay}). * Operations are uniquely identified by the combination of `mode`, `executionCalldata`, and `salt`. - * See {canSchedule} for authorization checks. + * See {_validateSchedule} for validation. */ function schedule(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) public virtual { - bool allowed = canSchedule(account, mode, executionCalldata, salt); - _schedule(account, mode, executionCalldata, salt); // Prioritize errors thrown in _schedule - require(allowed, ERC7579ExecutorUnauthorizedSchedule()); + _validateSchedule(account, mode, executionCalldata, salt); + _schedule(account, mode, executionCalldata, salt); } /** - * @dev Cancels a previously scheduled operation. Can only be called by the account that - * scheduled the operation. See {_cancel}. + * @dev Cancels a scheduled operation. See {_validateCancel} for validation. */ function cancel(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) public virtual { - bool allowed = canCancel(account, mode, executionCalldata, salt); - _cancel(account, mode, executionCalldata, salt); // Prioritize errors thrown in _cancel - require(allowed, ERC7579ExecutorUnauthorizedCancellation()); + _validateCancel(account, mode, executionCalldata, salt); + _cancel(account, mode, executionCalldata, salt); } /** @@ -354,42 +388,27 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { } /** - * @dev Internal version of {schedule} that takes an `account` address as an argument. - * - * Requirements: - * - * * The operation must be `Unknown`. + * @dev Schedules an operation. Does not perform any validation checks. * * Emits an {ERC7579ExecutorOperationScheduled} event. + * + * NOTE: Calling this function directly will NOT perform any validation checks. + * Scheduling a operation should be done using {schedule}. */ - function _schedule( - address account, - Mode mode, - bytes calldata executionCalldata, - bytes32 salt - ) internal virtual returns (bytes32 operationId, Schedule memory schedule_) { + function _schedule(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) internal virtual { bytes32 id = hashOperation(account, mode, executionCalldata, salt); - _validateStateBitmap(id, _encodeStateBitmap(OperationState.Unknown)); - (uint32 executableAfter, , ) = getDelay(account); - uint48 timepoint = Time.timestamp(); + _schedules[id].scheduledAt = timepoint; _schedules[id].executableAfter = executableAfter; _schedules[id].expiresAfter = getExpiration(account); emit ERC7579ExecutorOperationScheduled(account, id, mode, executionCalldata, salt, timepoint); - return (id, schedule_); } /** - * @dev See {ERC7579Executor-_execute}. - * - * Requirements: - * - * * The operation must be `Ready`. - * - * NOTE: Anyone can trigger execution once the operation is `Ready`. See {canExecute}. + * @inheritdoc ERC7579Executor */ function _execute( address account, @@ -398,7 +417,6 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { bytes32 salt ) internal virtual override returns (bytes[] memory returnData) { bytes32 id = hashOperation(account, mode, executionCalldata, salt); - _validateStateBitmap(id, _encodeStateBitmap(OperationState.Ready)); _schedules[id].executed = true; @@ -406,18 +424,17 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { } /** - * @dev Internal version of {cancel} that takes an `account` address as an argument. + * @dev Cancels an operation. * - * Requirements: + * Emits an {ERC7579ExecutorOperationCanceled} event. * - * * The operation must be `Scheduled` or `Ready`. + * NOTE: Calling this function directly will NOT perform any validation checks. + * Cancelling a operation should be done using {cancel}. * - * Canceled operations can't be rescheduled. Emits an {ERC7579ExecutorOperationCanceled} event. + * NOTE: Canceled operations can't be rescheduled. */ function _cancel(address account, Mode mode, bytes calldata executionCalldata, bytes32 salt) internal virtual { bytes32 id = hashOperation(account, mode, executionCalldata, salt); - bytes32 allowedStates = _encodeStateBitmap(OperationState.Scheduled) | _encodeStateBitmap(OperationState.Ready); - _validateStateBitmap(id, allowedStates); _schedules[id].canceled = true; diff --git a/contracts/account/modules/ERC7579Executor.sol b/contracts/account/modules/ERC7579Executor.sol index 846406e4..700a1717 100644 --- a/contracts/account/modules/ERC7579Executor.sol +++ b/contracts/account/modules/ERC7579Executor.sol @@ -11,7 +11,7 @@ import {Mode} from "@openzeppelin/contracts/account/utils/draft-ERC7579Utils.sol * The module enables accounts to execute arbitrary operations, leveraging the execution * capabilities defined in the ERC-7579 standard. By default, the executor is restricted to * operations initiated by the account itself, but this can be customized in derived contracts - * by overriding the {canExecute} function. + * by overriding the {_validateExecution} function. * * TIP: This is a simplified executor that directly executes operations without delay or expiration * mechanisms. For a more advanced implementation with time-delayed execution patterns and @@ -30,37 +30,34 @@ abstract contract ERC7579Executor is IERC7579Module { } /** - * @dev Check if the caller is authorized to execute operations. - * By default, this checks if the caller is the account itself. Derived contracts can - * override this to implement custom authorization logic. + * @dev Validates execution. By default, only validates the caller is the account itself. + * Derived contracts can override this function to add additional validation logic. * * Example extension: * * ```solidity - * function canExecute( + * function _validateExecution( * address account, * Mode mode, * bytes calldata executionCalldata, * bytes32 salt - * ) public view virtual returns (bool) { - * bool isAuthorized = ...; // custom logic to check authorization - * return isAuthorized || super.canExecute(account, mode, executionCalldata, salt); + * ) internal view virtual override { + * bool conditionMet = ...; // custom logic to check condition + * require(conditionMet, ERC7579ExecutorConditionNotMet()); + * super._validateExecution(account, mode, executionCalldata, salt); * } *``` */ - function canExecute( + function _validateExecution( address account, Mode /* mode */, bytes calldata /* executionCalldata */, bytes32 /* salt */ - ) public view virtual returns (bool) { - return msg.sender == account; - } + ) internal view virtual {} /** * @dev Executes an operation and returns the result data from the executed operation. - * Restricted to the account itself by default. See {_execute} for requirements and - * {canExecute} for authorization checks. + * See {_execute} for requirements and {_validateExecution} for validation. */ function execute( address account, @@ -68,18 +65,21 @@ abstract contract ERC7579Executor is IERC7579Module { bytes calldata executionCalldata, bytes32 salt ) public virtual returns (bytes[] memory returnData) { - bool allowed = canExecute(account, mode, executionCalldata, salt); - returnData = _execute(account, mode, executionCalldata, salt); // Prioritize errors thrown in _execute - require(allowed, ERC7579UnauthorizedExecution()); - return returnData; + _validateExecution(account, mode, executionCalldata, salt); + return _execute(account, mode, executionCalldata, salt); } /** - * @dev Internal version of {execute}. Emits {ERC7579ExecutorOperationExecuted} event. + * @dev Executes an operation. + * + * Emits {ERC7579ExecutorOperationExecuted} event. * * Requirements: * * * The `account` must implement the {IERC7579Execution-executeFromExecutor} function. + * + * NOTE: Calling this function directly will NOT perform any validation checks. + * Executing a operation should be done using {execute}. */ function _execute( address account, diff --git a/contracts/mocks/account/modules/ERC7579ExecutorMock.sol b/contracts/mocks/account/modules/ERC7579ExecutorMock.sol new file mode 100644 index 00000000..85dc0065 --- /dev/null +++ b/contracts/mocks/account/modules/ERC7579ExecutorMock.sol @@ -0,0 +1,20 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.27; + +import {Mode} from "@openzeppelin/contracts/account/utils/draft-ERC7579Utils.sol"; +import {ERC7579Executor} from "../../../account/modules/ERC7579Executor.sol"; + +contract ERC7579ExecutorMock is ERC7579Executor { + function onInstall(bytes calldata) public virtual {} + + function onUninstall(bytes calldata) public virtual {} + + function _validateExecution( + address account, + Mode /* mode */, + bytes calldata /* executionCalldata */, + bytes32 /* salt */ + ) internal view override { + if (account != msg.sender) revert ERC7579UnauthorizedExecution(); + } +} diff --git a/test/account/modules/ERC7579DelayedExecutor.test.js b/test/account/modules/ERC7579DelayedExecutor.test.js index d9fd9eae..11055a85 100644 --- a/test/account/modules/ERC7579DelayedExecutor.test.js +++ b/test/account/modules/ERC7579DelayedExecutor.test.js @@ -140,6 +140,13 @@ describe('ERC7579DelayedExecutor', function () { await this.mockAccountFromEntrypoint.installModule(this.moduleType, this.mock.target, this.installData); }); + it('reverts scheduling if the module is not installed', async function () { + await this.mockAccountFromEntrypoint.uninstallModule(this.moduleType, this.mock.target, '0x'); + await expect( + this.mock.schedule(this.mockAccount.address, this.mode, this.calldata, salt), + ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorModuleNotInstalled'); + }); + it('schedules an operation if called by the account', async function () { const id = this.mock.hashOperation(this.mockAccount.address, this.mode, this.calldata, salt); const tx = await this.mockFromAccount.schedule(this.mockAccount.address, this.mode, this.calldata, salt); @@ -147,15 +154,15 @@ describe('ERC7579DelayedExecutor', function () { await expect(tx) .to.emit(this.mock, 'ERC7579ExecutorOperationScheduled') .withArgs(this.mockAccount.address, id, this.mode, this.calldata, salt, now); - await expect( - this.mockFromAccount.schedule(this.mockAccount.address, this.mode, this.calldata, salt), - ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Can't schedule twice await expect( this.mock.getSchedule(this.mockAccount.address, this.mode, this.calldata, salt), ).to.eventually.deep.equal([now, now + this.delay, now + this.delay + this.expiration]); + await expect( + this.mockFromAccount.schedule(this.mockAccount.address, this.mode, this.calldata, salt), + ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Can't schedule twice }); - it('reverts with ERC7579ExecutorUnauthorizedSchedule if called by other account', async function () { + it('reverts with ERC7579ExecutorUnauthorizedSchedule if called by any other', async function () { await expect( this.mock.schedule(this.mockAccount.address, this.mode, this.calldata, salt), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnauthorizedSchedule'); @@ -168,50 +175,62 @@ describe('ERC7579DelayedExecutor', function () { await this.mock.$_schedule(this.mockAccount.address, this.mode, this.calldata, salt); }); - it('reverts with ERC7579ExecutorUnexpectedOperationState before delay passes with any caller', async function () { + it('reverts execution if the module is not installed', async function () { + await time.increase(this.delay); + await this.mockAccountFromEntrypoint.uninstallModule(this.moduleType, this.mock.target, '0x'); + await expect( + this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt), + ).to.be.revertedWithCustomError(this.mockAccount, 'ERC7579UninstalledModule'); + }); + + it('reverts if the operation is not scheduled', async function () { + await this.mockFromAccount.cancel(this.mockAccount.address, this.mode, this.calldata, salt); await expect( this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); }); - it('reverts with ERC7579ExecutorUnexpectedOperationState before delay passes with the account as caller', async function () { + it('reverts with ERC7579ExecutorUnexpectedOperationState before delay passes with any caller', async function () { + // call from the account await expect( this.mockFromAccount.execute(this.mockAccount.address, this.mode, this.calldata, salt), - ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Allowed, not ready + ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); + // call from anyone + await expect( + this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt), + ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); }); - it('executes if called by the account when delay passes but has not expired with any caller', async function () { + it('executes when delay passes but has not expired if called by the account', async function () { await time.increase(this.delay); - await expect(this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt)) + await expect(this.mockFromAccount.execute(this.mockAccount.address, this.mode, this.calldata, salt)) .to.emit(this.target, 'MockFunctionCalledWithArgs') .withArgs(...this.args); await expect( - this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt), + this.mockFromAccount.execute(this.mockAccount.address, this.mode, this.calldata, salt), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Can't execute twice }); - it('executes if called by the account when delay passes but has not expired with the account as caller', async function () { + it('executes when delay passes but has not expired if called by any other caller', async function () { await time.increase(this.delay); - await expect(this.mockFromAccount.execute(this.mockAccount.address, this.mode, this.calldata, salt)) + await expect(this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt)) .to.emit(this.target, 'MockFunctionCalledWithArgs') .withArgs(...this.args); await expect( - this.mockFromAccount.execute(this.mockAccount.address, this.mode, this.calldata, salt), + this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Can't execute twice }); it('reverts with ERC7579ExecutorUnexpectedOperationState if the operation was expired with any caller', async function () { await time.increase(this.delay + this.expiration); + // call from anyone await expect( this.mock.execute(this.mockAccount.address, this.mode, this.calldata, salt), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); - }); - - it('reverts if the operation was expired with the account as caller', async function () { - await time.increase(this.delay + this.expiration); + // call from the account await expect( this.mockFromAccount.execute(this.mockAccount.address, this.mode, this.calldata, salt), - ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Allowed, expired + ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); }); }); @@ -231,7 +250,7 @@ describe('ERC7579DelayedExecutor', function () { ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Can't cancel twice }); - it('reverts with ERC7579ExecutorUnauthorizedCancellation if called by other account', async function () { + it('reverts with ERC7579ExecutorUnauthorizedCancellation if called by any other', async function () { await expect( this.mock.cancel(this.mockAccount.address, this.mode, this.calldata, salt), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnauthorizedCancellation'); diff --git a/test/account/modules/ERC7579Executor.test.js b/test/account/modules/ERC7579Executor.test.js index f938d403..9337e296 100644 --- a/test/account/modules/ERC7579Executor.test.js +++ b/test/account/modules/ERC7579Executor.test.js @@ -15,7 +15,7 @@ const { shouldBehaveLikeERC7579Module } = require('./ERC7579Module.behavior'); async function fixture() { // Deploy ERC-7579 validator module - const mock = await ethers.deployContract('$ERC7579MultisigExecutorMock'); + const mock = await ethers.deployContract('$ERC7579ExecutorMock'); const target = await ethers.deployContract('CallReceiverMockExtended'); // ERC-4337 env diff --git a/test/utils/cryptography/ERC7739Utils.test.js b/test/utils/cryptography/ERC7739Utils.test.js index c568919a..bcd24c64 100644 --- a/test/utils/cryptography/ERC7739Utils.test.js +++ b/test/utils/cryptography/ERC7739Utils.test.js @@ -203,7 +203,7 @@ describe('ERC7739Utils', function () { it(descr, async function () { await expect(this.mock.$decodeContentsDescr(contentsDescr)).to.eventually.deep.equal([ contentTypeName ?? '', - contentTypeName ? (contentType ?? contentsDescr) : '', + contentTypeName ? contentType ?? contentsDescr : '', ]); }); }