diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 9cde18b4..2afe6638 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,18 +39,36 @@ 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); } } + /// @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) = toExecutionHook(key); + } + } + /// @inheritdoc IAccountLoupe function getPreValidationHooks(FunctionReference validationFunction) external diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0a992a33..352a3d5b 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 { @@ -52,7 +56,6 @@ struct AccountStorage { // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; mapping(FunctionReference validationFunction => ValidationData) validationData; - mapping(address caller => mapping(bytes4 selector => bool)) callPermitted; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 2ec06f81..a9b79802 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -125,12 +125,12 @@ abstract contract PluginManagerInternals is IPluginManager { } function _addExecHooks( - bytes4 selector, + EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, bool isPostExecHook ) internal { - getAccountStorage().selectorData[selector].executionHooks.add( + hooks.add( toSetValue( ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) ) @@ -138,12 +138,12 @@ abstract contract PluginManagerInternals is IPluginManager { } function _removeExecHooks( - bytes4 selector, + EnumerableSet.Bytes32Set storage hooks, FunctionReference hookFunction, bool isPreExecHook, bool isPostExecHook ) internal { - getAccountStorage().selectorData[selector].executionHooks.remove( + hooks.remove( toSetValue( ExecutionHook({hookFunction: hookFunction, isPreHook: isPreExecHook, isPostHook: isPostExecHook}) ) @@ -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]; @@ -238,8 +230,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); } length = manifest.interfaceIds.length; @@ -290,12 +283,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); } length = manifest.signatureValidationFunctions.length; @@ -313,11 +306,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..1a52b5dd 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,7 +465,7 @@ 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); + bytes32 key = executionHooks.at(i); (FunctionReference hookFunction,, bool isPostHook) = toExecutionHook(key); if (isPostHook) { postHooksToRun[i].postExecHook = hookFunction; @@ -465,7 +475,7 @@ 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); + bytes32 key = executionHooks.at(i); (FunctionReference hookFunction, bool isPreHook, bool isPostHook) = toExecutionHook(key); if (isPreHook) { @@ -555,11 +565,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/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 490b216c..b172464a 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -28,6 +28,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/IPlugin.sol b/src/interfaces/IPlugin.sol index 3241e497..7259d748 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -64,6 +64,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. @@ -73,8 +75,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/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 6a3b7d8c..f2e6391f 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -15,7 +15,6 @@ import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract AccountExecHooksTest is AccountTestBase { MockPlugin public mockPlugin1; - MockPlugin public mockPlugin2; bytes32 public manifestHash1; bytes32 public manifestHash2; @@ -25,7 +24,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); @@ -66,7 +64,7 @@ contract AccountExecHooksTest is AccountTestBase { IExecutionHook.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, address(this), // caller - 0, // msg.value in call to account + uint256(0), // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) ), 0 // msg value in call to plugin @@ -105,7 +103,7 @@ contract AccountExecHooksTest is AccountTestBase { IExecutionHook.preExecutionHook.selector, _BOTH_HOOKS_FUNCTION_ID_3, address(this), // caller - 0, // msg.value in call to account + uint256(0), // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) ), 0 // msg value in call to plugin 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; }