From e0df98f02be618b698f8b2f07b572bee9e565431 Mon Sep 17 00:00:00 2001 From: David Philipson Date: Wed, 17 Jan 2024 17:15:37 -0800 Subject: [PATCH] fix: [quantstamp-16] Disallow using dependencies for hooks Allowing hooks to be dependencies can lead to hooks being called in unexpected contexts, such as state-changing validation hooks being called outside of validation or one of a pre- and post-exec hook pair being called without the other. To prevent this without changing the interface from the spec, we disallow this case entirely. --- src/account/PluginManagerInternals.sol | 30 +- src/interfaces/IPlugin.sol | 2 +- test/account/AccountExecHooks.t.sol | 1003 ++---------------- test/account/AccountPreValidationHooks.t.sol | 247 ++--- 4 files changed, 209 insertions(+), 1073 deletions(-) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 50584ee19..7e6ed7ddd 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -394,7 +394,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _resolveManifestFunction( mh.associatedFunction, plugin, - dependencies, + new FunctionReference[](0), ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -409,7 +409,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _resolveManifestFunction( mh.associatedFunction, plugin, - dependencies, + new FunctionReference[](0), ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -422,10 +422,13 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _addExecHooks( mh.executionSelector, _resolveManifestFunction( - mh.preExecHook, plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY + mh.preExecHook, + plugin, + new FunctionReference[](0), + ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ), _resolveManifestFunction( - mh.postExecHook, plugin, dependencies, ManifestAssociatedFunctionType.NONE + mh.postExecHook, plugin, new FunctionReference[](0), ManifestAssociatedFunctionType.NONE ) ); } @@ -502,10 +505,13 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _removeExecHooks( mh.executionSelector, _resolveManifestFunction( - mh.preExecHook, args.plugin, dependencies, ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY + mh.preExecHook, + args.plugin, + new FunctionReference[](0), + ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ), _resolveManifestFunction( - mh.postExecHook, args.plugin, dependencies, ManifestAssociatedFunctionType.NONE + mh.postExecHook, args.plugin, new FunctionReference[](0), ManifestAssociatedFunctionType.NONE ) ); } @@ -520,7 +526,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _resolveManifestFunction( mh.associatedFunction, args.plugin, - dependencies, + new FunctionReference[](0), ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -536,7 +542,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { _resolveManifestFunction( mh.associatedFunction, args.plugin, - dependencies, + new FunctionReference[](0), ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY ) ); @@ -629,6 +635,7 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { function _resolveManifestFunction( ManifestFunction memory manifestFunction, address plugin, + // Can be empty to indicate that type DEPENDENCY is invalid for this function. FunctionReference[] memory dependencies, // Indicates which magic value, if any, is permissible for the function to resolve. ManifestAssociatedFunctionType allowedMagicValue @@ -636,7 +643,12 @@ abstract contract PluginManagerInternals is IPluginManager, AccountStorageV1 { if (manifestFunction.functionType == ManifestAssociatedFunctionType.SELF) { return FunctionReferenceLib.pack(plugin, manifestFunction.functionId); } else if (manifestFunction.functionType == ManifestAssociatedFunctionType.DEPENDENCY) { - return dependencies[manifestFunction.dependencyIndex]; + uint256 index = manifestFunction.dependencyIndex; + if (index < dependencies.length) { + 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 62b3831b2..1d6e292d7 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -56,7 +56,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 c7c694c16..d95336a38 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"; @@ -48,8 +49,6 @@ contract UpgradeableModularAccountExecHooksTest is Test { event PluginInstalled(address indexed plugin, bytes32 manifestHash, FunctionReference[] dependencies); event PluginUninstalled(address indexed plugin, bool indexed callbacksSucceeded); - // emitted by MockPlugin - event ReceivedCall(bytes msgData, uint256 msgValue); function setUp() public { entryPoint = IEntryPoint(address(new EntryPoint())); @@ -112,8 +111,8 @@ contract UpgradeableModularAccountExecHooksTest is Test { vm.startPrank(owner1); - vm.expectEmit(true, true, true, true); - emit ReceivedCall( + vm.expectCall( + address(mockPlugin1), abi.encodeWithSelector( IPlugin.preExecutionHook.selector, _PRE_HOOK_FUNCTION_ID_1, @@ -121,7 +120,7 @@ contract UpgradeableModularAccountExecHooksTest is Test { 0, // msg.value in call to account abi.encodeWithSelector(_EXEC_SELECTOR) ), - 0 // msg value in call to plugin + 1 ); (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); @@ -190,147 +189,92 @@ contract UpgradeableModularAccountExecHooksTest is Test { vm.stopPrank(); } - /// @dev Plugin 1 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_execHookPair_install() public { + function test_overlappingPreExecHookAlwaysDeny_install() public { vm.startPrank(owner1); _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.NONE, functionId: 0, dependencyIndex: 0}) + ); + + _installPlugin2WithHooks( + _EXEC_SELECTOR, ManifestFunction({ - functionType: ManifestAssociatedFunctionType.SELF, - functionId: _POST_HOOK_FUNCTION_ID_2, + functionType: ManifestAssociatedFunctionType.PRE_HOOK_ALWAYS_DENY, + functionId: 0, dependencyIndex: 0 - }) + }), + ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, dependencyIndex: 0}), + new FunctionReference[](0) ); vm.stopPrank(); } - /// @dev Plugin 1 hook pair: [1, 2] - /// Expected execution: [1, 2] - function test_execHookPair_run() public { - test_execHookPair_install(); + function test_overlappingPreExecHookAlwaysDeny_revert() public { + test_overlappingPreExecHookAlwaysDeny_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); + (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 pair: [1, 2] - /// Expected execution: [1, 2] - function test_execHookPair_uninstall() public { - test_execHookPair_install(); + function test_overlappingPreExecHookAlwaysDeny_uninstallPlugin1() public { + test_overlappingPreExecHookAlwaysDeny_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 - ); - + // Execution selector should no longer exist. (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); + assertFalse(success); vm.stopPrank(); } - /// @dev Plugin 1 hook pair: [null, 2] - /// Expected execution: [null, 2] - function test_postOnlyExecHook_uninstall() public { - test_postOnlyExecHook_install(); + function test_overlappingPreExecHookAlwaysDeny_uninstallPlugin2() public { + test_overlappingPreExecHookAlwaysDeny_install(); vm.startPrank(owner1); - _uninstallPlugin(mockPlugin1); + _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(); } - /// @dev Plugin 1 hook pair: [1, null] - /// Plugin 2 hook pair: [1, null] - /// Expected execution: [1, null] - function test_overlappingPreExecHooks_install() public { + /// Plugins cannot depend on hooks from other plugins. + function test_overlappingPreExecHook_invalid() 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.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( + dependencies[0] = FunctionReferenceLib.pack(address(mockPlugin1), 1); + + vm.expectRevert(abi.encodeWithSelector(PluginManagerInternals.InvalidPluginManifest.selector)); + this.installPlugin2WithHooksNoSuccessCheck( _EXEC_SELECTOR, ManifestFunction({ functionType: ManifestAssociatedFunctionType.DEPENDENCY, @@ -340,80 +284,13 @@ contract UpgradeableModularAccountExecHooksTest is Test { 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 { + function test_execHookPair_install() public { vm.startPrank(owner1); - // Install the first plugin. _installPlugin1WithHooks( _EXEC_SELECTOR, ManifestFunction({ @@ -428,37 +305,16 @@ contract UpgradeableModularAccountExecHooksTest is Test { }) ); - // 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(); + function test_execHookPair_run() public { + test_execHookPair_install(); vm.startPrank(owner1); - // Expect the pre hook to be called just once. vm.expectCall( address(mockPlugin1), abi.encodeWithSelector( @@ -470,14 +326,11 @@ contract UpgradeableModularAccountExecHooksTest is Test { ), 1 ); - - // Expect the post hook to be called just once, with the expected data. + vm.expectCall(address(mockPlugin1), abi.encodePacked(_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 + abi.encodeCall( + IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, abi.encode(_PRE_HOOK_FUNCTION_ID_1)) ), 1 ); @@ -489,64 +342,25 @@ contract UpgradeableModularAccountExecHooksTest is Test { } /// @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(); + 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: [1, 2] - /// Plugin 2 hook pair: [3, 2] - /// Expected execution: [1, 2], [3, 2] - function test_overlappingExecHookPairsOnPost_install() public { + /// @dev Plugin 1 hook pair: [null, 2] + /// Expected execution: [null, 2] + function test_postOnlyExecHook_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}), ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, functionId: _POST_HOOK_FUNCTION_ID_2, @@ -554,689 +368,34 @@ 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.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(); + /// @dev Plugin 1 hook pair: [null, 2] + /// Expected execution: [null, 2] + function test_postOnlyExecHook_run() public { + test_postOnlyExecHook_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 + address(mockPlugin1), abi.encodeCall(IPlugin.postExecutionHook, (_POST_HOOK_FUNCTION_ID_2, "")), 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], [null, 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) - ), - 1 - ); - - // 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 { - 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.NONE, functionId: 0, dependencyIndex: 0}), - dependencies - ); - - 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(); - - 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, null] - /// Expected execution: [1, 2] - function test_overlappingExecHookPairsOnPreWithNullPost_uninstall() public { - test_overlappingExecHookPairsOnPreWithNullPost_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: [null, 2] - /// Expected execution: [1, 2], [null, 2] - function test_overlappingExecHookPairsOnPostWithNullPre_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.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(); - - 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 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 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @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(); - - 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); + (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + assertTrue(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 { - vm.startPrank(owner1); - - // Install the first plugin. - _installPlugin1WithHooks( - _EXEC_SELECTOR, - ManifestFunction({functionType: ManifestAssociatedFunctionType.NONE, functionId: 0, 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.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(); - - 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 - ); - - (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); - - vm.stopPrank(); - } - - /// @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(); } @@ -1249,8 +408,7 @@ contract UpgradeableModularAccountExecHooksTest is Test { 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.expectCall(address(mockPlugin1), abi.encodeCall(IPlugin.onInstall, (bytes(""))), 1); vm.expectEmit(true, true, true, true); emit PluginInstalled(address(mockPlugin1), manifestHash1, new FunctionReference[](0)); @@ -1270,6 +428,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); } @@ -1282,10 +461,11 @@ 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); + if (expectSuccess) { + vm.expectCall(address(mockPlugin2), abi.encodeCall(IPlugin.onInstall, (bytes(""))), 1); + vm.expectEmit(true, true, true, true); + emit PluginInstalled(address(mockPlugin2), manifestHash2, dependencies); + } account1.installPlugin({ plugin: address(mockPlugin2), @@ -1293,11 +473,12 @@ contract UpgradeableModularAccountExecHooksTest is Test { pluginInitData: bytes(""), dependencies: dependencies }); + + vm.stopPrank(); } function _uninstallPlugin(MockPlugin plugin) internal { - vm.expectEmit(true, true, true, true); - emit ReceivedCall(abi.encodeCall(IPlugin.onUninstall, (bytes(""))), 0); + vm.expectCall(address(plugin), abi.encodeCall(IPlugin.onUninstall, (bytes(""))), 1); vm.expectEmit(true, true, true, true); emit PluginUninstalled(address(plugin), true); diff --git a/test/account/AccountPreValidationHooks.t.sol b/test/account/AccountPreValidationHooks.t.sol index 8836c00b9..23d72b67f 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"; @@ -242,91 +243,100 @@ 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); - - // 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 - ); + _uninstallPlugin(mockPlugin1); + // Execution selector should no longer exist. (bool success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); - assertTrue(success); + assertFalse(success); - // Uninstall the first plugin. - _uninstallPlugin(mockPlugin1); + vm.stopPrank(); + } - // Execution selector should no longer exist. - (success,) = address(account1).call(abi.encodeWithSelector(_EXEC_SELECTOR)); + function test_overlappingPreRuntimeValidationHookAlwaysDeny_uninstallPlugin2() public { + test_overlappingPreRuntimeValidationHookAlwaysDeny_install(); + + 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(); } @@ -488,118 +498,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(); } @@ -631,6 +551,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); } @@ -639,9 +578,11 @@ 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); + if (expectSuccess) { + vm.expectCall(address(mockPlugin2), abi.encodeCall(IPlugin.onInstall, ("")), 1); + vm.expectEmit(true, true, true, true); + emit PluginInstalled(address(mockPlugin2), manifestHash2, dependencies); + } account1.installPlugin({ plugin: address(mockPlugin2), @@ -649,6 +590,8 @@ contract UpgradeableModularAccountPreValidationHooksTest is Test { pluginInitData: bytes(""), dependencies: dependencies }); + + vm.stopPrank(); } function _installPlugin1WithPreUserOpValidationHook(bytes4 selector, ManifestFunction memory hook) internal {