diff --git a/contracts/account/modules/ERC7579DelayedExecutor.sol b/contracts/account/modules/ERC7579DelayedExecutor.sol index 49a2b4a9..e800fa79 100644 --- a/contracts/account/modules/ERC7579DelayedExecutor.sol +++ b/contracts/account/modules/ERC7579DelayedExecutor.sol @@ -32,6 +32,12 @@ import {ERC7579Executor} from "./ERC7579Executor.sol"; * after a transition period defined by the current delay or {minSetback}, whichever * is longer. * + * ==== Authorization + * + * Authorization for scheduling and canceling operations is controlled through the {_validateSchedule} + * and {_validateCancel} functions. These functions can be overridden to implement custom + * authorization logic, such as requiring specific signers or roles. + * * TIP: Use {_scheduleAt} to schedule operations at a specific points in time. This is * useful to pre-schedule operations for non-deployed accounts (e.g. subscriptions). */ @@ -94,12 +100,6 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { bytes32 allowedStates ); - /// @dev The operation is not authorized to be canceled. - error ERC7579ExecutorUnauthorizedCancellation(); - - /// @dev The operation is not authorized to be scheduled. - error ERC7579ExecutorUnauthorizedSchedule(); - /// @dev The module is not installed on the account. error ERC7579ExecutorModuleNotInstalled(); @@ -232,10 +232,9 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { */ function schedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual { require(_config[account].installed, ERC7579ExecutorModuleNotInstalled()); - bool allowed = _validateSchedule(account, salt, mode, data); + _validateSchedule(account, salt, mode, data); (uint32 executableAfter, , ) = getDelay(account); _scheduleAt(account, salt, mode, data, Time.timestamp(), executableAfter); - require(allowed, ERC7579ExecutorUnauthorizedSchedule()); } /** @@ -243,9 +242,8 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { * scheduled the operation. See {_cancel}. */ function cancel(address account, bytes32 salt, bytes32 mode, bytes calldata data) public virtual { - bool allowed = _validateCancel(account, salt, mode, data); + _validateCancel(account, salt, mode, data); _cancel(account, mode, data, salt); // Prioritize errors thrown in _cancel - require(allowed, ERC7579ExecutorUnauthorizedCancellation()); } /** @@ -267,32 +265,30 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { _setExpiration(msg.sender, 0); } - /// @inheritdoc ERC7579Executor + /** + * @dev Returns `data` as the execution calldata. See {ERC7579Executor-_execute}. + * + * NOTE: This function relies on the operation state validation in {_execute} for + * authorization. Extensions of this module should override this function to implement + * additional validation logic if needed. + */ function _validateExecution( address /* account */, bytes32 /* salt */, bytes32 /* mode */, bytes calldata data - ) internal view virtual override returns (bool valid, bytes calldata executionCalldata) { - return (true, data); // Anyone can execute, the state validation of the operation is enough + ) internal virtual override returns (bytes calldata) { + return data; } /** - * @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 whether an operation can be canceled. * * Example extension: * * ```solidity - * function _validateCancel( - * address account, - * bytes32 mode, - * bytes calldata data, - * bytes32 salt - * ) internal view override returns (bool) { - * bool isAuthorized = ...; // custom logic to check authorization - * return isAuthorized || super._validateCancel(account, mode, data, salt); + * function _validateCancel(address account, bytes32 salt, bytes32 mode, bytes calldata data) internal override { + * // e.g. require(msg.sender == account); * } *``` */ @@ -301,26 +297,16 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { bytes32 /* salt */, bytes32 /* mode */, bytes calldata /* data */ - ) internal view virtual returns (bool) { - return account == msg.sender; - } + ) internal virtual; /** - * @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 whether an operation can be scheduled. * * Example extension: * * ```solidity - * function _validateSchedule( - * address account, - * bytes32 mode, - * bytes calldata data, - * bytes32 salt - * ) internal view override returns (bool) { - * bool isAuthorized = ...; // custom logic to check authorization - * return isAuthorized || super._validateSchedule(account, mode, data, salt); + * function _validateSchedule(address account, bytes32 salt, bytes32 mode, bytes calldata data) internal override { + * // e.g. require(msg.sender == account); * } *``` */ @@ -329,9 +315,7 @@ abstract contract ERC7579DelayedExecutor is ERC7579Executor { bytes32 /* salt */, bytes32 /* mode */, bytes calldata /* data */ - ) internal view virtual returns (bool) { - return account == msg.sender; - } + ) internal virtual; /** * @dev Internal implementation for setting an account's delay. See {getDelay}. diff --git a/contracts/account/modules/ERC7579Executor.sol b/contracts/account/modules/ERC7579Executor.sol index 955e0cea..594ffe98 100644 --- a/contracts/account/modules/ERC7579Executor.sol +++ b/contracts/account/modules/ERC7579Executor.sol @@ -25,9 +25,6 @@ abstract contract ERC7579Executor is IERC7579Module { bytes executionCalldata ); - /// @dev Thrown when the execution is invalid. See {_validateExecution} for details. - error ERC7579InvalidExecution(); - /// @inheritdoc IERC7579Module function isModuleType(uint256 moduleTypeId) public pure virtual returns (bool) { return moduleTypeId == MODULE_TYPE_EXECUTOR; @@ -44,36 +41,38 @@ abstract contract ERC7579Executor is IERC7579Module { bytes32 mode, bytes calldata data ) public virtual returns (bytes[] memory returnData) { - (bool allowed, bytes calldata executionCalldata) = _validateExecution(account, salt, mode, data); + bytes calldata executionCalldata = _validateExecution(account, salt, mode, data); returnData = _execute(account, mode, salt, executionCalldata); // Prioritize errors thrown in _execute - require(allowed, ERC7579InvalidExecution()); return returnData; } /** - * @dev Check if the caller is authorized to execute operations. - * Derived contracts can implement this with custom authorization logic. + * @dev Validates whether the execution can proceed. This function is called before executing + * the operation and returns the execution calldata to be used. * * Example extension: * * ```solidity - * function _validateExecution( - * address account, - * bytes32 salt, - * bytes32 mode, - * bytes calldata data - * ) internal view override returns (bool valid, bytes calldata executionCalldata) { - * /// ... - * return isAuthorized; // custom logic to check authorization + * function _validateExecution(address account, bytes32 salt, bytes32 mode, bytes calldata data) + * internal + * override + * returns (bytes calldata) + * { + * // custom logic + * return data; * } *``` + * + * TIP: Pack extra data in the `data` arguments (e.g. a signature) to be used in the + * validation process. Calldata can be sliced to extract it and return only the + * execution calldata. */ function _validateExecution( address account, bytes32 salt, bytes32 mode, bytes calldata data - ) internal view virtual returns (bool valid, bytes calldata executionCalldata); + ) internal virtual returns (bytes calldata); /** * @dev Internal version of {execute}. Emits {ERC7579ExecutorOperationExecuted} event. diff --git a/contracts/mocks/account/modules/ERC7579ExecutorMocks.sol b/contracts/mocks/account/modules/ERC7579ExecutorMocks.sol new file mode 100644 index 00000000..85f7dd13 --- /dev/null +++ b/contracts/mocks/account/modules/ERC7579ExecutorMocks.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.27; + +import {ERC7579Executor} from "../../../account/modules/ERC7579Executor.sol"; +import {ERC7579DelayedExecutor} from "../../../account/modules/ERC7579DelayedExecutor.sol"; + +abstract contract ERC7579ExecutorMock is ERC7579Executor { + function onInstall(bytes calldata data) external {} + + function onUninstall(bytes calldata data) external {} + + function _validateExecution( + address, + bytes32, + bytes32, + bytes calldata data + ) internal pure override returns (bytes calldata) { + return data; + } +} + +abstract contract ERC7579DelayedExecutorMock is ERC7579DelayedExecutor { + function _validateSchedule(address account, bytes32, bytes32, bytes calldata) internal view override { + require(msg.sender == account); + } + + function _validateCancel(address account, bytes32, bytes32, bytes calldata) internal view override { + require(msg.sender == account); + } +} diff --git a/contracts/mocks/account/modules/ERC7579MultisigMocks.sol b/contracts/mocks/account/modules/ERC7579MultisigMocks.sol index 08678bb2..6f2b9a76 100644 --- a/contracts/mocks/account/modules/ERC7579MultisigMocks.sol +++ b/contracts/mocks/account/modules/ERC7579MultisigMocks.sol @@ -20,18 +20,12 @@ abstract contract ERC7579MultisigExecutorMock is EIP712, ERC7579Executor, ERC757 bytes32 salt, bytes32 mode, bytes calldata data - ) internal view override returns (bool valid, bytes calldata executionCalldata) { + ) internal view override returns (bytes calldata) { uint16 executionCalldataLength = uint16(uint256(bytes32(data[0:2]))); // First 2 bytes are the length - bytes calldata actualExecutionCalldata = data[2:2 + executionCalldataLength]; // Next bytes are the calldata - bytes calldata signature = data[2 + executionCalldataLength:]; // Remaining bytes are the signature - return ( - _validateMultisignature( - account, - _getExecuteTypeHash(account, salt, mode, actualExecutionCalldata), - signature - ), - actualExecutionCalldata - ); + bytes calldata executionCalldata = data[2:2 + executionCalldataLength]; // Next bytes are the calldata + bytes32 typeHash = _getExecuteTypeHash(account, salt, mode, executionCalldata); + require(_validateMultisignature(account, typeHash, data[2 + executionCalldataLength:])); // Remaining bytes are the signature + return executionCalldata; } function _getExecuteTypeHash( @@ -54,18 +48,12 @@ abstract contract ERC7579MultisigWeightedExecutorMock is EIP712, ERC7579Executor bytes32 salt, bytes32 mode, bytes calldata data - ) internal view override returns (bool valid, bytes calldata executionCalldata) { + ) internal view override returns (bytes calldata) { uint16 executionCalldataLength = uint16(uint256(bytes32(data[0:2]))); // First 2 bytes are the length - bytes calldata actualExecutionCalldata = data[2:2 + executionCalldataLength]; // Next bytes are the calldata - bytes calldata signature = data[2 + executionCalldataLength:]; // Remaining bytes are the signature - return ( - _validateMultisignature( - account, - _getExecuteTypeHash(account, salt, mode, actualExecutionCalldata), - signature - ), - actualExecutionCalldata - ); + bytes calldata executionCalldata = data[2:2 + executionCalldataLength]; // Next bytes are the calldata + bytes32 typeHash = _getExecuteTypeHash(account, salt, mode, executionCalldata); + require(_validateMultisignature(account, typeHash, data[2 + executionCalldataLength:])); // Remaining bytes are the signature + return executionCalldata; } function _getExecuteTypeHash( @@ -88,18 +76,12 @@ abstract contract ERC7579MultisigConfirmationExecutorMock is ERC7579Executor, ER bytes32 salt, bytes32 mode, bytes calldata data - ) internal view override returns (bool valid, bytes calldata executionCalldata) { + ) internal view override returns (bytes calldata) { uint16 executionCalldataLength = uint16(uint256(bytes32(data[0:2]))); // First 2 bytes are the length - bytes calldata actualExecutionCalldata = data[2:2 + executionCalldataLength]; // Next bytes are the calldata - bytes calldata signature = data[2 + executionCalldataLength:]; // Remaining bytes are the signature - return ( - _validateMultisignature( - account, - _getExecuteTypeHash(account, salt, mode, actualExecutionCalldata), - signature - ), - actualExecutionCalldata - ); + bytes calldata executionCalldata = data[2:2 + executionCalldataLength]; // Next bytes are the calldata + bytes32 typeHash = _getExecuteTypeHash(account, salt, mode, executionCalldata); + require(_validateMultisignature(account, typeHash, data[2 + executionCalldataLength:])); // Remaining bytes are the signature + return executionCalldata; } function _getExecuteTypeHash( diff --git a/test/account/modules/ERC7579DelayedExecutor.test.js b/test/account/modules/ERC7579DelayedExecutor.test.js index 5be26ebd..500287e6 100644 --- a/test/account/modules/ERC7579DelayedExecutor.test.js +++ b/test/account/modules/ERC7579DelayedExecutor.test.js @@ -18,7 +18,7 @@ async function fixture() { const [other] = await ethers.getSigners(); // Deploy ERC-7579 validator module - const mock = await ethers.deployContract('$ERC7579DelayedExecutor'); + const mock = await ethers.deployContract('$ERC7579DelayedExecutorMock'); const target = await ethers.deployContract('CallReceiverMockExtended'); // ERC-4337 env @@ -216,12 +216,6 @@ describe('ERC7579DelayedExecutor', function () { ).to.eventually.deep.equal([now, now + this.delay, now + this.delay + this.expiration]); }); - it('reverts with ERC7579ExecutorUnauthorizedSchedule if called by other account', async function () { - await expect( - this.mock.schedule(this.mockAccount.address, salt, this.mode, this.calldata), - ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnauthorizedSchedule'); - }); - it('reverts with ERC7579ExecutorModuleNotInstalled if the module is not installed', async function () { await expect( this.mock.schedule(this.other.address, salt, this.mode, this.calldata), @@ -301,11 +295,5 @@ describe('ERC7579DelayedExecutor', function () { this.mockFromAccount.cancel(this.mockAccount.address, salt, this.mode, this.calldata), ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnexpectedOperationState'); // Can't cancel twice }); - - it('reverts with ERC7579ExecutorUnauthorizedCancellation if called by other account', async function () { - await expect( - this.mock.cancel(this.mockAccount.address, salt, this.mode, this.calldata), - ).to.be.revertedWithCustomError(this.mock, 'ERC7579ExecutorUnauthorizedCancellation'); - }); }); }); diff --git a/test/account/modules/ERC7579Executor.test.js b/test/account/modules/ERC7579Executor.test.js index 2e6b1caf..5f4b6ac6 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', ['MultisigExecutor', '1']); + const mock = await ethers.deployContract('$ERC7579ExecutorMock'); const target = await ethers.deployContract('CallReceiverMockExtended'); // ERC-4337 env