From 8a1fe739e94d04da81532f53de827c6180dbdc4a Mon Sep 17 00:00:00 2001 From: adam Date: Thu, 18 Jul 2024 13:02:55 -0400 Subject: [PATCH] Only allow installing direct call validation via manifest --- src/account/PluginManagerInternals.sol | 43 +++++++++----------- src/account/UpgradeableModularAccount.sol | 8 ++-- src/helpers/Constants.sol | 8 ++++ src/interfaces/IPlugin.sol | 11 +---- test/account/AccountLoupe.t.sol | 16 +++++--- test/account/SelfCallAuthorization.t.sol | 12 ++++-- test/account/ValidationIntersection.t.sol | 39 +++++++++--------- test/mocks/plugins/ComprehensivePlugin.sol | 12 ------ test/mocks/plugins/ReturnDataPluginMocks.sol | 25 ++++-------- test/mocks/plugins/ValidationPluginMocks.sol | 40 +----------------- 10 files changed, 80 insertions(+), 134 deletions(-) create mode 100644 src/helpers/Constants.sol diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 9e4dd01c..1b8aac96 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -5,11 +5,12 @@ import {ERC165Checker} from "@openzeppelin/contracts/utils/introspection/ERC165C import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; +import {RESERVED_VALIDATION_DATA_INDEX, SELF_PERMIT_VALIDATION_FUNCTIONID} from "../helpers/Constants.sol"; import {KnownSelectors} from "../helpers/KnownSelectors.sol"; import {PluginEntityLib} from "../helpers/PluginEntityLib.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; import {ExecutionHook} from "../interfaces/IAccountLoupe.sol"; -import {IPlugin, ManifestExecutionHook, ManifestValidation, PluginManifest} from "../interfaces/IPlugin.sol"; +import {IPlugin, ManifestExecutionHook, PluginManifest} from "../interfaces/IPlugin.sol"; import {IPluginManager, PluginEntity, ValidationConfig} from "../interfaces/IPluginManager.sol"; import {AccountStorage, SelectorData, ValidationData, getAccountStorage, toSetValue} from "./AccountStorage.sol"; @@ -18,12 +19,6 @@ abstract contract PluginManagerInternals is IPluginManager { using PluginEntityLib for PluginEntity; using ValidationConfigLib for ValidationConfig; - // Index marking the start of the data for the validation function. - uint8 internal constant _RESERVED_VALIDATION_DATA_INDEX = 255; - - // Magic value for the Entity ID of direct call validation. - uint32 internal constant _SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint32).max; - error ArrayLengthMismatch(); error Erc4337FunctionNotAllowed(bytes4 selector); error ExecutionFunctionAlreadySet(bytes4 selector); @@ -163,16 +158,19 @@ abstract contract PluginManagerInternals is IPluginManager { _setExecutionFunction(selector, isPublic, allowGlobalValidation, plugin); } - length = manifest.validationFunctions.length; - for (uint256 i = 0; i < length; ++i) { - // Todo: limit this to only "direct runtime call" validation path (old EFP), - // and add a way for the user to specify permission/pre-val hooks here. - ManifestValidation memory mv = manifest.validationFunctions[i]; + // Install direct call validation, if any, from the manifest - ValidationConfig validationConfig = - ValidationConfigLib.pack(plugin, mv.entityId, mv.isGlobal, mv.isSignatureValidation); - _addValidationFunction(validationConfig, mv.selectors); - } + // Todo: add a way for the user to specify permission/pre-val hooks here. + + ValidationConfig directCallValidation = ValidationConfigLib.pack({ + _plugin: plugin, + _entityId: SELF_PERMIT_VALIDATION_FUNCTIONID, + _isGlobal: manifest.globalDirectCallValidation, + // Direct call validation is never a signature validation + _isSignatureValidation: false + }); + + _addValidationFunction(directCallValidation, manifest.directCallValidationSelectors); length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { @@ -212,11 +210,10 @@ abstract contract PluginManagerInternals is IPluginManager { _removeExecHooks(execHooks, hookFunction, mh.isPreHook, mh.isPostHook); } - length = manifest.validationFunctions.length; - for (uint256 i = 0; i < length; ++i) { - PluginEntity validationFunction = - PluginEntityLib.pack(plugin, manifest.validationFunctions[i].entityId); - _removeValidationFunction(validationFunction); + if (manifest.globalDirectCallValidation || manifest.directCallValidationSelectors.length > 0) { + PluginEntity directCallValidation = PluginEntityLib.pack(plugin, SELF_PERMIT_VALIDATION_FUNCTIONID); + + _removeValidationFunction(directCallValidation); } length = manifest.executionFunctions.length; @@ -267,7 +264,7 @@ abstract contract PluginManagerInternals is IPluginManager { } // Avoid collision between reserved index and actual indices - if (_validationData.preValidationHooks.length > _RESERVED_VALIDATION_DATA_INDEX) { + if (_validationData.preValidationHooks.length > RESERVED_VALIDATION_DATA_INDEX) { revert PreValidationHookLimitExceeded(); } } @@ -297,7 +294,7 @@ abstract contract PluginManagerInternals is IPluginManager { } } - if (validationConfig.entityId() != _SELF_PERMIT_VALIDATION_FUNCTIONID) { + if (validationConfig.entityId() != SELF_PERMIT_VALIDATION_FUNCTIONID) { // Only allow global validations and signature validations if they're not direct-call validations. _validationData.isGlobal = validationConfig.isGlobal(); diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index c4641be2..b15718c7 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -18,6 +18,7 @@ import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol" import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; import {_coalescePreValidation, _coalesceValidation} from "../helpers/ValidationResHelpers.sol"; +import {RESERVED_VALIDATION_DATA_INDEX, SELF_PERMIT_VALIDATION_FUNCTIONID} from "../helpers/Constants.sol"; import {IExecutionHook} from "../interfaces/IExecutionHook.sol"; import {PluginManifest} from "../interfaces/IPlugin.sol"; import {IPluginManager, PluginEntity, ValidationConfig} from "../interfaces/IPluginManager.sol"; @@ -28,7 +29,6 @@ import {AccountExecutor} from "./AccountExecutor.sol"; import {AccountLoupe} from "./AccountLoupe.sol"; import {AccountStorage, getAccountStorage, toExecutionHook, toSetValue} from "./AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; - import {PluginManagerInternals} from "./PluginManagerInternals.sol"; contract UpgradeableModularAccount is @@ -441,7 +441,7 @@ contract UpgradeableModularAccount is // Run the user op validationFunction { - if (signatureSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) { + if (signatureSegment.getIndex() != RESERVED_VALIDATION_DATA_INDEX) { revert ValidationSignatureSegmentMissing(); } @@ -497,7 +497,7 @@ contract UpgradeableModularAccount is _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], callData, currentAuthData); } - if (authSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) { + if (authSegment.getIndex() != RESERVED_VALIDATION_DATA_INDEX) { revert ValidationSignatureSegmentMissing(); } @@ -635,7 +635,7 @@ contract UpgradeableModularAccount is return (new PostExecToRun[](0), new PostExecToRun[](0)); } - PluginEntity directCallValidationKey = PluginEntityLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID); + PluginEntity directCallValidationKey = PluginEntityLib.pack(msg.sender, SELF_PERMIT_VALIDATION_FUNCTIONID); _checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false); diff --git a/src/helpers/Constants.sol b/src/helpers/Constants.sol new file mode 100644 index 00000000..6a9f5209 --- /dev/null +++ b/src/helpers/Constants.sol @@ -0,0 +1,8 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.25; + +// Index marking the start of the data for the validation function. +uint8 constant RESERVED_VALIDATION_DATA_INDEX = 255; + +// Magic value for the Entity ID of direct call validation. +uint32 constant SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint32).max; diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 0476e1e1..62d253e7 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -13,14 +13,6 @@ struct ManifestExecutionFunction { bool allowGlobalValidation; } -// todo: do we need these at all? Or do we fully switch to `installValidation`? -struct ManifestValidation { - uint32 entityId; - bool isGlobal; - bool isSignatureValidation; - bytes4[] selectors; -} - struct ManifestExecutionHook { // TODO(erc6900 spec): These fields can be packed into a single word bytes4 executionSelector; @@ -54,7 +46,8 @@ struct PluginMetadata { struct PluginManifest { // Execution functions defined in this plugin to be installed on the MSCA. ManifestExecutionFunction[] executionFunctions; - ManifestValidation[] validationFunctions; + bool globalDirectCallValidation; + bytes4[] directCallValidationSelectors; ManifestExecutionHook[] executionHooks; // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include // IPlugin's interface ID. diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index a03be802..9f837bd8 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -16,8 +16,12 @@ contract AccountLoupeTest is CustomValidationTestBase { event ReceivedCall(bytes msgData, uint256 msgValue); + PluginEntity public comprehensivePluginValidation; + function setUp() public { comprehensivePlugin = new ComprehensivePlugin(); + comprehensivePluginValidation = + PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION)); _customValidationSetup(); @@ -61,9 +65,6 @@ contract AccountLoupeTest is CustomValidationTestBase { } function test_pluginLoupe_getSelectors() public { - PluginEntity comprehensivePluginValidation = - PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION)); - bytes4[] memory selectors = account1.getSelectors(comprehensivePluginValidation); assertEq(selectors.length, 1); @@ -107,7 +108,7 @@ contract AccountLoupeTest is CustomValidationTestBase { } function test_pluginLoupe_getValidationHooks() public { - PluginEntity[] memory hooks = account1.getPreValidationHooks(_signerValidation); + PluginEntity[] memory hooks = account1.getPreValidationHooks(comprehensivePluginValidation); assertEq(hooks.length, 2); assertEq( @@ -144,12 +145,15 @@ contract AccountLoupeTest is CustomValidationTestBase { address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.PRE_VALIDATION_HOOK_2) ); + bytes4[] memory selectors = new bytes4[](1); + selectors[0] = comprehensivePlugin.foo.selector; + bytes[] memory installDatas = new bytes[](2); return ( - _signerValidation, + comprehensivePluginValidation, true, true, - new bytes4[](0), + selectors, bytes(""), abi.encode(preValidationHooks, installDatas), "" diff --git a/test/account/SelfCallAuthorization.t.sol b/test/account/SelfCallAuthorization.t.sol index 42032251..b966b734 100644 --- a/test/account/SelfCallAuthorization.t.sol +++ b/test/account/SelfCallAuthorization.t.sol @@ -24,12 +24,18 @@ contract SelfCallAuthorizationTest is AccountTestBase { comprehensivePlugin = new ComprehensivePlugin(); + comprehensivePluginValidation = + PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION)); + + bytes4[] memory validationSelectors = new bytes4[](1); + validationSelectors[0] = ComprehensivePlugin.foo.selector; + vm.startPrank(address(entryPoint)); account1.installPlugin(address(comprehensivePlugin), comprehensivePlugin.pluginManifest(), ""); + account1.installValidation( + ValidationConfigLib.pack(comprehensivePluginValidation, false, false), validationSelectors, "", "", "" + ); vm.stopPrank(); - - comprehensivePluginValidation = - PluginEntityLib.pack(address(comprehensivePlugin), uint32(ComprehensivePlugin.EntityId.VALIDATION)); } function test_selfCallFails_userOp() public { diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index 07dc6e0e..eed74dc0 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -46,19 +46,21 @@ contract ValidationIntersectionTest is AccountTestBase { entityId: uint32(MockBaseUserOpValidationPlugin.EntityId.USER_OP_VALIDATION) }); + bytes4[] memory validationSelectors = new bytes4[](1); + validationSelectors[0] = MockUserOpValidationPlugin.foo.selector; + vm.startPrank(address(entryPoint)); - account1.installPlugin({ - plugin: address(noHookPlugin), - manifest: noHookPlugin.pluginManifest(), - pluginInstallData: "" - }); - account1.installPlugin({ - plugin: address(oneHookPlugin), - manifest: oneHookPlugin.pluginManifest(), - pluginInstallData: "" - }); - // TODO: change with new install flow - // temporary fix to add the pre-validation hook + // Install noHookValidation + account1.installValidation( + ValidationConfigLib.pack(noHookValidation, true, true), + validationSelectors, + bytes(""), + bytes(""), + bytes("") + ); + + // Install oneHookValidation + validationSelectors[0] = MockUserOpValidation1HookPlugin.bar.selector; PluginEntity[] memory preValidationHooks = new PluginEntity[](1); preValidationHooks[0] = PluginEntityLib.pack({ addr: address(oneHookPlugin), @@ -67,17 +69,14 @@ contract ValidationIntersectionTest is AccountTestBase { bytes[] memory installDatas = new bytes[](1); account1.installValidation( ValidationConfigLib.pack(oneHookValidation, true, true), - new bytes4[](0), + validationSelectors, bytes(""), abi.encode(preValidationHooks, installDatas), bytes("") ); - account1.installPlugin({ - plugin: address(twoHookPlugin), - manifest: twoHookPlugin.pluginManifest(), - pluginInstallData: "" - }); - // temporary fix to add the pre-validation hook + + // Install twoHookValidation + validationSelectors[0] = MockUserOpValidation2HookPlugin.baz.selector; preValidationHooks = new PluginEntity[](2); preValidationHooks[0] = PluginEntityLib.pack({ addr: address(twoHookPlugin), @@ -90,7 +89,7 @@ contract ValidationIntersectionTest is AccountTestBase { installDatas = new bytes[](2); account1.installValidation( ValidationConfigLib.pack(twoHookValidation, true, true), - new bytes4[](0), + validationSelectors, bytes(""), abi.encode(preValidationHooks, installDatas), bytes("") diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 326bbf8c..f2689279 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -7,7 +7,6 @@ import {IExecutionHook} from "../../../src/interfaces/IExecutionHook.sol"; import { ManifestExecutionFunction, ManifestExecutionHook, - ManifestValidation, PluginManifest, PluginMetadata } from "../../../src/interfaces/IPlugin.sol"; @@ -140,17 +139,6 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba allowGlobalValidation: false }); - bytes4[] memory validationSelectors = new bytes4[](1); - validationSelectors[0] = this.foo.selector; - - manifest.validationFunctions = new ManifestValidation[](1); - manifest.validationFunctions[0] = ManifestValidation({ - entityId: uint32(EntityId.VALIDATION), - isGlobal: true, - isSignatureValidation: false, - selectors: validationSelectors - }); - manifest.executionHooks = new ManifestExecutionHook[](3); manifest.executionHooks[0] = ManifestExecutionHook({ executionSelector: this.foo.selector, diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index bcb2ce31..43e0dfa4 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -3,12 +3,9 @@ pragma solidity ^0.8.19; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import { - ManifestExecutionFunction, - ManifestValidation, - PluginManifest, - PluginMetadata -} from "../../../src/interfaces/IPlugin.sol"; +import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; + +import {SELF_PERMIT_VALIDATION_FUNCTIONID} from "../../../src/helpers/Constants.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {IValidation} from "../../../src/interfaces/IValidation.sol"; @@ -103,7 +100,8 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { // This result should be allowed based on the manifest permission request bytes memory returnData = IStandardExecutor(msg.sender).executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (target, 0, abi.encodeCall(RegularResultContract.foo, ()))), - abi.encodePacked(this, uint32(0), uint8(0), uint32(1), uint8(255)) // Validation function of self, + abi.encodePacked(this, SELF_PERMIT_VALIDATION_FUNCTIONID, uint8(0), uint32(1), uint8(255)) // Validation + // function of self, // selector-associated, with no auth data ); @@ -119,17 +117,8 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { function pluginManifest() external pure override returns (PluginManifest memory) { PluginManifest memory manifest; - // todo: this is the exact workflow that would benefit from a "permiteed call" setup in the manifest. - bytes4[] memory validationSelectors = new bytes4[](1); - validationSelectors[0] = IStandardExecutor.execute.selector; - - manifest.validationFunctions = new ManifestValidation[](1); - manifest.validationFunctions[0] = ManifestValidation({ - entityId: 0, - isGlobal: true, - isSignatureValidation: false, - selectors: validationSelectors - }); + manifest.directCallValidationSelectors = new bytes4[](1); + manifest.directCallValidationSelectors[0] = IStandardExecutor.execute.selector; manifest.executionFunctions = new ManifestExecutionFunction[](2); manifest.executionFunctions[0] = ManifestExecutionFunction({ diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 96775b9d..b3532581 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -3,12 +3,7 @@ pragma solidity ^0.8.19; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import { - ManifestExecutionFunction, - ManifestValidation, - PluginManifest, - PluginMetadata -} from "../../../src/interfaces/IPlugin.sol"; +import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; import {IValidation} from "../../../src/interfaces/IValidation.sol"; import {IValidationHook} from "../../../src/interfaces/IValidationHook.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; @@ -112,17 +107,6 @@ contract MockUserOpValidationPlugin is MockBaseUserOpValidationPlugin { allowGlobalValidation: false }); - bytes4[] memory validationSelectors = new bytes4[](1); - validationSelectors[0] = this.foo.selector; - - manifest.validationFunctions = new ManifestValidation[](1); - manifest.validationFunctions[0] = ManifestValidation({ - entityId: uint32(EntityId.USER_OP_VALIDATION), - isGlobal: false, - isSignatureValidation: false, - selectors: validationSelectors - }); - return manifest; } } @@ -155,17 +139,6 @@ contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { allowGlobalValidation: false }); - bytes4[] memory validationSelectors = new bytes4[](1); - validationSelectors[0] = this.bar.selector; - - manifest.validationFunctions = new ManifestValidation[](2); - manifest.validationFunctions[0] = ManifestValidation({ - entityId: uint32(EntityId.USER_OP_VALIDATION), - isGlobal: false, - isSignatureValidation: false, - selectors: validationSelectors - }); - return manifest; } } @@ -201,17 +174,6 @@ contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { allowGlobalValidation: false }); - bytes4[] memory validationSelectors = new bytes4[](1); - validationSelectors[0] = this.baz.selector; - - manifest.validationFunctions = new ManifestValidation[](1); - manifest.validationFunctions[0] = ManifestValidation({ - entityId: uint32(EntityId.USER_OP_VALIDATION), - isGlobal: false, - isSignatureValidation: false, - selectors: validationSelectors - }); - return manifest; } }