From fe5af61741df5ff3741704710590b13c4cedfc25 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 10 Jul 2024 20:22:33 +0800 Subject: [PATCH 01/24] feat: initial impl of simple plugin direct calls with validation hooks --- src/account/AccountStorage.sol | 6 +++++ src/account/UpgradeableModularAccount.sol | 31 ++++++++++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 78c06259..1c3d426a 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -40,6 +40,11 @@ struct ValidationData { EnumerableSet.Bytes32Set selectors; } +struct DirectCallValidationData { + bool allowed; // Whether or not this direct call is allowed. + FunctionReference[] preValidationHooks; // The set of pre validation hooks for this direct call. +} + struct AccountStorage { // AccountStorageInitializable variables uint8 initialized; @@ -51,6 +56,7 @@ struct AccountStorage { mapping(FunctionReference validationFunction => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; + mapping(address caller => mapping(bytes4 selector => DirectCallValidationData)) directCallData; } function getAccountStorage() pure returns (AccountStorage storage _storage) { diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 42db6967..7c1180ed 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -9,6 +9,7 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; +import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol"; @@ -40,6 +41,7 @@ contract UpgradeableModularAccount is UUPSUpgradeable { using EnumerableSet for EnumerableSet.Bytes32Set; + using EnumerableSet for EnumerableSet.AddressSet; using FunctionReferenceLib for FunctionReference; using SparseCalldataSegmentLib for bytes; @@ -80,6 +82,7 @@ contract UpgradeableModularAccount is error ValidationDoesNotApply(bytes4 selector, address plugin, uint8 functionId, bool isGlobal); error ValidationSignatureSegmentMissing(); error SignatureSegmentOutOfOrder(); + error DirectCallDisallowed(); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin @@ -696,7 +699,7 @@ contract UpgradeableModularAccount is return getAccountStorage().selectorData[selector].allowGlobalValidation; } - function _checkPermittedCallerIfNotFromEP() internal view { + function _checkPermittedCallerIfNotFromEP() internal { AccountStorage storage _storage = getAccountStorage(); if ( @@ -705,5 +708,31 @@ contract UpgradeableModularAccount is ) { revert ExecFromPluginNotPermitted(msg.sender, msg.sig); } + + // If direct calling isn't allowed OR direct calling is allowed, but the plugin is no longer installed, + // revert. TBD if there's a better way to do this, e.g. deleting this storage or segmenting per + // installation ID. + if ( + !_storage.directCallData[msg.sender][msg.sig].allowed + || !_storage.plugins.contains(msg.sender) + ) { + revert DirectCallDisallowed(); + } + + FunctionReference[] storage hooks = _storage.directCallData[msg.sender][msg.sig].preValidationHooks; + + uint256 hookLen = hooks.length; + for (uint256 i = 0; i < hookLen; ++i) { + (address hookPlugin, uint8 hookFunctionId) = hooks[i].unpack(); + try IValidationHook(hookPlugin).preRuntimeValidationHook( + hookFunctionId, msg.sender, msg.value, msg.data, "" + ) + // forgefmt: disable-start + // solhint-disable-next-line no-empty-blocks + {} catch (bytes memory revertReason) { + // forgefmt: disable-end + revert PreRuntimeValidationHookFailed(hookPlugin, hookFunctionId, revertReason); + } + } } } From b11b7b43ac6c8a2c033860acec97fd46dd003b9e Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 10 Jul 2024 22:17:58 +0800 Subject: [PATCH 02/24] chore: remove unused import, formatting --- src/account/UpgradeableModularAccount.sol | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 7c1180ed..92017aac 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -9,7 +9,6 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol"; @@ -712,10 +711,7 @@ contract UpgradeableModularAccount is // If direct calling isn't allowed OR direct calling is allowed, but the plugin is no longer installed, // revert. TBD if there's a better way to do this, e.g. deleting this storage or segmenting per // installation ID. - if ( - !_storage.directCallData[msg.sender][msg.sig].allowed - || !_storage.plugins.contains(msg.sender) - ) { + if (!_storage.directCallData[msg.sender][msg.sig].allowed || !_storage.plugins.contains(msg.sender)) { revert DirectCallDisallowed(); } From 222243ede6143bbd8f5c01c491016182cfa7b334 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 11 Jul 2024 21:00:40 +0800 Subject: [PATCH 03/24] chore: slight refactor for codesize --- src/account/UpgradeableModularAccount.sol | 56 ++++++++++++----------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 92017aac..24f855a3 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -9,6 +9,7 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; +import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {SparseCalldataSegmentLib} from "../helpers/SparseCalldataSegmentLib.sol"; @@ -41,6 +42,7 @@ contract UpgradeableModularAccount is { using EnumerableSet for EnumerableSet.Bytes32Set; using EnumerableSet for EnumerableSet.AddressSet; + using EnumerableMap for EnumerableMap.AddressToUintMap; using FunctionReferenceLib for FunctionReference; using SparseCalldataSegmentLib for bytes; @@ -81,7 +83,6 @@ contract UpgradeableModularAccount is error ValidationDoesNotApply(bytes4 selector, address plugin, uint8 functionId, bool isGlobal); error ValidationSignatureSegmentMissing(); error SignatureSegmentOutOfOrder(); - error DirectCallDisallowed(); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin @@ -506,17 +507,7 @@ contract UpgradeableModularAccount is } else { currentAuthData = ""; } - - (address hookPlugin, uint8 hookFunctionId) = preRuntimeValidationHooks[i].unpack(); - try IValidationHook(hookPlugin).preRuntimeValidationHook( - hookFunctionId, msg.sender, msg.value, callData, currentAuthData - ) - // forgefmt: disable-start - // solhint-disable-next-line no-empty-blocks - {} catch (bytes memory revertReason) { - // forgefmt: disable-end - revert PreRuntimeValidationHookFailed(hookPlugin, hookFunctionId, revertReason); - } + _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], callData, currentAuthData); } if (authSegment.getIndex() != _RESERVED_VALIDATION_DATA_INDEX) { @@ -609,6 +600,23 @@ contract UpgradeableModularAccount is } } + function _doPreRuntimeValidationHook( + FunctionReference validationHook, + bytes memory callData, + bytes memory currentAuthData + ) internal { + (address hookPlugin, uint8 hookFunctionId) = validationHook.unpack(); + try IValidationHook(hookPlugin).preRuntimeValidationHook( + hookFunctionId, msg.sender, msg.value, callData, currentAuthData + ) + // forgefmt: disable-start + // solhint-disable-next-line no-empty-blocks + {} catch (bytes memory revertReason) { + // forgefmt: disable-end + revert PreRuntimeValidationHookFailed(hookPlugin, hookFunctionId, revertReason); + } + } + // solhint-disable-next-line no-empty-blocks function _authorizeUpgrade(address newImplementation) internal override {} @@ -702,33 +710,27 @@ contract UpgradeableModularAccount is AccountStorage storage _storage = getAccountStorage(); if ( - msg.sender != address(_ENTRY_POINT) && msg.sender != address(this) - && !_storage.selectorData[msg.sig].isPublic + msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) + || _storage.selectorData[msg.sig].isPublic ) { - revert ExecFromPluginNotPermitted(msg.sender, msg.sig); + return; } // If direct calling isn't allowed OR direct calling is allowed, but the plugin is no longer installed, // revert. TBD if there's a better way to do this, e.g. deleting this storage or segmenting per // installation ID. - if (!_storage.directCallData[msg.sender][msg.sig].allowed || !_storage.plugins.contains(msg.sender)) { - revert DirectCallDisallowed(); + if ( + !_storage.directCallData[msg.sender][msg.sig].allowed + || !_storage.pluginManifestHashes.contains(msg.sender) + ) { + revert ExecFromPluginNotPermitted(msg.sender, msg.sig); } FunctionReference[] storage hooks = _storage.directCallData[msg.sender][msg.sig].preValidationHooks; uint256 hookLen = hooks.length; for (uint256 i = 0; i < hookLen; ++i) { - (address hookPlugin, uint8 hookFunctionId) = hooks[i].unpack(); - try IValidationHook(hookPlugin).preRuntimeValidationHook( - hookFunctionId, msg.sender, msg.value, msg.data, "" - ) - // forgefmt: disable-start - // solhint-disable-next-line no-empty-blocks - {} catch (bytes memory revertReason) { - // forgefmt: disable-end - revert PreRuntimeValidationHookFailed(hookPlugin, hookFunctionId, revertReason); - } + _doPreRuntimeValidationHook(hooks[i], msg.data, ""); } } } From e1adaca42cc93b12ada363ad1ec1acaa13435abb Mon Sep 17 00:00:00 2001 From: zer0dot Date: Thu, 11 Jul 2024 21:53:28 +0800 Subject: [PATCH 04/24] feat: add direct call selectors to plugin manifest and installation --- src/account/PluginManagerInternals.sol | 6 ++++++ src/interfaces/IPlugin.sol | 1 + 2 files changed, 7 insertions(+) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 4df90e1f..5fe5733a 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -192,6 +192,12 @@ abstract contract PluginManagerInternals is IPluginManager { _storage.supportedIfaces[manifest.interfaceIds[i]] += 1; } + length = manifest.DirectCallSelectors.length; + for (uint256 i = 0; i < length; ++i) { + _storage.directCallData[plugin][manifest.DirectCallSelectors[i]].allowed = true; + } + //TODO: Associate hooks with the direct call, but these should most likely be user-provided + // Initialize the plugin storage for the account. // solhint-disable-next-line no-empty-blocks try IPlugin(plugin).onInstall(pluginInstallData) {} diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index eb10e96b..9747a29d 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -59,6 +59,7 @@ struct PluginManifest { // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include // IPlugin's interface ID. bytes4[] interfaceIds; + bytes4[] DirectCallSelectors; } interface IPlugin is IERC165 { From 1d45b2e9993d11af60ed2e884ed2a110fe453222 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 12 Jul 2024 00:37:03 +0800 Subject: [PATCH 05/24] feat: test basic direct plugin call functionality --- test/account/DirectCallsFromPlugin.t.sol | 79 ++++++++++++++++++++++++ test/mocks/plugins/DirectCallPlugin.sol | 33 ++++++++++ 2 files changed, 112 insertions(+) create mode 100644 test/account/DirectCallsFromPlugin.t.sol create mode 100644 test/mocks/plugins/DirectCallPlugin.sol diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol new file mode 100644 index 00000000..445eca24 --- /dev/null +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -0,0 +1,79 @@ +pragma solidity ^0.8.19; + +import {DirectCallPlugin} from "../mocks/plugins/DirectCallPlugin.sol"; +import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; +import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; +import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; + +import {AccountTestBase} from "../utils/AccountTestBase.sol"; + +contract DirectCallsFromPluginTest is AccountTestBase { + DirectCallPlugin plugin; + + function setUp() public { + plugin = new DirectCallPlugin(); + } + + function test_Fail_DirectCallPluginNotInstalled() external { + vm.prank(address(plugin)); + vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); + account1.execute(address(0), 0, ""); + } + + function test_Fail_DirectCallPluginUninstalled() external { + _installPlugin(); + + vm.prank(address(entryPoint)); + account1.uninstallPlugin(address(plugin), "", ""); + + vm.prank(address(plugin)); + vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); + account1.execute(address(0), 0, ""); + } + + function test_Fail_DirectCallPluginCallOtherSelector() external { + _installPlugin(); + + Call[] memory calls = new Call[](0); + + vm.prank(address(plugin)); + vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.executeBatch.selector)); + account1.executeBatch(calls); + } + + function test_Pass_DirectCallFromPlugin_MockFlow() external { + _installPlugin(); + + vm.prank(address(plugin)); + account1.execute(address(0), 0, ""); + } + + function test_Pass_DirectCallFromPlugin_NormalFlow() external { + _installPlugin(); + + bytes memory encodedCall = abi.encodeCall(DirectCallPlugin.directCall, ()); + + vm.prank(address(entryPoint)); + bytes memory result = account1.execute(address(plugin), 0, encodedCall); + + assertEq(abi.decode(result, (bytes)), abi.encode(plugin.getData())); + } + + /* -------------------------------------------------------------------------- */ + /* Internals */ + /* -------------------------------------------------------------------------- */ + + function _installPlugin() internal { + bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); + + vm.prank(address(entryPoint)); + account1.installPlugin(address(plugin), manifestHash, ""); + } + + function _buildDirectCallDisallowedError(bytes4 selector) internal view returns (bytes memory) { + return abi.encodeWithSelector( + UpgradeableModularAccount.ExecFromPluginNotPermitted.selector, address(plugin), selector + ); + } +} diff --git a/test/mocks/plugins/DirectCallPlugin.sol b/test/mocks/plugins/DirectCallPlugin.sol new file mode 100644 index 00000000..31356452 --- /dev/null +++ b/test/mocks/plugins/DirectCallPlugin.sol @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; +import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; + +import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; +import {ResultCreatorPlugin} from "./ReturnDataPluginMocks.sol"; + +contract DirectCallPlugin is BasePlugin { + function onInstall(bytes calldata) external override {} + + function onUninstall(bytes calldata) external override {} + + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + + manifest.DirectCallSelectors = new bytes4[](1); + manifest.DirectCallSelectors[0] = IStandardExecutor.execute.selector; + + return manifest; + } + + function directCall() external returns (bytes memory) { + return IStandardExecutor(msg.sender).execute(address(this), 0, abi.encodeCall(this.getData, ())); + } + + function getData() external pure returns (bytes memory) { + return hex"04546b"; + } + + function pluginMetadata() external pure override returns (PluginMetadata memory) {} +} From 883eff5c06809965a15bebc6721e3f8dcc324d5d Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 12 Jul 2024 00:37:48 +0800 Subject: [PATCH 06/24] test: extra scenario test, minor renaming --- test/account/DirectCallsFromPlugin.t.sol | 22 ++++++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index 445eca24..758dd500 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -42,14 +42,14 @@ contract DirectCallsFromPluginTest is AccountTestBase { account1.executeBatch(calls); } - function test_Pass_DirectCallFromPlugin_MockFlow() external { + function test_Pass_DirectCallFromPluginPrank() external { _installPlugin(); vm.prank(address(plugin)); account1.execute(address(0), 0, ""); } - function test_Pass_DirectCallFromPlugin_NormalFlow() external { + function test_Pass_DirectCallFromPluginCallback() external { _installPlugin(); bytes memory encodedCall = abi.encodeCall(DirectCallPlugin.directCall, ()); @@ -57,9 +57,27 @@ contract DirectCallsFromPluginTest is AccountTestBase { vm.prank(address(entryPoint)); bytes memory result = account1.execute(address(plugin), 0, encodedCall); + // the directCall() function in the plugin calls back into `execute()` with an encoded call back into the + // plugin's getData() function. assertEq(abi.decode(result, (bytes)), abi.encode(plugin.getData())); } + function test_Flow_DirectCallFromPluginSequence() external { + // Install => Succeesfully call => uninstall => fail to call + + _installPlugin(); + + vm.prank(address(plugin)); + account1.execute(address(0), 0, ""); + + vm.prank(address(entryPoint)); + account1.uninstallPlugin(address(plugin), "", ""); + + vm.prank(address(plugin)); + vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); + account1.execute(address(0), 0, ""); + } + /* -------------------------------------------------------------------------- */ /* Internals */ /* -------------------------------------------------------------------------- */ From 691dd3102671b06026ef1637d4c9c0e9482f3ead Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 12 Jul 2024 23:23:18 +0800 Subject: [PATCH 07/24] refactor: migrate direct call installation to installValidation --- src/account/AccountStorage.sol | 3 +- src/account/PluginManager2.sol | 25 ++++++++----- src/account/PluginManagerInternals.sol | 6 ---- src/account/UpgradeableModularAccount.sol | 43 +++++++++++++++++------ src/interfaces/IPlugin.sol | 1 - test/account/DirectCallsFromPlugin.t.sol | 26 ++++++++++---- test/mocks/plugins/DirectCallPlugin.sol | 9 +---- 7 files changed, 71 insertions(+), 42 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 1c3d426a..06160c4b 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -32,7 +32,7 @@ struct ValidationData { bool isSignatureValidation; // How many execution hooks require the UO context. uint8 requireUOHookCount; - // The pre validation hooks for this function selector. + // The pre validation hooks for this validation function. FunctionReference[] preValidationHooks; // Permission hooks for this validation function. EnumerableSet.Bytes32Set permissionHooks; @@ -56,7 +56,6 @@ struct AccountStorage { mapping(FunctionReference validationFunction => ValidationData) validationData; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; - mapping(address caller => mapping(bytes4 selector => DirectCallValidationData)) directCallData; } function getAccountStorage() pure returns (AccountStorage storage _storage) { diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 2c9c4d6a..1b4379e6 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -15,6 +15,7 @@ abstract contract PluginManager2 { // Index marking the start of the data for the validation function. uint8 internal constant _RESERVED_VALIDATION_DATA_INDEX = 255; + uint8 internal constant _SELF_PERMIT_VALIDATION_FUNCTIONID = type(uint8).max; error GlobalValidationAlreadySet(FunctionReference validationFunction); error PreValidationAlreadySet(FunctionReference validationFunction, FunctionReference preValidationFunction); @@ -80,12 +81,9 @@ abstract contract PluginManager2 { } } - if (isGlobal) { - if (_storage.validationData[validationFunction].isGlobal) { - revert GlobalValidationAlreadySet(validationFunction); - } - _storage.validationData[validationFunction].isGlobal = true; - } + (address plugin, uint8 functionId) = FunctionReferenceLib.unpack(validationFunction); + // If the functionId indicates a self-permit for direct runtime calls from plugins, we don't need to + // install a function as the functionReference will consist of the msg.sender + constant_functionId for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; @@ -94,9 +92,18 @@ abstract contract PluginManager2 { } } - if (installData.length > 0) { - (address plugin,) = FunctionReferenceLib.unpack(validationFunction); - IPlugin(plugin).onInstall(installData); + if (functionId != _SELF_PERMIT_VALIDATION_FUNCTIONID) { + // Only allow global validations if they're not direct-calls. + if (isGlobal) { + if (_storage.validationData[validationFunction].isGlobal) { + revert GlobalValidationAlreadySet(validationFunction); + } + _storage.validationData[validationFunction].isGlobal = true; + } + + if (installData.length > 0) { + IPlugin(plugin).onInstall(installData); + } } } diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 5fe5733a..4df90e1f 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -192,12 +192,6 @@ abstract contract PluginManagerInternals is IPluginManager { _storage.supportedIfaces[manifest.interfaceIds[i]] += 1; } - length = manifest.DirectCallSelectors.length; - for (uint256 i = 0; i < length; ++i) { - _storage.directCallData[plugin][manifest.DirectCallSelectors[i]].allowed = true; - } - //TODO: Associate hooks with the direct call, but these should most likely be user-provided - // Initialize the plugin storage for the account. // solhint-disable-next-line no-empty-blocks try IPlugin(plugin).onInstall(pluginInstallData) {} diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 24f855a3..ebaf3cc3 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -87,7 +87,7 @@ contract UpgradeableModularAccount is // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin modifier wrapNativeFunction() { - _checkPermittedCallerIfNotFromEP(); + PostExecToRun[] memory postPermissionHooks = _checkPermittedCallerIfNotFromEP(); PostExecToRun[] memory postExecHooks = _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); @@ -95,6 +95,7 @@ contract UpgradeableModularAccount is _; _doCachedPostExecHooks(postExecHooks); + _doCachedPostExecHooks(postPermissionHooks); } constructor(IEntryPoint anEntryPoint) { @@ -706,31 +707,53 @@ contract UpgradeableModularAccount is return getAccountStorage().selectorData[selector].allowGlobalValidation; } - function _checkPermittedCallerIfNotFromEP() internal { + function _checkPermittedCallerIfNotFromEP() internal returns (PostExecToRun[] memory) { AccountStorage storage _storage = getAccountStorage(); if ( msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) || _storage.selectorData[msg.sig].isPublic ) { - return; + return new PostExecToRun[](0); } // If direct calling isn't allowed OR direct calling is allowed, but the plugin is no longer installed, // revert. TBD if there's a better way to do this, e.g. deleting this storage or segmenting per // installation ID. - if ( - !_storage.directCallData[msg.sender][msg.sig].allowed - || !_storage.pluginManifestHashes.contains(msg.sender) - ) { + // if ( + // !_storage.directCallData[msg.sender][msg.sig].allowed + // || !_storage.pluginManifestHashes.contains(msg.sender) + // ) { + // revert ExecFromPluginNotPermitted(msg.sender, msg.sig); + // } + + // FunctionReference[] storage hooks = _storage.directCallData[msg.sender][msg.sig].preValidationHooks; + + // uint256 hookLen = hooks.length; + // for (uint256 i = 0; i < hookLen; ++i) { + // _doPreRuntimeValidationHook(hooks[i], msg.data, ""); + // } + FunctionReference directCallValidationKey = + FunctionReferenceLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID); + + if (!_storage.validationData[directCallValidationKey].selectors.contains(toSetValue(msg.sig))) { revert ExecFromPluginNotPermitted(msg.sender, msg.sig); } - FunctionReference[] storage hooks = _storage.directCallData[msg.sender][msg.sig].preValidationHooks; + // Direct call is allowed, run associated permission & validation hooks + //TODO: handle uninstallation - uint256 hookLen = hooks.length; + // Validation hooks + FunctionReference[] memory preRuntimeValidationHooks = + _storage.validationData[directCallValidationKey].preValidationHooks; + + uint256 hookLen = preRuntimeValidationHooks.length; for (uint256 i = 0; i < hookLen; ++i) { - _doPreRuntimeValidationHook(hooks[i], msg.data, ""); + _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], msg.data, ""); } + + PostExecToRun[] memory postPermissionHooks = + _doPreHooks(_storage.validationData[directCallValidationKey].permissionHooks, msg.data); + return postPermissionHooks; } } diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index 9747a29d..eb10e96b 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -59,7 +59,6 @@ struct PluginManifest { // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include // IPlugin's interface ID. bytes4[] interfaceIds; - bytes4[] DirectCallSelectors; } interface IPlugin is IERC165 { diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index 758dd500..e7c35802 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -4,15 +4,18 @@ import {DirectCallPlugin} from "../mocks/plugins/DirectCallPlugin.sol"; import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; +import {FunctionReferenceLib, FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract DirectCallsFromPluginTest is AccountTestBase { DirectCallPlugin plugin; + FunctionReference pluginFunctionReference; function setUp() public { plugin = new DirectCallPlugin(); + pluginFunctionReference = FunctionReferenceLib.pack(address(plugin), type(uint8).max); } function test_Fail_DirectCallPluginNotInstalled() external { @@ -24,8 +27,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { function test_Fail_DirectCallPluginUninstalled() external { _installPlugin(); - vm.prank(address(entryPoint)); - account1.uninstallPlugin(address(plugin), "", ""); + _uninstallPlugin(); vm.prank(address(plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); @@ -70,8 +72,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { vm.prank(address(plugin)); account1.execute(address(0), 0, ""); - vm.prank(address(entryPoint)); - account1.uninstallPlugin(address(plugin), "", ""); + _uninstallPlugin(); vm.prank(address(plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); @@ -83,10 +84,23 @@ contract DirectCallsFromPluginTest is AccountTestBase { /* -------------------------------------------------------------------------- */ function _installPlugin() internal { - bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); + // bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); + + // vm.prank(address(entryPoint)); + // account1.installPlugin(address(plugin), manifestHash, ""); + + bytes4[] memory selectors = new bytes4[](1); + selectors[0] = IStandardExecutor.execute.selector; vm.prank(address(entryPoint)); - account1.installPlugin(address(plugin), manifestHash, ""); + account1.installValidation(pluginFunctionReference, false, selectors, "", "", ""); + } + + function _uninstallPlugin() internal { + vm.prank(address(entryPoint)); + account1.uninstallValidation( + pluginFunctionReference, "", abi.encode(new bytes[](0)), abi.encode(new bytes[](0)) + ); } function _buildDirectCallDisallowedError(bytes4 selector) internal view returns (bytes memory) { diff --git a/test/mocks/plugins/DirectCallPlugin.sol b/test/mocks/plugins/DirectCallPlugin.sol index 31356452..965fc52a 100644 --- a/test/mocks/plugins/DirectCallPlugin.sol +++ b/test/mocks/plugins/DirectCallPlugin.sol @@ -12,14 +12,7 @@ contract DirectCallPlugin is BasePlugin { function onUninstall(bytes calldata) external override {} - function pluginManifest() external pure override returns (PluginManifest memory) { - PluginManifest memory manifest; - - manifest.DirectCallSelectors = new bytes4[](1); - manifest.DirectCallSelectors[0] = IStandardExecutor.execute.selector; - - return manifest; - } + function pluginManifest() external pure override returns (PluginManifest memory) {} function directCall() external returns (bytes memory) { return IStandardExecutor(msg.sender).execute(address(this), 0, abi.encodeCall(this.getData, ())); From 4a990588d51e85498c64aa222ebfb148e7176a56 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Fri, 12 Jul 2024 23:57:54 +0800 Subject: [PATCH 08/24] chore: cleanup comments --- src/account/UpgradeableModularAccount.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index ebaf3cc3..8ce9f583 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -741,7 +741,6 @@ contract UpgradeableModularAccount is } // Direct call is allowed, run associated permission & validation hooks - //TODO: handle uninstallation // Validation hooks FunctionReference[] memory preRuntimeValidationHooks = @@ -752,6 +751,7 @@ contract UpgradeableModularAccount is _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], msg.data, ""); } + // Permission hooks PostExecToRun[] memory postPermissionHooks = _doPreHooks(_storage.validationData[directCallValidationKey].permissionHooks, msg.data); return postPermissionHooks; From efc111f77c27bf64e8f015bd8562bcc64ec58736 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Sat, 13 Jul 2024 19:23:08 +0800 Subject: [PATCH 09/24] chore: slight cleanup and renaming --- src/account/UpgradeableModularAccount.sol | 22 +++------------------- 1 file changed, 3 insertions(+), 19 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 8ce9f583..7b0932f0 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -87,7 +87,7 @@ contract UpgradeableModularAccount is // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin modifier wrapNativeFunction() { - PostExecToRun[] memory postPermissionHooks = _checkPermittedCallerIfNotFromEP(); + PostExecToRun[] memory postPermissionHooks = _checkPermittedCallerAndAssociatedHooks(); PostExecToRun[] memory postExecHooks = _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); @@ -138,7 +138,7 @@ contract UpgradeableModularAccount is revert UnrecognizedFunction(msg.sig); } - _checkPermittedCallerIfNotFromEP(); + _checkPermittedCallerAndAssociatedHooks(); PostExecToRun[] memory postExecHooks; // Cache post-exec hooks in memory @@ -707,7 +707,7 @@ contract UpgradeableModularAccount is return getAccountStorage().selectorData[selector].allowGlobalValidation; } - function _checkPermittedCallerIfNotFromEP() internal returns (PostExecToRun[] memory) { + function _checkPermittedCallerAndAssociatedHooks() internal returns (PostExecToRun[] memory) { AccountStorage storage _storage = getAccountStorage(); if ( @@ -717,22 +717,6 @@ contract UpgradeableModularAccount is return new PostExecToRun[](0); } - // If direct calling isn't allowed OR direct calling is allowed, but the plugin is no longer installed, - // revert. TBD if there's a better way to do this, e.g. deleting this storage or segmenting per - // installation ID. - // if ( - // !_storage.directCallData[msg.sender][msg.sig].allowed - // || !_storage.pluginManifestHashes.contains(msg.sender) - // ) { - // revert ExecFromPluginNotPermitted(msg.sender, msg.sig); - // } - - // FunctionReference[] storage hooks = _storage.directCallData[msg.sender][msg.sig].preValidationHooks; - - // uint256 hookLen = hooks.length; - // for (uint256 i = 0; i < hookLen; ++i) { - // _doPreRuntimeValidationHook(hooks[i], msg.data, ""); - // } FunctionReference directCallValidationKey = FunctionReferenceLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID); From 2a58118e857e1af2847b644f3600196dd52ff100 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Mon, 15 Jul 2024 22:44:51 +0800 Subject: [PATCH 10/24] test: add permission hooks to direct plugin call tests --- test/account/DirectCallsFromPlugin.t.sol | 40 +++++++++++++++++++----- test/mocks/plugins/DirectCallPlugin.sol | 24 +++++++++++++- 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index e7c35802..c659baaa 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -1,6 +1,7 @@ pragma solidity ^0.8.19; import {DirectCallPlugin} from "../mocks/plugins/DirectCallPlugin.sol"; +import {ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; @@ -15,9 +16,15 @@ contract DirectCallsFromPluginTest is AccountTestBase { function setUp() public { plugin = new DirectCallPlugin(); + assertFalse(plugin.preHookRan()); + assertFalse(plugin.postHookRan()); pluginFunctionReference = FunctionReferenceLib.pack(address(plugin), type(uint8).max); } + /* -------------------------------------------------------------------------- */ + /* Negatives */ + /* -------------------------------------------------------------------------- */ + function test_Fail_DirectCallPluginNotInstalled() external { vm.prank(address(plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); @@ -44,11 +51,18 @@ contract DirectCallsFromPluginTest is AccountTestBase { account1.executeBatch(calls); } + /* -------------------------------------------------------------------------- */ + /* Positives */ + /* -------------------------------------------------------------------------- */ + function test_Pass_DirectCallFromPluginPrank() external { _installPlugin(); vm.prank(address(plugin)); account1.execute(address(0), 0, ""); + + assertTrue(plugin.preHookRan()); + assertTrue(plugin.postHookRan()); } function test_Pass_DirectCallFromPluginCallback() external { @@ -59,6 +73,9 @@ contract DirectCallsFromPluginTest is AccountTestBase { vm.prank(address(entryPoint)); bytes memory result = account1.execute(address(plugin), 0, encodedCall); + assertTrue(plugin.preHookRan()); + assertTrue(plugin.postHookRan()); + // the directCall() function in the plugin calls back into `execute()` with an encoded call back into the // plugin's getData() function. assertEq(abi.decode(result, (bytes)), abi.encode(plugin.getData())); @@ -72,6 +89,9 @@ contract DirectCallsFromPluginTest is AccountTestBase { vm.prank(address(plugin)); account1.execute(address(0), 0, ""); + assertTrue(plugin.preHookRan()); + assertTrue(plugin.postHookRan()); + _uninstallPlugin(); vm.prank(address(plugin)); @@ -84,22 +104,28 @@ contract DirectCallsFromPluginTest is AccountTestBase { /* -------------------------------------------------------------------------- */ function _installPlugin() internal { - // bytes32 manifestHash = keccak256(abi.encode(plugin.pluginManifest())); - - // vm.prank(address(entryPoint)); - // account1.installPlugin(address(plugin), manifestHash, ""); - bytes4[] memory selectors = new bytes4[](1); selectors[0] = IStandardExecutor.execute.selector; + ExecutionHook[] memory permissionHooks = new ExecutionHook[](1); + bytes[] memory permissionHookInitDatas = new bytes[](1); + + permissionHooks[0] = ExecutionHook({ + hookFunction: FunctionReferenceLib.pack(address(plugin), 0xff), + isPreHook: true, + isPostHook: true + }); + + bytes memory encodedPermissionHooks = abi.encode(permissionHooks, permissionHookInitDatas); + vm.prank(address(entryPoint)); - account1.installValidation(pluginFunctionReference, false, selectors, "", "", ""); + account1.installValidation(pluginFunctionReference, false, selectors, "", "", encodedPermissionHooks); } function _uninstallPlugin() internal { vm.prank(address(entryPoint)); account1.uninstallValidation( - pluginFunctionReference, "", abi.encode(new bytes[](0)), abi.encode(new bytes[](0)) + pluginFunctionReference, "", abi.encode(new bytes[](0)), abi.encode(new bytes[](1)) ); } diff --git a/test/mocks/plugins/DirectCallPlugin.sol b/test/mocks/plugins/DirectCallPlugin.sol index 965fc52a..0bca4f9a 100644 --- a/test/mocks/plugins/DirectCallPlugin.sol +++ b/test/mocks/plugins/DirectCallPlugin.sol @@ -3,11 +3,15 @@ pragma solidity ^0.8.19; import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; +import {IExecutionHook} from "../../../src/interfaces/IExecutionHook.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; import {ResultCreatorPlugin} from "./ReturnDataPluginMocks.sol"; -contract DirectCallPlugin is BasePlugin { +contract DirectCallPlugin is BasePlugin, IExecutionHook { + bool public preHookRan = false; + bool public postHookRan = false; + function onInstall(bytes calldata) external override {} function onUninstall(bytes calldata) external override {} @@ -23,4 +27,22 @@ contract DirectCallPlugin is BasePlugin { } function pluginMetadata() external pure override returns (PluginMetadata memory) {} + + function preExecutionHook(uint8, address sender, uint256, bytes calldata) + external + override + returns (bytes memory) + { + require(sender == address(this), "mock direct call pre permission hook failed"); + preHookRan = true; + return abi.encode(keccak256(hex"04546b")); + } + + function postExecutionHook(uint8, bytes calldata preExecHookData) external override { + require( + abi.decode(preExecHookData, (bytes32)) == keccak256(hex"04546b"), + "mock direct call post permission hook failed" + ); + postHookRan = true; + } } From d614b0e984fbdf818257a648b93d789327902f8d Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 01:43:26 +0800 Subject: [PATCH 11/24] chore: update permission hook uninstallation to handle full execution hook structs --- src/account/PluginManager2.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 1b4379e6..25643887 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -142,9 +142,9 @@ abstract contract PluginManager2 { _storage.validationData[validationFunction].permissionHooks; uint256 i = 0; while (permissionHooks.length() > 0) { - FunctionReference permissionHook = toFunctionReference(permissionHooks.at(0)); - permissionHooks.remove(toSetValue(permissionHook)); - (address permissionHookPlugin,) = FunctionReferenceLib.unpack(permissionHook); + bytes32 permissionHook = permissionHooks.at(0); + permissionHooks.remove(permissionHook); + address permissionHookPlugin = address(uint160(bytes20(permissionHook))); IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i++]); } } From f0b8321df7fef54abf870086f504a09c2adb276f Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 20:22:31 +0800 Subject: [PATCH 12/24] refactor: refactor while to for loop for permission hook uninstallation --- src/account/PluginManager2.sol | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 25643887..f72f3225 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -140,12 +140,13 @@ abstract contract PluginManager2 { // Clear permission hooks EnumerableSet.Bytes32Set storage permissionHooks = _storage.validationData[validationFunction].permissionHooks; - uint256 i = 0; - while (permissionHooks.length() > 0) { + + uint256 len = permissionHooks.length(); + for (uint256 i = 0; i < len; ++i) { bytes32 permissionHook = permissionHooks.at(0); permissionHooks.remove(permissionHook); address permissionHookPlugin = address(uint160(bytes20(permissionHook))); - IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i++]); + IPlugin(permissionHookPlugin).onUninstall(permissionHookUninstallDatas[i]); } } delete _storage.validationData[validationFunction].preValidationHooks; From 43a3e5cf2b60b8c354adf5e6ecb77e00a3c19c49 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 20:31:58 +0800 Subject: [PATCH 13/24] chore: document direct-call flow --- src/account/UpgradeableModularAccount.sol | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 7b0932f0..5ff4d719 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -707,6 +707,19 @@ contract UpgradeableModularAccount is return getAccountStorage().selectorData[selector].allowGlobalValidation; } + /** + * Order of operations: + * 1. Check if the sender is the entry point, the account itself, or the selector called is public. + * - Yes: Return an empty array, there are no post-permissionHooks. + * - No: Continue + * 2. Check if the called selector (msg.sig) is included in the set of selectors the msg.sender can + * directly call. + * - Yes: Continue + * - No: Revert, the caller is not allowed to call this selector + * 3. If there are runtime validation hooks associated with this caller-sig combination, run them. + * 4. Run the pre-permissionHooks associated with this caller-sig combination, and return the + * post-permissionHooks to run later. + */ function _checkPermittedCallerAndAssociatedHooks() internal returns (PostExecToRun[] memory) { AccountStorage storage _storage = getAccountStorage(); From e615bc223cf0c0fca0f4d11345be2d557aaf57fa Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 20:42:50 +0800 Subject: [PATCH 14/24] refactor: consolidate pre-runtime-hooks into internal function --- src/account/UpgradeableModularAccount.sol | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 5ff4d719..b9e28be0 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -87,10 +87,8 @@ contract UpgradeableModularAccount is // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin modifier wrapNativeFunction() { - PostExecToRun[] memory postPermissionHooks = _checkPermittedCallerAndAssociatedHooks(); - - PostExecToRun[] memory postExecHooks = - _doPreHooks(getAccountStorage().selectorData[msg.sig].executionHooks, msg.data); + (PostExecToRun[] memory postPermissionHooks, PostExecToRun[] memory postExecHooks) = + _checkPermittedCallerAndAssociatedHooks(); _; @@ -720,14 +718,17 @@ contract UpgradeableModularAccount is * 4. Run the pre-permissionHooks associated with this caller-sig combination, and return the * post-permissionHooks to run later. */ - function _checkPermittedCallerAndAssociatedHooks() internal returns (PostExecToRun[] memory) { + function _checkPermittedCallerAndAssociatedHooks() + internal + returns (PostExecToRun[] memory, PostExecToRun[] memory) + { AccountStorage storage _storage = getAccountStorage(); if ( msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) || _storage.selectorData[msg.sig].isPublic ) { - return new PostExecToRun[](0); + return (new PostExecToRun[](0), new PostExecToRun[](0)); } FunctionReference directCallValidationKey = @@ -751,6 +752,11 @@ contract UpgradeableModularAccount is // Permission hooks PostExecToRun[] memory postPermissionHooks = _doPreHooks(_storage.validationData[directCallValidationKey].permissionHooks, msg.data); - return postPermissionHooks; + + // Exec hooks + PostExecToRun[] memory postExecutionHooks = + _doPreHooks(_storage.selectorData[msg.sig].executionHooks, msg.data); + + return (postPermissionHooks, postExecutionHooks); } } From b3b834f3204a239b1b5db3f6b416836cda6a9fa5 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 21:28:47 +0800 Subject: [PATCH 15/24] chore: remove unused import --- src/account/PluginManager2.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 00355ed8..510e1903 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -7,7 +7,7 @@ import {IPlugin} from "../interfaces/IPlugin.sol"; import {FunctionReference, ValidationConfig} from "../interfaces/IPluginManager.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; -import {ValidationData, getAccountStorage, toSetValue, toFunctionReference} from "./AccountStorage.sol"; +import {ValidationData, getAccountStorage, toSetValue} from "./AccountStorage.sol"; import {ExecutionHook} from "../interfaces/IAccountLoupe.sol"; // Temporary additional functions for a user-controlled install flow for validation functions. From 4aa008c46f57606809096ca98c3261dcc0a33b9c Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 21:33:00 +0800 Subject: [PATCH 16/24] chore: remove unused imports --- test/account/DirectCallsFromPlugin.t.sol | 6 ++---- test/mocks/plugins/DirectCallPlugin.sol | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index 6d95f2bd..10a8bf10 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -2,8 +2,6 @@ pragma solidity ^0.8.19; import {DirectCallPlugin} from "../mocks/plugins/DirectCallPlugin.sol"; import {ExecutionHook} from "../../src/interfaces/IAccountLoupe.sol"; -import {IPlugin, PluginManifest} from "../../src/interfaces/IPlugin.sol"; -import {IPluginManager} from "../../src/interfaces/IPluginManager.sol"; import {IStandardExecutor, Call} from "../../src/interfaces/IStandardExecutor.sol"; import {FunctionReferenceLib, FunctionReference} from "../../src/helpers/FunctionReferenceLib.sol"; import {ValidationConfig, ValidationConfigLib} from "../../src/helpers/ValidationConfigLib.sol"; @@ -14,8 +12,8 @@ import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract DirectCallsFromPluginTest is AccountTestBase { using ValidationConfigLib for ValidationConfig; - DirectCallPlugin plugin; - FunctionReference pluginFunctionReference; + DirectCallPlugin internal plugin; + FunctionReference internal pluginFunctionReference; function setUp() public { plugin = new DirectCallPlugin(); diff --git a/test/mocks/plugins/DirectCallPlugin.sol b/test/mocks/plugins/DirectCallPlugin.sol index 0bca4f9a..e741c223 100644 --- a/test/mocks/plugins/DirectCallPlugin.sol +++ b/test/mocks/plugins/DirectCallPlugin.sol @@ -1,12 +1,11 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {ManifestExecutionFunction, PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; +import {PluginManifest, PluginMetadata} from "../../../src/interfaces/IPlugin.sol"; import {IStandardExecutor} from "../../../src/interfaces/IStandardExecutor.sol"; import {IExecutionHook} from "../../../src/interfaces/IExecutionHook.sol"; import {BasePlugin} from "../../../src/plugins/BasePlugin.sol"; -import {ResultCreatorPlugin} from "./ReturnDataPluginMocks.sol"; contract DirectCallPlugin is BasePlugin, IExecutionHook { bool public preHookRan = false; From ae26e5f0bbd2d2abb5c4d727a0cb8b84e8d48a99 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Tue, 16 Jul 2024 21:34:32 +0800 Subject: [PATCH 17/24] chore: linting changes --- test/account/DirectCallsFromPlugin.t.sol | 52 ++++++++++++------------ 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index 10a8bf10..7d6f2efb 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -12,14 +12,14 @@ import {AccountTestBase} from "../utils/AccountTestBase.sol"; contract DirectCallsFromPluginTest is AccountTestBase { using ValidationConfigLib for ValidationConfig; - DirectCallPlugin internal plugin; - FunctionReference internal pluginFunctionReference; + DirectCallPlugin internal _plugin; + FunctionReference internal _pluginFunctionReference; function setUp() public { - plugin = new DirectCallPlugin(); - assertFalse(plugin.preHookRan()); - assertFalse(plugin.postHookRan()); - pluginFunctionReference = FunctionReferenceLib.pack(address(plugin), type(uint8).max); + _plugin = new DirectCallPlugin(); + assertFalse(_plugin.preHookRan()); + assertFalse(_plugin.postHookRan()); + _pluginFunctionReference = FunctionReferenceLib.pack(address(_plugin), type(uint8).max); } /* -------------------------------------------------------------------------- */ @@ -27,7 +27,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { /* -------------------------------------------------------------------------- */ function test_Fail_DirectCallPluginNotInstalled() external { - vm.prank(address(plugin)); + vm.prank(address(_plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); account1.execute(address(0), 0, ""); } @@ -37,7 +37,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { _uninstallPlugin(); - vm.prank(address(plugin)); + vm.prank(address(_plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); account1.execute(address(0), 0, ""); } @@ -47,7 +47,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { Call[] memory calls = new Call[](0); - vm.prank(address(plugin)); + vm.prank(address(_plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.executeBatch.selector)); account1.executeBatch(calls); } @@ -59,11 +59,11 @@ contract DirectCallsFromPluginTest is AccountTestBase { function test_Pass_DirectCallFromPluginPrank() external { _installPlugin(); - vm.prank(address(plugin)); + vm.prank(address(_plugin)); account1.execute(address(0), 0, ""); - assertTrue(plugin.preHookRan()); - assertTrue(plugin.postHookRan()); + assertTrue(_plugin.preHookRan()); + assertTrue(_plugin.postHookRan()); } function test_Pass_DirectCallFromPluginCallback() external { @@ -72,14 +72,14 @@ contract DirectCallsFromPluginTest is AccountTestBase { bytes memory encodedCall = abi.encodeCall(DirectCallPlugin.directCall, ()); vm.prank(address(entryPoint)); - bytes memory result = account1.execute(address(plugin), 0, encodedCall); + bytes memory result = account1.execute(address(_plugin), 0, encodedCall); - assertTrue(plugin.preHookRan()); - assertTrue(plugin.postHookRan()); + assertTrue(_plugin.preHookRan()); + assertTrue(_plugin.postHookRan()); - // the directCall() function in the plugin calls back into `execute()` with an encoded call back into the - // plugin's getData() function. - assertEq(abi.decode(result, (bytes)), abi.encode(plugin.getData())); + // the directCall() function in the _plugin calls back into `execute()` with an encoded call back into the + // _plugin's getData() function. + assertEq(abi.decode(result, (bytes)), abi.encode(_plugin.getData())); } function test_Flow_DirectCallFromPluginSequence() external { @@ -87,15 +87,15 @@ contract DirectCallsFromPluginTest is AccountTestBase { _installPlugin(); - vm.prank(address(plugin)); + vm.prank(address(_plugin)); account1.execute(address(0), 0, ""); - assertTrue(plugin.preHookRan()); - assertTrue(plugin.postHookRan()); + assertTrue(_plugin.preHookRan()); + assertTrue(_plugin.postHookRan()); _uninstallPlugin(); - vm.prank(address(plugin)); + vm.prank(address(_plugin)); vm.expectRevert(_buildDirectCallDisallowedError(IStandardExecutor.execute.selector)); account1.execute(address(0), 0, ""); } @@ -112,7 +112,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { bytes[] memory permissionHookInitDatas = new bytes[](1); permissionHooks[0] = ExecutionHook({ - hookFunction: FunctionReferenceLib.pack(address(plugin), 0xff), + hookFunction: FunctionReferenceLib.pack(address(_plugin), 0xff), isPreHook: true, isPostHook: true }); @@ -121,7 +121,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { vm.prank(address(entryPoint)); - ValidationConfig validationConfig = ValidationConfigLib.pack(pluginFunctionReference, false, false); + ValidationConfig validationConfig = ValidationConfigLib.pack(_pluginFunctionReference, false, false); account1.installValidation(validationConfig, selectors, "", "", encodedPermissionHooks); } @@ -129,13 +129,13 @@ contract DirectCallsFromPluginTest is AccountTestBase { function _uninstallPlugin() internal { vm.prank(address(entryPoint)); account1.uninstallValidation( - pluginFunctionReference, "", abi.encode(new bytes[](0)), abi.encode(new bytes[](1)) + _pluginFunctionReference, "", abi.encode(new bytes[](0)), abi.encode(new bytes[](1)) ); } function _buildDirectCallDisallowedError(bytes4 selector) internal view returns (bytes memory) { return abi.encodeWithSelector( - UpgradeableModularAccount.ExecFromPluginNotPermitted.selector, address(plugin), selector + UpgradeableModularAccount.ExecFromPluginNotPermitted.selector, address(_plugin), selector ); } } From a09ffb20ee47ad8ab7f2287882dfa754bb236fc3 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 01:51:40 +0800 Subject: [PATCH 18/24] chore: remove unused struct and using for statements --- src/account/AccountStorage.sol | 5 ----- src/account/UpgradeableModularAccount.sol | 3 --- 2 files changed, 8 deletions(-) diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 06160c4b..ce60ba7c 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -40,11 +40,6 @@ struct ValidationData { EnumerableSet.Bytes32Set selectors; } -struct DirectCallValidationData { - bool allowed; // Whether or not this direct call is allowed. - FunctionReference[] preValidationHooks; // The set of pre validation hooks for this direct call. -} - struct AccountStorage { // AccountStorageInitializable variables uint8 initialized; diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index d6ac19ac..004507a9 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -9,7 +9,6 @@ import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeab import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; -import {EnumerableMap} from "@openzeppelin/contracts/utils/structs/EnumerableMap.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; import {ValidationConfigLib} from "../helpers/ValidationConfigLib.sol"; @@ -42,8 +41,6 @@ contract UpgradeableModularAccount is UUPSUpgradeable { using EnumerableSet for EnumerableSet.Bytes32Set; - using EnumerableSet for EnumerableSet.AddressSet; - using EnumerableMap for EnumerableMap.AddressToUintMap; using FunctionReferenceLib for FunctionReference; using ValidationConfigLib for ValidationConfig; using SparseCalldataSegmentLib for bytes; From a978cd7d307f27652e31ee7d2e1d508109dd3366 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 03:04:03 +0800 Subject: [PATCH 19/24] feat: use _checkIfValidationAppliesCallData() rather than manually checking storage to handle self-calls --- src/account/UpgradeableModularAccount.sol | 7 +++---- test/account/DirectCallsFromPlugin.t.sol | 6 ++---- test/account/PermittedCallPermissions.t.sol | 3 +-- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 004507a9..76085747 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -675,7 +675,8 @@ contract UpgradeableModularAccount is // Check that the provided validation function is applicable to the selector if (isGlobal) { if (!_globalValidationAllowed(selector) || !_storage.validationData[validationFunction].isGlobal) { - revert UserOpValidationFunctionMissing(selector); + revert UserOpValidationFunctionMissing(selector); //TODO: Update this error as it can be runtime + // validation too } } else { // Not global validation, but per-selector @@ -727,9 +728,7 @@ contract UpgradeableModularAccount is FunctionReference directCallValidationKey = FunctionReferenceLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID); - if (!_storage.validationData[directCallValidationKey].selectors.contains(toSetValue(msg.sig))) { - revert ExecFromPluginNotPermitted(msg.sender, msg.sig); - } + _checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false); // Direct call is allowed, run associated permission & validation hooks diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index 7d6f2efb..eaf8a271 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -133,9 +133,7 @@ contract DirectCallsFromPluginTest is AccountTestBase { ); } - function _buildDirectCallDisallowedError(bytes4 selector) internal view returns (bytes memory) { - return abi.encodeWithSelector( - UpgradeableModularAccount.ExecFromPluginNotPermitted.selector, address(_plugin), selector - ); + function _buildDirectCallDisallowedError(bytes4 selector) internal pure returns (bytes memory) { + return abi.encodeWithSelector(UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, selector); } } diff --git a/test/account/PermittedCallPermissions.t.sol b/test/account/PermittedCallPermissions.t.sol index 18257955..2d12ccc0 100644 --- a/test/account/PermittedCallPermissions.t.sol +++ b/test/account/PermittedCallPermissions.t.sol @@ -48,8 +48,7 @@ contract PermittedCallPermissionsTest is AccountTestBase { function test_permittedCall_NotAllowed() public { vm.expectRevert( abi.encodeWithSelector( - UpgradeableModularAccount.ExecFromPluginNotPermitted.selector, - address(permittedCallerPlugin), + UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, ResultCreatorPlugin.bar.selector ) ); From ad4fcb66a525ac2b8c097d9cb8e9b63fd3f123ce Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 19:10:40 +0800 Subject: [PATCH 20/24] chore: rename missing validation error to encapsulate runtime as well --- src/account/UpgradeableModularAccount.sol | 7 +++---- test/account/DirectCallsFromPlugin.t.sol | 2 +- test/account/PermittedCallPermissions.t.sol | 3 +-- test/account/SelfCallAuthorization.t.sol | 12 ++++++------ 4 files changed, 11 insertions(+), 13 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 76085747..50b725ae 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -78,7 +78,7 @@ contract UpgradeableModularAccount is error SignatureValidationInvalid(address plugin, uint8 functionId); error UnexpectedAggregator(address plugin, uint8 functionId, address aggregator); error UnrecognizedFunction(bytes4 selector); - error UserOpValidationFunctionMissing(bytes4 selector); + error ValidationFunctionMissing(bytes4 selector); error ValidationDoesNotApply(bytes4 selector, address plugin, uint8 functionId, bool isGlobal); error ValidationSignatureSegmentMissing(); error SignatureSegmentOutOfOrder(); @@ -675,13 +675,12 @@ contract UpgradeableModularAccount is // Check that the provided validation function is applicable to the selector if (isGlobal) { if (!_globalValidationAllowed(selector) || !_storage.validationData[validationFunction].isGlobal) { - revert UserOpValidationFunctionMissing(selector); //TODO: Update this error as it can be runtime - // validation too + revert ValidationFunctionMissing(selector); } } else { // Not global validation, but per-selector if (!getAccountStorage().validationData[validationFunction].selectors.contains(toSetValue(selector))) { - revert UserOpValidationFunctionMissing(selector); + revert ValidationFunctionMissing(selector); } } } diff --git a/test/account/DirectCallsFromPlugin.t.sol b/test/account/DirectCallsFromPlugin.t.sol index eaf8a271..084938a0 100644 --- a/test/account/DirectCallsFromPlugin.t.sol +++ b/test/account/DirectCallsFromPlugin.t.sol @@ -134,6 +134,6 @@ contract DirectCallsFromPluginTest is AccountTestBase { } function _buildDirectCallDisallowedError(bytes4 selector) internal pure returns (bytes memory) { - return abi.encodeWithSelector(UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, selector); + return abi.encodeWithSelector(UpgradeableModularAccount.ValidationFunctionMissing.selector, selector); } } diff --git a/test/account/PermittedCallPermissions.t.sol b/test/account/PermittedCallPermissions.t.sol index 2d12ccc0..885015b0 100644 --- a/test/account/PermittedCallPermissions.t.sol +++ b/test/account/PermittedCallPermissions.t.sol @@ -48,8 +48,7 @@ contract PermittedCallPermissionsTest is AccountTestBase { function test_permittedCall_NotAllowed() public { vm.expectRevert( abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, - ResultCreatorPlugin.bar.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ResultCreatorPlugin.bar.selector ) ); PermittedCallerPlugin(address(account1)).usePermittedCallNotAllowed(); diff --git a/test/account/SelfCallAuthorization.t.sol b/test/account/SelfCallAuthorization.t.sol index 5bcf64b3..5be8e212 100644 --- a/test/account/SelfCallAuthorization.t.sol +++ b/test/account/SelfCallAuthorization.t.sol @@ -41,7 +41,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) @@ -57,7 +57,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) @@ -69,7 +69,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { _runtimeCall( abi.encodeCall(ComprehensivePlugin.foo, ()), abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ); @@ -100,7 +100,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) @@ -137,7 +137,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) @@ -160,7 +160,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { _runtimeExecBatchExpFail( calls, abi.encodeWithSelector( - UpgradeableModularAccount.UserOpValidationFunctionMissing.selector, + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ); From d263656b02f7fc0c960a308f85731ff6ad85325c Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 19:20:10 +0800 Subject: [PATCH 21/24] chore: fix function ordering --- src/account/UpgradeableModularAccount.sol | 132 +++++++++++----------- 1 file changed, 66 insertions(+), 66 deletions(-) diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 50b725ae..5c924534 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -612,6 +612,59 @@ contract UpgradeableModularAccount is // solhint-disable-next-line no-empty-blocks function _authorizeUpgrade(address newImplementation) internal override {} + /** + * Order of operations: + * 1. Check if the sender is the entry point, the account itself, or the selector called is public. + * - Yes: Return an empty array, there are no post-permissionHooks. + * - No: Continue + * 2. Check if the called selector (msg.sig) is included in the set of selectors the msg.sender can + * directly call. + * - Yes: Continue + * - No: Revert, the caller is not allowed to call this selector + * 3. If there are runtime validation hooks associated with this caller-sig combination, run them. + * 4. Run the pre-permissionHooks associated with this caller-sig combination, and return the + * post-permissionHooks to run later. + */ + function _checkPermittedCallerAndAssociatedHooks() + internal + returns (PostExecToRun[] memory, PostExecToRun[] memory) + { + AccountStorage storage _storage = getAccountStorage(); + + if ( + msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) + || _storage.selectorData[msg.sig].isPublic + ) { + return (new PostExecToRun[](0), new PostExecToRun[](0)); + } + + FunctionReference directCallValidationKey = + FunctionReferenceLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID); + + _checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false); + + // Direct call is allowed, run associated permission & validation hooks + + // Validation hooks + FunctionReference[] memory preRuntimeValidationHooks = + _storage.validationData[directCallValidationKey].preValidationHooks; + + uint256 hookLen = preRuntimeValidationHooks.length; + for (uint256 i = 0; i < hookLen; ++i) { + _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], msg.data, ""); + } + + // Permission hooks + PostExecToRun[] memory postPermissionHooks = + _doPreHooks(_storage.validationData[directCallValidationKey].permissionHooks, msg.data); + + // Exec hooks + PostExecToRun[] memory postExecutionHooks = + _doPreHooks(_storage.selectorData[msg.sig].executionHooks, msg.data); + + return (postPermissionHooks, postExecutionHooks); + } + function _checkIfValidationAppliesCallData( bytes calldata callData, FunctionReference validationFunction, @@ -665,6 +718,19 @@ contract UpgradeableModularAccount is } } + function _globalValidationAllowed(bytes4 selector) internal view returns (bool) { + if ( + selector == this.execute.selector || selector == this.executeBatch.selector + || selector == this.installPlugin.selector || selector == this.uninstallPlugin.selector + || selector == this.installValidation.selector || selector == this.uninstallValidation.selector + || selector == this.upgradeToAndCall.selector + ) { + return true; + } + + return getAccountStorage().selectorData[selector].allowGlobalValidation; + } + function _checkIfValidationAppliesSelector( bytes4 selector, FunctionReference validationFunction, @@ -684,70 +750,4 @@ contract UpgradeableModularAccount is } } } - - function _globalValidationAllowed(bytes4 selector) internal view returns (bool) { - if ( - selector == this.execute.selector || selector == this.executeBatch.selector - || selector == this.installPlugin.selector || selector == this.uninstallPlugin.selector - || selector == this.installValidation.selector || selector == this.uninstallValidation.selector - || selector == this.upgradeToAndCall.selector - ) { - return true; - } - - return getAccountStorage().selectorData[selector].allowGlobalValidation; - } - - /** - * Order of operations: - * 1. Check if the sender is the entry point, the account itself, or the selector called is public. - * - Yes: Return an empty array, there are no post-permissionHooks. - * - No: Continue - * 2. Check if the called selector (msg.sig) is included in the set of selectors the msg.sender can - * directly call. - * - Yes: Continue - * - No: Revert, the caller is not allowed to call this selector - * 3. If there are runtime validation hooks associated with this caller-sig combination, run them. - * 4. Run the pre-permissionHooks associated with this caller-sig combination, and return the - * post-permissionHooks to run later. - */ - function _checkPermittedCallerAndAssociatedHooks() - internal - returns (PostExecToRun[] memory, PostExecToRun[] memory) - { - AccountStorage storage _storage = getAccountStorage(); - - if ( - msg.sender == address(_ENTRY_POINT) || msg.sender == address(this) - || _storage.selectorData[msg.sig].isPublic - ) { - return (new PostExecToRun[](0), new PostExecToRun[](0)); - } - - FunctionReference directCallValidationKey = - FunctionReferenceLib.pack(msg.sender, _SELF_PERMIT_VALIDATION_FUNCTIONID); - - _checkIfValidationAppliesCallData(msg.data, directCallValidationKey, false); - - // Direct call is allowed, run associated permission & validation hooks - - // Validation hooks - FunctionReference[] memory preRuntimeValidationHooks = - _storage.validationData[directCallValidationKey].preValidationHooks; - - uint256 hookLen = preRuntimeValidationHooks.length; - for (uint256 i = 0; i < hookLen; ++i) { - _doPreRuntimeValidationHook(preRuntimeValidationHooks[i], msg.data, ""); - } - - // Permission hooks - PostExecToRun[] memory postPermissionHooks = - _doPreHooks(_storage.validationData[directCallValidationKey].permissionHooks, msg.data); - - // Exec hooks - PostExecToRun[] memory postExecutionHooks = - _doPreHooks(_storage.selectorData[msg.sig].executionHooks, msg.data); - - return (postPermissionHooks, postExecutionHooks); - } } From 0f04d85bdfbd1a52832d681055a760093a678cb5 Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 19:20:44 +0800 Subject: [PATCH 22/24] chore: double linter max line-length for test error strings --- .solhint-test.json | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/.solhint-test.json b/.solhint-test.json index fd2b1007..3224b9d0 100644 --- a/.solhint-test.json +++ b/.solhint-test.json @@ -1,20 +1,20 @@ { - "extends": "solhint:recommended", - "rules": { - "func-name-mixedcase": "off", - "immutable-vars-naming": ["error"], - "no-unused-import": ["error"], - "compiler-version": ["error", ">=0.8.19"], - "custom-errors": "off", - "func-visibility": ["error", { "ignoreConstructors": true }], - "max-line-length": ["error", 120], - "max-states-count": ["warn", 30], - "modifier-name-mixedcase": ["error"], - "private-vars-leading-underscore": ["error"], - "no-inline-assembly": "off", - "avoid-low-level-calls": "off", - "one-contract-per-file": "off", - "no-empty-blocks": "off" - } + "extends": "solhint:recommended", + "rules": { + "func-name-mixedcase": "off", + "immutable-vars-naming": ["error"], + "no-unused-import": ["error"], + "compiler-version": ["error", ">=0.8.19"], + "custom-errors": "off", + "func-visibility": ["error", { "ignoreConstructors": true }], + "max-line-length": ["error", 120], + "max-states-count": ["warn", 30], + "modifier-name-mixedcase": ["error"], + "private-vars-leading-underscore": ["error"], + "no-inline-assembly": "off", + "avoid-low-level-calls": "off", + "one-contract-per-file": "off", + "no-empty-blocks": "off", + "reason-string": ["warn", { "maxLength": 64 }] } - \ No newline at end of file +} From a425c9b88042f1c31e67fabd035b903c29cfaf4d Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 19:39:59 +0800 Subject: [PATCH 23/24] chore: formatting --- test/account/SelfCallAuthorization.t.sol | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/test/account/SelfCallAuthorization.t.sol b/test/account/SelfCallAuthorization.t.sol index 3565c737..4b6d04c7 100644 --- a/test/account/SelfCallAuthorization.t.sol +++ b/test/account/SelfCallAuthorization.t.sol @@ -40,8 +40,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.ValidationFunctionMissing.selector, - ComprehensivePlugin.foo.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) ); @@ -56,8 +55,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.ValidationFunctionMissing.selector, - ComprehensivePlugin.foo.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) ); @@ -68,8 +66,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { _runtimeCall( abi.encodeCall(ComprehensivePlugin.foo, ()), abi.encodeWithSelector( - UpgradeableModularAccount.ValidationFunctionMissing.selector, - ComprehensivePlugin.foo.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ); } @@ -99,8 +96,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.ValidationFunctionMissing.selector, - ComprehensivePlugin.foo.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) ); @@ -136,8 +132,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { 0, "AA23 reverted", abi.encodeWithSelector( - UpgradeableModularAccount.ValidationFunctionMissing.selector, - ComprehensivePlugin.foo.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ) ); @@ -159,8 +154,7 @@ contract SelfCallAuthorizationTest is AccountTestBase { _runtimeExecBatchExpFail( calls, abi.encodeWithSelector( - UpgradeableModularAccount.ValidationFunctionMissing.selector, - ComprehensivePlugin.foo.selector + UpgradeableModularAccount.ValidationFunctionMissing.selector, ComprehensivePlugin.foo.selector ) ); } From 4a29ae39eec9bb3bea29a13a7a4f4a8f84a6577f Mon Sep 17 00:00:00 2001 From: zer0dot Date: Wed, 17 Jul 2024 21:45:56 +0800 Subject: [PATCH 24/24] chore: rename old function to modern naming (functionReference => pluginEntity) --- src/account/PluginManager2.sol | 6 +++--- src/helpers/ValidationConfigLib.sol | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 1454d13f..2ff6a9d1 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -33,7 +33,7 @@ abstract contract PluginManager2 { bytes memory permissionHooks ) internal { ValidationData storage _validationData = - getAccountStorage().validationData[validationConfig.functionReference()]; + getAccountStorage().validationData[validationConfig.pluginEntity()]; if (preValidationHooks.length > 0) { (PluginEntity[] memory preValidationFunctions, bytes[] memory initDatas) = @@ -64,7 +64,7 @@ abstract contract PluginManager2 { ExecutionHook memory permissionFunction = permissionFunctions[i]; if (!_validationData.permissionHooks.add(toSetValue(permissionFunction))) { - revert PermissionAlreadySet(validationConfig.functionReference(), permissionFunction); + revert PermissionAlreadySet(validationConfig.pluginEntity(), permissionFunction); } if (initDatas[i].length > 0) { @@ -77,7 +77,7 @@ abstract contract PluginManager2 { for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; if (!_validationData.selectors.add(toSetValue(selector))) { - revert ValidationAlreadySet(selector, validationConfig.functionReference()); + revert ValidationAlreadySet(selector, validationConfig.pluginEntity()); } } diff --git a/src/helpers/ValidationConfigLib.sol b/src/helpers/ValidationConfigLib.sol index 95e8ea90..6d27b907 100644 --- a/src/helpers/ValidationConfigLib.sol +++ b/src/helpers/ValidationConfigLib.sol @@ -78,7 +78,7 @@ library ValidationConfigLib { return uint32(bytes4(ValidationConfig.unwrap(config) << 160)); } - function functionReference(ValidationConfig config) internal pure returns (PluginEntity) { + function pluginEntity(ValidationConfig config) internal pure returns (PluginEntity) { return PluginEntity.wrap(bytes24(ValidationConfig.unwrap(config))); }