diff --git a/modules/4337/contracts/Safe4337Module.sol b/modules/4337/contracts/Safe4337Module.sol index d83b79624..4348002e5 100644 --- a/modules/4337/contracts/Safe4337Module.sol +++ b/modules/4337/contracts/Safe4337Module.sol @@ -134,7 +134,7 @@ contract Safe4337Module is IAccount, HandlerContext, CompatibilityFallbackHandle PackedUserOperation calldata userOp, bytes32, uint256 missingAccountFunds - ) external onlySupportedEntryPoint returns (uint256 validationData) { + ) external onlySupportedEntryPoint virtual returns (uint256 validationData) { address payable safeAddress = payable(userOp.sender); // The entry point address is appended to the calldata by the Safe in the `FallbackManager` contract, // following ERC-2771. Because of this, the relayer may manipulate the entry point address, therefore diff --git a/modules/6900/contracts/Safe6900DelegateCallReceiver.sol b/modules/6900/contracts/Safe6900DelegateCallReceiver.sol index b0e117c29..308477834 100644 --- a/modules/6900/contracts/Safe6900DelegateCallReceiver.sol +++ b/modules/6900/contracts/Safe6900DelegateCallReceiver.sol @@ -7,8 +7,9 @@ import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {PluginManifest} from "./interfaces/DataTypes.sol"; import {IPlugin} from "./interfaces/IPlugin.sol"; import {IPluginManager} from "./interfaces/IPluginManager.sol"; +import {Base6900} from "./base/Base6900.sol"; -abstract contract Safe6900DelegateCallReceiver is SafeStorage, IPluginManager { +abstract contract Safe6900DelegateCallReceiver is SafeStorage, IPluginManager, Base6900 { struct PluginData { address plugin; bytes32 manifestHash; @@ -28,11 +29,6 @@ abstract contract Safe6900DelegateCallReceiver is SafeStorage, IPluginManager { mapping(bytes4 => SelectorData) selectorData; } - /** - * @notice Address of this contract - */ - address public immutable SELF; - /// @dev Store the installed plugin info for an account. ERC6900AccountData private accountData; @@ -50,17 +46,9 @@ abstract contract Safe6900DelegateCallReceiver is SafeStorage, IPluginManager { * @notice Constructor */ constructor() { - SELF = address(this); } - /** - * @notice Modifier to make a function callable via delegatecall only. - * If the function is called via a regular call, it will revert. - */ - modifier onlyDelegateCall() { - require(address(this) != SELF, "must only be called via delegatecall"); - _; - } + function installPluginDelegateCallReceiver( address plugin, diff --git a/modules/6900/contracts/Safe6900Module.sol b/modules/6900/contracts/Safe6900Module.sol index 20fd4db82..164e3c34d 100644 --- a/modules/6900/contracts/Safe6900Module.sol +++ b/modules/6900/contracts/Safe6900Module.sol @@ -3,8 +3,10 @@ pragma solidity ^0.8.23; import {PluginManager} from "./base/PluginManager.sol"; import {Executor} from "./base/Executor.sol"; +import {Safe4337} from "./base/Safe4337.sol"; import {EntryPointValidator} from "./base/EntryPointValidator.sol"; -contract Safe6900Module is PluginManager, Executor { - constructor(address entryPoint) EntryPointValidator(entryPoint) {} + +contract Safe6900Module is PluginManager, Executor, Safe4337 { + constructor(address entryPoint) EntryPointValidator(entryPoint) Safe4337(entryPoint) {} } diff --git a/modules/6900/contracts/base/Base6900.sol b/modules/6900/contracts/base/Base6900.sol new file mode 100644 index 000000000..7cebc31cc --- /dev/null +++ b/modules/6900/contracts/base/Base6900.sol @@ -0,0 +1,25 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity ^0.8.23; + +abstract contract Base6900 { + /** + * @notice Address of this contract + */ + address public immutable SELF; + + /** + * @notice Constructor + */ + constructor() { + SELF = address(this); + } + + /** + * @notice Modifier to make a function callable via delegatecall only. + * If the function is called via a regular call, it will revert. + */ + modifier onlyDelegateCall() { + require(address(this) != SELF, "must only be called via delegatecall"); + _; + } +} diff --git a/modules/6900/contracts/base/Executor.sol b/modules/6900/contracts/base/Executor.sol index 49d8edc4e..11445117f 100644 --- a/modules/6900/contracts/base/Executor.sol +++ b/modules/6900/contracts/base/Executor.sol @@ -5,13 +5,13 @@ import {IStandardExecutor, Call} from "../interfaces/IStandardExecutor.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IPlugin} from "../interfaces/IPlugin.sol"; -import {IAccount} from "../interfaces/IAccount.sol"; +import {ISafe} from "../interfaces/ISafe.sol"; import {HandlerContext} from "@safe-global/safe-contracts/contracts/handler/HandlerContext.sol"; import {EntryPointValidator} from "../base/EntryPointValidator.sol"; abstract contract Executor is IStandardExecutor, HandlerContext, EntryPointValidator { error CallToPluginNotAllowed(address plugin); - error ExecutionFailed(); + error TxExecutionFailed(); function execute(address target, uint256 value, bytes calldata data) external payable override returns (bytes memory) { return _execute(target, value, data); @@ -33,7 +33,7 @@ abstract contract Executor is IStandardExecutor, HandlerContext, EntryPointValid validateExecution(); - (bool isActionSuccessful, bytes memory resultData) = IAccount(msg.sender).execTransactionFromModuleReturnData( + (bool isActionSuccessful, bytes memory resultData) = ISafe(msg.sender).execTransactionFromModuleReturnData( target, value, data, @@ -42,7 +42,7 @@ abstract contract Executor is IStandardExecutor, HandlerContext, EntryPointValid if (isActionSuccessful) { return resultData; } else { - revert ExecutionFailed(); + revert TxExecutionFailed(); } } diff --git a/modules/6900/contracts/base/PluginManager.sol b/modules/6900/contracts/base/PluginManager.sol index ef985317d..e028da4a1 100644 --- a/modules/6900/contracts/base/PluginManager.sol +++ b/modules/6900/contracts/base/PluginManager.sol @@ -6,7 +6,7 @@ import {IPlugin} from "../interfaces/IPlugin.sol"; import {OnlyAccountCallable} from "../base/OnlyAccountCallable.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {PluginManifest} from "../interfaces/DataTypes.sol"; -import {IAccount} from "../interfaces/IAccount.sol"; +import {ISafe} from "../interfaces/ISafe.sol"; import {HandlerContext} from "@safe-global/safe-contracts/contracts/handler/HandlerContext.sol"; import {Safe6900DelegateCallReceiver} from "../Safe6900DelegateCallReceiver.sol"; @@ -20,7 +20,7 @@ abstract contract PluginManager is IPluginManager, OnlyAccountCallable, HandlerC ) public override onlyAccount { address safe = _msgSender(); - (bool success, bytes memory data) = IAccount(safe).execTransactionFromModuleReturnData( + (bool success, bytes memory data) = ISafe(safe).execTransactionFromModuleReturnData( payable(address(this)), 0, abi.encodeWithSelector( @@ -44,7 +44,7 @@ abstract contract PluginManager is IPluginManager, OnlyAccountCallable, HandlerC function uninstallPlugin(address plugin, bytes calldata config, bytes calldata pluginUninstallData) external override onlyAccount { address safe = _msgSender(); - (bool success, bytes memory data) = IAccount(safe).execTransactionFromModuleReturnData( + (bool success, bytes memory data) = ISafe(safe).execTransactionFromModuleReturnData( payable(address(this)), 0, abi.encodeWithSelector( @@ -66,7 +66,7 @@ abstract contract PluginManager is IPluginManager, OnlyAccountCallable, HandlerC } function isPluginInstalled(address safe, address plugin) external returns (bool) { - (bool success, bytes memory data) = IAccount(safe).execTransactionFromModuleReturnData( + (bool success, bytes memory data) = ISafe(safe).execTransactionFromModuleReturnData( payable(address(this)), 0, abi.encodeWithSelector(Safe6900DelegateCallReceiver.isPluginInstalledDelegateCallReceiver.selector, plugin), diff --git a/modules/6900/contracts/base/Safe4337.sol b/modules/6900/contracts/base/Safe4337.sol new file mode 100644 index 000000000..0cec3a717 --- /dev/null +++ b/modules/6900/contracts/base/Safe4337.sol @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity ^0.8.23; + +import {Safe4337Module} from "@safe-global/safe-4337/contracts/Safe4337Module.sol"; +import {PackedUserOperation} from "@account-abstraction/contracts/interfaces/PackedUserOperation.sol"; +import {ISafe} from "../interfaces/ISafe.sol"; +import {Base6900} from "./Base6900.sol"; + +contract Safe4337 is Safe4337Module, Base6900 { + constructor(address _entryPoint) Safe4337Module(_entryPoint) {} + + function validateUserOp( + PackedUserOperation calldata userOp, + bytes32, + uint256 missingAccountFunds + ) external override onlySupportedEntryPoint returns (uint256 validationData) { + (bool success, bytes memory data) = ISafe(userOp.sender).execTransactionFromModuleReturnData( + payable(address(this)), + 0, + abi.encodeWithSelector(Safe4337.validateUserOpDelegatecallReciever.selector, userOp, missingAccountFunds), + 1 // delegatecall + ); + + if (!success) { + if (data.length == 0) revert(); + assembly { + // We use Yul's revert() to bubble up errors from the target contract. + revert(add(32, data), mload(data)) + } + } + + validationData = abi.decode(data, (uint256)); + } + + function validateUserOpDelegatecallReciever( + PackedUserOperation calldata userOp, + bytes32, + uint256 missingAccountFunds + ) external onlyDelegateCall returns (uint256 validationData) { + address payable safeAddress = payable(userOp.sender); + // The entry point address is appended to the calldata by the Safe in the `FallbackManager` contract, + // following ERC-2771. Because of this, the relayer may manipulate the entry point address, therefore + // we have to verify that the sender is the Safe specified in the userOperation. + if (safeAddress != msg.sender) { + revert InvalidCaller(); + } + + // We check the execution function signature to make sure the entry point can't call any other function + // and make sure the execution of the user operation is handled by the module + bytes4 selector = bytes4(userOp.callData); + if (selector != this.executeUserOp.selector && selector != this.executeUserOpWithErrorString.selector) { + revert UnsupportedExecutionFunction(selector); + } + + // The userOp nonce is validated in the entry point (for 0.6.0+), therefore we will not check it again + validationData = _validateSignatures(userOp); + + // We trust the entry point to set the correct prefund value, based on the operation params + // We need to perform this even if the signature is not valid, else the simulation function of the entry point will not work. + if (missingAccountFunds != 0) { + // We intentionally ignore errors in paying the missing account funds, as the entry point is responsible for + // verifying the prefund has been paid. This behaviour matches the reference base account implementation. + ISafe(safeAddress).execTransactionFromModule(SUPPORTED_ENTRYPOINT, missingAccountFunds, "", 0); + } + } +} diff --git a/modules/6900/contracts/interfaces/IAccount.sol b/modules/6900/contracts/interfaces/ISafe.sol similarity index 81% rename from modules/6900/contracts/interfaces/IAccount.sol rename to modules/6900/contracts/interfaces/ISafe.sol index 860217e29..d7751c288 100644 --- a/modules/6900/contracts/interfaces/IAccount.sol +++ b/modules/6900/contracts/interfaces/ISafe.sol @@ -2,11 +2,11 @@ pragma solidity ^0.8.23; /** - * @title IAccount Declares the functions that are called on an account. + * @title ISafe Declares the functions that are called on an Safe account. */ -interface IAccount { +interface ISafe { function execTransactionFromModule( - address payable to, + address to, uint256 value, bytes calldata data, uint8 operation diff --git a/modules/6900/package.json b/modules/6900/package.json index ab357923b..8d2d3f216 100644 --- a/modules/6900/package.json +++ b/modules/6900/package.json @@ -51,6 +51,7 @@ "@openzeppelin/contracts": "^5.0.2", "@safe-global/mock-contract": "^4.1.0", "@safe-global/safe-4337-local-bundler": "workspace:^", + "@safe-global/safe-4337": "workspace:^", "@types/chai": "^4.3.14", "@types/mocha": "^10.0.6", "@types/node": "^20.11.30", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 50a95c351..5183cb8d7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -225,6 +225,82 @@ importers: specifier: ^17.7.2 version: 17.7.2 + modules/6900: + dependencies: + '@safe-global/safe-contracts': + specifier: ^1.4.1-build.0 + version: 1.4.1-build.0(ethers@6.13.1(bufferutil@4.0.8)(utf-8-validate@5.0.10)) + devDependencies: + '@account-abstraction/contracts': + specifier: ^0.7.0 + version: 0.7.0 + '@nomicfoundation/hardhat-ethers': + specifier: ^3.0.6 + version: 3.0.6(ethers@6.13.1(bufferutil@4.0.8)(utf-8-validate@5.0.10))(hardhat@2.22.5(bufferutil@4.0.8)(ts-node@10.9.2(@swc/core@1.6.5)(@types/node@20.14.8)(typescript@5.5.2))(typescript@5.5.2)(utf-8-validate@5.0.10)) + '@nomicfoundation/hardhat-toolbox': + specifier: ^5.0.0 + version: 5.0.0(4aqepof5a6e772tyoaoz72gjuy) + '@openzeppelin/contracts': + specifier: ^5.0.2 + version: 5.0.2 + '@safe-global/mock-contract': + specifier: ^4.1.0 + version: 4.1.0 + '@safe-global/safe-4337': + specifier: workspace:^ + version: link:../4337 + '@safe-global/safe-4337-local-bundler': + specifier: workspace:^ + version: link:../../packages/4337-local-bundler + '@types/chai': + specifier: ^4.3.14 + version: 4.3.16 + '@types/mocha': + specifier: ^10.0.6 + version: 10.0.7 + '@types/node': + specifier: ^20.11.30 + version: 20.14.8 + '@types/yargs': + specifier: ^17.0.32 + version: 17.0.32 + cbor: + specifier: ^9.0.2 + version: 9.0.2 + debug: + specifier: ^4.3.4 + version: 4.3.5 + dotenv: + specifier: ^16.4.5 + version: 16.4.5 + ethers: + specifier: ^6.13.1 + version: 6.13.1(bufferutil@4.0.8)(utf-8-validate@5.0.10) + hardhat: + specifier: ^2.22.2 + version: 2.22.5(bufferutil@4.0.8)(ts-node@10.9.2(@swc/core@1.6.5)(@types/node@20.14.8)(typescript@5.5.2))(typescript@5.5.2)(utf-8-validate@5.0.10) + hardhat-deploy: + specifier: ^0.12.2 + version: 0.12.4(bufferutil@4.0.8)(utf-8-validate@5.0.10) + husky: + specifier: ^9.0.11 + version: 9.0.11 + solc: + specifier: ^0.8.25 + version: 0.8.26(debug@4.3.5) + solhint: + specifier: ^4.5.2 + version: 4.5.4(typescript@5.5.2) + ts-node: + specifier: ^10.9.2 + version: 10.9.2(@swc/core@1.6.5)(@types/node@20.14.8)(typescript@5.5.2) + typescript: + specifier: ^5.4.3 + version: 5.5.2 + yargs: + specifier: ^17.7.2 + version: 17.7.2 + modules/allowances: devDependencies: '@nomicfoundation/hardhat-toolbox': @@ -4423,6 +4499,10 @@ packages: engines: {node: '>=10.0.0'} hasBin: true + solhint@4.5.4: + resolution: {integrity: sha512-Cu1XiJXub2q1eCr9kkJ9VPv1sGcmj3V7Zb76B0CoezDOB9bu3DxKIFFH7ggCl9fWpEPD6xBmRLfZrYijkVmujQ==} + hasBin: true + solhint@5.0.1: resolution: {integrity: sha512-QeQLS9HGCnIiibt+xiOa/+MuP7BWz9N7C5+Mj9pLHshdkNhuo3AzCpWmjfWVZBUuwIUO3YyCRVIcYLR3YOKGfg==} hasBin: true @@ -10156,7 +10236,7 @@ snapshots: rimraf@2.7.1: dependencies: - glob: 7.2.0 + glob: 7.2.3 rimraf@3.0.2: dependencies: @@ -10350,6 +10430,31 @@ snapshots: transitivePeerDependencies: - debug + solhint@4.5.4(typescript@5.5.2): + dependencies: + '@solidity-parser/parser': 0.18.0 + ajv: 6.12.6 + antlr4: 4.13.1-patch-1 + ast-parents: 0.0.1 + chalk: 4.1.2 + commander: 10.0.1 + cosmiconfig: 8.3.6(typescript@5.5.2) + fast-diff: 1.3.0 + glob: 8.1.0 + ignore: 5.3.1 + js-yaml: 4.1.0 + latest-version: 7.0.0 + lodash: 4.17.21 + pluralize: 8.0.0 + semver: 7.6.2 + strip-ansi: 6.0.1 + table: 6.8.2 + text-table: 0.2.0 + optionalDependencies: + prettier: 2.8.8 + transitivePeerDependencies: + - typescript + solhint@5.0.1(typescript@5.4.5): dependencies: '@solidity-parser/parser': 0.18.0