diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 9e9053fe..9cde18b4 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -52,14 +52,14 @@ abstract contract AccountLoupe is IAccountLoupe { } /// @inheritdoc IAccountLoupe - function getPreValidationHooks(bytes4 selector) + function getPreValidationHooks(FunctionReference validationFunction) external view override returns (FunctionReference[] memory preValidationHooks) { preValidationHooks = - toFunctionReferenceArray(getAccountStorage().selectorData[selector].preValidationHooks); + toFunctionReferenceArray(getAccountStorage().validationData[validationFunction].preValidationHooks); } /// @inheritdoc IAccountLoupe diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index f463a629..61ddabb3 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -41,17 +41,19 @@ struct SelectorData { bool isPublic; // Whether or not a default validation function may be used to validate this function. bool allowDefaultValidation; - // How many times a `PRE_HOOK_ALWAYS_DENY` has been added for this function. - // Since that is the only type of hook that may overlap, we can use this to track the number of times it has - // been applied, and whether or not the deny should apply. The size `uint48` was chosen somewhat arbitrarily, - // but it packs alongside `plugin` while still leaving some other space in the slot for future packing. - uint48 denyExecutionCount; - // User operation validation and runtime validation share a function reference. + // The execution hooks for this function selector. + EnumerableSet.Bytes32Set executionHooks; + // Which validation functions are associated with this function selector. EnumerableSet.Bytes32Set validations; +} + +struct ValidationData { + // Whether or not this validation can be used as a default validation function. + bool isDefault; + // Whether or not this validation is a signature validator. + bool isSignatureValidation; // The pre validation hooks for this function selector. EnumerableSet.Bytes32Set preValidationHooks; - // The execution hooks for this function selector. - EnumerableSet.Bytes32Set executionHooks; } struct AccountStorage { @@ -63,21 +65,14 @@ 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; // key = address(calling plugin) || target address mapping(IPlugin => mapping(address => PermittedExternalCallData)) permittedExternalCalls; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; - // Installed plugins capable of signature validation. - EnumerableSet.Bytes32Set signatureValidations; - // Todo: merge this with other validation storage? - EnumerableSet.Bytes32Set defaultValidations; } -// TODO: Change how pre-validation hooks work to allow association with validation, rather than selector. -// Pre signature validation hooks -// mapping(FunctionReference => EnumerableSet.Bytes32Set) preSignatureValidationHooks; - function getAccountStorage() pure returns (AccountStorage storage _storage) { assembly ("memory-safe") { _storage.slot := _ACCOUNT_STORAGE_SLOT diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index c8945ef1..9e73e306 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -6,63 +6,109 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet import {IPlugin} from "../interfaces/IPlugin.sol"; import {FunctionReference} from "../interfaces/IPluginManager.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; -import {AccountStorage, getAccountStorage, toSetValue} from "./AccountStorage.sol"; +import {AccountStorage, getAccountStorage, toSetValue, toFunctionReference} from "./AccountStorage.sol"; // Temporary additional functions for a user-controlled install flow for validation functions. abstract contract PluginManager2 { using EnumerableSet for EnumerableSet.Bytes32Set; - error DefaultValidationAlreadySet(address plugin, uint8 functionId); - error ValidationAlreadySet(bytes4 selector, address plugin, uint8 functionId); - error ValidationNotSet(bytes4 selector, address plugin, uint8 functionId); + error DefaultValidationAlreadySet(FunctionReference validationFunction); + error PreValidationAlreadySet(FunctionReference validationFunction, FunctionReference preValidationFunction); + error ValidationAlreadySet(bytes4 selector, FunctionReference validationFunction); + error ValidationNotSet(bytes4 selector, FunctionReference validationFunction); function _installValidation( - address plugin, - uint8 functionId, + FunctionReference validationFunction, bool isDefault, bytes4[] memory selectors, - bytes calldata installData - ) internal { - FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId); - + bytes calldata installData, + bytes memory preValidationHooks + ) + // TODO: flag for signature validation + internal + { AccountStorage storage _storage = getAccountStorage(); + if (preValidationHooks.length > 0) { + (FunctionReference[] memory preValidationFunctions, bytes[] memory initDatas) = + abi.decode(preValidationHooks, (FunctionReference[], bytes[])); + + for (uint256 i = 0; i < preValidationFunctions.length; ++i) { + FunctionReference preValidationFunction = preValidationFunctions[i]; + + if ( + !_storage.validationData[validationFunction].preValidationHooks.add( + toSetValue(preValidationFunction) + ) + ) { + revert PreValidationAlreadySet(validationFunction, preValidationFunction); + } + + if (initDatas[i].length > 0) { + (address preValidationPlugin,) = FunctionReferenceLib.unpack(preValidationFunction); + IPlugin(preValidationPlugin).onInstall(initDatas[i]); + } + } + } + if (isDefault) { - if (!_storage.defaultValidations.add(toSetValue(validationFunction))) { - revert DefaultValidationAlreadySet(plugin, functionId); + if (_storage.validationData[validationFunction].isDefault) { + revert DefaultValidationAlreadySet(validationFunction); } + _storage.validationData[validationFunction].isDefault = true; } for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; if (!_storage.selectorData[selector].validations.add(toSetValue(validationFunction))) { - revert ValidationAlreadySet(selector, plugin, functionId); + revert ValidationAlreadySet(selector, validationFunction); } } - IPlugin(plugin).onInstall(installData); + if (installData.length > 0) { + (address plugin,) = FunctionReferenceLib.unpack(validationFunction); + IPlugin(plugin).onInstall(installData); + } } function _uninstallValidation( - address plugin, - uint8 functionId, + FunctionReference validationFunction, bytes4[] calldata selectors, - bytes calldata uninstallData + bytes calldata uninstallData, + bytes calldata preValidationHookUninstallData ) internal { - FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId); - AccountStorage storage _storage = getAccountStorage(); - // Ignore return value - remove if present, do nothing otherwise. - _storage.defaultValidations.remove(toSetValue(validationFunction)); + _storage.validationData[validationFunction].isDefault = false; + _storage.validationData[validationFunction].isSignatureValidation = false; + 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) { + IPlugin(preValidationPlugin).onUninstall(preValidationHookUninstallDatas[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. for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; if (!_storage.selectorData[selector].validations.remove(toSetValue(validationFunction))) { - revert ValidationNotSet(selector, plugin, functionId); + revert ValidationNotSet(selector, validationFunction); } } - IPlugin(plugin).onUninstall(uninstallData); + if (uninstallData.length > 0) { + (address plugin,) = FunctionReferenceLib.unpack(validationFunction); + IPlugin(plugin).onUninstall(uninstallData); + } } } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index d8966275..4db38b55 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -113,7 +113,7 @@ abstract contract PluginManagerInternals is IPluginManager { SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; // Fail on duplicate validation functions. Otherwise, dependency validation functions could shadow - // non-depdency validation functions. Then, if a either plugin is uninstall, it would cause a partial + // non-depdency validation functions. Then, if a either plugin is uninstalled, it would cause a partial // uninstall of the other. if (!_selectorData.validations.add(toSetValue(validationFunction))) { revert ValidationFunctionAlreadySet(selector, validationFunction); @@ -157,33 +157,6 @@ abstract contract PluginManagerInternals is IPluginManager { ); } - function _addPreValidationHook(bytes4 selector, FunctionReference preValidationHook) - internal - notNullFunction(preValidationHook) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (preValidationHook.eq(FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY)) { - // Increment `denyExecutionCount`, because this pre validation hook may be applied multiple times. - _selectorData.denyExecutionCount += 1; - return; - } - _selectorData.preValidationHooks.add(toSetValue(preValidationHook)); - } - - function _removePreValidationHook(bytes4 selector, FunctionReference preValidationHook) - internal - notNullFunction(preValidationHook) - { - SelectorData storage _selectorData = getAccountStorage().selectorData[selector]; - if (preValidationHook.eq(FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY)) { - // Decrement `denyExecutionCount`, because this pre exec hook may be applied multiple times. - _selectorData.denyExecutionCount -= 1; - return; - } - // May ignore return value, as the manifest hash is validated to ensure that the hook exists. - _selectorData.preValidationHooks.remove(toSetValue(preValidationHook)); - } - function _installPlugin( address plugin, bytes32 manifestHash, @@ -288,10 +261,7 @@ abstract contract PluginManagerInternals is IPluginManager { for (uint256 i = 0; i < length; ++i) { ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; _addValidationFunction( - mv.executionSelector, - _resolveManifestFunction( - mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE - ) + mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies) ); } @@ -299,24 +269,7 @@ abstract contract PluginManagerInternals is IPluginManager { for (uint256 i = 0; i < length; ++i) { FunctionReference signatureValidationFunction = FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); - _storage.signatureValidations.add(toSetValue(signatureValidationFunction)); - } - - // Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them. - FunctionReference[] memory emptyDependencies; - - length = manifest.preValidationHooks.length; - for (uint256 i = 0; i < length; ++i) { - ManifestAssociatedFunction memory mh = manifest.preValidationHooks[i]; - _addPreValidationHook( - mh.executionSelector, - _resolveManifestFunction( - mh.associatedFunction, - plugin, - emptyDependencies, - ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY - ) - ); + _storage.validationData[signatureValidationFunction].isSignatureValidation = true; } length = manifest.executionHooks.length; @@ -375,9 +328,6 @@ abstract contract PluginManagerInternals is IPluginManager { // Remove components according to the manifest, in reverse order (by component type) of their installation. - // Hooks are not allowed to be provided as dependencies, so we use an empty array for resolving them. - FunctionReference[] memory emptyDependencies; - length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { ManifestExecutionHook memory mh = manifest.executionHooks[i]; @@ -385,35 +335,18 @@ abstract contract PluginManagerInternals is IPluginManager { _removeExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); } - length = manifest.preValidationHooks.length; - for (uint256 i = 0; i < length; ++i) { - ManifestAssociatedFunction memory mh = manifest.preValidationHooks[i]; - _removePreValidationHook( - mh.executionSelector, - _resolveManifestFunction( - mh.associatedFunction, - plugin, - emptyDependencies, - ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY - ) - ); - } - length = manifest.signatureValidationFunctions.length; for (uint256 i = 0; i < length; ++i) { FunctionReference signatureValidationFunction = FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); - _storage.signatureValidations.remove(toSetValue(signatureValidationFunction)); + _storage.validationData[signatureValidationFunction].isSignatureValidation = false; } length = manifest.validationFunctions.length; for (uint256 i = 0; i < length; ++i) { ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; _removeValidationFunction( - mv.executionSelector, - _resolveManifestFunction( - mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE - ) + mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies) ); } @@ -486,9 +419,7 @@ abstract contract PluginManagerInternals is IPluginManager { function _resolveManifestFunction( ManifestFunction memory manifestFunction, address plugin, - FunctionReference[] memory dependencies, - // Indicates which magic value, if any, is permissible for the function to resolve. - ManifestAssociatedFunctionType allowedMagicValue + FunctionReference[] memory dependencies ) internal pure returns (FunctionReference) { if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) { return FunctionReferenceLib.pack(plugin, manifestFunction.functionId); @@ -497,12 +428,6 @@ abstract contract PluginManagerInternals is IPluginManager { revert InvalidPluginManifest(); } return dependencies[manifestFunction.dependencyIndex]; - } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY) { - if (allowedMagicValue == ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY) { - return FunctionReferenceLib._PRE_HOOK_ALWAYS_DENY; - } else { - revert InvalidPluginManifest(); - } } return FunctionReferenceLib._EMPTY_FUNCTION_REFERENCE; // Empty checks are done elsewhere } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 047e73c0..c603b0ca 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -65,7 +65,6 @@ contract UpgradeableModularAccount is event ModularAccountInitialized(IEntryPoint indexed entryPoint); - error AlwaysDenyRule(); error AuthorizeUpgradeReverted(bytes revertReason); error ExecFromPluginNotPermitted(address plugin, bytes4 selector); error ExecFromPluginExternalNotPermitted(address plugin, address target, uint256 value, bytes data); @@ -278,13 +277,6 @@ contract UpgradeableModularAccount is // Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. FunctionReference runtimeValidationFunction = FunctionReference.wrap(bytes21(authorization[:21])); - AccountStorage storage _storage = getAccountStorage(); - - // check if that runtime validation function is allowed to be called - if (_storage.selectorData[execSelector].denyExecutionCount > 0) { - revert AlwaysDenyRule(); - } - // Check if the runtime validation function is allowed to be called bool isDefaultValidation = uint8(authorization[21]) == 1; _checkIfValidationApplies(execSelector, runtimeValidationFunction, isDefaultValidation); @@ -338,35 +330,35 @@ contract UpgradeableModularAccount is /// with user install configs. /// @dev This function is only callable once, and only by the EntryPoint. - function initializeDefaultValidation(address plugin, uint8 functionId, bytes calldata installData) + function initializeDefaultValidation(FunctionReference validationFunction, bytes calldata installData) external initializer { - _installValidation(plugin, functionId, true, new bytes4[](0), installData); + _installValidation(validationFunction, true, new bytes4[](0), installData, bytes("")); emit ModularAccountInitialized(_ENTRY_POINT); } /// @inheritdoc IPluginManager /// @notice May be validated by a default validation. function installValidation( - address plugin, - uint8 functionId, + FunctionReference validationFunction, bool isDefault, - bytes4[] calldata selectors, - bytes calldata installData + bytes4[] memory selectors, + bytes calldata installData, + bytes calldata preValidationHooks ) external wrapNativeFunction { - _installValidation(plugin, functionId, isDefault, selectors, installData); + _installValidation(validationFunction, isDefault, selectors, installData, preValidationHooks); } /// @inheritdoc IPluginManager /// @notice May be validated by a default validation. function uninstallValidation( - address plugin, - uint8 functionId, + FunctionReference validationFunction, bytes4[] calldata selectors, - bytes calldata uninstallData + bytes calldata uninstallData, + bytes calldata preValidationHookUninstallData ) external wrapNativeFunction { - _uninstallValidation(plugin, functionId, selectors, uninstallData); + _uninstallValidation(validationFunction, selectors, uninstallData, preValidationHookUninstallData); } /// @notice ERC165 introspection @@ -402,7 +394,7 @@ contract UpgradeableModularAccount is FunctionReference sigValidation = FunctionReference.wrap(bytes21(signature)); (address plugin, uint8 functionId) = sigValidation.unpack(); - if (!_storage.signatureValidations.contains(toSetValue(sigValidation))) { + if (!_storage.validationData[sigValidation].isSignatureValidation) { revert SignatureValidationInvalid(plugin, functionId); } @@ -435,12 +427,6 @@ contract UpgradeableModularAccount is } bytes4 selector = bytes4(userOp.callData); - AccountStorage storage _storage = getAccountStorage(); - - if (_storage.selectorData[selector].denyExecutionCount > 0) { - revert AlwaysDenyRule(); - } - // Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21])); bool isDefaultValidation = uint8(userOp.signature[21]) == 1; @@ -470,7 +456,7 @@ contract UpgradeableModularAccount is // Do preUserOpValidation hooks EnumerableSet.Bytes32Set storage preUserOpValidationHooks = - getAccountStorage().selectorData[selector].preValidationHooks; + getAccountStorage().validationData[userOpValidationFunction].preValidationHooks; uint256 preUserOpValidationHooksLength = preUserOpValidationHooks.length(); for (uint256 i = 0; i < preUserOpValidationHooksLength; ++i) { @@ -508,7 +494,7 @@ contract UpgradeableModularAccount is ) internal { // run all preRuntimeValidation hooks EnumerableSet.Bytes32Set storage preRuntimeValidationHooks = - getAccountStorage().selectorData[bytes4(callData[:4])].preValidationHooks; + getAccountStorage().validationData[runtimeValidationFunction].preValidationHooks; uint256 preRuntimeValidationHooksLength = preRuntimeValidationHooks.length(); for (uint256 i = 0; i < preRuntimeValidationHooksLength; ++i) { @@ -624,10 +610,7 @@ contract UpgradeableModularAccount is // Check that the provided validation function is applicable to the selector if (isDefault) { - if ( - !_defaultValidationAllowed(selector) - || !_storage.defaultValidations.contains(toSetValue(validationFunction)) - ) { + if (!_defaultValidationAllowed(selector) || !_storage.validationData[validationFunction].isDefault) { revert UserOpValidationFunctionMissing(selector); } } else { @@ -654,9 +637,6 @@ contract UpgradeableModularAccount is function _checkPermittedCallerIfNotFromEP() internal view { AccountStorage storage _storage = getAccountStorage(); - if (_storage.selectorData[msg.sig].denyExecutionCount > 0) { - revert AlwaysDenyRule(); - } if ( msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) || _storage.selectorData[msg.sig].isPublic diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index b474149c..490b216c 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -29,9 +29,9 @@ interface IAccountLoupe { function getExecutionHooks(bytes4 selector) external view returns (ExecutionHook[] memory); /// @notice Get the pre user op and runtime validation hooks associated with a selector. - /// @param selector The selector to get the hooks for. + /// @param validationFunction The validation function to get the hooks for. /// @return preValidationHooks The pre validation hooks for this selector. - function getPreValidationHooks(bytes4 selector) + function getPreValidationHooks(FunctionReference validationFunction) external view returns (FunctionReference[] memory preValidationHooks); diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index e9ea8045..b34e924f 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -12,12 +12,7 @@ enum ManifestAssociatedFunctionType { // Function belongs to this plugin. SELF, // Function belongs to an external plugin provided as a dependency during plugin installation. - DEPENDENCY, - // Resolves to a magic value to always fail in a hook for a given function. - // This is only assignable to pre execution hooks. It should not be used on validation functions themselves, because - // this is equivalent to leaving the validation functions unset. It should not be used in post-exec hooks, because - // if it is known to always revert, that should happen as early as possible to save gas. - PRE_HOOK_ALWAYS_DENY + DEPENDENCY } // forgefmt: disable-end @@ -82,7 +77,6 @@ struct PluginManifest { // Execution functions defined in this plugin to be installed on the MSCA. ManifestExecutionFunction[] executionFunctions; ManifestAssociatedFunction[] validationFunctions; - ManifestAssociatedFunction[] preValidationHooks; ManifestExecutionHook[] executionHooks; uint8[] signatureValidationFunctions; // Plugin execution functions already installed on the MSCA that this plugin will be able to call. diff --git a/src/interfaces/IPluginManager.sol b/src/interfaces/IPluginManager.sol index 1c21aadd..717e1fa0 100644 --- a/src/interfaces/IPluginManager.sol +++ b/src/interfaces/IPluginManager.sol @@ -28,31 +28,29 @@ interface IPluginManager { /// validation. /// TODO: remove or update. /// @dev This does not validate anything against the manifest - the caller must ensure validity. - /// @param plugin The plugin to install. - /// @param functionId The function ID of the validation function to install. + /// @param validationFunction The validation function to install. /// @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. function installValidation( - address plugin, - uint8 functionId, + FunctionReference validationFunction, bool isDefault, - bytes4[] calldata selectors, - bytes calldata installData + bytes4[] memory selectors, + bytes calldata installData, + bytes calldata preValidationHooks ) external; /// @notice Uninstall a validation function from a set of execution selectors. /// TODO: remove or update. - /// @param plugin The plugin to uninstall. - /// @param functionId The function ID of the validation function to uninstall. + /// @param validationFunction The validation function to uninstall. /// @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. function uninstallValidation( - address plugin, - uint8 functionId, + FunctionReference validationFunction, bytes4[] calldata selectors, - bytes calldata uninstallData + bytes calldata uninstallData, + bytes calldata preValidationHookUninstallData ) external; /// @notice Uninstall a plugin from the modular account. diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 3d8a0c69..842cf84c 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -3,16 +3,12 @@ pragma solidity ^0.8.19; import { IPlugin, - ManifestAssociatedFunctionType, - ManifestAssociatedFunction, ManifestExecutionHook, ManifestExecutionFunction, - ManifestFunction, PluginManifest } 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"; @@ -166,64 +162,6 @@ contract AccountExecHooksTest is AccountTestBase { _uninstallPlugin(mockPlugin1); } - function test_overlappingPreValidationHooks_install() public { - // Install the first plugin. - _installPlugin1WithPreValidationHook( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, - dependencyIndex: 0 - }) - ); - - // Expect the call to fail due to the "always deny" pre hook. - vm.breakpoint("a"); - (bool success, bytes memory retData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - assertEq(retData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); - - // Install a second plugin that applies the same pre hook on the same selector. - _installPlugin2WithPreValidationHook( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, - dependencyIndex: 0 - }) - ); - - // Still expect the call to fail. - (success, retData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - assertEq(retData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); - - vm.stopPrank(); - } - - function test_overlappingPreValidationHooks_uninstall() public { - test_overlappingPreValidationHooks_install(); - - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre validation hook of "always deny" to still exist. - (bool success, bytes memory retData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - assertEq(retData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); - - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); - - // // Execution selector should no longer exist. - (success, retData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - assertEq( - retData, - abi.encodeWithSelector(UpgradeableModularAccount.UnrecognizedFunction.selector, _EXEC_SELECTOR) - ); - } - function _installPlugin1WithHooks(ManifestExecutionHook memory execHooks) internal { m1.executionHooks.push(execHooks); mockPlugin1 = new MockPlugin(m1); @@ -243,54 +181,6 @@ contract AccountExecHooksTest is AccountTestBase { }); } - function _installPlugin1WithPreValidationHook(bytes4 selector, ManifestFunction memory preValidationHook) - internal - { - m1.preValidationHooks.push( - ManifestAssociatedFunction({executionSelector: selector, associatedFunction: preValidationHook}) - ); - - mockPlugin1 = new MockPlugin(m1); - manifestHash1 = keccak256(abi.encode(mockPlugin1.pluginManifest())); - - vm.expectEmit(true, true, true, true); - emit ReceivedCall(abi.encodeCall(IPlugin.onInstall, (bytes(""))), 0); - vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(mockPlugin1), manifestHash1, new FunctionReference[](0)); - - vm.prank(address(entryPoint)); - account1.installPlugin({ - plugin: address(mockPlugin1), - manifestHash: manifestHash1, - pluginInstallData: bytes(""), - dependencies: new FunctionReference[](0) - }); - } - - function _installPlugin2WithPreValidationHook(bytes4 selector, ManifestFunction memory preValidationHook) - internal - { - m2.preValidationHooks.push( - ManifestAssociatedFunction({executionSelector: selector, associatedFunction: preValidationHook}) - ); - - mockPlugin2 = new MockPlugin(m2); - manifestHash2 = keccak256(abi.encode(mockPlugin2.pluginManifest())); - - vm.expectEmit(true, true, true, true); - emit ReceivedCall(abi.encodeCall(IPlugin.onInstall, (bytes(""))), 0); - vm.expectEmit(true, true, true, true); - emit PluginInstalled(address(mockPlugin2), manifestHash2, new FunctionReference[](0)); - - vm.prank(address(entryPoint)); - account1.installPlugin({ - plugin: address(mockPlugin2), - manifestHash: manifestHash2, - pluginInstallData: bytes(""), - dependencies: new FunctionReference[](0) - }); - } - function _uninstallPlugin(MockPlugin plugin) internal { vm.expectEmit(true, true, true, true); emit ReceivedCall(abi.encodeCall(IPlugin.onUninstall, (bytes(""))), 0); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index a6ed44eb..fa92ab00 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -31,6 +31,20 @@ contract AccountLoupeTest is AccountTestBase { ownerValidation = FunctionReferenceLib.pack( address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) ); + + FunctionReference[] memory preValidationHooks = new FunctionReference[](2); + preValidationHooks[0] = FunctionReferenceLib.pack( + address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_VALIDATION_HOOK_1) + ); + preValidationHooks[1] = FunctionReferenceLib.pack( + address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.PRE_VALIDATION_HOOK_2) + ); + + bytes[] memory installDatas = new bytes[](2); + vm.prank(address(entryPoint)); + account1.installValidation( + ownerValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + ); } function test_pluginLoupe_getInstalledPlugins_initial() public { @@ -133,7 +147,7 @@ contract AccountLoupeTest is AccountTestBase { } function test_pluginLoupe_getValidationHooks() public { - FunctionReference[] memory hooks = account1.getPreValidationHooks(comprehensivePlugin.foo.selector); + FunctionReference[] memory hooks = account1.getPreValidationHooks(ownerValidation); assertEq(hooks.length, 2); assertEq( diff --git a/test/account/ManifestValidity.t.sol b/test/account/ManifestValidity.t.sol deleted file mode 100644 index 08c2609d..00000000 --- a/test/account/ManifestValidity.t.sol +++ /dev/null @@ -1,30 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; - -import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; -import {FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; - -import {BadHookMagicValue_ValidationFunction_Plugin} from "../mocks/plugins/ManifestValidityMocks.sol"; -import {AccountTestBase} from "../utils/AccountTestBase.sol"; - -contract ManifestValidityTest is AccountTestBase { - function setUp() public { - _transferOwnershipToTest(); - } - - // Tests that the plugin manager rejects a plugin with a validation function set to "hook always deny" - function test_ManifestValidity_invalid_HookAlwaysDeny_Validation() public { - BadHookMagicValue_ValidationFunction_Plugin plugin = new BadHookMagicValue_ValidationFunction_Plugin(); - - bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - - vm.prank(address(entryPoint)); - vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); - account1.installPlugin({ - plugin: address(plugin), - manifestHash: manifestHash, - pluginInstallData: "", - dependencies: new FunctionReference[](0) - }); - } -} diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index de0e7ef1..877f8ff9 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -58,12 +58,37 @@ contract ValidationIntersectionTest is AccountTestBase { pluginInstallData: "", dependencies: new FunctionReference[](0) }); + // TODO: change with new install flow + // temporary fix to add the pre-validation hook + FunctionReference[] memory preValidationHooks = new FunctionReference[](1); + preValidationHooks[0] = FunctionReferenceLib.pack({ + addr: address(oneHookPlugin), + functionId: uint8(MockBaseUserOpValidationPlugin.FunctionId.PRE_VALIDATION_HOOK_1) + }); + bytes[] memory installDatas = new bytes[](1); + account1.installValidation( + oneHookValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + ); account1.installPlugin({ plugin: address(twoHookPlugin), manifestHash: keccak256(abi.encode(twoHookPlugin.pluginManifest())), pluginInstallData: "", dependencies: new FunctionReference[](0) }); + // temporary fix to add the pre-validation hook + preValidationHooks = new FunctionReference[](2); + preValidationHooks[0] = FunctionReferenceLib.pack({ + addr: address(twoHookPlugin), + functionId: uint8(MockBaseUserOpValidationPlugin.FunctionId.PRE_VALIDATION_HOOK_1) + }); + preValidationHooks[1] = FunctionReferenceLib.pack({ + addr: address(twoHookPlugin), + functionId: uint8(MockBaseUserOpValidationPlugin.FunctionId.PRE_VALIDATION_HOOK_2) + }); + installDatas = new bytes[](2); + account1.installValidation( + twoHookValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + ); vm.stopPrank(); } diff --git a/test/mocks/DefaultValidationFactoryFixture.sol b/test/mocks/DefaultValidationFactoryFixture.sol index 5fbefe7c..a4836ad8 100644 --- a/test/mocks/DefaultValidationFactoryFixture.sol +++ b/test/mocks/DefaultValidationFactoryFixture.sol @@ -6,6 +6,7 @@ import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.s import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; @@ -55,8 +56,9 @@ contract DefaultValidationFactoryFixture is OptimizedTest { // point proxy to actual implementation and init plugins UpgradeableModularAccount(payable(addr)).initializeDefaultValidation( - address(singleOwnerPlugin), - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), + FunctionReferenceLib.pack( + address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) + ), pluginInstallData ); } diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 8257a2b8..6ef654c7 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -17,7 +17,6 @@ import {IValidation} from "../../../src/interfaces/IValidation.sol"; import {IValidationHook} from "../../../src/interfaces/IValidationHook.sol"; import {IExecutionHook} from "../../../src/interfaces/IExecutionHook.sol"; -import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, BasePlugin { @@ -150,40 +149,6 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba associatedFunction: fooValidationFunction }); - manifest.preValidationHooks = new ManifestAssociatedFunction[](4); - manifest.preValidationHooks[0] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preValidationHooks[1] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_2), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preValidationHooks[2] = ManifestAssociatedFunction({ - executionSelector: IStandardExecutor.execute.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preValidationHooks[3] = ManifestAssociatedFunction({ - executionSelector: IStandardExecutor.execute.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_2), - dependencyIndex: 0 // Unused. - }) - }); - manifest.executionHooks = new ManifestExecutionHook[](3); manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol deleted file mode 100644 index d5c8ce1a..00000000 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ /dev/null @@ -1,49 +0,0 @@ -// SPDX-License-Identifier: UNLICENSED -pragma solidity ^0.8.19; - -import { - ManifestFunction, - ManifestExecutionFunction, - ManifestAssociatedFunctionType, - ManifestAssociatedFunction, - PluginManifest, - PluginMetadata -} from "../../../src/interfaces/IPlugin.sol"; - -import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; - -// solhint-disable-next-line contract-name-camelcase -contract BadHookMagicValue_ValidationFunction_Plugin is BasePlugin { - function onInstall(bytes calldata) external override {} - - function onUninstall(bytes calldata) external override {} - - function foo() external pure returns (bytes32) { - return keccak256("bar"); - } - - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = ManifestExecutionFunction({ - executionSelector: this.foo.selector, - isPublic: false, - allowDefaultValidation: false - }); - - manifest.validationFunctions = new ManifestAssociatedFunction[](1); - manifest.validationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.foo.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, - functionId: 0, - dependencyIndex: 0 - }) - }); - - return manifest; - } - - function pluginMetadata() external pure override returns (PluginMetadata memory) {} -} diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 770c6791..d5f75e99 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -154,16 +154,6 @@ contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { associatedFunction: userOpValidationFunctionRef }); - manifest.preValidationHooks = new ManifestAssociatedFunction[](1); - manifest.preValidationHooks[0] = ManifestAssociatedFunction({ - executionSelector: this.bar.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - return manifest; } } @@ -210,24 +200,6 @@ contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { associatedFunction: userOpValidationFunctionRef }); - manifest.preValidationHooks = new ManifestAssociatedFunction[](2); - manifest.preValidationHooks[0] = ManifestAssociatedFunction({ - executionSelector: this.baz.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_1), - dependencyIndex: 0 // Unused. - }) - }); - manifest.preValidationHooks[1] = ManifestAssociatedFunction({ - executionSelector: this.baz.selector, - associatedFunction: ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: uint8(FunctionId.PRE_VALIDATION_HOOK_2), - dependencyIndex: 0 // Unused. - }) - }); - return manifest; } }