diff --git a/foundry.toml b/foundry.toml index 5aad32a4..67d9eb7c 100644 --- a/foundry.toml +++ b/foundry.toml @@ -1,5 +1,5 @@ [profile.default] -solc = '0.8.25' +solc = '0.8.26' via_ir = false src = 'src' test = 'test' diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 4319f470..f463a629 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -39,6 +39,8 @@ struct SelectorData { // Note that even if this is set to true, user op validation will still be required, otherwise anyone could // drain the account of native tokens by wasting gas. bool isPublic; + // Whether or not a default validation function may be used to validate this function. + bool allowDefaultValidation; // How many times a `PRE_HOOK_ALWAYS_DENY` has been added for this function. // Since that is the only type of hook that may overlap, we can use this to track the number of times it has // been applied, and whether or not the deny should apply. The size `uint48` was chosen somewhat arbitrarily, @@ -68,6 +70,8 @@ struct AccountStorage { mapping(bytes4 => uint256) supportedIfaces; // Installed plugins capable of signature validation. EnumerableSet.Bytes32Set signatureValidations; + // Todo: merge this with other validation storage? + EnumerableSet.Bytes32Set defaultValidations; } // TODO: Change how pre-validation hooks work to allow association with validation, rather than selector. diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol new file mode 100644 index 00000000..c8945ef1 --- /dev/null +++ b/src/account/PluginManager2.sol @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.25; + +import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet.sol"; + +import {IPlugin} from "../interfaces/IPlugin.sol"; +import {FunctionReference} from "../interfaces/IPluginManager.sol"; +import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; +import {AccountStorage, getAccountStorage, toSetValue} from "./AccountStorage.sol"; + +// Temporary additional functions for a user-controlled install flow for validation functions. +abstract contract PluginManager2 { + using EnumerableSet for EnumerableSet.Bytes32Set; + + error DefaultValidationAlreadySet(address plugin, uint8 functionId); + error ValidationAlreadySet(bytes4 selector, address plugin, uint8 functionId); + error ValidationNotSet(bytes4 selector, address plugin, uint8 functionId); + + function _installValidation( + address plugin, + uint8 functionId, + bool isDefault, + bytes4[] memory selectors, + bytes calldata installData + ) internal { + FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId); + + AccountStorage storage _storage = getAccountStorage(); + + if (isDefault) { + if (!_storage.defaultValidations.add(toSetValue(validationFunction))) { + revert DefaultValidationAlreadySet(plugin, functionId); + } + } + + for (uint256 i = 0; i < selectors.length; ++i) { + bytes4 selector = selectors[i]; + if (!_storage.selectorData[selector].validations.add(toSetValue(validationFunction))) { + revert ValidationAlreadySet(selector, plugin, functionId); + } + } + + IPlugin(plugin).onInstall(installData); + } + + function _uninstallValidation( + address plugin, + uint8 functionId, + bytes4[] calldata selectors, + bytes calldata uninstallData + ) internal { + FunctionReference validationFunction = FunctionReferenceLib.pack(plugin, functionId); + + AccountStorage storage _storage = getAccountStorage(); + + // Ignore return value - remove if present, do nothing otherwise. + _storage.defaultValidations.remove(toSetValue(validationFunction)); + + for (uint256 i = 0; i < selectors.length; ++i) { + bytes4 selector = selectors[i]; + if (!_storage.selectorData[selector].validations.remove(toSetValue(validationFunction))) { + revert ValidationNotSet(selector, plugin, functionId); + } + } + + IPlugin(plugin).onUninstall(uninstallData); + } +} diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index cfdbef16..f61c7403 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -59,7 +59,7 @@ abstract contract PluginManagerInternals is IPluginManager { // Storage update operations - function _setExecutionFunction(bytes4 selector, bool isPublic, address plugin) + function _setExecutionFunction(bytes4 selector, bool isPublic, bool allowDefaultValidation, address plugin) internal notNullPlugin(plugin) { @@ -71,6 +71,7 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.plugin = plugin; _selectorData.isPublic = isPublic; + _selectorData.allowDefaultValidation = allowDefaultValidation; } function _removeExecutionFunction(bytes4 selector) internal { @@ -78,6 +79,7 @@ abstract contract PluginManagerInternals is IPluginManager { _selectorData.plugin = address(0); _selectorData.isPublic = false; + _selectorData.allowDefaultValidation = false; } function _addValidationFunction(bytes4 selector, FunctionReference validationFunction) @@ -221,7 +223,8 @@ abstract contract PluginManagerInternals is IPluginManager { for (uint256 i = 0; i < length; ++i) { bytes4 selector = manifest.executionFunctions[i].executionSelector; bool isPublic = manifest.executionFunctions[i].isPublic; - _setExecutionFunction(selector, isPublic, plugin); + bool allowDefaultValidation = manifest.executionFunctions[i].allowDefaultValidation; + _setExecutionFunction(selector, isPublic, allowDefaultValidation, plugin); } // Add installed plugin and selectors this plugin can call diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 69a11cbd..047e73c0 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -30,6 +30,7 @@ import { } from "./AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {PluginManagerInternals} from "./PluginManagerInternals.sol"; +import {PluginManager2} from "./PluginManager2.sol"; contract UpgradeableModularAccount is AccountExecutor, @@ -41,6 +42,7 @@ contract UpgradeableModularAccount is IPluginExecutor, IStandardExecutor, PluginManagerInternals, + PluginManager2, UUPSUpgradeable { using EnumerableSet for EnumerableSet.Bytes32Set; @@ -77,6 +79,7 @@ contract UpgradeableModularAccount is error UnexpectedAggregator(address plugin, uint8 functionId, address aggregator); error UnrecognizedFunction(bytes4 selector); error UserOpValidationFunctionMissing(bytes4 selector); + error ValidationDoesNotApply(bytes4 selector, address plugin, uint8 functionId, bool isDefault); // Wraps execution of a native function with runtime validation and hooks // Used for upgradeTo, upgradeToAndCall, execute, executeBatch, installPlugin, uninstallPlugin @@ -155,6 +158,7 @@ contract UpgradeableModularAccount is } /// @inheritdoc IStandardExecutor + /// @notice May be validated by a default validation. function execute(address target, uint256 value, bytes calldata data) external payable @@ -166,6 +170,7 @@ contract UpgradeableModularAccount is } /// @inheritdoc IStandardExecutor + /// @notice May be validated by a default validation function. function executeBatch(Call[] calldata calls) external payable @@ -279,11 +284,12 @@ contract UpgradeableModularAccount is if (_storage.selectorData[execSelector].denyExecutionCount > 0) { revert AlwaysDenyRule(); } - if (!_storage.selectorData[execSelector].validations.contains(toSetValue(runtimeValidationFunction))) { - revert RuntimeValidationFunctionMissing(execSelector); - } - _doRuntimeValidation(runtimeValidationFunction, data, authorization[21:]); + // Check if the runtime validation function is allowed to be called + bool isDefaultValidation = uint8(authorization[21]) == 1; + _checkIfValidationApplies(execSelector, runtimeValidationFunction, isDefaultValidation); + + _doRuntimeValidation(runtimeValidationFunction, data, authorization[22:]); // If runtime validation passes, execute the call @@ -299,6 +305,7 @@ contract UpgradeableModularAccount is } /// @inheritdoc IPluginManager + /// @notice May be validated by a default validation. function installPlugin( address plugin, bytes32 manifestHash, @@ -309,6 +316,7 @@ contract UpgradeableModularAccount is } /// @inheritdoc IPluginManager + /// @notice May be validated by a default validation. function uninstallPlugin(address plugin, bytes calldata config, bytes calldata pluginUninstallData) external override @@ -325,6 +333,42 @@ contract UpgradeableModularAccount is _uninstallPlugin(plugin, manifest, pluginUninstallData); } + /// @notice Initializes the account with a validation function added to the default pool. + /// TODO: remove and merge with regular initialization, after we figure out a better install/uninstall workflow + /// with user install configs. + /// @dev This function is only callable once, and only by the EntryPoint. + + function initializeDefaultValidation(address plugin, uint8 functionId, bytes calldata installData) + external + initializer + { + _installValidation(plugin, functionId, true, new bytes4[](0), installData); + emit ModularAccountInitialized(_ENTRY_POINT); + } + + /// @inheritdoc IPluginManager + /// @notice May be validated by a default validation. + function installValidation( + address plugin, + uint8 functionId, + bool isDefault, + bytes4[] calldata selectors, + bytes calldata installData + ) external wrapNativeFunction { + _installValidation(plugin, functionId, isDefault, selectors, installData); + } + + /// @inheritdoc IPluginManager + /// @notice May be validated by a default validation. + function uninstallValidation( + address plugin, + uint8 functionId, + bytes4[] calldata selectors, + bytes calldata uninstallData + ) external wrapNativeFunction { + _uninstallValidation(plugin, functionId, selectors, uninstallData); + } + /// @notice ERC165 introspection /// @dev returns true for `IERC165.interfaceId` and false for `0xFFFFFFFF` /// @param interfaceId interface id to check against @@ -341,6 +385,7 @@ contract UpgradeableModularAccount is } /// @inheritdoc UUPSUpgradeable + /// @notice May be validated by a default validation. function upgradeToAndCall(address newImplementation, bytes memory data) public payable @@ -398,14 +443,12 @@ contract UpgradeableModularAccount is // Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. FunctionReference userOpValidationFunction = FunctionReference.wrap(bytes21(userOp.signature[:21])); + bool isDefaultValidation = uint8(userOp.signature[21]) == 1; - if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(userOpValidationFunction))) - { - revert UserOpValidationFunctionMissing(selector); - } + _checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation); validationData = - _doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[21:], userOpHash); + _doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[22:], userOpHash); } // To support gas estimation, we don't fail early when the failure is caused by a signature failure @@ -573,6 +616,41 @@ contract UpgradeableModularAccount is // solhint-disable-next-line no-empty-blocks function _authorizeUpgrade(address newImplementation) internal override {} + function _checkIfValidationApplies(bytes4 selector, FunctionReference validationFunction, bool isDefault) + internal + view + { + AccountStorage storage _storage = getAccountStorage(); + + // Check that the provided validation function is applicable to the selector + if (isDefault) { + if ( + !_defaultValidationAllowed(selector) + || !_storage.defaultValidations.contains(toSetValue(validationFunction)) + ) { + revert UserOpValidationFunctionMissing(selector); + } + } else { + // Not default validation, but per-selector + if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(validationFunction))) { + revert UserOpValidationFunctionMissing(selector); + } + } + } + + function _defaultValidationAllowed(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].allowDefaultValidation; + } + function _checkPermittedCallerIfNotFromEP() internal view { AccountStorage storage _storage = getAccountStorage(); diff --git a/src/interfaces/IPlugin.sol b/src/interfaces/IPlugin.sol index a74dfaf8..e9ea8045 100644 --- a/src/interfaces/IPlugin.sol +++ b/src/interfaces/IPlugin.sol @@ -27,6 +27,8 @@ struct ManifestExecutionFunction { bytes4 executionSelector; // If true, the function won't need runtime validation, and can be called by anyone. bool isPublic; + // If true, the function can be validated by a default validation function. + bool allowDefaultValidation; } /// @dev For functions of type `ManifestAssociatedFunctionType.DEPENDENCY`, the MSCA MUST find the plugin address @@ -77,15 +79,12 @@ struct PluginMetadata { /// @dev A struct describing how the plugin should be installed on a modular account. 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; - // If this plugin depends on other plugins' validation functions, the interface IDs of those plugins MUST be - // provided here, with its position in the array matching the `dependencyIndex` members of `ManifestFunction` - // structs used in the manifest. - bytes4[] dependencyInterfaceIds; // Execution functions defined in this plugin to be installed on the MSCA. ManifestExecutionFunction[] executionFunctions; + ManifestAssociatedFunction[] validationFunctions; + ManifestAssociatedFunction[] preValidationHooks; + ManifestExecutionHook[] executionHooks; + uint8[] signatureValidationFunctions; // Plugin execution functions already installed on the MSCA that this plugin will be able to call. bytes4[] permittedExecutionSelectors; // Boolean to indicate whether the plugin can call any external address. @@ -94,10 +93,13 @@ struct PluginManifest { // plugin MUST still be able to spend up to the balance that it sends to the account in the same call. bool canSpendNativeToken; ManifestExternalCallPermission[] permittedExternalCalls; - ManifestAssociatedFunction[] validationFunctions; - ManifestAssociatedFunction[] preValidationHooks; - ManifestExecutionHook[] executionHooks; - uint8[] signatureValidationFunctions; + // List of ERC-165 interface IDs to add to account to support introspection checks. This MUST NOT include + // IPlugin's interface ID. + bytes4[] interfaceIds; + // If this plugin depends on other plugins' validation functions, the interface IDs of those plugins MUST be + // provided here, with its position in the array matching the `dependencyIndex` members of `ManifestFunction` + // structs used in the manifest. + bytes4[] dependencyInterfaceIds; } interface IPlugin is IERC165 { diff --git a/src/interfaces/IPluginManager.sol b/src/interfaces/IPluginManager.sol index dd4dc672..1c21aadd 100644 --- a/src/interfaces/IPluginManager.sol +++ b/src/interfaces/IPluginManager.sol @@ -22,8 +22,40 @@ interface IPluginManager { FunctionReference[] calldata dependencies ) external; + /// @notice Temporary install function - pending a different user-supplied install config & manifest validation + /// path. + /// Installs a validation function across a set of execution selectors, and optionally mark it as a default + /// validation. + /// TODO: remove or update. + /// @dev This does not validate anything against the manifest - the caller must ensure validity. + /// @param plugin The plugin to install. + /// @param functionId The function ID of the validation function to install. + /// @param isDefault Whether the validation function applies for all selectors in the default pool. + /// @param selectors The selectors to install the validation function for. + /// @param installData Optional data to be decoded and used by the plugin to setup initial plugin state. + function installValidation( + address plugin, + uint8 functionId, + bool isDefault, + bytes4[] calldata selectors, + bytes calldata installData + ) external; + + /// @notice Uninstall a validation function from a set of execution selectors. + /// TODO: remove or update. + /// @param plugin The plugin to uninstall. + /// @param functionId The function ID of the validation function to uninstall. + /// @param selectors The selectors to uninstall the validation function for. + /// @param uninstallData Optional data to be decoded and used by the plugin to clear plugin data for the + /// account. + function uninstallValidation( + address plugin, + uint8 functionId, + bytes4[] calldata selectors, + bytes calldata uninstallData + ) external; + /// @notice Uninstall a plugin from the modular account. - /// @dev Uninstalling owner plugins outside of a replace operation via executeBatch risks losing the account! /// @param plugin The plugin to uninstall. /// @param config An optional, implementation-specific field that accounts may use to ensure consistency /// guarantees. diff --git a/src/plugins/TokenReceiverPlugin.sol b/src/plugins/TokenReceiverPlugin.sol index f3379f1a..5bfddc0f 100644 --- a/src/plugins/TokenReceiverPlugin.sol +++ b/src/plugins/TokenReceiverPlugin.sol @@ -59,12 +59,21 @@ contract TokenReceiverPlugin is BasePlugin, IERC721Receiver, IERC1155Receiver { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](3); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.onERC721Received.selector, isPublic: true}); - manifest.executionFunctions[1] = - ManifestExecutionFunction({executionSelector: this.onERC1155Received.selector, isPublic: true}); - manifest.executionFunctions[2] = - ManifestExecutionFunction({executionSelector: this.onERC1155BatchReceived.selector, isPublic: true}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.onERC721Received.selector, + isPublic: true, + allowDefaultValidation: false + }); + manifest.executionFunctions[1] = ManifestExecutionFunction({ + executionSelector: this.onERC1155Received.selector, + isPublic: true, + allowDefaultValidation: false + }); + manifest.executionFunctions[2] = ManifestExecutionFunction({ + executionSelector: this.onERC1155BatchReceived.selector, + isPublic: true, + allowDefaultValidation: false + }); manifest.interfaceIds = new bytes4[](2); manifest.interfaceIds[0] = type(IERC721Receiver).interfaceId; diff --git a/test/account/AccountExecHooks.t.sol b/test/account/AccountExecHooks.t.sol index ee229e3e..3d8a0c69 100644 --- a/test/account/AccountExecHooks.t.sol +++ b/test/account/AccountExecHooks.t.sol @@ -39,7 +39,13 @@ contract AccountExecHooksTest is AccountTestBase { function setUp() public { _transferOwnershipToTest(); - m1.executionFunctions.push(ManifestExecutionFunction({executionSelector: _EXEC_SELECTOR, isPublic: true})); + m1.executionFunctions.push( + ManifestExecutionFunction({ + executionSelector: _EXEC_SELECTOR, + isPublic: true, + allowDefaultValidation: false + }) + ); } function test_preExecHook_install() public { diff --git a/test/account/AccountReturnData.t.sol b/test/account/AccountReturnData.t.sol index 085fa4bf..b3d2419a 100644 --- a/test/account/AccountReturnData.t.sol +++ b/test/account/AccountReturnData.t.sol @@ -59,7 +59,9 @@ contract AccountReturnDataTest is AccountTestBase { account1.execute, (address(regularResultContract), 0, abi.encodeCall(RegularResultContract.foo, ())) ), - abi.encodePacked(singleOwnerPlugin, ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) + abi.encodePacked( + singleOwnerPlugin, ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER, SELECTOR_ASSOCIATED_VALIDATION + ) ); bytes32 result = abi.decode(abi.decode(returnData, (bytes)), (bytes32)); @@ -83,7 +85,9 @@ contract AccountReturnDataTest is AccountTestBase { bytes memory retData = account1.executeWithAuthorization( abi.encodeCall(account1.executeBatch, (calls)), - abi.encodePacked(singleOwnerPlugin, ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) + abi.encodePacked( + singleOwnerPlugin, ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER, SELECTOR_ASSOCIATED_VALIDATION + ) ); bytes[] memory returnDatas = abi.decode(retData, (bytes[])); diff --git a/test/account/DefaultValidationTest.t.sol b/test/account/DefaultValidationTest.t.sol new file mode 100644 index 00000000..fc93060d --- /dev/null +++ b/test/account/DefaultValidationTest.t.sol @@ -0,0 +1,82 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.25; + +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; + +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; +import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; + +import {AccountTestBase} from "../utils/AccountTestBase.sol"; +import {DefaultValidationFactoryFixture} from "../mocks/DefaultValidationFactoryFixture.sol"; + +contract DefaultValidationTest is AccountTestBase { + using MessageHashUtils for bytes32; + + DefaultValidationFactoryFixture public defaultValidationFactoryFixture; + + uint256 public constant CALL_GAS_LIMIT = 50000; + uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; + + FunctionReference public ownerValidation; + + address public ethRecipient; + + function setUp() public { + defaultValidationFactoryFixture = new DefaultValidationFactoryFixture(entryPoint, singleOwnerPlugin); + + account1 = UpgradeableModularAccount(payable(defaultValidationFactoryFixture.getAddress(owner1, 0))); + + vm.deal(address(account1), 100 ether); + + ethRecipient = makeAddr("ethRecipient"); + vm.deal(ethRecipient, 1 wei); + + ownerValidation = FunctionReferenceLib.pack( + address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) + ); + } + + function test_defaultValidation_userOp_simple() public { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account1), + nonce: 0, + initCode: abi.encodePacked( + defaultValidationFactoryFixture, + abi.encodeCall(DefaultValidationFactoryFixture.createAccount, (owner1, 0)) + ), + callData: abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), + accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), + preVerificationGas: 0, + gasFees: _encodeGas(1, 1), + paymasterAndData: "", + signature: "" + }); + + // Generate signature + bytes32 userOpHash = entryPoint.getUserOpHash(userOp); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); + userOp.signature = abi.encodePacked(ownerValidation, DEFAULT_VALIDATION, r, s, v); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + entryPoint.handleOps(userOps, beneficiary); + + assertEq(ethRecipient.balance, 2 wei); + } + + function test_defaultValidation_runtime_simple() public { + // Deploy the account first + defaultValidationFactoryFixture.createAccount(owner1, 0); + + vm.prank(owner1); + account1.executeWithAuthorization( + abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), + abi.encodePacked(ownerValidation, DEFAULT_VALIDATION) + ); + + assertEq(ethRecipient.balance, 2 wei); + } +} diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index 9ca70857..9b22f5a0 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -67,13 +67,21 @@ contract MultiValidationTest is AccountTestBase { ); account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), - abi.encodePacked(address(validator2), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER)) + abi.encodePacked( + address(validator2), + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), + SELECTOR_ASSOCIATED_VALIDATION + ) ); vm.prank(owner2); account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), - abi.encodePacked(address(validator2), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER)) + abi.encodePacked( + address(validator2), + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), + SELECTOR_ASSOCIATED_VALIDATION + ) ); } @@ -97,8 +105,14 @@ contract MultiValidationTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = - abi.encodePacked(address(validator2), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), r, s, v); + userOp.signature = abi.encodePacked( + address(validator2), + SELECTOR_ASSOCIATED_VALIDATION, + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), + r, + s, + v + ); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 1cbcb78d..15ad262e 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -87,7 +87,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(ownerValidation, r, s, v); + userOp.signature = abi.encodePacked(ownerValidation, SELECTOR_ASSOCIATED_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -116,7 +116,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(ownerValidation, r, s, v); + userOp.signature = abi.encodePacked(ownerValidation, SELECTOR_ASSOCIATED_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -142,7 +142,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner2Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(ownerValidation, r, s, v); + userOp.signature = abi.encodePacked(ownerValidation, SELECTOR_ASSOCIATED_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -168,7 +168,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(ownerValidation, r, s, v); + userOp.signature = abi.encodePacked(ownerValidation, SELECTOR_ASSOCIATED_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -196,7 +196,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(ownerValidation, r, s, v); + userOp.signature = abi.encodePacked(ownerValidation, SELECTOR_ASSOCIATED_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -227,7 +227,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { // Generate signature bytes32 userOpHash = entryPoint.getUserOpHash(userOp); (uint8 v, bytes32 r, bytes32 s) = vm.sign(owner1Key, userOpHash.toEthSignedMessageHash()); - userOp.signature = abi.encodePacked(ownerValidation, r, s, v); + userOp.signature = abi.encodePacked(ownerValidation, SELECTOR_ASSOCIATED_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index 9c54e7b0..de0e7ef1 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -72,7 +72,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(noHookPlugin.foo.selector); - userOp.signature = abi.encodePacked(noHookValidation); + userOp.signature = abi.encodePacked(noHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -89,7 +89,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -107,7 +107,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -130,7 +130,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -152,7 +152,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -172,7 +172,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -197,7 +197,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -221,7 +221,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(oneHookPlugin.bar.selector); - userOp.signature = abi.encodePacked(oneHookValidation); + userOp.signature = abi.encodePacked(oneHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -245,7 +245,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(twoHookPlugin.baz.selector); - userOp.signature = abi.encodePacked(twoHookValidation); + userOp.signature = abi.encodePacked(twoHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); @@ -264,7 +264,7 @@ contract ValidationIntersectionTest is AccountTestBase { PackedUserOperation memory userOp; userOp.callData = bytes.concat(twoHookPlugin.baz.selector); - userOp.signature = abi.encodePacked(twoHookValidation); + userOp.signature = abi.encodePacked(twoHookValidation, SELECTOR_ASSOCIATED_VALIDATION); bytes32 uoHash = entryPoint.getUserOpHash(userOp); vm.prank(address(entryPoint)); diff --git a/test/mocks/DefaultValidationFactoryFixture.sol b/test/mocks/DefaultValidationFactoryFixture.sol new file mode 100644 index 00000000..5fbefe7c --- /dev/null +++ b/test/mocks/DefaultValidationFactoryFixture.sol @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {Create2} from "@openzeppelin/contracts/utils/Create2.sol"; +import {ERC1967Proxy} from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol"; +import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntryPoint.sol"; + +import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; +import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; +import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; + +import {OptimizedTest} from "../utils/OptimizedTest.sol"; + +contract DefaultValidationFactoryFixture is OptimizedTest { + UpgradeableModularAccount public accountImplementation; + SingleOwnerPlugin public singleOwnerPlugin; + bytes32 private immutable _PROXY_BYTECODE_HASH; + + uint32 public constant UNSTAKE_DELAY = 1 weeks; + + IEntryPoint public entryPoint; + + address public self; + + bytes32 public singleOwnerPluginManifestHash; + + constructor(IEntryPoint _entryPoint, SingleOwnerPlugin _singleOwnerPlugin) { + entryPoint = _entryPoint; + accountImplementation = _deployUpgradeableModularAccount(_entryPoint); + _PROXY_BYTECODE_HASH = keccak256( + abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) + ); + singleOwnerPlugin = _singleOwnerPlugin; + self = address(this); + // The manifest hash is set this way in this factory just for testing purposes. + // For production factories the manifest hashes should be passed as a constructor argument. + singleOwnerPluginManifestHash = keccak256(abi.encode(singleOwnerPlugin.pluginManifest())); + } + + /** + * create an account, and return its address. + * returns the address even if the account is already deployed. + * Note that during user operation execution, this method is called only if the account is not deployed. + * This method returns an existing account address so that entryPoint.getSenderAddress() would work even after + * account creation + */ + function createAccount(address owner, uint256 salt) public returns (UpgradeableModularAccount) { + address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); + + // short circuit if exists + if (addr.code.length == 0) { + bytes memory pluginInstallData = abi.encode(owner); + // not necessary to check return addr since next call will fail if so + new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); + + // point proxy to actual implementation and init plugins + UpgradeableModularAccount(payable(addr)).initializeDefaultValidation( + address(singleOwnerPlugin), + uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), + pluginInstallData + ); + } + + return UpgradeableModularAccount(payable(addr)); + } + + /** + * calculate the counterfactual address of this account as it would be returned by createAccount() + */ + function getAddress(address owner, uint256 salt) public view returns (address) { + return Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); + } + + function addStake() external payable { + entryPoint.addStake{value: msg.value}(UNSTAKE_DELAY); + } + + function getSalt(address owner, uint256 salt) public pure returns (bytes32) { + return keccak256(abi.encodePacked(owner, salt)); + } +} diff --git a/test/mocks/plugins/BadTransferOwnershipPlugin.sol b/test/mocks/plugins/BadTransferOwnershipPlugin.sol index 6561bb1b..dcb9a935 100644 --- a/test/mocks/plugins/BadTransferOwnershipPlugin.sol +++ b/test/mocks/plugins/BadTransferOwnershipPlugin.sol @@ -33,8 +33,11 @@ contract BadTransferOwnershipPlugin is BasePlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.evilTransferOwnership.selector, isPublic: true}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.evilTransferOwnership.selector, + isPublic: true, + allowDefaultValidation: false + }); manifest.permittedExecutionSelectors = new bytes4[](1); manifest.permittedExecutionSelectors[0] = ISingleOwnerPlugin.transferOwnership.selector; diff --git a/test/mocks/plugins/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index ec40368f..8257a2b8 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -133,8 +133,11 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.foo.selector, isPublic: false}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.foo.selector, + isPublic: false, + allowDefaultValidation: false + }); ManifestFunction memory fooValidationFunction = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, diff --git a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol index 11970dfa..1d9f566f 100644 --- a/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol +++ b/test/mocks/plugins/ExecFromPluginPermissionsMocks.sol @@ -186,8 +186,11 @@ contract EFPCallerPluginAnyExternal is BasePlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.passthroughExecute.selector, isPublic: true}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.passthroughExecute.selector, + isPublic: true, + allowDefaultValidation: false + }); manifest.permitAnyExternalAddress = true; diff --git a/test/mocks/plugins/ManifestValidityMocks.sol b/test/mocks/plugins/ManifestValidityMocks.sol index 302226c0..d5c8ce1a 100644 --- a/test/mocks/plugins/ManifestValidityMocks.sol +++ b/test/mocks/plugins/ManifestValidityMocks.sol @@ -26,8 +26,11 @@ contract BadHookMagicValue_ValidationFunction_Plugin is BasePlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.foo.selector, isPublic: false}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.foo.selector, + isPublic: false, + allowDefaultValidation: false + }); manifest.validationFunctions = new ManifestAssociatedFunction[](1); manifest.validationFunctions[0] = ManifestAssociatedFunction({ diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index 788cb4c7..5ab3a811 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -38,10 +38,16 @@ contract ResultCreatorPlugin is BasePlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](2); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.foo.selector, isPublic: true}); - manifest.executionFunctions[1] = - ManifestExecutionFunction({executionSelector: this.bar.selector, isPublic: false}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.foo.selector, + isPublic: true, + allowDefaultValidation: false + }); + manifest.executionFunctions[1] = ManifestExecutionFunction({ + executionSelector: this.bar.selector, + isPublic: false, + allowDefaultValidation: false + }); return manifest; } @@ -108,10 +114,16 @@ contract ResultConsumerPlugin is BasePlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](2); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.checkResultEFPFallback.selector, isPublic: true}); - manifest.executionFunctions[1] = - ManifestExecutionFunction({executionSelector: this.checkResultEFPExternal.selector, isPublic: true}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.checkResultEFPFallback.selector, + isPublic: true, + allowDefaultValidation: false + }); + manifest.executionFunctions[1] = ManifestExecutionFunction({ + executionSelector: this.checkResultEFPExternal.selector, + isPublic: true, + allowDefaultValidation: false + }); manifest.permittedExecutionSelectors = new bytes4[](1); manifest.permittedExecutionSelectors[0] = ResultCreatorPlugin.foo.selector; diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index 443ee0bb..770c6791 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -95,8 +95,11 @@ contract MockUserOpValidationPlugin is MockBaseUserOpValidationPlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.foo.selector, isPublic: false}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.foo.selector, + isPublic: false, + allowDefaultValidation: false + }); manifest.validationFunctions = new ManifestAssociatedFunction[](1); manifest.validationFunctions[0] = ManifestAssociatedFunction({ @@ -134,8 +137,11 @@ contract MockUserOpValidation1HookPlugin is MockBaseUserOpValidationPlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.bar.selector, isPublic: false}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.bar.selector, + isPublic: false, + allowDefaultValidation: false + }); ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, @@ -187,8 +193,11 @@ contract MockUserOpValidation2HookPlugin is MockBaseUserOpValidationPlugin { PluginManifest memory manifest; manifest.executionFunctions = new ManifestExecutionFunction[](1); - manifest.executionFunctions[0] = - ManifestExecutionFunction({executionSelector: this.baz.selector, isPublic: false}); + manifest.executionFunctions[0] = ManifestExecutionFunction({ + executionSelector: this.baz.selector, + isPublic: false, + allowDefaultValidation: false + }); ManifestFunction memory userOpValidationFunctionRef = ManifestFunction({ functionType: ManifestAssociatedFunctionType.SELF, diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 6eca6626..059e9cac 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -23,6 +23,9 @@ abstract contract AccountTestBase is OptimizedTest { uint256 public owner1Key; UpgradeableModularAccount public account1; + uint8 public constant SELECTOR_ASSOCIATED_VALIDATION = 0; + uint8 public constant DEFAULT_VALIDATION = 1; + constructor() { entryPoint = new EntryPoint(); (owner1, owner1Key) = makeAddrAndKey("owner1"); @@ -47,7 +50,11 @@ abstract contract AccountTestBase is OptimizedTest { abi.encodeCall(SingleOwnerPlugin.transferOwnership, (address(this))) ) ), - abi.encodePacked(address(singleOwnerPlugin), ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) + abi.encodePacked( + address(singleOwnerPlugin), + ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER, + SELECTOR_ASSOCIATED_VALIDATION + ) ); }