From f95611a63fe4c24d2674c9b1fcce05655cdb4c8f Mon Sep 17 00:00:00 2001 From: adam Date: Mon, 19 Aug 2024 12:02:19 -0700 Subject: [PATCH 1/2] feat: add interface checks for validations and hooks --- src/account/ModuleManagerInternals.sol | 33 ++++++++++++++----- src/modules/ERC20TokenLimitModule.sol | 2 +- .../permissionhooks/AllowlistModule.sol | 11 +++++++ .../SingleSignerValidationModule.sol | 18 ++++++++-- test/account/UpgradeableModularAccount.t.sol | 2 +- test/mocks/modules/DirectCallModule.sol | 12 +++++++ .../modules/MockAccessControlHookModule.sol | 11 +++++++ 7 files changed, 76 insertions(+), 13 deletions(-) diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index 5b8e39cd..018811cf 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -10,9 +10,13 @@ import {HookConfigLib} from "../helpers/HookConfigLib.sol"; import {KnownSelectors} from "../helpers/KnownSelectors.sol"; import {ModuleEntityLib} from "../helpers/ModuleEntityLib.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; +import {IExecutionHookModule} from "../interfaces/IExecutionHookModule.sol"; import {ExecutionManifest, ManifestExecutionHook} from "../interfaces/IExecutionModule.sol"; import {HookConfig, IModularAccount, ModuleEntity, ValidationConfig} from "../interfaces/IModularAccount.sol"; import {IModule} from "../interfaces/IModule.sol"; +import {IValidationHookModule} from "../interfaces/IValidationHookModule.sol"; +import {IValidationModule} from "../interfaces/IValidationModule.sol"; + import { AccountStorage, ExecutionData, @@ -32,11 +36,11 @@ abstract contract ModuleManagerInternals is IModularAccount { error Erc4337FunctionNotAllowed(bytes4 selector); error ExecutionFunctionAlreadySet(bytes4 selector); error IModuleFunctionNotAllowed(bytes4 selector); + error InterfaceNotSupported(address module); error NativeFunctionNotAllowed(bytes4 selector); error NullModule(); error PermissionAlreadySet(ModuleEntity validationFunction, HookConfig hookConfig); error ModuleInstallCallbackFailed(address module, bytes revertReason); - error ModuleInterfaceNotSupported(address module); error ModuleNotInstalled(address module); error PreValidationHookLimitExceeded(); error ValidationAlreadySet(bytes4 selector, ModuleEntity validationFunction); @@ -134,10 +138,8 @@ abstract contract ModuleManagerInternals is IModularAccount { revert NullModule(); } - // TODO: do we need this check? Or switch to a non-165 checking function? - // Check that the module supports the IModule interface. if (!ERC165Checker.supportsInterface(module, type(IModule).interfaceId)) { - revert ModuleInterfaceNotSupported(module); + revert InterfaceNotSupported(module); } // Update components according to the manifest. @@ -228,6 +230,16 @@ abstract contract ModuleManagerInternals is IModularAccount { } } + function _onInstall(address module, bytes calldata data, bytes4 interfaceId) internal { + if (data.length > 0) { + if (!ERC165Checker.supportsInterface(module, interfaceId)) { + revert InterfaceNotSupported(module); + } + + IModule(module).onInstall(data); + } + } + function _onUninstall(address module, bytes calldata data) internal returns (bool onUninstallSuccess) { onUninstallSuccess = true; if (data.length > 0) { @@ -261,12 +273,17 @@ abstract contract ModuleManagerInternals is IModularAccount { if (_validationData.preValidationHooks.length > MAX_PRE_VALIDATION_HOOKS) { revert PreValidationHookLimitExceeded(); } - } // Hook is an execution hook - else if (!_validationData.permissionHooks.add(toSetValue(hookConfig))) { + + _onInstall(hookConfig.module(), hookData, type(IValidationHookModule).interfaceId); + + continue; + } + // Hook is a permission hook + if (!_validationData.permissionHooks.add(toSetValue(hookConfig))) { revert PermissionAlreadySet(moduleEntity, hookConfig); } - _onInstall(hookConfig.module(), hookData); + _onInstall(hookConfig.module(), hookData, type(IExecutionHookModule).interfaceId); } for (uint256 i = 0; i < selectors.length; ++i) { @@ -279,7 +296,7 @@ abstract contract ModuleManagerInternals is IModularAccount { _validationData.isGlobal = validationConfig.isGlobal(); _validationData.isSignatureValidation = validationConfig.isSignatureValidation(); - _onInstall(validationConfig.module(), installData); + _onInstall(validationConfig.module(), installData, type(IValidationModule).interfaceId); emit ValidationInstalled(validationConfig.module(), validationConfig.entityId()); } diff --git a/src/modules/ERC20TokenLimitModule.sol b/src/modules/ERC20TokenLimitModule.sol index 406937b1..c72bace3 100644 --- a/src/modules/ERC20TokenLimitModule.sol +++ b/src/modules/ERC20TokenLimitModule.sol @@ -120,7 +120,7 @@ contract ERC20TokenLimitModule is BaseModule, IExecutionHookModule { /// @inheritdoc BaseModule function supportsInterface(bytes4 interfaceId) public view override(BaseModule, IERC165) returns (bool) { - return super.supportsInterface(interfaceId); + return interfaceId == type(IExecutionHookModule).interfaceId || super.supportsInterface(interfaceId); } function _decrementLimit(uint32 entityId, address token, bytes memory innerCalldata) internal { diff --git a/src/modules/permissionhooks/AllowlistModule.sol b/src/modules/permissionhooks/AllowlistModule.sol index 3e4edb04..4d82bcf6 100644 --- a/src/modules/permissionhooks/AllowlistModule.sol +++ b/src/modules/permissionhooks/AllowlistModule.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.25; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {IERC165} from "@openzeppelin/contracts/interfaces/IERC165.sol"; import {IModule} from "../../interfaces/IModule.sol"; @@ -118,6 +119,16 @@ contract AllowlistModule is IValidationHookModule, BaseModule { } } + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(BaseModule, IERC165) + returns (bool) + { + return interfaceId == type(IValidationHookModule).interfaceId || super.supportsInterface(interfaceId); + } + function _checkCallPermission(uint32 entityId, address account, address target, bytes memory data) internal view diff --git a/src/modules/validation/SingleSignerValidationModule.sol b/src/modules/validation/SingleSignerValidationModule.sol index d3dd71da..2dd0b28d 100644 --- a/src/modules/validation/SingleSignerValidationModule.sol +++ b/src/modules/validation/SingleSignerValidationModule.sol @@ -1,15 +1,17 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.25; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {IERC165} from "@openzeppelin/contracts/interfaces/IERC165.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; + import {IModule} from "../../interfaces/IModule.sol"; import {IValidationModule} from "../../interfaces/IValidationModule.sol"; import {BaseModule} from "../BaseModule.sol"; import {ReplaySafeWrapper} from "../ReplaySafeWrapper.sol"; import {ISingleSignerValidationModule} from "./ISingleSignerValidationModule.sol"; -import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; -import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; /// @title ECSDA Validation /// @author ERC-6900 Authors @@ -115,6 +117,16 @@ contract SingleSignerValidationModule is ISingleSignerValidationModule, ReplaySa return "erc6900/single-signer-validation-module/1.0.0"; } + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(BaseModule, IERC165) + returns (bool) + { + return (interfaceId == type(IValidationModule).interfaceId || super.supportsInterface(interfaceId)); + } + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ // ┃ Internal / Private functions ┃ // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index c0713dca..f7f727e3 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -288,7 +288,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { address badModule = address(1); vm.expectRevert( - abi.encodeWithSelector(ModuleManagerInternals.ModuleInterfaceNotSupported.selector, address(badModule)) + abi.encodeWithSelector(ModuleManagerInternals.InterfaceNotSupported.selector, address(badModule)) ); ExecutionManifest memory m; diff --git a/test/mocks/modules/DirectCallModule.sol b/test/mocks/modules/DirectCallModule.sol index 76505dde..35adde84 100644 --- a/test/mocks/modules/DirectCallModule.sol +++ b/test/mocks/modules/DirectCallModule.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; +import {IERC165} from "@openzeppelin/contracts/interfaces/IERC165.sol"; + import {IExecutionHookModule} from "../../../src/interfaces/IExecutionHookModule.sol"; import {IModularAccount} from "../../../src/interfaces/IModularAccount.sol"; import {BaseModule} from "../../../src/modules/BaseModule.sol"; @@ -42,4 +44,14 @@ contract DirectCallModule is BaseModule, IExecutionHookModule { ); postHookRan = true; } + + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(BaseModule, IERC165) + returns (bool) + { + return interfaceId == type(IExecutionHookModule).interfaceId || super.supportsInterface(interfaceId); + } } diff --git a/test/mocks/modules/MockAccessControlHookModule.sol b/test/mocks/modules/MockAccessControlHookModule.sol index e37e9630..8bf23110 100644 --- a/test/mocks/modules/MockAccessControlHookModule.sol +++ b/test/mocks/modules/MockAccessControlHookModule.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.25; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {IERC165} from "@openzeppelin/contracts/interfaces/IERC165.sol"; import {IModularAccount} from "../../../src/interfaces/IModularAccount.sol"; import {IValidationHookModule} from "../../../src/interfaces/IValidationHookModule.sol"; @@ -86,4 +87,14 @@ contract MockAccessControlHookModule is IValidationHookModule, BaseModule { function moduleId() external pure returns (string memory) { return "erc6900/mock-access-control-hook-module/1.0.0"; } + + function supportsInterface(bytes4 interfaceId) + public + view + virtual + override(BaseModule, IERC165) + returns (bool) + { + return interfaceId == type(IValidationHookModule).interfaceId || super.supportsInterface(interfaceId); + } } From 9d8e1ffd9e5f0d2184d343689128a2d58674ee12 Mon Sep 17 00:00:00 2001 From: adam Date: Thu, 22 Aug 2024 15:37:10 -0700 Subject: [PATCH 2/2] refactor: reuse internal install func --- src/account/ModuleManagerInternals.sol | 43 +++++++------------- test/account/AccountExecHooks.t.sol | 8 ++-- test/account/DirectCallsFromModule.t.sol | 23 ++++++++++- test/account/UpgradeableModularAccount.t.sol | 2 +- 4 files changed, 41 insertions(+), 35 deletions(-) diff --git a/src/account/ModuleManagerInternals.sol b/src/account/ModuleManagerInternals.sol index 018811cf..e4092904 100644 --- a/src/account/ModuleManagerInternals.sol +++ b/src/account/ModuleManagerInternals.sol @@ -129,19 +129,17 @@ abstract contract ModuleManagerInternals is IModularAccount { hooks.remove(toSetValue(hookConfig)); } - function _installExecution(address module, ExecutionManifest calldata manifest, bytes memory moduleInstallData) - internal - { + function _installExecution( + address module, + ExecutionManifest calldata manifest, + bytes calldata moduleInstallData + ) internal { AccountStorage storage _storage = getAccountStorage(); if (module == address(0)) { revert NullModule(); } - if (!ERC165Checker.supportsInterface(module, type(IModule).interfaceId)) { - revert InterfaceNotSupported(module); - } - // Update components according to the manifest. uint256 length = manifest.executionFunctions.length; for (uint256 i = 0; i < length; ++i) { @@ -170,18 +168,12 @@ abstract contract ModuleManagerInternals is IModularAccount { _storage.supportedIfaces[manifest.interfaceIds[i]] += 1; } - // Initialize the module storage for the account. - // solhint-disable-next-line no-empty-blocks - try IModule(module).onInstall(moduleInstallData) {} - catch { - bytes memory revertReason = collectReturnData(); - revert ModuleInstallCallbackFailed(module, revertReason); - } + _onInstall(module, moduleInstallData, type(IModule).interfaceId); emit ExecutionInstalled(module, manifest); } - function _uninstallExecution(address module, ExecutionManifest calldata manifest, bytes memory uninstallData) + function _uninstallExecution(address module, ExecutionManifest calldata manifest, bytes calldata uninstallData) internal { AccountStorage storage _storage = getAccountStorage(); @@ -214,29 +206,22 @@ abstract contract ModuleManagerInternals is IModularAccount { } // Clear the module storage for the account. - bool onUninstallSuccess = true; - // solhint-disable-next-line no-empty-blocks - try IModule(module).onUninstall(uninstallData) {} - catch { - onUninstallSuccess = false; - } + bool onUninstallSuccess = _onUninstall(module, uninstallData); emit ExecutionUninstalled(module, onUninstallSuccess, manifest); } - function _onInstall(address module, bytes calldata data) internal { - if (data.length > 0) { - IModule(module).onInstall(data); - } - } - function _onInstall(address module, bytes calldata data, bytes4 interfaceId) internal { if (data.length > 0) { if (!ERC165Checker.supportsInterface(module, interfaceId)) { revert InterfaceNotSupported(module); } - - IModule(module).onInstall(data); + // solhint-disable-next-line no-empty-blocks + try IModule(module).onInstall(data) {} + catch { + bytes memory revertReason = collectReturnData(); + revert ModuleInstallCallbackFailed(module, revertReason); + } } } diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 351f9208..bc61a205 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -162,7 +162,7 @@ contract AccountExecHooksTest is AccountTestBase { mockModule1 = new MockModule(_m1); vm.expectEmit(true, true, true, true); - emit ReceivedCall(abi.encodeCall(IModule.onInstall, (bytes(""))), 0); + emit ReceivedCall(abi.encodeCall(IModule.onInstall, (bytes("a"))), 0); vm.expectEmit(true, true, true, true); emit ExecutionInstalled(address(mockModule1), _m1); @@ -170,19 +170,19 @@ contract AccountExecHooksTest is AccountTestBase { account1.installExecution({ module: address(mockModule1), manifest: mockModule1.executionManifest(), - moduleInstallData: bytes("") + moduleInstallData: bytes("a") }); vm.stopPrank(); } function _uninstallExecution(MockModule module) internal { vm.expectEmit(true, true, true, true); - emit ReceivedCall(abi.encodeCall(IModule.onUninstall, (bytes(""))), 0); + emit ReceivedCall(abi.encodeCall(IModule.onUninstall, (bytes("b"))), 0); vm.expectEmit(true, true, true, true); emit ExecutionUninstalled(address(module), true, module.executionManifest()); vm.startPrank(address(entryPoint)); - account1.uninstallExecution(address(module), module.executionManifest(), bytes("")); + account1.uninstallExecution(address(module), module.executionManifest(), bytes("b")); vm.stopPrank(); } } diff --git a/test/account/DirectCallsFromModule.t.sol b/test/account/DirectCallsFromModule.t.sol index f77b7fd5..3816678b 100644 --- a/test/account/DirectCallsFromModule.t.sol +++ b/test/account/DirectCallsFromModule.t.sol @@ -11,6 +11,8 @@ import {DirectCallModule} from "../mocks/modules/DirectCallModule.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; +import {DIRECT_CALL_VALIDATION_ENTITYID} from "../../src/helpers/Constants.sol"; + contract DirectCallsFromModuleTest is AccountTestBase { using ValidationConfigLib for ValidationConfig; @@ -23,7 +25,7 @@ contract DirectCallsFromModuleTest is AccountTestBase { _module = new DirectCallModule(); assertFalse(_module.preHookRan()); assertFalse(_module.postHookRan()); - _moduleEntity = ModuleEntityLib.pack(address(_module), type(uint32).max); + _moduleEntity = ModuleEntityLib.pack(address(_module), DIRECT_CALL_VALIDATION_ENTITYID); } /* -------------------------------------------------------------------------- */ @@ -104,6 +106,25 @@ contract DirectCallsFromModuleTest is AccountTestBase { account1.execute(address(0), 0, ""); } + function test_directCallsFromEOA() external { + address extraOwner = makeAddr("extraOwner"); + + bytes4[] memory selectors = new bytes4[](1); + selectors[0] = IModularAccount.execute.selector; + + vm.prank(address(entryPoint)); + + account1.installValidation( + ValidationConfigLib.pack(extraOwner, DIRECT_CALL_VALIDATION_ENTITYID, false, false), + selectors, + "", + new bytes[](0) + ); + + vm.prank(extraOwner); + account1.execute(makeAddr("dead"), 0, ""); + } + /* -------------------------------------------------------------------------- */ /* Internals */ /* -------------------------------------------------------------------------- */ diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index f7f727e3..1a3631ed 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -293,7 +293,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { ExecutionManifest memory m; - account1.installExecution({module: address(badModule), manifest: m, moduleInstallData: ""}); + account1.installExecution({module: address(badModule), manifest: m, moduleInstallData: "a"}); } function test_installExecution_alreadyInstalled() public {