From 7d9b2f1b12fc3f19af6b84d055e8cc5b3b7bd18b Mon Sep 17 00:00:00 2001 From: adam Date: Fri, 17 May 2024 15:44:00 -0400 Subject: [PATCH] Remove non-plugin restriction on calls, remove exec functions from SingleOwnerPlugin --- src/account/AccountExecutor.sol | 13 --------- src/plugins/owner/SingleOwnerPlugin.sol | 28 ++++---------------- test/account/AccountLoupe.t.sol | 10 +++---- test/account/UpgradeableModularAccount.t.sol | 13 ++++++--- test/utils/AccountTestBase.sol | 4 ++- 5 files changed, 20 insertions(+), 48 deletions(-) diff --git a/src/account/AccountExecutor.sol b/src/account/AccountExecutor.sol index 136676f6..4ed2c57a 100644 --- a/src/account/AccountExecutor.sol +++ b/src/account/AccountExecutor.sol @@ -1,25 +1,12 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.25; -import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165Checker.sol"; -import {IPlugin} from "../interfaces/IPlugin.sol"; - abstract contract AccountExecutor { - error PluginExecutionDenied(address plugin); - - /// @dev If the target is a plugin (as determined by its support for the IPlugin interface), revert. - /// This prevents the modular account from calling plugins (both installed and uninstalled) outside - /// of the normal flow (via execution functions installed on the account), which could lead to data - /// inconsistencies and unexpected behavior. /// @param target The address of the contract to call. /// @param value The value to send with the call. /// @param data The call data. /// @return result The return data of the call, or the error message from the call if call reverts. function _exec(address target, uint256 value, bytes memory data) internal returns (bytes memory result) { - if (ERC165Checker.supportsInterface(target, type(IPlugin).interfaceId)) { - revert PluginExecutionDenied(target); - } - bool success; (success, result) = target.call{value: value}(data); diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 825b0230..bebbad21 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -152,51 +152,33 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - manifest.executionFunctions = new bytes4[](2); - manifest.executionFunctions[0] = this.transferOwnership.selector; - manifest.executionFunctions[1] = this.owner.selector; - ManifestFunction memory ownerValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, functionId: uint8(FunctionId.VALIDATION_OWNER_OR_SELF), dependencyIndex: 0 // Unused. }); - manifest.validationFunctions = new ManifestAssociatedFunction[](7); + manifest.validationFunctions = new ManifestAssociatedFunction[](5); manifest.validationFunctions[0] = ManifestAssociatedFunction({ - executionSelector: this.transferOwnership.selector, - associatedFunction: ownerValidationFunction - }); - manifest.validationFunctions[1] = ManifestAssociatedFunction({ executionSelector: IStandardExecutor.execute.selector, associatedFunction: ownerValidationFunction }); - manifest.validationFunctions[2] = ManifestAssociatedFunction({ + manifest.validationFunctions[1] = ManifestAssociatedFunction({ executionSelector: IStandardExecutor.executeBatch.selector, associatedFunction: ownerValidationFunction }); - manifest.validationFunctions[3] = ManifestAssociatedFunction({ + manifest.validationFunctions[2] = ManifestAssociatedFunction({ executionSelector: IPluginManager.installPlugin.selector, associatedFunction: ownerValidationFunction }); - manifest.validationFunctions[4] = ManifestAssociatedFunction({ + manifest.validationFunctions[3] = ManifestAssociatedFunction({ executionSelector: IPluginManager.uninstallPlugin.selector, associatedFunction: ownerValidationFunction }); - manifest.validationFunctions[5] = ManifestAssociatedFunction({ + manifest.validationFunctions[4] = ManifestAssociatedFunction({ executionSelector: UUPSUpgradeable.upgradeToAndCall.selector, associatedFunction: ownerValidationFunction }); - ManifestFunction memory alwaysAllowRuntime = ManifestFunction({ - functionType: ManifestAssociatedFunctionType.RUNTIME_VALIDATION_ALWAYS_ALLOW, - functionId: 0, // Unused. - dependencyIndex: 0 // Unused. - }); - manifest.validationFunctions[6] = ManifestAssociatedFunction({ - executionSelector: this.owner.selector, - associatedFunction: alwaysAllowRuntime - }); - manifest.signatureValidationFunctions = new uint8[](1); manifest.signatureValidationFunctions[0] = uint8(FunctionId.SIG_VALIDATION); diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index 9fbc1e21..34c914a7 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -73,9 +73,9 @@ contract AccountLoupeTest is AccountTestBase { } function test_pluginLoupe_getExecutionFunctionConfig_plugin() public { - bytes4[] memory selectorsToCheck = new bytes4[](2); - address[] memory expectedPluginAddress = new address[](2); - FunctionReference[] memory expectedValidations = new FunctionReference[](2); + bytes4[] memory selectorsToCheck = new bytes4[](1); + address[] memory expectedPluginAddress = new address[](1); + FunctionReference[] memory expectedValidations = new FunctionReference[](1); selectorsToCheck[0] = comprehensivePlugin.foo.selector; expectedPluginAddress[0] = address(comprehensivePlugin); @@ -83,10 +83,6 @@ contract AccountLoupeTest is AccountTestBase { address(comprehensivePlugin), uint8(ComprehensivePlugin.FunctionId.VALIDATION) ); - selectorsToCheck[1] = singleOwnerPlugin.transferOwnership.selector; - expectedPluginAddress[1] = address(singleOwnerPlugin); - expectedValidations[1] = ownerValidation; - for (uint256 i = 0; i < selectorsToCheck.length; i++) { IAccountLoupe.ExecutionFunctionConfig memory config = account1.getExecutionFunctionConfig(selectorsToCheck[i]); diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 363d1654..c5a061e0 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -96,7 +96,10 @@ contract UpgradeableModularAccountTest is AccountTestBase { sender: address(account2), nonce: 0, initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner2, 0))), - callData: abi.encodeCall(SingleOwnerPlugin.transferOwnership, (owner2)), + callData: abi.encodeCall( + UpgradeableModularAccount.execute, + (address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (owner2))) + ), accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), preVerificationGas: 0, gasFees: _encodeGas(1, 2), @@ -422,12 +425,14 @@ contract UpgradeableModularAccountTest is AccountTestBase { } function test_transferOwnership() public { - assertEq(SingleOwnerPlugin(address(account1)).owner(), owner1); + assertEq(singleOwnerPlugin.ownerOf(address(account1)), owner1); vm.prank(owner1); - SingleOwnerPlugin(address(account1)).transferOwnership(owner2); + account1.execute( + address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (owner2)) + ); - assertEq(SingleOwnerPlugin(address(account1)).owner(), owner2); + assertEq(singleOwnerPlugin.ownerOf(address(account1)), owner2); } function test_isValidSignature() public { diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 5d9880f9..d8c890fe 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -37,7 +37,9 @@ abstract contract AccountTestBase is OptimizedTest { function _transferOwnershipToTest() internal { // Transfer ownership to test contract for easier invocation. vm.prank(owner1); - SingleOwnerPlugin(address(account1)).transferOwnership(address(this)); + account1.execute( + address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (address(this))) + ); } // helper function to compress 2 gas values into a single bytes32