From 77421625ed64d6c603cf442a1b9df6e8467edcaa Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Sun, 19 Nov 2023 17:48:27 -0500 Subject: [PATCH] test: native token spend permissions, unrecognized selector (#2) --- .../ExecuteFromPluginPermissions.t.sol | 75 ++++++++++++++- .../ExecFromPluginPermissionsMocks.sol | 92 +++++++++++++++---- 2 files changed, 149 insertions(+), 18 deletions(-) diff --git a/test/account/ExecuteFromPluginPermissions.t.sol b/test/account/ExecuteFromPluginPermissions.t.sol index a79347ef..e6664754 100644 --- a/test/account/ExecuteFromPluginPermissions.t.sol +++ b/test/account/ExecuteFromPluginPermissions.t.sol @@ -19,8 +19,9 @@ import {ResultCreatorPlugin} from "../mocks/plugins/ReturnDataPluginMocks.sol"; import { EFPCallerPlugin, EFPCallerPluginAnyExternal, - EFPPermittedCallHookPlugin, - EFPExternalPermittedCallHookPlugin + EFPCallerPluginAnyExternalCanSpendNativeToken, + EFPExternalPermittedCallHookPlugin, + EFPPermittedCallHookPlugin } from "../mocks/plugins/ExecFromPluginPermissionsMocks.sol"; contract ExecuteFromPluginPermissionsTest is Test { @@ -36,6 +37,7 @@ contract ExecuteFromPluginPermissionsTest is Test { EFPCallerPlugin public efpCallerPlugin; EFPCallerPluginAnyExternal public efpCallerPluginAnyExternal; + EFPCallerPluginAnyExternalCanSpendNativeToken public efpCallerPluginAnyExternalCanSpendNativeToken; EFPPermittedCallHookPlugin public efpPermittedCallHookPlugin; EFPExternalPermittedCallHookPlugin public efpExternalPermittedCallHookPlugin; @@ -62,6 +64,7 @@ contract ExecuteFromPluginPermissionsTest is Test { // Initialize the EFP caller plugins, which will attempt to use the permissions system to authorize calls. efpCallerPlugin = new EFPCallerPlugin(); efpCallerPluginAnyExternal = new EFPCallerPluginAnyExternal(); + efpCallerPluginAnyExternalCanSpendNativeToken = new EFPCallerPluginAnyExternalCanSpendNativeToken(); efpPermittedCallHookPlugin = new EFPPermittedCallHookPlugin(); efpExternalPermittedCallHookPlugin = new EFPExternalPermittedCallHookPlugin(); @@ -101,6 +104,17 @@ contract ExecuteFromPluginPermissionsTest is Test { injectedHooks: new IPluginManager.InjectedHook[](0) }); + // Add the EFP caller plugin with any external permissions and native token spend permission to the account + bytes32 efpCallerAnyExternalCanSpendNativeTokenManifestHash = + keccak256(abi.encode(efpCallerPluginAnyExternalCanSpendNativeToken.pluginManifest())); + account.installPlugin({ + plugin: address(efpCallerPluginAnyExternalCanSpendNativeToken), + manifestHash: efpCallerAnyExternalCanSpendNativeTokenManifestHash, + pluginInitData: "", + dependencies: new FunctionReference[](0), + injectedHooks: new IPluginManager.InjectedHook[](0) + }); + // Add the EFP caller plugin with permitted call hooks to the account bytes32 efpPermittedCallHookManifestHash = keccak256(abi.encode(efpPermittedCallHookPlugin.pluginManifest())); @@ -152,6 +166,24 @@ contract ExecuteFromPluginPermissionsTest is Test { EFPCallerPlugin(address(account)).useEFPPermissionNotAllowed(); } + function test_executeFromPluginUnrecognizedFunction() public { + // Permitted but uninstalled selector + vm.expectRevert( + abi.encodeWithSelector( + UpgradeableModularAccount.UnrecognizedFunction.selector, bytes4(keccak256("baz()")) + ) + ); + EFPCallerPlugin(address(account)).passthroughExecuteFromPlugin( + abi.encodeWithSelector(bytes4(keccak256("baz()"))) + ); + + // Invalid selector < 4 bytes + vm.expectRevert( + abi.encodeWithSelector(UpgradeableModularAccount.UnrecognizedFunction.selector, bytes4(hex"11")) + ); + EFPCallerPlugin(address(account)).passthroughExecuteFromPlugin(hex"11"); + } + function test_executeFromPluginExternal_Allowed_IndividualSelectors() public { EFPCallerPlugin(address(account)).setNumberCounter1(17); uint256 retrievedNumber = EFPCallerPlugin(address(account)).getNumberCounter1(); @@ -268,6 +300,45 @@ contract ExecuteFromPluginPermissionsTest is Test { assertEq(retrievedNumber, 18); } + function test_executeFromPluginExternal_NotAllowed_NativeTokenSpending() public { + vm.expectRevert( + abi.encodeWithSelector( + UpgradeableModularAccount.NativeTokenSpendingNotPermitted.selector, + address(efpCallerPluginAnyExternal) + ) + ); + EFPCallerPluginAnyExternal(address(account)).passthroughExecute(address(counter1), 1 ether, ""); + + address recipient = makeAddr("recipient"); + vm.deal(address(efpCallerPluginAnyExternal), 1 ether); + // This function forwards 1 eth from the plugin to the account and tries to send 2 eth to the recipient. + // This is not allowed because there would be a net decrease of the balance on the account. + vm.expectRevert( + abi.encodeWithSelector( + UpgradeableModularAccount.NativeTokenSpendingNotPermitted.selector, + address(efpCallerPluginAnyExternal) + ) + ); + EFPCallerPluginAnyExternal(address(account)).passthroughExecuteWith1Eth(address(recipient), 2 ether, ""); + } + + function test_executeFromPluginExternal_Allowed_NativeTokenSpending() public { + address recipient = makeAddr("recipient"); + + vm.deal(address(efpCallerPluginAnyExternal), 1 ether); + assertEq(address(recipient).balance, 0); + // This function forwards 1 eth from the plugin to the account and sends 1 eth to the recipient. This is + // allowed because there is no net change to the balance on the account. + EFPCallerPluginAnyExternal(address(account)).passthroughExecuteWith1Eth(address(recipient), 1 ether, ""); + assertEq(address(efpCallerPluginAnyExternal).balance, 0); + assertEq(address(recipient).balance, 1 ether); + + vm.deal(address(account), 1 ether); + EFPCallerPluginAnyExternalCanSpendNativeToken(address(account)) + .passthroughExecuteWithNativeTokenSpendPermission(address(recipient), 1 ether, ""); + assertEq(address(recipient).balance, 2 ether); + } + function test_executeFromPlugin_PermittedCallHooks() public { assertFalse(efpPermittedCallHookPlugin.preExecHookCalled()); assertFalse(efpPermittedCallHookPlugin.postExecHookCalled()); diff --git a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol index b634fa1a..dd717c34 100644 --- a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol +++ b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol @@ -32,20 +32,21 @@ contract EFPCallerPlugin is BaseTestPlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new bytes4[](11); + manifest.executionFunctions = new bytes4[](12); manifest.executionFunctions[0] = this.useEFPPermissionAllowed.selector; manifest.executionFunctions[1] = this.useEFPPermissionNotAllowed.selector; - manifest.executionFunctions[2] = this.setNumberCounter1.selector; - manifest.executionFunctions[3] = this.getNumberCounter1.selector; - manifest.executionFunctions[4] = this.incrementCounter1.selector; - manifest.executionFunctions[5] = this.setNumberCounter2.selector; - manifest.executionFunctions[6] = this.getNumberCounter2.selector; - manifest.executionFunctions[7] = this.incrementCounter2.selector; - manifest.executionFunctions[8] = this.setNumberCounter3.selector; - manifest.executionFunctions[9] = this.getNumberCounter3.selector; - manifest.executionFunctions[10] = this.incrementCounter3.selector; - - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](11); + manifest.executionFunctions[2] = this.passthroughExecuteFromPlugin.selector; + manifest.executionFunctions[3] = this.setNumberCounter1.selector; + manifest.executionFunctions[4] = this.getNumberCounter1.selector; + manifest.executionFunctions[5] = this.incrementCounter1.selector; + manifest.executionFunctions[6] = this.setNumberCounter2.selector; + manifest.executionFunctions[7] = this.getNumberCounter2.selector; + manifest.executionFunctions[8] = this.incrementCounter2.selector; + manifest.executionFunctions[9] = this.setNumberCounter3.selector; + manifest.executionFunctions[10] = this.getNumberCounter3.selector; + manifest.executionFunctions[11] = this.incrementCounter3.selector; + + manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](12); ManifestFunction memory alwaysAllowValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, @@ -60,9 +61,11 @@ contract EFPCallerPlugin is BaseTestPlugin { }); } - // Request permission only for "foo", but not "bar", from ResultCreatorPlugin - manifest.permittedExecutionSelectors = new bytes4[](1); + // Request permission for "foo" and the non-existent selector "baz", but not "bar", from + // ResultCreatorPlugin + manifest.permittedExecutionSelectors = new bytes4[](2); manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; + manifest.permittedExecutionSelectors[1] = bytes4(keccak256("baz()")); // Request permission for: // - `setNumber` and `number` on counter 1 @@ -100,6 +103,10 @@ contract EFPCallerPlugin is BaseTestPlugin { return IPluginExecutor(msg.sender).executeFromPlugin(abi.encodeCall(ResultCreatorPlugin.bar, ())); } + function passthroughExecuteFromPlugin(bytes calldata data) external returns (bytes memory) { + return IPluginExecutor(msg.sender).executeFromPlugin(data); + } + // Should be allowed function setNumberCounter1(uint256 number) external { IPluginExecutor(msg.sender).executeFromPluginExternal( @@ -178,10 +185,11 @@ contract EFPCallerPluginAnyExternal is BaseTestPlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions = new bytes4[](2); manifest.executionFunctions[0] = this.passthroughExecute.selector; + manifest.executionFunctions[1] = this.passthroughExecuteWith1Eth.selector; - manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); + manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](2); manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ executionSelector: this.passthroughExecute.selector, associatedFunction: ManifestFunction({ @@ -190,6 +198,14 @@ contract EFPCallerPluginAnyExternal is BaseTestPlugin { dependencyIndex: 0 }) }); + manifest.runtimeValidationFunctions[1] = ManifestAssociatedFunction({ + executionSelector: this.passthroughExecuteWith1Eth.selector, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, + functionId: 0, + dependencyIndex: 0 + }) + }); manifest.permitAnyExternalAddress = true; @@ -203,6 +219,50 @@ contract EFPCallerPluginAnyExternal is BaseTestPlugin { { return IPluginExecutor(msg.sender).executeFromPluginExternal(target, value, data); } + + function passthroughExecuteWith1Eth(address target, uint256 value, bytes calldata data) + external + payable + returns (bytes memory) + { + return IPluginExecutor(msg.sender).executeFromPluginExternal{value: 1 ether}(target, value, data); + } +} + +contract EFPCallerPluginAnyExternalCanSpendNativeToken is BaseTestPlugin { + function onInstall(bytes calldata) external override {} + + function onUninstall(bytes calldata) external override {} + + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + + manifest.executionFunctions = new bytes4[](1); + manifest.executionFunctions[0] = this.passthroughExecuteWithNativeTokenSpendPermission.selector; + + manifest.runtimeValidationFunctions = new ManifestAssociatedFunction[](1); + manifest.runtimeValidationFunctions[0] = ManifestAssociatedFunction({ + executionSelector: this.passthroughExecuteWithNativeTokenSpendPermission.selector, + associatedFunction: ManifestFunction({ + functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, + functionId: 0, + dependencyIndex: 0 + }) + }); + + manifest.canSpendNativeToken = true; + manifest.permitAnyExternalAddress = true; + + return manifest; + } + + function passthroughExecuteWithNativeTokenSpendPermission(address target, uint256 value, bytes calldata data) + external + payable + returns (bytes memory) + { + return IPluginExecutor(msg.sender).executeFromPluginExternal(target, value, data); + } } // Create pre and post permitted call hooks for calling ResultCreatorPlugin.foo via `executeFromPlugin`