diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index a4791da9a..a4d36430a 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -479,7 +479,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _addUserOpValidationFunction( mv.executionSelector, _resolveManifestFunction( - mv.associatedFunction, plugin, dependencies, ManifestAssociatedFunctionType.NONE + mv.associatedFunction, plugin, dependencies, true, ManifestAssociatedFunctionType.NONE ) ); @@ -498,6 +498,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { mv.associatedFunction, plugin, dependencies, + true, ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW ) ); @@ -517,6 +518,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { mh.associatedFunction, plugin, dependencies, + false, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -536,6 +538,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { mh.associatedFunction, plugin, dependencies, + false, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -551,10 +554,14 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _addExecHooks( mh.executionSelector, _resolveManifestFunction( - mh.preExecHook, plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY + mh.preExecHook, + plugin, + dependencies, + false, + ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ), _resolveManifestFunction( - mh.postExecHook, plugin, dependencies, ManifestAssociatedFunctionType.NONE + mh.postExecHook, plugin, dependencies, false, ManifestAssociatedFunctionType.NONE ) ); @@ -573,12 +580,14 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { manifest.permittedCallHooks[i].preExecHook, plugin, dependencies, + false, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ), _resolveManifestFunction( manifest.permittedCallHooks[i].postExecHook, plugin, dependencies, + false, ManifestAssociatedFunctionType.NONE ) ); @@ -695,12 +704,14 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { args.manifest.permittedCallHooks[i].preExecHook, args.plugin, dependencies, + false, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ), _resolveManifestFunction( args.manifest.permittedCallHooks[i].postExecHook, args.plugin, dependencies, + false, ManifestAssociatedFunctionType.NONE ) ); @@ -717,10 +728,14 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _removeExecHooks( mh.executionSelector, _resolveManifestFunction( - mh.preExecHook, args.plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY + mh.preExecHook, + args.plugin, + dependencies, + false, + ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ), _resolveManifestFunction( - mh.postExecHook, args.plugin, dependencies, ManifestAssociatedFunctionType.NONE + mh.postExecHook, args.plugin, dependencies, false, ManifestAssociatedFunctionType.NONE ) ); @@ -740,6 +755,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { mh.associatedFunction, args.plugin, dependencies, + false, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -760,6 +776,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { mh.associatedFunction, args.plugin, dependencies, + false, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -939,13 +956,18 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { ManifestFunction memory manifestFunction, address plugin, FunctionReference[] memory dependencies, + bool allowDependencyType, // Indicates which magic value, if any, is permissible for the function to resolve. ManifestAssociatedFunctionType allowedMagicValue ) internal pure returns (FunctionReference) { if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) { return FunctionReferenceLib.pack(plugin, manifestFunction.functionId); } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { - return dependencies[manifestFunction.dependencyIndex]; + if (allowDependencyType) { + return dependencies[manifestFunction.dependencyIndex]; + } else { + revert InvalidPluginManifest(); + } } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW) { if (allowedMagicValue == ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW) { diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index b5b2e2c1e..4a990bed8 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -58,7 +58,7 @@ struct ManifestExternalCallPermission { struct PluginManifest { // List of ERC-165 interfaceIds to add to account to support introspection checks. bytes4[] interfaceIds; - // If this plugin depends on other plugins' validation functions and/or hooks, the interface IDs of + // If this plugin depends on other plugins' validation functions, the interface IDs of // those plugins MUST be provided here, with its position in the array matching the `dependencyIndex` // members of `ManifestFunction` structs used in the manifest. bytes4[] dependencyInterfaceIds; diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index 1130fc265..375e0998b 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -6,6 +6,7 @@ import {Test} from "forge-std/Test.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; +import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {MultiOwnerPlugin} from "../../src/plugins/owner/MultiOwnerPlugin.sol"; import {IEntryPoint} from "../../src/interfaces/erc4337/IEntryPoint.sol"; @@ -196,756 +197,122 @@ contract UpgradeableModularAccountExecHooksTest is Test { vm.stopPrank(); } - /// @dev Plugin 1 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_execHookPair_install() public { - vm.startPrank(owner1); - - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_execHookPair_run() public { - test_execHookPair_install(); - - vm.startPrank(owner1); - - vm.expectEmit(true, true, true, true); - // pre hook call - emit ReceivedCall( - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 0 // msg value in call to plugin - ); - vm.expectEmit(true, true, true, true); - // exec call - emit ReceivedCall(abi.encodePacked(_EXEC_SELECTOR), 0); - vm.expectEmit(true, true, true, true); - // post hook call - emit ReceivedCall( - abi.encodeCall( - IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, abi.encode(_PRE_HOOK_FUNCTION_ID_1)) - ), - 0 // msg value in call to plugin - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_execHookPair_uninstall() public { - test_execHookPair_install(); - - vm.startPrank(owner1); - - _uninstallPlugin(mockPlugin1); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [null, 2] - /// Expected execution: [null, 2] - function test_postOnlyExecHook_install() public { - vm.startPrank(owner1); - - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [null, 2] - /// Expected execution: [null, 2] - function test_postOnlyExecHook_run() public { - test_postOnlyExecHook_install(); - - vm.startPrank(owner1); - - vm.expectEmit(true, true, true, true); - emit ReceivedCall( - abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), - 0 // msg value in call to plugin - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [null, 2] - /// Expected execution: [null, 2] - function test_postOnlyExecHook_uninstall() public { - test_postOnlyExecHook_install(); - - vm.startPrank(owner1); - - _uninstallPlugin(mockPlugin1); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, null] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, null] - function test_overlappingPreExecHooks_install() public { - vm.startPrank(owner1); - - // Install the first plugin. - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) - ); - - // Install a second plugin that applies the first plugin's hook to the same selector. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1); - _installPlugin2WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - dependencies - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, null] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, null] - function test_overlappingPreExecHooks_run() public { - test_overlappingPreExecHooks_install(); - - vm.startPrank(owner1); - - // Expect the pre hook to be called just once. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, null] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, null] - function test_overlappingPreExecHooks_uninstall() public { - test_overlappingPreExecHooks_install(); - - vm.startPrank(owner1); - - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre hook to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); - - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairs_install() public { - vm.startPrank(owner1); - - // Install the first plugin. - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - // Install a second plugin that applies the first plugin's hook pair to the same selector. - FunctionReference[] memory dependencies = new FunctionReference[](2); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1); - dependencies[1] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); - _installPlugin2WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 1 - }), - dependencies - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairs_run() public { - test_overlappingExecHookPairs_install(); - - vm.startPrank(owner1); - - // Expect the pre hook to be called just once. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - - // Expect the post hook to be called just once, with the expected data. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairs_uninstall() public { - test_overlappingExecHookPairs_install(); - - vm.startPrank(owner1); - - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); - - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [3, 2] - /// Expected execution: [1, 2], [3, 2] - function test_overlappingExecHookPairsOnPost_install() public { - vm.startPrank(owner1); - - // Install the first plugin. - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - // Install the second plugin. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); - _installPlugin2WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_3, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - dependencies - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [3, 2] - /// Expected execution: [1, 2], [3, 2] - function test_overlappingExecHookPairsOnPost_run() public { - test_overlappingExecHookPairsOnPost_install(); - - vm.startPrank(owner1); - - // Expect each pre hook to be called once. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - vm.expectCall( - address(mockPlugin2), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_3, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - - // Expect the post hook to be called twice, with the expected data. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_3) // preExecHookData - ), - 1 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [3, 2] - /// Expected execution: [1, 2], [3, 2] - function test_overlappingExecHookPairsOnPost_uninstall() public { - test_overlappingExecHookPairsOnPost_install(); - - vm.startPrank(owner1); - - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); - - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, 4] - /// Expected execution: [1, 2], [1, 4] - function test_overlappingExecHookPairsOnPre_install() public { - vm.startPrank(owner1); - - // Install the first plugin. - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - // Install the second plugin. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1); - _installPlugin2WithHooks( - _EXEC_SELECTOR, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_4, - dependencyIndex: 0 - }), - dependencies - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, 4] - /// Expected execution: [1, 2], [1, 4] - function test_overlappingExecHookPairsOnPre_run() public { - test_overlappingExecHookPairsOnPre_install(); - - vm.startPrank(owner1); - - // Expect the pre hook to be called twice, each passing data over to their respective post hooks. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 2 - ); - - // Expect each post hook to be called once, with the expected data. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - vm.expectCall( - address(mockPlugin2), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_4, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, 4] - /// Expected execution: [1, 2], [1, 4] - function test_overlappingExecHookPairsOnPre_uninstall() public { - test_overlappingExecHookPairsOnPre_install(); - - vm.startPrank(owner1); - - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); - - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairsOnPreWithNullPost_install() public { + function test_overlappingPreExecHookAlwaysDeny_install() public { vm.startPrank(owner1); - // Install the first plugin. _installPlugin1WithHooks( _EXEC_SELECTOR, ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, + functionId: 0, dependencyIndex: 0 }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) ); - // Install the second plugin. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _PRE_HOOK_FUNCTION_ID_1); _installPlugin2WithHooks( _EXEC_SELECTOR, ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, functionId: 0, dependencyIndex: 0 }), ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - dependencies + new FunctionReference[](0) ); vm.stopPrank(); } - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairsOnPreWithNullPost_run() public { - test_overlappingExecHookPairsOnPreWithNullPost_install(); + function test_overlappingPreExecHookAlwaysDeny_revert() public { + test_overlappingPreExecHookAlwaysDeny_install(); vm.startPrank(owner1); - // Expect the pre hook to be called just once. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); + (bool success, bytes memory returnData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq(returnData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); - // Expect the post hook to be called just once, with the expected data. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); + vm.stopPrank(); + } - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); + function test_overlappingPreExecHookAlwaysDeny_uninstallPlugin1() public { + test_overlappingPreExecHookAlwaysDeny_install(); + + vm.startPrank(owner1); + + _uninstallPlugin(mockPlugin1); + + // Execution selector should no longer exist. + (bool success, bytes memory returnData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq( + returnData, + abi.encodeWithSelector(UpgradeableModularAccount.UnrecognizedFunction.selector, _EXEC_SELECTOR) + ); vm.stopPrank(); } - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairsOnPreWithNullPost_uninstall() public { - test_overlappingExecHookPairsOnPreWithNullPost_install(); + function test_overlappingPreExecHookAlwaysDeny_uninstallPlugin2() public { + test_overlappingPreExecHookAlwaysDeny_install(); vm.startPrank(owner1); - // Uninstall the second plugin. _uninstallPlugin(mockPlugin2); - // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 + (bool success, bytes memory returnData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq(returnData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); + + vm.stopPrank(); + } + + /// Plugins cannot depend on hooks from other plugins. + function test_overlappingPreExecHook_invalid() public { + vm.startPrank(owner1); + + _installPlugin1WithHooks( + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, + functionId: 0, + dependencyIndex: 0 + }), + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}) ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); + FunctionReference[] memory dependencies = new FunctionReference[](1); + dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), 1); - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + (bool success, bytes memory returnData) = address(this).call( + abi.encodeWithSelector( + this._installPlugin2WithHooksNoSuccessCheck.selector, + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionId: 0, + dependencyIndex: 0 + }), + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.NONE, + functionId: 0, + dependencyIndex: 0 + }), + dependencies + ) + ); assertFalse(success); + assertEq(returnData, abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); vm.stopPrank(); } /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [null, 2] - /// Expected execution: [1, 2], [null, 2] - function test_overlappingExecHookPairsOnPostWithNullPre_install() public { + /// Expected execution: [1, 2] + function test_execHookPair_install() public { vm.startPrank(owner1); - // Install the first plugin. _installPlugin1WithHooks( _EXEC_SELECTOR, ManifestFunction({ @@ -960,34 +327,19 @@ contract UpgradeableModularAccountExecHooksTest is Test { }) ); - // Install the second plugin. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); - _installPlugin2WithHooks( - _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - dependencies - ); - vm.stopPrank(); } /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [null, 2] - /// Expected execution: [1, 2], [null, 2] - function test_overlappingExecHookPairsOnPostWithNullPre_run() public { - test_overlappingExecHookPairsOnPostWithNullPre_install(); + /// Expected execution: [1, 2] + function test_execHookPair_run() public { + test_execHookPair_install(); vm.startPrank(owner1); - // Expect the pre hook to be called just once. - vm.expectCall( - address(mockPlugin1), + vm.expectEmit(true, true, true, true); + // pre hook call + emit ReceivedCall( abi.encodeWithSelector( IPlugin.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, @@ -995,27 +347,18 @@ contract UpgradeableModularAccountExecHooksTest is Test { 0, // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) ), - 1 - ); - - // Expect the post hook to be called twice, with the expected data. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 + 0 // msg value in call to plugin ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - "" // preExecHookData + vm.expectEmit(true, true, true, true); + // exec call + emit ReceivedCall(abi.encodePacked(_EXEC_SELECTOR), 0); + vm.expectEmit(true, true, true, true); + // post hook call + emit ReceivedCall( + abi.encodeCall( + IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, abi.encode(_PRE_HOOK_FUNCTION_ID_1)) ), - 1 + 0 // msg value in call to plugin ); (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); @@ -1025,57 +368,22 @@ contract UpgradeableModularAccountExecHooksTest is Test { } /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [null, 2] - /// Expected execution: [1, 2], [null, 2] - function test_overlappingExecHookPairsOnPostWithNullPre_uninstall() public { - test_overlappingExecHookPairsOnPostWithNullPre_install(); + /// Expected execution: [1, 2] + function test_execHookPair_uninstall() public { + test_execHookPair_install(); vm.startPrank(owner1); - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - // Uninstall the first plugin. _uninstallPlugin(mockPlugin1); - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - vm.stopPrank(); } /// @dev Plugin 1 hook pair: [null, 2] - /// Plugin 2 hook pair: [null, 2] /// Expected execution: [null, 2] - function test_overlappingPostExecHooks_install() public { + function test_postOnlyExecHook_install() public { vm.startPrank(owner1); - // Install the first plugin. _installPlugin1WithHooks( _EXEC_SELECTOR, ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), @@ -1086,40 +394,20 @@ contract UpgradeableModularAccountExecHooksTest is Test { }) ); - // Install the second plugin. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); - _installPlugin2WithHooks( - _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - dependencies - ); - vm.stopPrank(); } /// @dev Plugin 1 hook pair: [null, 2] - /// Plugin 2 hook pair: [null, 2] /// Expected execution: [null, 2] - function test_overlappingPostExecHooks_run() public { - test_overlappingPostExecHooks_install(); + function test_postOnlyExecHook_run() public { + test_postOnlyExecHook_install(); vm.startPrank(owner1); - // Expect the post hook to be called just once. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - "" // preExecHookData - ), - 1 + vm.expectEmit(true, true, true, true); + emit ReceivedCall( + abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), + 0 // msg value in call to plugin ); (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); @@ -1129,120 +417,14 @@ contract UpgradeableModularAccountExecHooksTest is Test { } /// @dev Plugin 1 hook pair: [null, 2] - /// Plugin 2 hook pair: [null, 2] /// Expected execution: [null, 2] - function test_overlappingPostExecHooks_uninstall() public { - test_overlappingPostExecHooks_install(); + function test_postOnlyExecHook_uninstall() public { + test_postOnlyExecHook_install(); vm.startPrank(owner1); - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - // Expect the pre/post hooks to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - "" // preExecHookData - ), - 1 - ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - // Uninstall the first plugin. _uninstallPlugin(mockPlugin1); - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertFalse(success); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [null, 2] - /// Expected execution: [1, 2], [null, 2] - function test_execHooksWithPostOnlyForNativeFunction_install() public { - vm.startPrank(owner1); - - // Install the first plugin. - _installPlugin1WithHooks( - UpgradeableModularAccount.execute.selector, - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _PRE_HOOK_FUNCTION_ID_1, - dependencyIndex: 0 - }), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, - dependencyIndex: 0 - }) - ); - - // Install the second plugin. - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), _POST_HOOK_FUNCTION_ID_2); - _installPlugin2WithHooks( - UpgradeableModularAccount.execute.selector, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), - ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, - functionId: 0, - dependencyIndex: 0 - }), - dependencies - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook pair: [1, 2] - /// Plugin 2 hook pair: [null, 2] - /// Expected execution: [1, 2], [null, 2] - function test_execHooksWithPostOnlyForNativeFunction_run() public { - test_execHooksWithPostOnlyForNativeFunction_install(); - - vm.startPrank(owner1); - - // Expect the pre hook to be called just once. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preExecutionHook.selector, - _PRE_HOOK_FUNCTION_ID_1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(UpgradeableModularAccount.execute.selector, address(0), 0, "") - ), - 1 - ); - - // Expect the post hook to be called twice, with the expected data. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - abi.encode(_PRE_HOOK_FUNCTION_ID_1) // preExecHookData - ), - 1 - ); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.postExecutionHook.selector, - _POST_HOOK_FUNCTION_ID_2, - "" // preExecHookData - ), - 1 - ); - - account1.execute(address(0), 0, ""); - vm.stopPrank(); } @@ -1279,6 +461,27 @@ contract UpgradeableModularAccountExecHooksTest is Test { ManifestFunction memory postHook, FunctionReference[] memory dependencies ) internal { + _installPlugin2WithHooksInternal(selector, preHook, postHook, dependencies, true); + } + + function _installPlugin2WithHooksNoSuccessCheck( + bytes4 selector, + ManifestFunction memory preHook, + ManifestFunction memory postHook, + FunctionReference[] memory dependencies + ) external { + _installPlugin2WithHooksInternal(selector, preHook, postHook, dependencies, false); + } + + function _installPlugin2WithHooksInternal( + bytes4 selector, + ManifestFunction memory preHook, + ManifestFunction memory postHook, + FunctionReference[] memory dependencies, + bool expectSuccess + ) internal { + vm.startPrank(owner1); + if (preHook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); } @@ -1291,12 +494,14 @@ contract UpgradeableModularAccountExecHooksTest is Test { 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, dependencies, new IPluginManager.InjectedHook[](0) - ); + if (expectSuccess) { + 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, dependencies, new IPluginManager.InjectedHook[](0) + ); + } account1.installPlugin({ plugin: address(mockPlugin2), @@ -1305,6 +510,8 @@ contract UpgradeableModularAccountExecHooksTest is Test { dependencies: dependencies, injectedHooks: new IPluginManager.InjectedHook[](0) }); + + vm.stopPrank(); } function _uninstallPlugin(MockPlugin plugin) internal { diff --git a/test/account/AccountPreValidationHooks.t.sol b/test/account/AccountPreValidationHooks.t.sol index 1f4488ef4..ed3c44fe9 100644 --- a/test/account/AccountPreValidationHooks.t.sol +++ b/test/account/AccountPreValidationHooks.t.sol @@ -6,6 +6,7 @@ import {Test} from "forge-std/Test.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; +import {PluginManagerInternals} from "../../src/account/PluginManagerInternals.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {IMultiOwnerPlugin} from "../../src/plugins/owner/IMultiOwnerPlugin.sol"; import {MultiOwnerPlugin} from "../../src/plugins/owner/MultiOwnerPlugin.sol"; @@ -248,91 +249,104 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { vm.stopPrank(); } - /// @dev Plugin 1 hook: 1 - /// Plugin 2 hook: 1 - /// Expected execution: [1] - function test_overlappingPreRuntimeValidationHook_install() public { + /// Plugins cannot depend on hooks from other plugins. + function test_overlappingPreRuntimeValidationHook_invalid() public { vm.startPrank(owner1); _installPlugin1WithPreRuntimeValidationHook( _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, functionId: 1, dependencyIndex: 0}) + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, + functionId: 0, + dependencyIndex: 0 + }) ); FunctionReference[] memory dependencies = new FunctionReference[](1); dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), 1); + + (bool success, bytes memory returnData) = address(this).call( + abi.encodeWithSelector( + this._installPlugin2WithPreRuntimeValidationHookNoSuccessCheck.selector, + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionId: 0, + dependencyIndex: 0 + }), + dependencies + ) + ); + assertFalse(success); + assertEq(returnData, abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); + vm.stopPrank(); + } + + function test_overlappingPreRuntimeValidationHookAlwaysDeny_install() public { + vm.startPrank(owner1); + + _installPlugin1WithPreRuntimeValidationHook( + _EXEC_SELECTOR, + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, + functionId: 0, + dependencyIndex: 0 + }) + ); + _installPlugin2WithPreRuntimeValidationHook( _EXEC_SELECTOR, ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, functionId: 0, dependencyIndex: 0 }), - dependencies + new FunctionReference[](0) ); vm.stopPrank(); } - /// @dev Plugin 1 hook: 1 - /// Plugin 2 hook: 1 - /// Expected execution: [1] - function test_overlappingPreRuntimeValidationHooks_run() public { - test_overlappingPreRuntimeValidationHook_install(); + function test_overlappingPreRuntimeValidationHookAlwaysDeny_revert() public { + test_overlappingPreRuntimeValidationHookAlwaysDeny_install(); vm.startPrank(owner1); - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preRuntimeValidationHook.selector, - 1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); + (bool success, bytes memory returnData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq(returnData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); vm.stopPrank(); } - /// @dev Plugin 1 hook: 1 - /// Plugin 2 hook: 1 - /// Expected execution: [1] - function test_overlappingPreRuntimeValidationHook_uninstall() public { - test_overlappingPreRuntimeValidationHook_install(); + function test_overlappingPreRuntimeValidationHookAlwaysDeny_uninstallPlugin1() public { + test_overlappingPreRuntimeValidationHookAlwaysDeny_install(); vm.startPrank(owner1); - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); + _uninstallPlugin(mockPlugin1); - // Expect the hook to still exist after uninstalling a plugin with a duplicate hook. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector( - IPlugin.preRuntimeValidationHook.selector, - 1, - owner1, // caller - 0, // msg.value in call to account - abi.encodeWithSelector(_EXEC_SELECTOR) - ), - 1 + // Execution selector should no longer exist. + (bool success, bytes memory returnData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertFalse(success); + assertEq( + returnData, + abi.encodeWithSelector(UpgradeableModularAccount.UnrecognizedFunction.selector, _EXEC_SELECTOR) ); - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); + vm.stopPrank(); + } - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); + function test_overlappingPreRuntimeValidationHookAlwaysDeny_uninstallPlugin2() public { + test_overlappingPreRuntimeValidationHookAlwaysDeny_install(); - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + vm.startPrank(owner1); + + _uninstallPlugin(mockPlugin2); + + (bool success, bytes memory returnData) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); assertFalse(success); + assertEq(returnData, abi.encodeWithSelector(UpgradeableModularAccount.AlwaysDenyRule.selector)); vm.stopPrank(); } @@ -494,118 +508,28 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { vm.stopPrank(); } - /// @dev Plugin 1 hook: 1 - /// Plugin 2 hook: 1 - /// Expected execution: [1, 1] - function test_overlappingPreUserOpValidationHooks_install() public { + function test_overlappingPreUserOpValidationHookAlwaysDeny_install() public { vm.startPrank(owner1); _installPlugin1WithPreUserOpValidationHook( _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.SELF, functionId: 1, dependencyIndex: 0}) + ManifestFunction({ + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, + functionId: 0, + dependencyIndex: 0 + }) ); - FunctionReference[] memory dependencies = new FunctionReference[](1); - dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), 1); _installPlugin2WithPreUserOpValidationHook( _EXEC_SELECTOR, ManifestFunction({ - functionType: ManifestAssociatedFunctionType.DEPENDENCY, + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, functionId: 0, dependencyIndex: 0 }), - dependencies - ); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook: 1 - /// Plugin 2 hook: 1 - /// Expected execution: [1] - function test_overlappingPreUserOpValidationHooks_run() public { - test_overlappingPreUserOpValidationHooks_install(); - - vm.startPrank(owner1); - - UserOperation memory userOp = UserOperation({ - sender: address(account1), - nonce: 0, - initCode: "", - callData: abi.encodeWithSelector(_EXEC_SELECTOR), - callGasLimit: CALL_GAS_LIMIT, - verificationGasLimit: VERIFICATION_GAS_LIMIT, - preVerificationGas: 0, - maxFeePerGas: 2, - maxPriorityFeePerGas: 1, - paymasterAndData: "", - signature: "" - }); - - // Generate signature - bytes32 userOpHash = entryPoint.getUserOpHash(userOp); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(r, s, v); - - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector(IPlugin.preUserOpValidationHook.selector, 1, userOp, userOpHash), - 1 - ); - - UserOperation[] memory userOps = new UserOperation[](1); - userOps[0] = userOp; - - entryPoint.handleOps(userOps, beneficiary); - - vm.stopPrank(); - } - - /// @dev Plugin 1 hook: 1 - /// Plugin 2 hook: 1 - /// Expected execution: [1] - function test_overlappingPreUserOpValidationHooks_uninstall() public { - test_overlappingPreUserOpValidationHooks_install(); - - vm.startPrank(owner1); - - // Uninstall the second plugin. - _uninstallPlugin(mockPlugin2); - - UserOperation memory userOp = UserOperation({ - sender: address(account1), - nonce: 0, - initCode: "", - callData: abi.encodeWithSelector(_EXEC_SELECTOR), - callGasLimit: CALL_GAS_LIMIT, - verificationGasLimit: VERIFICATION_GAS_LIMIT, - preVerificationGas: 0, - maxFeePerGas: 2, - maxPriorityFeePerGas: 1, - paymasterAndData: "", - signature: "" - }); - - // Generate signature - bytes32 userOpHash = entryPoint.getUserOpHash(userOp); - (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(r, s, v); - - // Expect hook 1 to still exist. - vm.expectCall( - address(mockPlugin1), - abi.encodeWithSelector(IPlugin.preUserOpValidationHook.selector, 1, userOp, userOpHash), - 1 + new FunctionReference[](0) ); - UserOperation[] memory userOps = new UserOperation[](1); - userOps[0] = userOp; - - entryPoint.handleOps(userOps, beneficiary); - - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); - vm.stopPrank(); } @@ -640,6 +564,25 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { ManifestFunction memory hook, FunctionReference[] memory dependencies ) internal { + _installPlugin2WithPreRuntimeValidationHookInternal(selector, hook, dependencies, true); + } + + function _installPlugin2WithPreRuntimeValidationHookNoSuccessCheck( + bytes4 selector, + ManifestFunction memory hook, + FunctionReference[] memory dependencies + ) external { + _installPlugin2WithPreRuntimeValidationHookInternal(selector, hook, dependencies, false); + } + + function _installPlugin2WithPreRuntimeValidationHookInternal( + bytes4 selector, + ManifestFunction memory hook, + FunctionReference[] memory dependencies, + bool expectSuccess + ) internal { + vm.startPrank(owner1); + if (hook.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { m2.dependencyInterfaceIds.push(type(IPlugin).interfaceId); } @@ -648,11 +591,13 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { mockPlugin2 = new MockPlugin(m2); manifestHash2 = keccak256(abi.encode(mockPlugin2.pluginManifest())); - vm.expectCall(address(mockPlugin2), abi.encodeCall(IPlugin.onInstall, ("")), 1); - vm.expectEmit(true, true, true, true); - emit PluginInstalled( - address(mockPlugin2), manifestHash2, dependencies, new IPluginManager.InjectedHook[](0) - ); + if (expectSuccess) { + vm.expectCall(address(mockPlugin2), abi.encodeCall(IPlugin.onInstall, ("")), 1); + vm.expectEmit(true, true, true, true); + emit PluginInstalled( + address(mockPlugin2), manifestHash2, dependencies, new IPluginManager.InjectedHook[](0) + ); + } account1.installPlugin({ plugin: address(mockPlugin2), @@ -661,6 +606,8 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { dependencies: dependencies, injectedHooks: new IPluginManager.InjectedHook[](0) }); + + vm.stopPrank(); } function _installPlugin1WithPreUserOpValidationHook(bytes4 selector, ManifestFunction memory hook) internal { @@ -736,4 +683,20 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { account1.uninstallPlugin(address(plugin), bytes(""), bytes(""), new bytes[](0)); } + + function _userOperation() internal returns (UserOperation memory) { + return UserOperation({ + sender: address(account1), + nonce: 0, + initCode: "", + callData: abi.encodeWithSelector(_EXEC_SELECTOR), + callGasLimit: CALL_GAS_LIMIT, + verificationGasLimit: VERIFICATION_GAS_LIMIT, + preVerificationGas: 0, + maxFeePerGas: 2, + maxPriorityFeePerGas: 1, + paymasterAndData: "", + signature: "" + }); + } }