From db9960fd428cb5066e414d53aa59dbc9fe8e4086 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:54:49 -0400 Subject: [PATCH 01/13] chore: remove existing permissions system --- src/account/AccountStorage.sol | 3 +-- src/account/PluginManagerInternals.sol | 13 ------------- src/account/UpgradeableModularAccount.sol | 8 +++----- src/interfaces/IPlugin.sol | 2 -- test/account/UpgradeableModularAccount.t.sol | 4 +--- test/mocks/plugins/PermittedCallMocks.sol | 4 ---- test/mocks/plugins/ReturnDataPluginMocks.sol | 3 --- 7 files changed, 5 insertions(+), 32 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0a992a33..5a3b8c12 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -51,8 +51,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference validationFunction => ValidationData) validationData; - mapping(address caller => mapping(bytes4 selector => bool)) callPermitted; + mapping(FunctionReference => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 2ec06f81..05943e09 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -212,14 +212,6 @@ abstract contract PluginManagerInternals is IPluginManager { _setExecutionFunction(selector, isPublic, allowDefaultValidation, plugin); } - // Add installed plugin and selectors this plugin can call - length = manifest.permittedExecutionSelectors.length; - for (uint256 i = 0; i < length; ++i) { - // If there are duplicates, this will just enable the flag again. This is not a problem, since the - // boolean will be set to false twice during uninstall, which is fine. - _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = true; - } - length = manifest.validationFunctions.length; for (uint256 i = 0; i < length; ++i) { ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; @@ -313,11 +305,6 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - length = manifest.permittedExecutionSelectors.length; - for (uint256 i = 0; i < length; ++i) { - _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = false; - } - length = manifest.executionFunctions.length; for (uint256 i = 0; i < length; ++i) { bytes4 selector = manifest.executionFunctions[i].executionSelector; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 169b17a3..0c67ac81 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -555,11 +555,9 @@ contract UpgradeableModularAccount is AccountStorage storage _storage = getAccountStorage(); if ( - msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) - || _storage.selectorData[msg.sig].isPublic - ) return; - - if (!_storage.callPermitted[msg.sender][msg.sig]) { + msg.sender != address(_ENTRY_POINT) && msg.sender != address(this) + && !_storage.selectorData[msg.sig].isPublic + ) { revert ExecFromPluginNotPermitted(msg.sender, msg.sig); } } diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 3241e497..22d74546 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -73,8 +73,6 @@ struct PluginManifest { ManifestAssociatedFunction[] validationFunctions; ManifestExecutionHook[] executionHooks; uint8[] signatureValidationFunctions; - // Plugin execution functions already installed on the MSCA that this plugin will be able to call. - bytes4[] permittedExecutionSelectors; // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include // IPlugin's interface ID. bytes4[] interfaceIds; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e3d09e7f..46ea1d5f 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -11,7 +11,7 @@ import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; -import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; +import {PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IAccountLoupe} from "../../src/interfaces/IAccountLoupe.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {Call} from "../../src/interfaces/IStandardExecutor.sol"; @@ -262,8 +262,6 @@ contract UpgradeableModularAccountTest is AccountTestBase { vm.startPrank(address(entryPoint)); PluginManifest memory m; - m.permittedExecutionSelectors = new bytes4[](1); - m.permittedExecutionSelectors[0] = IPlugin.onInstall.selector; MockPlugin mockPluginWithBadPermittedExec = new MockPlugin(m); bytes32 manifestHash = keccak256(abi.encode(mockPluginWithBadPermittedExec.pluginManifest())); diff --git a/test/mocks/plugins/PermittedCallMocks.sol b/test/mocks/plugins/PermittedCallMocks.sol index 450a70fb..77548225 100644 --- a/test/mocks/plugins/PermittedCallMocks.sol +++ b/test/mocks/plugins/PermittedCallMocks.sol @@ -22,10 +22,6 @@ contract PermittedCallerPlugin is BasePlugin { manifest.executionFunctions[i].isPublic = true; } - // Request permission only for "foo", but not "bar", from ResultCreatorPlugin - manifest.permittedExecutionSelectors = new bytes4[](1); - manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; - return manifest; } diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index dae2c8e4..485b8456 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -138,9 +138,6 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { allowDefaultValidation: false }); - manifest.permittedExecutionSelectors = new bytes4[](1); - manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; - return manifest; } From 538375b5bb78d75a87a7b8f2b39a897f7890240a Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Wed, 26 Jun 2024 17:05:19 -0400 Subject: [PATCH 02/13] fix: revert change --- src/account/AccountStorage.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 5a3b8c12..5d08ab6d 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -51,7 +51,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference => ValidationData) validationData; + mapping(FunctionReference validationFunction => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; } From 420e485d83502e1abb8a0eda42e7083fcba8e138 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 7 Jun 2024 11:54:49 -0400 Subject: [PATCH 03/13] feat: add permission hooks to manifest and install flow and cache required context count --- src/account/AccountLoupe.sol | 30 ++++++++++++--- src/account/AccountStorage.sol | 10 ++++- src/account/PluginManagerInternals.sol | 35 +++++++++++------ src/account/UpgradeableModularAccount.sol | 43 ++++++++++++++------- src/interfaces/IAccountLoupe.sol | 9 +++++ src/interfaces/IExecutionHook.sol | 9 ++--- src/interfaces/IPlugin.sol | 3 ++ test/account/AccountExecHooks.t.sol | 45 ++++++++++++++++------ test/account/AccountLoupe.t.sol | 9 +++-- test/mocks/plugins/ComprehensivePlugin.sol | 16 ++++---- 10 files changed, 148 insertions(+), 61 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 9cde18b4..99b5bd14 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -7,7 +7,7 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet import {IAccountLoupe, ExecutionHook} from "../interfaces/IAccountLoupe.sol"; import {FunctionReference, IPluginManager} from "../interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../interfaces/IStandardExecutor.sol"; -import {getAccountStorage, SelectorData, toFunctionReferenceArray, toExecutionHook} from "./AccountStorage.sol"; +import {getAccountStorage, toFunctionReferenceArray, toExecutionHook} from "./AccountStorage.sol"; abstract contract AccountLoupe is IAccountLoupe { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -39,15 +39,35 @@ abstract contract AccountLoupe is IAccountLoupe { override returns (ExecutionHook[] memory execHooks) { - SelectorData storage selectorData = getAccountStorage().selectorData[selector]; - uint256 executionHooksLength = selectorData.executionHooks.length(); + EnumerableSet.Bytes32Set storage hooks = getAccountStorage().selectorData[selector].executionHooks; + uint256 executionHooksLength = hooks.length(); execHooks = new ExecutionHook[](executionHooksLength); for (uint256 i = 0; i < executionHooksLength; ++i) { - bytes32 key = selectorData.executionHooks.at(i); + bytes32 key = hooks.at(i); ExecutionHook memory execHook = execHooks[i]; - (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = + toExecutionHook(key); + } + } + + /// @inheritdoc IAccountLoupe + function getPermissionHooks(FunctionReference validationFunction) + external + view + override + returns (ExecutionHook[] memory permissionHooks) + { + EnumerableSet.Bytes32Set storage hooks = + getAccountStorage().validationData[validationFunction].permissionHooks; + uint256 executionHooksLength = hooks.length(); + permissionHooks = new ExecutionHook[](executionHooksLength); + for (uint256 i = 0; i < executionHooksLength; ++i) { + bytes32 key = hooks.at(i); + ExecutionHook memory execHook = permissionHooks[i]; + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = + toExecutionHook(key); } } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 5d08ab6d..70de42af 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -38,8 +38,12 @@ struct ValidationData { bool isDefault; // Whether or not this validation is a signature validator. bool isSignatureValidation; + // How many execution hooks require the UO context. + uint8 requireUOHookCount; // The pre validation hooks for this function selector. EnumerableSet.Bytes32Set preValidationHooks; + // Permission hooks for this validation function. + EnumerableSet.Bytes32Set permissionHooks; } struct AccountStorage { @@ -80,16 +84,18 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) function toSetValue(ExecutionHook memory executionHook) pure returns (bytes32) { return bytes32(FunctionReference.unwrap(executionHook.hookFunction)) | bytes32(executionHook.isPreHook ? uint256(1) << 80 : 0) - | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0); + | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0) + | bytes32(executionHook.requireUOContext ? uint256(1) << 64 : 0); } function toExecutionHook(bytes32 setValue) pure - returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook) + returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook, bool requireUOContext) { hookFunction = FunctionReference.wrap(bytes21(setValue)); isPreHook = (uint256(setValue) >> 80) & 0xFF == 1; isPostHook = (uint256(setValue) >> 72) & 0xFF == 1; + requireUOContext = (uint256(setValue) >> 64) & 0xFF == 1; } /// @dev Helper function to get all elements of a set into memory. diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 05943e09..a2974e85 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -125,27 +125,39 @@ abstract contract PluginManagerInternals is IPluginManager { } function _addExecHooks( - bytes4 selector, + EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook + bool isPostExecHook, + bool requireUOContext ) internal { - getAccountStorage().selectorData[selector].executionHooks.add( + hooks.add( toSetValue( - ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) + ExecutionHook({ + hookFunction: hookFunction, + isPreHook: isPreExecHook, + isPostHook: isPostExecHook, + requireUOContext: requireUOContext + }) ) ); } function _removeExecHooks( - bytes4 selector, + EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook + bool isPostExecHook, + bool requireUOContext ) internal { - getAccountStorage().selectorData[selector].executionHooks.remove( + hooks.remove( toSetValue( - ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) + ExecutionHook({ + hookFunction: hookFunction, + isPreHook: isPreExecHook, + isPostHook: isPostExecHook, + requireUOContext: requireUOContext + }) ) ); } @@ -230,8 +242,9 @@ abstract contract PluginManagerInternals is IPluginManager { length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; + EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); - _addExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); + _addExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); } length = manifest.interfaceIds.length; @@ -282,12 +295,12 @@ abstract contract PluginManagerInternals is IPluginManager { } // Remove components according to the manifest, in reverse order (by component type) of their installation. - length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); - _removeExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); + EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; + _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); } length = manifest.signatureValidationFunctions.length; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 0c67ac81..cd938c93 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -22,7 +22,6 @@ import {AccountLoupe} from "./AccountLoupe.sol"; import { AccountStorage, getAccountStorage, - SelectorData, toSetValue, toFunctionReference, toExecutionHook @@ -70,6 +69,7 @@ contract UpgradeableModularAccount is error PostExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreRuntimeValidationHookFailed(address plugin, uint8 functionId, bytes revertReason); + error RequireUserOperationContext(); error RuntimeValidationFunctionMissing(bytes4 selector); error RuntimeValidationFunctionReverted(address plugin, uint8 functionId, bytes revertReason); error SignatureValidationInvalid(address plugin, uint8 functionId); @@ -83,7 +83,8 @@ contract UpgradeableModularAccount is modifier wrapNativeFunction() { _checkPermittedCallerIfNotFromEP(); - PostExecToRun[] memory postExecHooks = _doPreExecHooks(msg.sig, msg.data); + PostExecToRun[] memory postExecHooks = + _doPreExecHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); _; @@ -137,7 +138,7 @@ contract UpgradeableModularAccount is PostExecToRun[] memory postExecHooks; // Cache post-exec hooks in memory - postExecHooks = _doPreExecHooks(msg.sig, msg.data); + postExecHooks = _doPreExecHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); // execute the function, bubbling up any reverts (bool execSuccess, bytes memory execReturnData) = execPlugin.call(msg.data); @@ -350,6 +351,17 @@ contract UpgradeableModularAccount is _checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation); + // Check if there are exec hooks associated with the validator that require UO context, and revert if the + // call isn't to `executeUserOp` + // This check must be here because if context isn't passed, we wouldn't be able to get the exec hooks + // associated with the validator + if (getAccountStorage().validationData[userOpValidationFunction].requireUOHookCount > 0) { + /** + * && msg.sig != this.executeUserOp.selector + */ + revert RequireUserOperationContext(); + } + validationData = _doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[22:], userOpHash); } @@ -441,13 +453,11 @@ contract UpgradeableModularAccount is } } - function _doPreExecHooks(bytes4 selector, bytes calldata data) + function _doPreExecHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes calldata data) internal returns (PostExecToRun[] memory postHooksToRun) { - SelectorData storage selectorData = getAccountStorage().selectorData[selector]; - - uint256 hooksLength = selectorData.executionHooks.length(); + uint256 hooksLength = executionHooks.length(); // Overallocate on length - not all of this may get filled up. We set the correct length later. postHooksToRun = new PostExecToRun[](hooksLength); @@ -455,8 +465,14 @@ contract UpgradeableModularAccount is // Copy all post hooks to the array. This happens before any pre hooks are run, so we can // be sure that the set of hooks to run will not be affected by state changes mid-execution. for (uint256 i = 0; i < hooksLength; ++i) { - bytes32 key = selectorData.executionHooks.at(i); - (FunctionReference hookFunction,, bool isPostHook) = toExecutionHook(key); + bytes32 key = executionHooks.at(i); + (FunctionReference hookFunction,, bool isPostHook, bool requireUOContext) = toExecutionHook(key); + if (requireUOContext) { + /** + * && msg.sig != this.executeUserOp.selector + */ + revert RequireUserOperationContext(); + } if (isPostHook) { postHooksToRun[i].postExecHook = hookFunction; } @@ -465,8 +481,8 @@ contract UpgradeableModularAccount is // Run the pre hooks and copy their return data to the post hooks array, if an associated post-exec hook // exists. for (uint256 i = 0; i < hooksLength; ++i) { - bytes32 key = selectorData.executionHooks.at(i); - (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); + bytes32 key = executionHooks.at(i); + (FunctionReference hookFunction, bool isPreHook, bool isPostHook,) = toExecutionHook(key); if (isPreHook) { bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); @@ -484,9 +500,8 @@ contract UpgradeableModularAccount is returns (bytes memory preExecHookReturnData) { (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( - bytes memory returnData - ) { + try IExecutionHook(plugin).preExecutionHook(functionId, abi.encodePacked(msg.sender, msg.value, data)) + returns (bytes memory returnData) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { revert PreExecHookReverted(plugin, functionId, revertReason); diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 490b216c..1f1c354e 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -9,6 +9,7 @@ struct ExecutionHook { FunctionReference hookFunction; bool isPreHook; bool isPostHook; + bool requireUOContext; } interface IAccountLoupe { @@ -28,6 +29,14 @@ interface IAccountLoupe { /// @return The pre and post execution hooks for this selector. function getExecutionHooks(bytes4 selector) external view returns (ExecutionHook[] memory); + /// @notice Get the pre and post execution hooks for a validation function. + /// @param validationFunction The validation function to get the hooks for. + /// @return The pre and post execution hooks for this validation function. + function getPermissionHooks(FunctionReference validationFunction) + external + view + returns (ExecutionHook[] memory); + /// @notice Get the pre user op and runtime validation hooks associated with a selector. /// @param validationFunction The validation function to get the hooks for. /// @return preValidationHooks The pre validation hooks for this selector. diff --git a/src/interfaces/IExecutionHook.sol b/src/interfaces/IExecutionHook.sol index 3240c489..b0dd40c2 100644 --- a/src/interfaces/IExecutionHook.sol +++ b/src/interfaces/IExecutionHook.sol @@ -8,13 +8,10 @@ interface IExecutionHook is IPlugin { /// @dev To indicate the entire call should revert, the function MUST revert. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. - /// @param sender The caller address. - /// @param value The call value. - /// @param data The calldata sent. + /// @param data If hook requires UO context, data is abi.encode(PackedUserOperation), else its + /// abi.encodePacked(sender, value, calldata) /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) - external - returns (bytes memory); + function preExecutionHook(uint8 functionId, bytes calldata data) external returns (bytes memory); /// @notice Run the post execution hook specified by the `functionId`. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 22d74546..248f48af 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -45,6 +45,7 @@ struct ManifestExecutionHook { uint8 functionId; bool isPreHook; bool isPostHook; + bool requireUOContext; } struct SelectorPermission { @@ -64,6 +65,8 @@ struct PluginMetadata { // String desciptions of the relative sensitivity of specific functions. The selectors MUST be selectors for // functions implemented by this plugin. SelectorPermission[] permissionDescriptors; + // A list of all ERC-7715 permission strings that the plugin could possibly use + string[] permissionRequest; } /// @dev A struct describing how the plugin should be installed on a modular account. diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 6a3b7d8c..82bf1ae7 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -9,13 +9,13 @@ import { } from "../../src/interfaces/IPlugin.sol"; import {IExecutionHook} from "../../src/interfaces/IExecutionHook.sol"; import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract AccountExecHooksTest is AccountTestBase { MockPlugin public mockPlugin1; - MockPlugin public mockPlugin2; bytes32 public manifestHash1; bytes32 public manifestHash2; @@ -25,7 +25,6 @@ contract AccountExecHooksTest is AccountTestBase { uint8 internal constant _BOTH_HOOKS_FUNCTION_ID_3 = 3; PluginManifest internal _m1; - PluginManifest internal _m2; event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); @@ -50,7 +49,8 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _PRE_HOOK_FUNCTION_ID_1, isPreHook: true, - isPostHook: false + isPostHook: false, + requireUOContext: false }) ); } @@ -65,9 +65,11 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, - address(this), // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) + abi.encodePacked( + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ) ), 0 // msg value in call to plugin ); @@ -88,7 +90,8 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _BOTH_HOOKS_FUNCTION_ID_3, isPreHook: true, - isPostHook: true + isPostHook: true, + requireUOContext: false }) ); } @@ -104,9 +107,11 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _BOTH_HOOKS_FUNCTION_ID_3, - address(this), // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) + abi.encodePacked( + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) + ) ), 0 // msg value in call to plugin ); @@ -136,7 +141,8 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _POST_HOOK_FUNCTION_ID_2, isPreHook: false, - isPostHook: true + isPostHook: true, + requireUOContext: false }) ); } @@ -162,6 +168,23 @@ contract AccountExecHooksTest is AccountTestBase { _uninstallPlugin(mockPlugin1); } + function test_requireUOContextHook() public { + _installPlugin1WithHooks( + ManifestExecutionHook({ + executionSelector: _EXEC_SELECTOR, + functionId: _POST_HOOK_FUNCTION_ID_2, + isPreHook: false, + isPostHook: true, + requireUOContext: true + }) + ); + + (bool success, bytes memory errMsg) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq(errMsg.length, 4); + assertEq(bytes4(errMsg), UpgradeableModularAccount.RequireUserOperationContext.selector); + } + function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal { _m1.executionHooks.push(execHooks); mockPlugin1 = new MockPlugin(_m1); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index fa92ab00..e21f232f 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -117,21 +117,24 @@ contract AccountLoupeTest is AccountTestBase { address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.BOTH_EXECUTION_HOOKS) ), isPreHook: true, - isPostHook: true + isPostHook: true, + requireUOContext: false }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) ), isPreHook: true, - isPostHook: false + isPostHook: false, + requireUOContext: false }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) ), isPreHook: false, - isPostHook: true + isPostHook: true, + requireUOContext: false }) ]; diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 6ef654c7..2b742648 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -105,12 +105,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) - external - pure - override - returns (bytes memory) - { + function preExecutionHook(uint8 functionId, bytes calldata) external pure override returns (bytes memory) { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { @@ -154,19 +149,22 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba executionSelector: this.foo.selector, functionId: uint8(FunctionId.BOTH_EXECUTION_HOOKS), isPreHook: true, - isPostHook: true + isPostHook: true, + requireUOContext: false }); manifest.executionHooks[1] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), isPreHook: true, - isPostHook: false + isPostHook: false, + requireUOContext: false }); manifest.executionHooks[2] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.POST_EXECUTION_HOOK), isPreHook: false, - isPostHook: true + isPostHook: true, + requireUOContext: false }); return manifest; From ce2330e5504feb94b5a81d7b89353e608f1cbd15 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Wed, 26 Jun 2024 17:07:13 -0400 Subject: [PATCH 04/13] docs: update exec hook documentation --- src/account/AccountStorage.sol | 1 + 1 file changed, 1 insertion(+) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 70de42af..f55b4513 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -80,6 +80,7 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) // 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF______________________ Hook Function Reference // 0x__________________________________________AA____________________ is pre hook // 0x____________________________________________BB__________________ is post hook +// 0x______________________________________________CC________________ require UO context function toSetValue(ExecutionHook memory executionHook) pure returns (bytes32) { return bytes32(FunctionReference.unwrap(executionHook.hookFunction)) From d6d82c0e349ef2baf84b497397f3ed9fcc659004 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Thu, 27 Jun 2024 14:37:46 -0400 Subject: [PATCH 05/13] chore: remove require uo context param --- src/account/AccountLoupe.sol | 6 ++--- src/account/AccountStorage.sol | 7 ++---- src/account/PluginManagerInternals.sol | 24 +++++-------------- src/account/UpgradeableModularAccount.sol | 10 ++------ src/interfaces/IAccountLoupe.sol | 1 - src/interfaces/IPlugin.sol | 1 - test/account/AccountExecHooks.t.sol | 27 +++------------------- test/account/AccountLoupe.t.sol | 9 +++----- test/mocks/plugins/ComprehensivePlugin.sol | 9 +++----- 9 files changed, 21 insertions(+), 73 deletions(-) diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 99b5bd14..2afe6638 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -47,8 +47,7 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < executionHooksLength; ++i) { bytes32 key = hooks.at(i); ExecutionHook memory execHook = execHooks[i]; - (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = - toExecutionHook(key); + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); } } @@ -66,8 +65,7 @@ abstract contract AccountLoupe is IAccountLoupe { for (uint256 i = 0; i < executionHooksLength; ++i) { bytes32 key = hooks.at(i); ExecutionHook memory execHook = permissionHooks[i]; - (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook, execHook.requireUOContext) = - toExecutionHook(key); + (execHook.hookFunction, execHook.isPreHook, execHook.isPostHook) = toExecutionHook(key); } } diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index f55b4513..352a3d5b 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -80,23 +80,20 @@ function toFunctionReference(bytes32 setValue) pure returns (FunctionReference) // 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF______________________ Hook Function Reference // 0x__________________________________________AA____________________ is pre hook // 0x____________________________________________BB__________________ is post hook -// 0x______________________________________________CC________________ require UO context function toSetValue(ExecutionHook memory executionHook) pure returns (bytes32) { return bytes32(FunctionReference.unwrap(executionHook.hookFunction)) | bytes32(executionHook.isPreHook ? uint256(1) << 80 : 0) - | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0) - | bytes32(executionHook.requireUOContext ? uint256(1) << 64 : 0); + | bytes32(executionHook.isPostHook ? uint256(1) << 72 : 0); } function toExecutionHook(bytes32 setValue) pure - returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook, bool requireUOContext) + returns (FunctionReference hookFunction, bool isPreHook, bool isPostHook) { hookFunction = FunctionReference.wrap(bytes21(setValue)); isPreHook = (uint256(setValue) >> 80) & 0xFF == 1; isPostHook = (uint256(setValue) >> 72) & 0xFF == 1; - requireUOContext = (uint256(setValue) >> 64) & 0xFF == 1; } /// @dev Helper function to get all elements of a set into memory. diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index a2974e85..a9b79802 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -128,17 +128,11 @@ abstract contract PluginManagerInternals is IPluginManager { EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook, - bool requireUOContext + bool isPostExecHook ) internal { hooks.add( toSetValue( - ExecutionHook({ - hookFunction: hookFunction, - isPreHook: isPreExecHook, - isPostHook: isPostExecHook, - requireUOContext: requireUOContext - }) + ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) ) ); } @@ -147,17 +141,11 @@ abstract contract PluginManagerInternals is IPluginManager { EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, - bool isPostExecHook, - bool requireUOContext + bool isPostExecHook ) internal { hooks.remove( toSetValue( - ExecutionHook({ - hookFunction: hookFunction, - isPreHook: isPreExecHook, - isPostHook: isPostExecHook, - requireUOContext: requireUOContext - }) + ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) ) ); } @@ -244,7 +232,7 @@ abstract contract PluginManagerInternals is IPluginManager { ManifestExecutionHook memory mh = manifest.executionHooks[i]; EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); - _addExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); + _addExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook); } length = manifest.interfaceIds.length; @@ -300,7 +288,7 @@ abstract contract PluginManagerInternals is IPluginManager { ManifestExecutionHook memory mh = manifest.executionHooks[i]; FunctionReference hookFunction = FunctionReferenceLib.pack(plugin, mh.functionId); EnumerableSet.Bytes32Set storage execHooks = _storage.selectorData[mh.executionSelector].executionHooks; - _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook, mh.requireUOContext); + _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook); } length = manifest.signatureValidationFunctions.length; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index cd938c93..20ab0082 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -466,13 +466,7 @@ contract UpgradeableModularAccount is // be sure that the set of hooks to run will not be affected by state changes mid-execution. for (uint256 i = 0; i < hooksLength; ++i) { bytes32 key = executionHooks.at(i); - (FunctionReference hookFunction,, bool isPostHook, bool requireUOContext) = toExecutionHook(key); - if (requireUOContext) { - /** - * && msg.sig != this.executeUserOp.selector - */ - revert RequireUserOperationContext(); - } + (FunctionReference hookFunction,, bool isPostHook) = toExecutionHook(key); if (isPostHook) { postHooksToRun[i].postExecHook = hookFunction; } @@ -482,7 +476,7 @@ contract UpgradeableModularAccount is // exists. for (uint256 i = 0; i < hooksLength; ++i) { bytes32 key = executionHooks.at(i); - (FunctionReference hookFunction, bool isPreHook, bool isPostHook,) = toExecutionHook(key); + (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); if (isPreHook) { bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 1f1c354e..b172464a 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -9,7 +9,6 @@ struct ExecutionHook { FunctionReference hookFunction; bool isPreHook; bool isPostHook; - bool requireUOContext; } interface IAccountLoupe { diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 248f48af..7259d748 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -45,7 +45,6 @@ struct ManifestExecutionHook { uint8 functionId; bool isPreHook; bool isPostHook; - bool requireUOContext; } struct SelectorPermission { diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 82bf1ae7..124a87dc 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -9,7 +9,6 @@ import { } from "../../src/interfaces/IPlugin.sol"; import {IExecutionHook} from "../../src/interfaces/IExecutionHook.sol"; import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; -import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; @@ -49,8 +48,7 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _PRE_HOOK_FUNCTION_ID_1, isPreHook: true, - isPostHook: false, - requireUOContext: false + isPostHook: false }) ); } @@ -90,8 +88,7 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _BOTH_HOOKS_FUNCTION_ID_3, isPreHook: true, - isPostHook: true, - requireUOContext: false + isPostHook: true }) ); } @@ -141,8 +138,7 @@ contract AccountExecHooksTest is AccountTestBase { executionSelector: _EXEC_SELECTOR, functionId: _POST_HOOK_FUNCTION_ID_2, isPreHook: false, - isPostHook: true, - requireUOContext: false + isPostHook: true }) ); } @@ -168,23 +164,6 @@ contract AccountExecHooksTest is AccountTestBase { _uninstallPlugin(mockPlugin1); } - function test_requireUOContextHook() public { - _installPlugin1WithHooks( - ManifestExecutionHook({ - executionSelector: _EXEC_SELECTOR, - functionId: _POST_HOOK_FUNCTION_ID_2, - isPreHook: false, - isPostHook: true, - requireUOContext: true - }) - ); - - (bool success, bytes memory errMsg) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - assertEq(errMsg.length, 4); - assertEq(bytes4(errMsg), UpgradeableModularAccount.RequireUserOperationContext.selector); - } - function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal { _m1.executionHooks.push(execHooks); mockPlugin1 = new MockPlugin(_m1); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index e21f232f..fa92ab00 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -117,24 +117,21 @@ contract AccountLoupeTest is AccountTestBase { address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.BOTH_EXECUTION_HOOKS) ), isPreHook: true, - isPostHook: true, - requireUOContext: false + isPostHook: true }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_EXECUTION_HOOK) ), isPreHook: true, - isPostHook: false, - requireUOContext: false + isPostHook: false }), ExecutionHook({ hookFunction: FunctionReferenceLib.pack( address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.POST_EXECUTION_HOOK) ), isPreHook: false, - isPostHook: true, - requireUOContext: false + isPostHook: true }) ]; diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 2b742648..aa7e5314 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -149,22 +149,19 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba executionSelector: this.foo.selector, functionId: uint8(FunctionId.BOTH_EXECUTION_HOOKS), isPreHook: true, - isPostHook: true, - requireUOContext: false + isPostHook: true }); manifest.executionHooks[1] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.PRE_EXECUTION_HOOK), isPreHook: true, - isPostHook: false, - requireUOContext: false + isPostHook: false }); manifest.executionHooks[2] = ManifestExecutionHook({ executionSelector: this.foo.selector, functionId: uint8(FunctionId.POST_EXECUTION_HOOK), isPreHook: false, - isPostHook: true, - requireUOContext: false + isPostHook: true }); return manifest; From e0d0c7f97dfc943c9524b1638affdd9b0e5b1561 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:04:28 -0400 Subject: [PATCH 06/13] chore: revert hook changes --- src/account/UpgradeableModularAccount.sol | 2 +- src/interfaces/IExecutionHook.sol | 9 ++++++--- test/mocks/plugins/ComprehensivePlugin.sol | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 20ab0082..4a274b3e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -494,7 +494,7 @@ contract UpgradeableModularAccount is returns (bytes memory preExecHookReturnData) { (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, abi.encodePacked(msg.sender, msg.value, data)) + try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns (bytes memory returnData) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { diff --git a/src/interfaces/IExecutionHook.sol b/src/interfaces/IExecutionHook.sol index b0dd40c2..3240c489 100644 --- a/src/interfaces/IExecutionHook.sol +++ b/src/interfaces/IExecutionHook.sol @@ -8,10 +8,13 @@ interface IExecutionHook is IPlugin { /// @dev To indicate the entire call should revert, the function MUST revert. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. - /// @param data If hook requires UO context, data is abi.encode(PackedUserOperation), else its - /// abi.encodePacked(sender, value, calldata) + /// @param sender The caller address. + /// @param value The call value. + /// @param data The calldata sent. /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preExecutionHook(uint8 functionId, bytes calldata data) external returns (bytes memory); + function preExecutionHook(uint8 functionId, address sender, uint256 value, bytes calldata data) + external + returns (bytes memory); /// @notice Run the post execution hook specified by the `functionId`. /// @dev To indicate the entire call should revert, the function MUST revert. diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index aa7e5314..e31d4c64 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -105,7 +105,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function preExecutionHook(uint8 functionId, bytes calldata) external pure override returns (bytes memory) { + function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) external pure override returns (bytes memory) { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { From d0447f08734a99c1cd45da66b2aaf2a6599a06dd Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:06:49 -0400 Subject: [PATCH 07/13] fix: tests --- src/account/UpgradeableModularAccount.sol | 5 +++-- test/account/AccountExecHooks.t.sol | 16 ++++++---------- test/mocks/plugins/ComprehensivePlugin.sol | 7 ++++++- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 4a274b3e..1a52b5dd 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -494,8 +494,9 @@ contract UpgradeableModularAccount is returns (bytes memory preExecHookReturnData) { (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) - returns (bytes memory returnData) { + try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( + bytes memory returnData + ) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { revert PreExecHookReverted(plugin, functionId, revertReason); diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 124a87dc..f2e6391f 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -63,11 +63,9 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, - abi.encodePacked( - address(this), // caller - uint256(0), // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ) + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) ), 0 // msg value in call to plugin ); @@ -104,11 +102,9 @@ contract AccountExecHooksTest is AccountTestBase { abi.encodeWithSelector( IExecutionHook.preExecutionHook.selector, _BOTH_HOOKS_FUNCTION_ID_3, - abi.encodePacked( - address(this), // caller - uint256(0), // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ) + address(this), // caller + uint256(0), // msg.value in call to account + abi.encodeWithSelector(_EXEC_SELECTOR) ), 0 // msg value in call to plugin ); diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index e31d4c64..6ef654c7 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -105,7 +105,12 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) external pure override returns (bytes memory) { + function preExecutionHook(uint8 functionId, address, uint256, bytes calldata) + external + pure + override + returns (bytes memory) + { if (functionId == uint8(FunctionId.PRE_EXECUTION_HOOK)) { return ""; } else if (functionId == uint8(FunctionId.BOTH_EXECUTION_HOOKS)) { From 75b3844e52ad74d51406cda438a82725957d03c6 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Mon, 10 Jun 2024 11:40:00 -0400 Subject: [PATCH 08/13] feat: add execute user op --- src/account/AccountStorage.sol | 2 - src/account/PluginManager2.sol | 62 ++++++++++-- src/account/UpgradeableModularAccount.sol | 118 ++++++++++++++++++---- src/interfaces/IPermissionHook.sol | 18 ++++ src/interfaces/IPluginManager.sol | 11 +- test/account/AccountLoupe.t.sol | 7 +- test/account/ValidationIntersection.t.sol | 14 ++- 7 files changed, 194 insertions(+), 38 deletions(-) create mode 100644 src/interfaces/IPermissionHook.sol diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 352a3d5b..6d5ff5ba 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -38,8 +38,6 @@ struct ValidationData { bool isDefault; // Whether or not this validation is a signature validator. bool isSignatureValidation; - // How many execution hooks require the UO context. - uint8 requireUOHookCount; // The pre validation hooks for this function selector. EnumerableSet.Bytes32Set preValidationHooks; // Permission hooks for this validation function. diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 9e73e306..221c741c 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -7,6 +7,7 @@ import {IPlugin} from "../interfaces/IPlugin.sol"; import {FunctionReference} from "../interfaces/IPluginManager.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {AccountStorage, getAccountStorage, toSetValue, toFunctionReference} from "./AccountStorage.sol"; +import {ExecutionHook} from "../interfaces/IAccountLoupe.sol"; // Temporary additional functions for a user-controlled install flow for validation functions. abstract contract PluginManager2 { @@ -16,13 +17,15 @@ abstract contract PluginManager2 { error PreValidationAlreadySet(FunctionReference validationFunction, FunctionReference preValidationFunction); error ValidationAlreadySet(bytes4 selector, FunctionReference validationFunction); error ValidationNotSet(bytes4 selector, FunctionReference validationFunction); + error PermissionAlreadySet(FunctionReference validationFunction, ExecutionHook hook); function _installValidation( FunctionReference validationFunction, bool isDefault, bytes4[] memory selectors, bytes calldata installData, - bytes memory preValidationHooks + bytes memory preValidationHooks, + bytes memory permissionHooks ) // TODO: flag for signature validation internal @@ -51,6 +54,26 @@ abstract contract PluginManager2 { } } + if (permissionHooks.length > 0) { + (ExecutionHook[] memory permissionFunctions, bytes[] memory initDatas) = + abi.decode(permissionHooks, (ExecutionHook[], bytes[])); + + for (uint256 i = 0; i < permissionFunctions.length; ++i) { + ExecutionHook memory permissionFunction = permissionFunctions[i]; + + if ( + !_storage.validationData[validationFunction].permissionHooks.add(toSetValue(permissionFunction)) + ) { + revert PermissionAlreadySet(validationFunction, permissionFunction); + } + + if (initDatas[i].length > 0) { + (address executionPlugin,) = FunctionReferenceLib.unpack(permissionFunction.hookFunction); + IPlugin(executionPlugin).onInstall(initDatas[i]); + } + } + } + if (isDefault) { if (_storage.validationData[validationFunction].isDefault) { revert DefaultValidationAlreadySet(validationFunction); @@ -75,27 +98,44 @@ abstract contract PluginManager2 { FunctionReference validationFunction, bytes4[] calldata selectors, bytes calldata uninstallData, - bytes calldata preValidationHookUninstallData + bytes calldata preValidationHookUninstallData, + bytes calldata permissionHookUninstallData ) internal { AccountStorage storage _storage = getAccountStorage(); _storage.validationData[validationFunction].isDefault = false; _storage.validationData[validationFunction].isSignatureValidation = false; - bytes[] memory preValidationHookUninstallDatas = abi.decode(preValidationHookUninstallData, (bytes[])); + { + bytes[] memory preValidationHookUninstallDatas = abi.decode(preValidationHookUninstallData, (bytes[])); - // Clear pre validation hooks - EnumerableSet.Bytes32Set storage preValidationHooks = - _storage.validationData[validationFunction].preValidationHooks; - while (preValidationHooks.length() > 0) { - FunctionReference preValidationFunction = toFunctionReference(preValidationHooks.at(0)); - preValidationHooks.remove(toSetValue(preValidationFunction)); - (address preValidationPlugin,) = FunctionReferenceLib.unpack(preValidationFunction); - if (preValidationHookUninstallDatas[0].length > 0) { + // Clear pre validation hooks + EnumerableSet.Bytes32Set storage preValidationHooks = + _storage.validationData[validationFunction].preValidationHooks; + while (preValidationHooks.length() > 0) { + FunctionReference preValidationFunction = toFunctionReference(preValidationHooks.at(0)); + preValidationHooks.remove(toSetValue(preValidationFunction)); + (address preValidationPlugin,) = FunctionReferenceLib.unpack(preValidationFunction); IPlugin(preValidationPlugin).onUninstall(preValidationHookUninstallDatas[0]); } } + { + bytes[] memory permissionHookUninstallDatas = abi.decode(permissionHookUninstallData, (bytes[])); + + // Clear permission hooks + EnumerableSet.Bytes32Set storage permissionHooks = + _storage.validationData[validationFunction].permissionHooks; + while (permissionHooks.length() > 0) { + FunctionReference permissionHook = toFunctionReference(permissionHooks.at(0)); + permissionHooks.remove(toSetValue(permissionHook)); + (address permissionHookPlugin,) = FunctionReferenceLib.unpack(permissionHook); + if (permissionHookUninstallDatas[0].length > 0) { + IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[0]); + } + } + } + // Because this function also calls `onUninstall`, and removes the default flag from validation, we must // assume these selectors passed in to be exhaustive. // TODO: consider enforcing this from user-supplied install config. diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 1a52b5dd..1791212e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.25; import {BaseAccount} from "@eth-infinitism/account-abstraction/core/BaseAccount.sol"; import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {IAccountExecute} from "@eth-infinitism/account-abstraction/interfaces/IAccountExecute.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; @@ -38,6 +39,7 @@ contract UpgradeableModularAccount is IERC165, IERC1271, IStandardExecutor, + IAccountExecute, PluginManagerInternals, PluginManager2, UUPSUpgradeable @@ -66,6 +68,7 @@ contract UpgradeableModularAccount is error ExecFromPluginNotPermitted(address plugin, bytes4 selector); error ExecFromPluginExternalNotPermitted(address plugin, address target, uint256 value, bytes data); error NativeTokenSpendingNotPermitted(address plugin); + error NotEntryPoint(); error PostExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreExecHookReverted(address plugin, uint8 functionId, bytes revertReason); error PreRuntimeValidationHookFailed(address plugin, uint8 functionId, bytes revertReason); @@ -84,7 +87,7 @@ contract UpgradeableModularAccount is _checkPermittedCallerIfNotFromEP(); PostExecToRun[] memory postExecHooks = - _doPreExecHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); + _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data, false); _; @@ -138,7 +141,7 @@ contract UpgradeableModularAccount is PostExecToRun[] memory postExecHooks; // Cache post-exec hooks in memory - postExecHooks = _doPreExecHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); + postExecHooks = _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data, false); // execute the function, bubbling up any reverts (bool execSuccess, bytes memory execReturnData) = execPlugin.call(msg.data); @@ -155,6 +158,41 @@ contract UpgradeableModularAccount is return execReturnData; } + /// @notice Execution function that allows UO context to be passed to execution hooks + /// @dev This function is only callable by the EntryPoint + function executeUserOp(PackedUserOperation calldata userOp, bytes32) external { + if (msg.sender != address(_ENTRY_POINT)) { + revert NotEntryPoint(); + } + + FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21])); + + // remove first 4 bytes which is executeUserOp.selector + PackedUserOperation memory uo; + bytes memory callData = uo.callData; + assembly { + let len := mload(callData) + mstore(add(callData, 4), sub(len, 4)) + callData := add(callData, 4) + } + uo.callData = callData; + + PostExecToRun[] memory postPermissionHooks = _doPreHooks( + getAccountStorage().validationData[userOpValidationFunction].permissionHooks, abi.encode(uo), true + ); + + (bool success, bytes memory result) = address(this).call(userOp.callData[4:]); + + if (!success) { + // Directly bubble up revert messages + assembly ("memory-safe") { + revert(add(result, 32), mload(result)) + } + } + + _doCachedPostExecHooks(postPermissionHooks); + } + /// @inheritdoc IStandardExecutor /// @notice May be validated by a default validation. function execute(address target, uint256 value, bytes calldata data) @@ -201,8 +239,11 @@ contract UpgradeableModularAccount is _doRuntimeValidation(runtimeValidationFunction, data, authorization[22:]); - // If runtime validation passes, execute the call + // If runtime validation passes, do runtime permission checks + PostExecToRun[] memory postPermissionHooks = + _doPreHooks(getAccountStorage().validationData[runtimeValidationFunction].permissionHooks, data, false); + // Execute the call (bool success, bytes memory returnData) = address(this).call(data); if (!success) { @@ -211,6 +252,8 @@ contract UpgradeableModularAccount is } } + _doCachedPostExecHooks(postPermissionHooks); + return returnData; } @@ -252,7 +295,7 @@ contract UpgradeableModularAccount is external initializer { - _installValidation(validationFunction, true, new bytes4[](0), installData, bytes("")); + _installValidation(validationFunction, true, new bytes4[](0), installData, bytes(""), bytes("")); emit ModularAccountInitialized(_ENTRY_POINT); } @@ -263,9 +306,12 @@ contract UpgradeableModularAccount is bool isDefault, bytes4[] memory selectors, bytes calldata installData, - bytes calldata preValidationHooks + bytes calldata preValidationHooks, + bytes calldata permissionHooks ) external wrapNativeFunction { - _installValidation(validationFunction, isDefault, selectors, installData, preValidationHooks); + _installValidation( + validationFunction, isDefault, selectors, installData, preValidationHooks, permissionHooks + ); } /// @inheritdoc IPluginManager @@ -274,9 +320,16 @@ contract UpgradeableModularAccount is FunctionReference validationFunction, bytes4[] calldata selectors, bytes calldata uninstallData, - bytes calldata preValidationHookUninstallData + bytes calldata preValidationHookUninstallData, + bytes calldata permissionHookUninstallData ) external wrapNativeFunction { - _uninstallValidation(validationFunction, selectors, uninstallData, preValidationHookUninstallData); + _uninstallValidation( + validationFunction, + selectors, + uninstallData, + preValidationHookUninstallData, + permissionHookUninstallData + ); } /// @notice ERC165 introspection @@ -344,6 +397,9 @@ contract UpgradeableModularAccount is revert UnrecognizedFunction(bytes4(userOp.callData)); } bytes4 selector = bytes4(userOp.callData); + if (selector == this.executeUserOp.selector) { + selector = bytes4(userOp.callData[4:8]); + } // Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21])); @@ -351,14 +407,14 @@ contract UpgradeableModularAccount is _checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation); - // Check if there are exec hooks associated with the validator that require UO context, and revert if the - // call isn't to `executeUserOp` - // This check must be here because if context isn't passed, we wouldn't be able to get the exec hooks - // associated with the validator - if (getAccountStorage().validationData[userOpValidationFunction].requireUOHookCount > 0) { - /** - * && msg.sig != this.executeUserOp.selector - */ + // Check if there are permission hooks associated with the validator, and revert if the call isn't to + // `executeUserOp` + // This check must be here because if context isn't passed, we can't tell in execution which hooks should + // have ran + if ( + getAccountStorage().validationData[userOpValidationFunction].permissionHooks.length() > 0 + && bytes4(userOp.callData[:4]) != this.executeUserOp.selector + ) { revert RequireUserOperationContext(); } @@ -453,12 +509,11 @@ contract UpgradeableModularAccount is } } - function _doPreExecHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes calldata data) + function _doPreHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes memory data, bool isPackedUO) internal returns (PostExecToRun[] memory postHooksToRun) { uint256 hooksLength = executionHooks.length(); - // Overallocate on length - not all of this may get filled up. We set the correct length later. postHooksToRun = new PostExecToRun[](hooksLength); @@ -479,7 +534,14 @@ contract UpgradeableModularAccount is (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); if (isPreHook) { - bytes memory preExecHookReturnData = _runPreExecHook(hookFunction, data); + bytes memory preExecHookReturnData; + + // isPackedUO implies it is a permission hook + if (isPackedUO) { + preExecHookReturnData = _runPreUserOpExecHook(hookFunction, data); + } else { + preExecHookReturnData = _runPreExecHook(hookFunction, data); + } // If there is an associated post-exec hook, save the return data. if (isPostHook) { @@ -489,7 +551,22 @@ contract UpgradeableModularAccount is } } - function _runPreExecHook(FunctionReference preExecHook, bytes calldata data) + function _runPreUserOpExecHook(FunctionReference preExecHook, bytes memory data) + internal + returns (bytes memory preExecHookReturnData) + { + (address plugin, uint8 functionId) = preExecHook.unpack(); + try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( + bytes memory returnData + ) { + preExecHookReturnData = returnData; + } catch (bytes memory revertReason) { + // TODO: same issue with EP0.6 - we can't do bytes4 error codes in plugins + revert PreExecHookReverted(plugin, functionId, revertReason); + } + } + + function _runPreExecHook(FunctionReference preExecHook, bytes memory data) internal returns (bytes memory preExecHookReturnData) { @@ -499,6 +576,7 @@ contract UpgradeableModularAccount is ) { preExecHookReturnData = returnData; } catch (bytes memory revertReason) { + // TODO: same issue with EP0.6 - we can't do bytes4 error codes in plugins revert PreExecHookReverted(plugin, functionId, revertReason); } } diff --git a/src/interfaces/IPermissionHook.sol b/src/interfaces/IPermissionHook.sol new file mode 100644 index 00000000..5ff0d44a --- /dev/null +++ b/src/interfaces/IPermissionHook.sol @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: CC0-1.0 +pragma solidity ^0.8.25; + +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {IExecutionHook} from "./IExecutionHook.sol"; + +interface IPermissionHook is IExecutionHook { + /// @notice Run the pre execution permission hook specified by the `functionId`, passing in the whole user + /// operation. + /// @dev To indicate the entire call should revert, the function MUST revert. + /// @param functionId An identifier that routes the call to different internal implementations, should there be + /// more than one. + /// @param uo The packed user operation + /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. + function preUserOpExecutionHook(uint8 functionId, PackedUserOperation calldata uo) + external + returns (bytes memory); +} diff --git a/src/interfaces/IPluginManager.sol b/src/interfaces/IPluginManager.sol index 717e1fa0..32634e34 100644 --- a/src/interfaces/IPluginManager.sol +++ b/src/interfaces/IPluginManager.sol @@ -32,12 +32,15 @@ interface IPluginManager { /// @param isDefault Whether the validation function applies for all selectors in the default pool. /// @param selectors The selectors to install the validation function for. /// @param installData Optional data to be decoded and used by the plugin to setup initial plugin state. + /// @param preValidationHooks Optional pre-validation hooks to install for the validation function. + /// @param permissionHooks Optional permission hooks to install for the validation function. function installValidation( FunctionReference validationFunction, bool isDefault, bytes4[] memory selectors, bytes calldata installData, - bytes calldata preValidationHooks + bytes calldata preValidationHooks, + bytes calldata permissionHooks ) external; /// @notice Uninstall a validation function from a set of execution selectors. @@ -46,11 +49,15 @@ interface IPluginManager { /// @param selectors The selectors to uninstall the validation function for. /// @param uninstallData Optional data to be decoded and used by the plugin to clear plugin data for the /// account. + /// @param preValidationHookUninstallData Optional data to be decoded and used by the plugin to clear account + /// data + /// @param permissionHookUninstallData Optional data to be decoded and used by the plugin to clear account data function uninstallValidation( FunctionReference validationFunction, bytes4[] calldata selectors, bytes calldata uninstallData, - bytes calldata preValidationHookUninstallData + bytes calldata preValidationHookUninstallData, + bytes calldata permissionHookUninstallData ) external; /// @notice Uninstall a plugin from the modular account. diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index fa92ab00..412e548c 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -43,7 +43,12 @@ contract AccountLoupeTest is AccountTestBase { bytes[] memory installDatas = new bytes[](2); vm.prank(address(entryPoint)); account1.installValidation( - ownerValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + ownerValidation, + true, + new bytes4[](0), + bytes(""), + abi.encode(preValidationHooks, installDatas), + bytes("") ); } diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index 877f8ff9..6f451a16 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -67,7 +67,12 @@ contract ValidationIntersectionTest is AccountTestBase { }); bytes[] memory installDatas = new bytes[](1); account1.installValidation( - oneHookValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + oneHookValidation, + true, + new bytes4[](0), + bytes(""), + abi.encode(preValidationHooks, installDatas), + bytes("") ); account1.installPlugin({ plugin: address(twoHookPlugin), @@ -87,7 +92,12 @@ contract ValidationIntersectionTest is AccountTestBase { }); installDatas = new bytes[](2); account1.installValidation( - twoHookValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + twoHookValidation, + true, + new bytes4[](0), + bytes(""), + abi.encode(preValidationHooks, installDatas), + bytes("") ); vm.stopPrank(); } From 40e308afd5e217afac8a32ef712ddb6454eaf61a Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:11:26 -0400 Subject: [PATCH 09/13] chore: remove ipermission hook --- src/interfaces/IPermissionHook.sol | 18 ------------------ 1 file changed, 18 deletions(-) delete mode 100644 src/interfaces/IPermissionHook.sol diff --git a/src/interfaces/IPermissionHook.sol b/src/interfaces/IPermissionHook.sol deleted file mode 100644 index 5ff0d44a..00000000 --- a/src/interfaces/IPermissionHook.sol +++ /dev/null @@ -1,18 +0,0 @@ -// SPDX-License-Identifier: CC0-1.0 -pragma solidity ^0.8.25; - -import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {IExecutionHook} from "./IExecutionHook.sol"; - -interface IPermissionHook is IExecutionHook { - /// @notice Run the pre execution permission hook specified by the `functionId`, passing in the whole user - /// operation. - /// @dev To indicate the entire call should revert, the function MUST revert. - /// @param functionId An identifier that routes the call to different internal implementations, should there be - /// more than one. - /// @param uo The packed user operation - /// @return Context to pass to a post execution hook, if present. An empty bytes array MAY be returned. - function preUserOpExecutionHook(uint8 functionId, PackedUserOperation calldata uo) - external - returns (bytes memory); -} From 71a5034f226d3cc6ee8f2bdc78f1e979152d252d Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Fri, 28 Jun 2024 16:15:53 -0400 Subject: [PATCH 10/13] chore: remove new hook type, use whole msg.data in hook --- src/account/UpgradeableModularAccount.sol | 45 ++++------------------- 1 file changed, 7 insertions(+), 38 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 1791212e..4175c5bc 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -87,7 +87,7 @@ contract UpgradeableModularAccount is _checkPermittedCallerIfNotFromEP(); PostExecToRun[] memory postExecHooks = - _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data, false); + _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); _; @@ -141,7 +141,7 @@ contract UpgradeableModularAccount is PostExecToRun[] memory postExecHooks; // Cache post-exec hooks in memory - postExecHooks = _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data, false); + postExecHooks = _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); // execute the function, bubbling up any reverts (bool execSuccess, bytes memory execReturnData) = execPlugin.call(msg.data); @@ -167,19 +167,8 @@ contract UpgradeableModularAccount is FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21])); - // remove first 4 bytes which is executeUserOp.selector - PackedUserOperation memory uo; - bytes memory callData = uo.callData; - assembly { - let len := mload(callData) - mstore(add(callData, 4), sub(len, 4)) - callData := add(callData, 4) - } - uo.callData = callData; - - PostExecToRun[] memory postPermissionHooks = _doPreHooks( - getAccountStorage().validationData[userOpValidationFunction].permissionHooks, abi.encode(uo), true - ); + PostExecToRun[] memory postPermissionHooks = + _doPreHooks(getAccountStorage().validationData[userOpValidationFunction].permissionHooks, msg.data); (bool success, bytes memory result) = address(this).call(userOp.callData[4:]); @@ -241,7 +230,7 @@ contract UpgradeableModularAccount is // If runtime validation passes, do runtime permission checks PostExecToRun[] memory postPermissionHooks = - _doPreHooks(getAccountStorage().validationData[runtimeValidationFunction].permissionHooks, data, false); + _doPreHooks(getAccountStorage().validationData[runtimeValidationFunction].permissionHooks, data); // Execute the call (bool success, bytes memory returnData) = address(this).call(data); @@ -509,7 +498,7 @@ contract UpgradeableModularAccount is } } - function _doPreHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes memory data, bool isPackedUO) + function _doPreHooks(EnumerableSet.Bytes32Set storage executionHooks, bytes memory data) internal returns (PostExecToRun[] memory postHooksToRun) { @@ -536,12 +525,7 @@ contract UpgradeableModularAccount is if (isPreHook) { bytes memory preExecHookReturnData; - // isPackedUO implies it is a permission hook - if (isPackedUO) { - preExecHookReturnData = _runPreUserOpExecHook(hookFunction, data); - } else { - preExecHookReturnData = _runPreExecHook(hookFunction, data); - } + preExecHookReturnData = _runPreExecHook(hookFunction, data); // If there is an associated post-exec hook, save the return data. if (isPostHook) { @@ -551,21 +535,6 @@ contract UpgradeableModularAccount is } } - function _runPreUserOpExecHook(FunctionReference preExecHook, bytes memory data) - internal - returns (bytes memory preExecHookReturnData) - { - (address plugin, uint8 functionId) = preExecHook.unpack(); - try IExecutionHook(plugin).preExecutionHook(functionId, msg.sender, msg.value, data) returns ( - bytes memory returnData - ) { - preExecHookReturnData = returnData; - } catch (bytes memory revertReason) { - // TODO: same issue with EP0.6 - we can't do bytes4 error codes in plugins - revert PreExecHookReverted(plugin, functionId, revertReason); - } - } - function _runPreExecHook(FunctionReference preExecHook, bytes memory data) internal returns (bytes memory preExecHookReturnData) From e412a85a46ac91438be3f0681df6b03328823e87 Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Mon, 8 Jul 2024 11:48:03 -0400 Subject: [PATCH 11/13] fix: use right uninstall data idx --- src/account/PluginManager2.sol | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 221c741c..0a344c6a 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -112,11 +112,12 @@ abstract contract PluginManager2 { // Clear pre validation hooks EnumerableSet.Bytes32Set storage preValidationHooks = _storage.validationData[validationFunction].preValidationHooks; + uint256 i = 0; while (preValidationHooks.length() > 0) { FunctionReference preValidationFunction = toFunctionReference(preValidationHooks.at(0)); preValidationHooks.remove(toSetValue(preValidationFunction)); (address preValidationPlugin,) = FunctionReferenceLib.unpack(preValidationFunction); - IPlugin(preValidationPlugin).onUninstall(preValidationHookUninstallDatas[0]); + IPlugin(preValidationPlugin).onUninstall(preValidationHookUninstallDatas[i++]); } } @@ -126,12 +127,13 @@ abstract contract PluginManager2 { // Clear permission hooks EnumerableSet.Bytes32Set storage permissionHooks = _storage.validationData[validationFunction].permissionHooks; + uint256 i = 0; while (permissionHooks.length() > 0) { FunctionReference permissionHook = toFunctionReference(permissionHooks.at(0)); permissionHooks.remove(toSetValue(permissionHook)); (address permissionHookPlugin,) = FunctionReferenceLib.unpack(permissionHook); if (permissionHookUninstallDatas[0].length > 0) { - IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[0]); + IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i++]); } } } From bd943003b3719b5bd553ae120e13b98ed7436f2c Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Mon, 8 Jul 2024 15:30:45 -0400 Subject: [PATCH 12/13] fix: remove [0] and length check --- src/account/PluginManager2.sol | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 0a344c6a..f733eb60 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -132,9 +132,7 @@ abstract contract PluginManager2 { FunctionReference permissionHook = toFunctionReference(permissionHooks.at(0)); permissionHooks.remove(toSetValue(permissionHook)); (address permissionHookPlugin,) = FunctionReferenceLib.unpack(permissionHook); - if (permissionHookUninstallDatas[0].length > 0) { - IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i++]); - } + IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i++]); } } From 9f08a1bc327cb43314f1f30eede702c95f1ae11c Mon Sep 17 00:00:00 2001 From: howydev <132113803+howydev@users.noreply.github.com> Date: Tue, 9 Jul 2024 14:51:59 -0400 Subject: [PATCH 13/13] fix: lint --- src/account/UpgradeableModularAccount.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 8b74c6cd..4175c5bc 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -88,7 +88,7 @@ contract UpgradeableModularAccount is PostExecToRun[] memory postExecHooks = _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); - + _; _doCachedPostExecHooks(postExecHooks);