From 3768d9144b4b903725386d7cbb87ab09e3fb0e6b Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Wed, 26 Jun 2024 17:30:29 -0700 Subject: [PATCH 1/4] Use validationId as validation identifier in account and allow user to choose if a validation is allowed to validate signature --- src/account/AccountLoupe.sol | 4 +- src/account/AccountStorage.sol | 5 +- src/account/PluginManager2.sol | 64 +++++---- src/account/UpgradeableModularAccount.sol | 111 ++++++++++------ src/interfaces/IAccountLoupe.sol | 4 +- src/interfaces/IPluginManager.sol | 12 +- src/interfaces/IValidation.sol | 24 +++- src/plugins/owner/SingleOwnerPlugin.sol | 6 +- .../validation/EcdsaValidationPlugin.sol | 123 ++++++++++++++++++ .../validation/IEcdsaValidationPlugin.sol | 31 +++++ 10 files changed, 299 insertions(+), 85 deletions(-) create mode 100644 src/plugins/validation/EcdsaValidationPlugin.sol create mode 100644 src/plugins/validation/IEcdsaValidationPlugin.sol diff --git a/src/account/AccountLoupe.sol b/src/account/AccountLoupe.sol index 9cde18b4..ccbc89dd 100644 --- a/src/account/AccountLoupe.sol +++ b/src/account/AccountLoupe.sol @@ -52,14 +52,14 @@ abstract contract AccountLoupe is IAccountLoupe { } /// @inheritdoc IAccountLoupe - function getPreValidationHooks(FunctionReference validationFunction) + function getPreValidationHooks(bytes32 validationId) external view override returns (FunctionReference[] memory preValidationHooks) { preValidationHooks = - toFunctionReferenceArray(getAccountStorage().validationData[validationFunction].preValidationHooks); + toFunctionReferenceArray(getAccountStorage().validationData[validationId].preValidationHooks); } /// @inheritdoc IAccountLoupe diff --git a/src/account/AccountStorage.sol b/src/account/AccountStorage.sol index 0a992a33..77dd7f36 100644 --- a/src/account/AccountStorage.sol +++ b/src/account/AccountStorage.sol @@ -34,10 +34,11 @@ struct SelectorData { } struct ValidationData { + FunctionReference validationFunction; // Whether or not this validation can be used as a default validation function. bool isDefault; // Whether or not this validation is a signature validator. - bool isSignatureValidation; + bool isSignatureValidationAllowed; // The pre validation hooks for this function selector. EnumerableSet.Bytes32Set preValidationHooks; } @@ -51,7 +52,7 @@ struct AccountStorage { mapping(address => PluginData) pluginData; // Execution functions and their associated functions mapping(bytes4 => SelectorData) selectorData; - mapping(FunctionReference validationFunction => ValidationData) validationData; + mapping(bytes32 validationId => ValidationData) validationData; mapping(address caller => mapping(bytes4 selector => bool)) callPermitted; // For ERC165 introspection mapping(bytes4 => uint256) supportedIfaces; diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index 9e73e306..a70bc76b 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -6,28 +6,52 @@ import {EnumerableSet} from "@openzeppelin/contracts/utils/structs/EnumerableSet import {IPlugin} from "../interfaces/IPlugin.sol"; import {FunctionReference} from "../interfaces/IPluginManager.sol"; import {FunctionReferenceLib} from "../helpers/FunctionReferenceLib.sol"; -import {AccountStorage, getAccountStorage, toSetValue, toFunctionReference} from "./AccountStorage.sol"; +import { + AccountStorage, + getAccountStorage, + toSetValue, + toFunctionReference, + ValidationData +} from "./AccountStorage.sol"; // Temporary additional functions for a user-controlled install flow for validation functions. abstract contract PluginManager2 { using EnumerableSet for EnumerableSet.Bytes32Set; + event ValidationUpdated(bytes32 validationId); + event ValidationUninstalled(bytes32 validationId); + error DefaultValidationAlreadySet(FunctionReference validationFunction); error PreValidationAlreadySet(FunctionReference validationFunction, FunctionReference preValidationFunction); + // TODO to be renamed once PR https://github.com/erc6900/reference-implementation/pull/85/files merged + error ValidationAlreadySetNew(bytes32 validationId); error ValidationAlreadySet(bytes4 selector, FunctionReference validationFunction); error ValidationNotSet(bytes4 selector, FunctionReference validationFunction); function _installValidation( + bytes32 validationIdToUpdate, FunctionReference validationFunction, bool isDefault, + bool isSignatureValidationAllowed, bytes4[] memory selectors, bytes calldata installData, bytes memory preValidationHooks - ) - // TODO: flag for signature validation - internal - { + ) internal returns (bytes32 validationId) { AccountStorage storage _storage = getAccountStorage(); + validationId = validationIdToUpdate; + + if (validationId == bytes32(0)) { + validationId = keccak256(abi.encode(validationFunction, isDefault, installData)); + + if (FunctionReferenceLib.notEmpty(_storage.validationData[validationId].validationFunction)) { + revert ValidationAlreadySetNew(validationId); + } + } + + // TODO: all fields can be updated on the validation except selectors and hooks are addition only, to + // update, require uninstall and reinstall of validation + + _storage.validationData[validationId].isSignatureValidationAllowed = isSignatureValidationAllowed; if (preValidationHooks.length > 0) { (FunctionReference[] memory preValidationFunctions, bytes[] memory initDatas) = @@ -37,9 +61,7 @@ abstract contract PluginManager2 { FunctionReference preValidationFunction = preValidationFunctions[i]; if ( - !_storage.validationData[validationFunction].preValidationHooks.add( - toSetValue(preValidationFunction) - ) + !_storage.validationData[validationId].preValidationHooks.add(toSetValue(preValidationFunction)) ) { revert PreValidationAlreadySet(validationFunction, preValidationFunction); } @@ -51,13 +73,6 @@ abstract contract PluginManager2 { } } - if (isDefault) { - if (_storage.validationData[validationFunction].isDefault) { - revert DefaultValidationAlreadySet(validationFunction); - } - _storage.validationData[validationFunction].isDefault = true; - } - for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; if (!_storage.selectorData[selector].validations.add(toSetValue(validationFunction))) { @@ -69,24 +84,25 @@ abstract contract PluginManager2 { (address plugin,) = FunctionReferenceLib.unpack(validationFunction); IPlugin(plugin).onInstall(installData); } + emit ValidationUpdated(validationId); } function _uninstallValidation( - FunctionReference validationFunction, + bytes32 validationId, bytes4[] calldata selectors, bytes calldata uninstallData, bytes calldata preValidationHookUninstallData ) internal { AccountStorage storage _storage = getAccountStorage(); + ValidationData storage validationData = _storage.validationData[validationId]; - _storage.validationData[validationFunction].isDefault = false; - _storage.validationData[validationFunction].isSignatureValidation = false; + validationData.isDefault = false; + validationData.isSignatureValidationAllowed = false; bytes[] memory preValidationHookUninstallDatas = abi.decode(preValidationHookUninstallData, (bytes[])); // Clear pre validation hooks - EnumerableSet.Bytes32Set storage preValidationHooks = - _storage.validationData[validationFunction].preValidationHooks; + EnumerableSet.Bytes32Set storage preValidationHooks = validationData.preValidationHooks; while (preValidationHooks.length() > 0) { FunctionReference preValidationFunction = toFunctionReference(preValidationHooks.at(0)); preValidationHooks.remove(toSetValue(preValidationFunction)); @@ -101,14 +117,16 @@ abstract contract PluginManager2 { // TODO: consider enforcing this from user-supplied install config. for (uint256 i = 0; i < selectors.length; ++i) { bytes4 selector = selectors[i]; - if (!_storage.selectorData[selector].validations.remove(toSetValue(validationFunction))) { - revert ValidationNotSet(selector, validationFunction); + if (!_storage.selectorData[selector].validations.remove(toSetValue(validationData.validationFunction))) + { + revert ValidationNotSet(selector, validationData.validationFunction); } } if (uninstallData.length > 0) { - (address plugin,) = FunctionReferenceLib.unpack(validationFunction); + (address plugin,) = FunctionReferenceLib.unpack(validationData.validationFunction); IPlugin(plugin).onUninstall(uninstallData); } + emit ValidationUninstalled(validationId); } } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 169b17a3..56a31dc9 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -25,7 +25,8 @@ import { SelectorData, toSetValue, toFunctionReference, - toExecutionHook + toExecutionHook, + ValidationData } from "./AccountStorage.sol"; import {AccountStorageInitializable} from "./AccountStorageInitializable.sol"; import {PluginManagerInternals} from "./PluginManagerInternals.sol"; @@ -192,13 +193,13 @@ contract UpgradeableModularAccount is bytes4 execSelector = bytes4(data[:4]); // Revert if the provided `authorization` less than 21 bytes long, rather than right-padding. - FunctionReference runtimeValidationFunction = FunctionReference.wrap(bytes21(authorization[:21])); + bytes32 validationId = bytes32(authorization[:32]); // Check if the runtime validation function is allowed to be called - bool isDefaultValidation = uint8(authorization[21]) == 1; - _checkIfValidationApplies(execSelector, runtimeValidationFunction, isDefaultValidation); + bool isDefaultValidation = uint8(authorization[32]) == 1; + _checkIfValidationApplies(execSelector, validationId, isDefaultValidation); - _doRuntimeValidation(runtimeValidationFunction, data, authorization[22:]); + _doRuntimeValidation(validationId, data, authorization[33:]); // If runtime validation passes, execute the call @@ -247,35 +248,54 @@ contract UpgradeableModularAccount is /// with user install configs. /// @dev This function is only callable once, and only by the EntryPoint. - function initializeDefaultValidation(FunctionReference validationFunction, bytes calldata installData) - external - initializer - { - _installValidation(validationFunction, true, new bytes4[](0), installData, bytes("")); + function initializeDefaultValidation( + FunctionReference validationFunction, + bool isSignatureValidationAllowed, + bytes calldata installData + ) external initializer returns (bytes32 validationId) { + validationId = _installValidation( + bytes32(0), + validationFunction, + true, + isSignatureValidationAllowed, + new bytes4[](0), + installData, + bytes("") + ); emit ModularAccountInitialized(_ENTRY_POINT); } /// @inheritdoc IPluginManager /// @notice May be validated by a default validation. function installValidation( + bytes32 validationIdToUpdate, FunctionReference validationFunction, bool isDefault, + bool isSignatureValidationAllowed, bytes4[] memory selectors, bytes calldata installData, bytes calldata preValidationHooks - ) external wrapNativeFunction { - _installValidation(validationFunction, isDefault, selectors, installData, preValidationHooks); + ) external wrapNativeFunction returns (bytes32 validationId) { + validationId = _installValidation( + validationIdToUpdate, + validationFunction, + isDefault, + isSignatureValidationAllowed, + selectors, + installData, + preValidationHooks + ); } /// @inheritdoc IPluginManager /// @notice May be validated by a default validation. function uninstallValidation( - FunctionReference validationFunction, + bytes32 validationId, bytes4[] calldata selectors, bytes calldata uninstallData, bytes calldata preValidationHookUninstallData ) external wrapNativeFunction { - _uninstallValidation(validationFunction, selectors, uninstallData, preValidationHookUninstallData); + _uninstallValidation(validationId, selectors, uninstallData, preValidationHookUninstallData); } /// @notice ERC165 introspection @@ -308,15 +328,17 @@ contract UpgradeableModularAccount is function isValidSignature(bytes32 hash, bytes calldata signature) public view override returns (bytes4) { AccountStorage storage _storage = getAccountStorage(); - FunctionReference sigValidation = FunctionReference.wrap(bytes21(signature)); + bytes32 validationId = bytes32(signature); + + ValidationData storage validationData = _storage.validationData[validationId]; - (address plugin, uint8 functionId) = sigValidation.unpack(); - if (!_storage.validationData[sigValidation].isSignatureValidation) { + (address plugin, uint8 functionId) = validationData.validationFunction.unpack(); + if (!validationData.isSignatureValidationAllowed) { revert SignatureValidationInvalid(plugin, functionId); } if ( - IValidation(plugin).validateSignature(functionId, msg.sender, hash, signature[21:]) + IValidation(plugin).validateSignature(validationId, functionId, msg.sender, hash, signature[32:]) == _1271_MAGIC_VALUE ) { return _1271_MAGIC_VALUE; @@ -344,27 +366,26 @@ contract UpgradeableModularAccount is } bytes4 selector = bytes4(userOp.callData); - // 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; + // Revert if the provided `authorization` less than 32 bytes long, rather than right-padding. + bytes32 validationId = bytes32(userOp.signature[:32]); + bool isDefaultValidation = uint8(userOp.signature[33]) == 1; - _checkIfValidationApplies(selector, userOpValidationFunction, isDefaultValidation); + _checkIfValidationApplies(selector, validationId, isDefaultValidation); - validationData = - _doUserOpValidation(selector, userOpValidationFunction, userOp, userOp.signature[22:], userOpHash); + validationData = _doUserOpValidation(selector, validationId, userOp, userOp.signature[33:], userOpHash); } // To support gas estimation, we don't fail early when the failure is caused by a signature failure function _doUserOpValidation( bytes4 selector, - FunctionReference userOpValidationFunction, + bytes32 validationId, PackedUserOperation memory userOp, bytes calldata signature, bytes32 userOpHash ) internal returns (uint256 validationData) { userOp.signature = signature; - if (userOpValidationFunction.isEmpty()) { + if (validationId == bytes32(0)) { // If the validation function is empty, then the call cannot proceed. revert UserOpValidationFunctionMissing(selector); } @@ -373,7 +394,7 @@ contract UpgradeableModularAccount is // Do preUserOpValidation hooks EnumerableSet.Bytes32Set storage preUserOpValidationHooks = - getAccountStorage().validationData[userOpValidationFunction].preValidationHooks; + getAccountStorage().validationData[validationId].preValidationHooks; uint256 preUserOpValidationHooksLength = preUserOpValidationHooks.length(); for (uint256 i = 0; i < preUserOpValidationHooksLength; ++i) { @@ -392,8 +413,10 @@ contract UpgradeableModularAccount is // Run the user op validationFunction { - (address plugin, uint8 functionId) = userOpValidationFunction.unpack(); - currentValidationData = IValidation(plugin).validateUserOp(functionId, userOp, userOpHash); + (address plugin, uint8 functionId) = + getAccountStorage().validationData[validationId].validationFunction.unpack(); + currentValidationData = + IValidation(plugin).validateUserOp(validationId, functionId, userOp, userOpHash); if (preUserOpValidationHooksLength != 0) { // If we have other validation data we need to coalesce with @@ -404,14 +427,12 @@ contract UpgradeableModularAccount is } } - function _doRuntimeValidation( - FunctionReference runtimeValidationFunction, - bytes calldata callData, - bytes calldata authorizationData - ) internal { + function _doRuntimeValidation(bytes32 validationId, bytes calldata callData, bytes calldata authorizationData) + internal + { // run all preRuntimeValidation hooks EnumerableSet.Bytes32Set storage preRuntimeValidationHooks = - getAccountStorage().validationData[runtimeValidationFunction].preValidationHooks; + getAccountStorage().validationData[validationId].preValidationHooks; uint256 preRuntimeValidationHooksLength = preRuntimeValidationHooks.length(); for (uint256 i = 0; i < preRuntimeValidationHooksLength; ++i) { @@ -430,9 +451,12 @@ contract UpgradeableModularAccount is } } - (address plugin, uint8 functionId) = runtimeValidationFunction.unpack(); + (address plugin, uint8 functionId) = + getAccountStorage().validationData[validationId].validationFunction.unpack(); - try IValidation(plugin).validateRuntime(functionId, msg.sender, msg.value, callData, authorizationData) + try IValidation(plugin).validateRuntime( + validationId, functionId, msg.sender, msg.value, callData, authorizationData + ) // forgefmt: disable-start // solhint-disable-next-line no-empty-blocks {} catch (bytes memory revertReason) { @@ -519,20 +543,21 @@ 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 - { + function _checkIfValidationApplies(bytes4 selector, bytes32 validationId, 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.validationData[validationFunction].isDefault) { + if (!_defaultValidationAllowed(selector) || !_storage.validationData[validationId].isDefault) { revert UserOpValidationFunctionMissing(selector); } } else { // Not default validation, but per-selector - if (!getAccountStorage().selectorData[selector].validations.contains(toSetValue(validationFunction))) { + if ( + !getAccountStorage().selectorData[selector].validations.contains( + toSetValue(_storage.validationData[validationId].validationFunction) + ) + ) { revert UserOpValidationFunctionMissing(selector); } } diff --git a/src/interfaces/IAccountLoupe.sol b/src/interfaces/IAccountLoupe.sol index 490b216c..8f0a850b 100644 --- a/src/interfaces/IAccountLoupe.sol +++ b/src/interfaces/IAccountLoupe.sol @@ -29,9 +29,9 @@ interface IAccountLoupe { function getExecutionHooks(bytes4 selector) external view returns (ExecutionHook[] memory); /// @notice Get the pre user op and runtime validation hooks associated with a selector. - /// @param validationFunction The validation function to get the hooks for. + /// @param validationId The validationId of the validation function to get the hooks for. /// @return preValidationHooks The pre validation hooks for this selector. - function getPreValidationHooks(FunctionReference validationFunction) + function getPreValidationHooks(bytes32 validationId) external view returns (FunctionReference[] memory preValidationHooks); diff --git a/src/interfaces/IPluginManager.sol b/src/interfaces/IPluginManager.sol index 717e1fa0..a45a52bc 100644 --- a/src/interfaces/IPluginManager.sol +++ b/src/interfaces/IPluginManager.sol @@ -28,26 +28,32 @@ interface IPluginManager { /// validation. /// TODO: remove or update. /// @dev This does not validate anything against the manifest - the caller must ensure validity. + /// @param validationIdToUpdate The optional validationId if the tx is to update a validation function like + /// adding pre-validation-hooks and/or add selectors. /// @param validationFunction The validation function to install. /// @param isDefault Whether the validation function applies for all selectors in the default pool. + /// @param isSignatureValidationAllowed Whether the validation function can validation signature, can be + /// updated. /// @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( + bytes32 validationIdToUpdate, FunctionReference validationFunction, bool isDefault, + bool isSignatureValidationAllowed, bytes4[] memory selectors, bytes calldata installData, bytes calldata preValidationHooks - ) external; + ) external returns (bytes32 validationId); /// @notice Uninstall a validation function from a set of execution selectors. /// TODO: remove or update. - /// @param validationFunction The validation function to uninstall. + /// @param validationId The validationId 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( - FunctionReference validationFunction, + bytes32 validationId, bytes4[] calldata selectors, bytes calldata uninstallData, bytes calldata preValidationHookUninstallData diff --git a/src/interfaces/IValidation.sol b/src/interfaces/IValidation.sol index b3adcd3d..e9e91152 100644 --- a/src/interfaces/IValidation.sol +++ b/src/interfaces/IValidation.sol @@ -9,21 +9,27 @@ interface IValidation is IPlugin { /// @notice Run the user operation validationFunction specified by the `functionId`. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. + /// @param validationId The validationId for the account. /// @param userOp The user operation. /// @param userOpHash The user operation hash. /// @return Packed validation data for validAfter (6 bytes), validUntil (6 bytes), and authorizer (20 bytes). - function validateUserOp(uint8 functionId, PackedUserOperation calldata userOp, bytes32 userOpHash) - external - returns (uint256); + function validateUserOp( + bytes32 validationId, + uint8 functionId, + PackedUserOperation calldata userOp, + bytes32 userOpHash + ) external returns (uint256); /// @notice Run the runtime validationFunction specified by the `functionId`. /// @dev To indicate the entire call should revert, the function MUST revert. + /// @param validationId The validationId for the account. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. /// @param sender The caller address. /// @param value The call value. /// @param data The calldata sent. function validateRuntime( + bytes32 validationId, uint8 functionId, address sender, uint256 value, @@ -33,14 +39,18 @@ interface IValidation is IPlugin { /// @notice Validates a signature using ERC-1271. /// @dev To indicate the entire call should revert, the function MUST revert. + /// @param validationId The validationId for the account. /// @param functionId An identifier that routes the call to different internal implementations, should there be /// more than one. /// @param sender the address that sent the ERC-1271 request to the smart account /// @param hash the hash of the ERC-1271 request /// @param signature the signature of the ERC-1271 request /// @return the ERC-1271 `MAGIC_VALUE` if the signature is valid, or 0xFFFFFFFF if invalid. - function validateSignature(uint8 functionId, address sender, bytes32 hash, bytes calldata signature) - external - view - returns (bytes4); + function validateSignature( + bytes32 validationId, + uint8 functionId, + address sender, + bytes32 hash, + bytes calldata signature + ) external view returns (bytes4); } diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index dbdd41b2..0567bc69 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -79,7 +79,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { } /// @inheritdoc IValidation - function validateRuntime(uint8 functionId, address sender, uint256, bytes calldata, bytes calldata) + function validateRuntime(bytes32, uint8 functionId, address sender, uint256, bytes calldata, bytes calldata) external view override @@ -95,7 +95,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { } /// @inheritdoc IValidation - function validateUserOp(uint8 functionId, PackedUserOperation calldata userOp, bytes32 userOpHash) + function validateUserOp(bytes32, uint8 functionId, PackedUserOperation calldata userOp, bytes32 userOpHash) external view override @@ -123,7 +123,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { /// validation used in `validateUserOp`, this does///*not** wrap the digest in /// an "Ethereum Signed Message" envelope before checking the signature in /// the EOA-owner case. - function validateSignature(uint8 functionId, address, bytes32 digest, bytes calldata signature) + function validateSignature(bytes32, uint8 functionId, address, bytes32 digest, bytes calldata signature) external view override diff --git a/src/plugins/validation/EcdsaValidationPlugin.sol b/src/plugins/validation/EcdsaValidationPlugin.sol new file mode 100644 index 00000000..61fc6e87 --- /dev/null +++ b/src/plugins/validation/EcdsaValidationPlugin.sol @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; + +import { + IPlugin, + ManifestAssociatedFunction, + ManifestAssociatedFunctionType, + ManifestFunction, + PluginManifest, + PluginMetadata, + SelectorPermission +} from "../../interfaces/IPlugin.sol"; +import {IValidation} from "../../interfaces/IValidation.sol"; +import {BasePlugin} from "../BasePlugin.sol"; +import {IEcdsaValidationPlugin} from "./IEcdsaValidationPlugin.sol"; + +contract EcdsaValidationPlugin is IEcdsaValidationPlugin, BasePlugin { + using ECDSA for bytes32; + using MessageHashUtils for bytes32; + + string public constant NAME = "Ecdsa Validation Plugin"; + string public constant VERSION = "1.0.0"; + string public constant AUTHOR = "ERC-6900 Authors"; + + uint256 internal constant _SIG_VALIDATION_PASSED = 0; + uint256 internal constant _SIG_VALIDATION_FAILED = 1; + + mapping(bytes32 validationId => mapping(address account => address)) public signer; + + /// @inheritdoc IEcdsaValidationPlugin + function signerOf(bytes32 validationId, address account) external view returns (address) { + return signer[validationId][account]; + } + + /// @inheritdoc IEcdsaValidationPlugin + function transferSigner(bytes32 validationId, address newSigner) external { + _transferSigner(validationId, newSigner); + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Plugin interface functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + /// @inheritdoc IPlugin + function pluginManifest() external pure override returns (PluginManifest memory) { + PluginManifest memory manifest; + return manifest; + } + + /// @inheritdoc IPlugin + function pluginMetadata() external pure virtual override returns (PluginMetadata memory) { + PluginMetadata memory metadata; + metadata.name = NAME; + metadata.version = VERSION; + metadata.author = AUTHOR; + return metadata; + } + + /// @inheritdoc IPlugin + function onInstall(bytes calldata data) external override { + (bytes32 validationId, address newSigner) = abi.decode(data, (bytes32, address)); + _transferSigner(validationId, newSigner); + } + + /// @inheritdoc IPlugin + function onUninstall(bytes calldata data) external override { + // ToDo: what does it mean in the world of composable validation world to uninstall one type of validation + // We can either get rid of all Ecdsa signers. What about the nested ones? + _transferSigner(abi.decode(data, (bytes32)), address(0)); + } + + /// @inheritdoc IValidation + function validateUserOp(bytes32 validationId, uint8, PackedUserOperation calldata userOp, bytes32 userOpHash) + external + view + override + returns (uint256) + { + // Validate the user op signature against the owner. + (address sigSigner,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); + if (sigSigner == address(0) || sigSigner != signer[validationId][msg.sender]) { + return _SIG_VALIDATION_FAILED; + } + return _SIG_VALIDATION_PASSED; + } + + /// @inheritdoc IValidation + function validateRuntime(bytes32 validationId, uint8, address sender, uint256, bytes calldata, bytes calldata) + external + view + override + { + // Validate that the sender is the owner of the account or self. + if (sender != signer[validationId][msg.sender] && sender != msg.sender) { + revert NotAuthorized(); + } + return; + } + + /// @inheritdoc IValidation + function validateSignature(bytes32, uint8, address, bytes32, bytes calldata) + external + pure + override + returns (bytes4) + { + revert NotImplemented(); + } + + // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ + // ┃ Internal / Private functions ┃ + // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ + + function _transferSigner(bytes32 validationId, address newSigner) internal { + address previousSigner = signer[validationId][msg.sender]; + signer[validationId][msg.sender] = newSigner; + emit SignerTransferred(msg.sender, validationId, previousSigner, newSigner); + } +} diff --git a/src/plugins/validation/IEcdsaValidationPlugin.sol b/src/plugins/validation/IEcdsaValidationPlugin.sol new file mode 100644 index 00000000..0cfc5dd5 --- /dev/null +++ b/src/plugins/validation/IEcdsaValidationPlugin.sol @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {IValidation} from "../../interfaces/IValidation.sol"; + +interface IEcdsaValidationPlugin is IValidation { + /// @notice This event is emitted when Signer of the account's validation changes. + /// @param account The account whose validation Signer changed. + /// @param validationId The validationId for the account and the signer. + /// @param previousSigner The address of the previous signer. + /// @param newSigner The address of the new signer. + event SignerTransferred( + address indexed account, bytes32 validationId, address indexed previousSigner, address indexed newSigner + ); + + error NotAuthorized(); + + /// @notice Transfer Signer of the account's validation to `newSigner`. + /// @dev This function is installed on the account as part of plugin installation, and should + /// only be called from an account. + /// @param validationId The validationId for the account and the signer. + /// @param newSigner The address of the new signer. + function transferSigner(bytes32 validationId, address newSigner) external; + + /// @notice Get the signer of the `account`'s validation. + /// @dev This function is not installed on the account, and can be called by anyone. + /// @param validationId The validationId for the account and the signer. + /// @param account The account to get the signer of. + /// @return The address of the signer. + function signerOf(bytes32 validationId, address account) external view returns (address); +} From 52318dee71e224a5ba8fe709bdaea9c16d46ead3 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Thu, 27 Jun 2024 15:09:48 -0700 Subject: [PATCH 2/4] comment out validation installation from install plugin path --- src/account/PluginManagerInternals.sol | 56 +++++++++++++------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/src/account/PluginManagerInternals.sol b/src/account/PluginManagerInternals.sol index 2ec06f81..7ac22675 100644 --- a/src/account/PluginManagerInternals.sol +++ b/src/account/PluginManagerInternals.sol @@ -220,20 +220,20 @@ abstract contract PluginManagerInternals is IPluginManager { _storage.callPermitted[plugin][manifest.permittedExecutionSelectors[i]] = true; } - length = manifest.validationFunctions.length; - for (uint256 i = 0; i < length; ++i) { - ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; - _addValidationFunction( - mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies) - ); - } - - length = manifest.signatureValidationFunctions.length; - for (uint256 i = 0; i < length; ++i) { - FunctionReference signatureValidationFunction = - FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); - _storage.validationData[signatureValidationFunction].isSignatureValidation = true; - } + // length = manifest.validationFunctions.length; + // for (uint256 i = 0; i < length; ++i) { + // ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; + // _addValidationFunction( + // mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies) + // ); + // } + + // length = manifest.signatureValidationFunctions.length; + // for (uint256 i = 0; i < length; ++i) { + // FunctionReference signatureValidationFunction = + // FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); + // _storage.validationData[signatureValidationFunction].isSignatureValidation = true; + // } length = manifest.executionHooks.length; for (uint256 i = 0; i < length; ++i) { @@ -298,20 +298,20 @@ abstract contract PluginManagerInternals is IPluginManager { _removeExecHooks(mh.executionSelector, hookFunction, mh.isPreHook, mh.isPostHook); } - length = manifest.signatureValidationFunctions.length; - for (uint256 i = 0; i < length; ++i) { - FunctionReference signatureValidationFunction = - FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); - _storage.validationData[signatureValidationFunction].isSignatureValidation = false; - } - - length = manifest.validationFunctions.length; - for (uint256 i = 0; i < length; ++i) { - ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; - _removeValidationFunction( - mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies) - ); - } + // length = manifest.signatureValidationFunctions.length; + // for (uint256 i = 0; i < length; ++i) { + // FunctionReference signatureValidationFunction = + // FunctionReferenceLib.pack(plugin, manifest.signatureValidationFunctions[i]); + // _storage.validationData[signatureValidationFunction].isSignatureValidation = false; + // } + + // length = manifest.validationFunctions.length; + // for (uint256 i = 0; i < length; ++i) { + // ManifestAssociatedFunction memory mv = manifest.validationFunctions[i]; + // _removeValidationFunction( + // mv.executionSelector, _resolveManifestFunction(mv.associatedFunction, plugin, dependencies) + // ); + // } length = manifest.permittedExecutionSelectors.length; for (uint256 i = 0; i < length; ++i) { From e504cfb58d9044588abe6bee5f96262633fcc90c Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Thu, 27 Jun 2024 16:13:57 -0700 Subject: [PATCH 3/4] add missed field and fix validation oninstall data format --- src/account/PluginManager2.sol | 4 +++- src/account/UpgradeableModularAccount.sol | 2 +- src/plugins/owner/SingleOwnerPlugin.sol | 4 +++- src/plugins/validation/EcdsaValidationPlugin.sol | 3 ++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/account/PluginManager2.sol b/src/account/PluginManager2.sol index a70bc76b..6bb563cb 100644 --- a/src/account/PluginManager2.sol +++ b/src/account/PluginManager2.sol @@ -51,6 +51,8 @@ abstract contract PluginManager2 { // TODO: all fields can be updated on the validation except selectors and hooks are addition only, to // update, require uninstall and reinstall of validation + _storage.validationData[validationId].validationFunction = validationFunction; + _storage.validationData[validationId].isDefault = isDefault; _storage.validationData[validationId].isSignatureValidationAllowed = isSignatureValidationAllowed; if (preValidationHooks.length > 0) { @@ -82,7 +84,7 @@ abstract contract PluginManager2 { if (installData.length > 0) { (address plugin,) = FunctionReferenceLib.unpack(validationFunction); - IPlugin(plugin).onInstall(installData); + IPlugin(plugin).onInstall(abi.encode(validationId, installData)); } emit ValidationUpdated(validationId); } diff --git a/src/account/UpgradeableModularAccount.sol b/src/account/UpgradeableModularAccount.sol index 56a31dc9..a5c7a82e 100644 --- a/src/account/UpgradeableModularAccount.sol +++ b/src/account/UpgradeableModularAccount.sol @@ -368,7 +368,7 @@ contract UpgradeableModularAccount is // Revert if the provided `authorization` less than 32 bytes long, rather than right-padding. bytes32 validationId = bytes32(userOp.signature[:32]); - bool isDefaultValidation = uint8(userOp.signature[33]) == 1; + bool isDefaultValidation = uint8(userOp.signature[32]) == 1; _checkIfValidationApplies(selector, validationId, isDefaultValidation); diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 0567bc69..61bba496 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -70,7 +70,9 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { /// @inheritdoc IPlugin function onInstall(bytes calldata data) external override { - _transferOwnership(abi.decode(data, (address))); + (, bytes memory newOwnerBytes) = abi.decode(data, (bytes32, bytes)); + (address newOwner) = abi.decode(newOwnerBytes, (address)); + _transferOwnership(newOwner); } /// @inheritdoc IPlugin diff --git a/src/plugins/validation/EcdsaValidationPlugin.sol b/src/plugins/validation/EcdsaValidationPlugin.sol index 61fc6e87..82c0ff56 100644 --- a/src/plugins/validation/EcdsaValidationPlugin.sol +++ b/src/plugins/validation/EcdsaValidationPlugin.sol @@ -62,7 +62,8 @@ contract EcdsaValidationPlugin is IEcdsaValidationPlugin, BasePlugin { /// @inheritdoc IPlugin function onInstall(bytes calldata data) external override { - (bytes32 validationId, address newSigner) = abi.decode(data, (bytes32, address)); + (bytes32 validationId, bytes memory newSignerBytes) = abi.decode(data, (bytes32, bytes)); + (address newSigner) = abi.decode(newSignerBytes, (address)); _transferSigner(validationId, newSigner); } From 6a320ece00063b7b8ece65a2cb4dfc444aae9cb3 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Thu, 27 Jun 2024 16:46:11 -0700 Subject: [PATCH 4/4] add ecdsa validation tests and fix other tests --- test/account/AccountLoupe.t.sol | 24 +++--- test/account/DefaultValidationTest.t.sol | 10 +-- test/account/MultiValidation.t.sol | 32 ++++---- test/account/UpgradeableModularAccount.t.sol | 17 ++-- test/account/ValidationIntersection.t.sol | 16 +++- test/libraries/FunctionReferenceLib.t.sol | 2 +- .../mocks/DefaultValidationFactoryFixture.sol | 10 ++- test/mocks/EcdsaValidationFactoryFixture.sol | 77 ++++++++++++++++++ test/mocks/plugins/ComprehensivePlugin.sol | 6 +- test/mocks/plugins/ReturnDataPluginMocks.sol | 13 ++- test/mocks/plugins/ValidationPluginMocks.sol | 15 +++- test/plugin/SingleOwnerPlugin.t.sol | 21 +++-- test/plugin/TokenReceiverPlugin.t.sol | 8 +- test/utils/AccountTestBase.sol | 15 ++-- test/utils/OptimizedTest.sol | 7 ++ test/validation/EcdsaValidationPlugin.t.sol | 80 +++++++++++++++++++ 16 files changed, 273 insertions(+), 80 deletions(-) create mode 100644 test/mocks/EcdsaValidationFactoryFixture.sol create mode 100644 test/validation/EcdsaValidationPlugin.t.sol diff --git a/test/account/AccountLoupe.t.sol b/test/account/AccountLoupe.t.sol index fa92ab00..cedb6257 100644 --- a/test/account/AccountLoupe.t.sol +++ b/test/account/AccountLoupe.t.sol @@ -16,6 +16,7 @@ contract AccountLoupeTest is AccountTestBase { ComprehensivePlugin public comprehensivePlugin; FunctionReference public ownerValidation; + bytes32 public ownerValidationId; event ReceivedCall(bytes msgData, uint256 msgValue); @@ -42,18 +43,22 @@ contract AccountLoupeTest is AccountTestBase { bytes[] memory installDatas = new bytes[](2); vm.prank(address(entryPoint)); - account1.installValidation( - ownerValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + ownerValidationId = account1.installValidation( + bytes32(0), + ownerValidation, + true, + true, + new bytes4[](0), + bytes(""), + abi.encode(preValidationHooks, installDatas) ); } function test_pluginLoupe_getInstalledPlugins_initial() public { address[] memory plugins = account1.getInstalledPlugins(); - assertEq(plugins.length, 2); - - assertEq(plugins[0], address(singleOwnerPlugin)); - assertEq(plugins[1], address(comprehensivePlugin)); + assertEq(plugins.length, 1); + assertEq(plugins[0], address(comprehensivePlugin)); } function test_pluginLoupe_getExecutionFunctionHandler_native() public { @@ -102,11 +107,6 @@ contract AccountLoupeTest is AccountTestBase { ) ) ); - - validations = account1.getValidations(account1.execute.selector); - - assertEq(validations.length, 1); - assertEq(FunctionReference.unwrap(validations[0]), FunctionReference.unwrap(ownerValidation)); } function test_pluginLoupe_getExecutionHooks() public { @@ -147,7 +147,7 @@ contract AccountLoupeTest is AccountTestBase { } function test_pluginLoupe_getValidationHooks() public { - FunctionReference[] memory hooks = account1.getPreValidationHooks(ownerValidation); + FunctionReference[] memory hooks = account1.getPreValidationHooks(ownerValidationId); assertEq(hooks.length, 2); assertEq( diff --git a/test/account/DefaultValidationTest.t.sol b/test/account/DefaultValidationTest.t.sol index fc93060d..096ddcb7 100644 --- a/test/account/DefaultValidationTest.t.sol +++ b/test/account/DefaultValidationTest.t.sol @@ -19,8 +19,6 @@ contract DefaultValidationTest is AccountTestBase { uint256 public constant CALL_GAS_LIMIT = 50000; uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; - FunctionReference public ownerValidation; - address public ethRecipient; function setUp() public { @@ -32,10 +30,6 @@ contract DefaultValidationTest is AccountTestBase { ethRecipient = makeAddr("ethRecipient"); vm.deal(ethRecipient, 1 wei); - - ownerValidation = FunctionReferenceLib.pack( - address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) - ); } function test_defaultValidation_userOp_simple() public { @@ -57,7 +51,7 @@ contract DefaultValidationTest 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, DEFAULT_VALIDATION, r, s, v); + userOp.signature = abi.encodePacked(validationId, DEFAULT_VALIDATION, r, s, v); PackedUserOperation[] memory userOps = new PackedUserOperation[](1); userOps[0] = userOp; @@ -74,7 +68,7 @@ contract DefaultValidationTest is AccountTestBase { vm.prank(owner1); account1.executeWithAuthorization( abi.encodeCall(UpgradeableModularAccount.execute, (ethRecipient, 1 wei, "")), - abi.encodePacked(ownerValidation, DEFAULT_VALIDATION) + abi.encodePacked(validationId, DEFAULT_VALIDATION) ); assertEq(ethRecipient.balance, 2 wei); diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index 9b22f5a0..eacd6fb8 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -15,12 +15,14 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; +import {EcdsaValidationPlugin} from "../../src/plugins/validation/EcdsaValidationPlugin.sol"; contract MultiValidationTest is AccountTestBase { using ECDSA for bytes32; using MessageHashUtils for bytes32; SingleOwnerPlugin public validator2; + EcdsaValidationPlugin public ecdsaValidationPlugin; address public owner2; uint256 public owner2Key; @@ -30,25 +32,23 @@ contract MultiValidationTest is AccountTestBase { function setUp() public { validator2 = new SingleOwnerPlugin(); + ecdsaValidationPlugin = new EcdsaValidationPlugin(); (owner2, owner2Key) = makeAddrAndKey("owner2"); } function test_overlappingValidationInstall() public { - bytes32 manifestHash = keccak256(abi.encode(validator2.pluginManifest())); vm.prank(address(entryPoint)); - account1.installPlugin(address(validator2), manifestHash, abi.encode(owner2), new FunctionReference[](0)); - - FunctionReference[] memory validations = new FunctionReference[](2); - validations[0] = FunctionReferenceLib.pack( - address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) + bytes4[] memory selectors = new bytes4[](1); + selectors[0] = IStandardExecutor.execute.selector; + FunctionReference validationFunction = FunctionReferenceLib.pack(address(ecdsaValidationPlugin), uint8(0)); + account1.installValidation( + bytes32(0), validationFunction, true, false, selectors, abi.encode(owner2), bytes("") ); - validations[1] = - FunctionReferenceLib.pack(address(validator2), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER)); - FunctionReference[] memory validations2 = account1.getValidations(IStandardExecutor.execute.selector); - assertEq(validations2.length, 2); - assertEq(FunctionReference.unwrap(validations2[0]), FunctionReference.unwrap(validations[0])); - assertEq(FunctionReference.unwrap(validations2[1]), FunctionReference.unwrap(validations[1])); + + FunctionReference[] memory validations = account1.getValidations(IStandardExecutor.execute.selector); + assertEq(validations.length, 1); + assertEq(FunctionReference.unwrap(validations[0]), FunctionReference.unwrap(validationFunction)); } function test_runtimeValidation_specify() public { @@ -68,9 +68,7 @@ contract MultiValidationTest is AccountTestBase { account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), abi.encodePacked( - address(validator2), - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), - SELECTOR_ASSOCIATED_VALIDATION + address(validator2), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), DEFAULT_VALIDATION ) ); @@ -78,9 +76,7 @@ contract MultiValidationTest is AccountTestBase { account1.executeWithAuthorization( abi.encodeCall(IStandardExecutor.execute, (address(0), 0, "")), abi.encodePacked( - address(validator2), - uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), - SELECTOR_ASSOCIATED_VALIDATION + address(validator2), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), DEFAULT_VALIDATION ) ); } diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e3d09e7f..0e789d7f 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -2,7 +2,6 @@ pragma solidity ^0.8.19; import {console} from "forge-std/Test.sol"; - import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; @@ -253,9 +252,8 @@ contract UpgradeableModularAccountTest is AccountTestBase { }); address[] memory plugins = IAccountLoupe(account1).getInstalledPlugins(); - assertEq(plugins.length, 2); - assertEq(plugins[0], address(singleOwnerPlugin)); - assertEq(plugins[1], address(tokenReceiverPlugin)); + assertEq(plugins.length, 1); + assertEq(plugins[0], address(tokenReceiverPlugin)); } function test_installPlugin_PermittedCallSelectorNotInstalled() public { @@ -343,8 +341,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { emit PluginUninstalled(address(plugin), true); IPluginManager(account1).uninstallPlugin({plugin: address(plugin), config: "", pluginUninstallData: ""}); address[] memory plugins = IAccountLoupe(account1).getInstalledPlugins(); - assertEq(plugins.length, 1); - assertEq(plugins[0], address(singleOwnerPlugin)); + assertEq(plugins.length, 0); } function test_uninstallPlugin_manifestParameter() public { @@ -368,8 +365,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { pluginUninstallData: "" }); address[] memory plugins = IAccountLoupe(account1).getInstalledPlugins(); - assertEq(plugins.length, 1); - assertEq(plugins[0], address(singleOwnerPlugin)); + assertEq(plugins.length, 0); } function test_uninstallPlugin_invalidManifestFails() public { @@ -395,9 +391,8 @@ contract UpgradeableModularAccountTest is AccountTestBase { pluginUninstallData: "" }); address[] memory plugins = IAccountLoupe(account1).getInstalledPlugins(); - assertEq(plugins.length, 2); - assertEq(plugins[0], address(singleOwnerPlugin)); - assertEq(plugins[1], address(plugin)); + assertEq(plugins.length, 1); + assertEq(plugins[0], address(plugin)); } function _installPluginWithExecHooks() internal returns (MockPlugin plugin) { diff --git a/test/account/ValidationIntersection.t.sol b/test/account/ValidationIntersection.t.sol index 877f8ff9..0e2fe7d0 100644 --- a/test/account/ValidationIntersection.t.sol +++ b/test/account/ValidationIntersection.t.sol @@ -67,7 +67,13 @@ contract ValidationIntersectionTest is AccountTestBase { }); bytes[] memory installDatas = new bytes[](1); account1.installValidation( - oneHookValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + bytes32(0), + oneHookValidation, + true, + true, + new bytes4[](0), + bytes(""), + abi.encode(preValidationHooks, installDatas) ); account1.installPlugin({ plugin: address(twoHookPlugin), @@ -87,7 +93,13 @@ contract ValidationIntersectionTest is AccountTestBase { }); installDatas = new bytes[](2); account1.installValidation( - twoHookValidation, true, new bytes4[](0), bytes(""), abi.encode(preValidationHooks, installDatas) + bytes32(0), + twoHookValidation, + true, + true, + new bytes4[](0), + bytes(""), + abi.encode(preValidationHooks, installDatas) ); vm.stopPrank(); } diff --git a/test/libraries/FunctionReferenceLib.t.sol b/test/libraries/FunctionReferenceLib.t.sol index 6471fbd0..7c5faee5 100644 --- a/test/libraries/FunctionReferenceLib.t.sol +++ b/test/libraries/FunctionReferenceLib.t.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {Test} from "forge-std/Test.sol"; +import {Test, console} from "forge-std/Test.sol"; import {FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; import {FunctionReference} from "../../src/interfaces/IPluginManager.sol"; diff --git a/test/mocks/DefaultValidationFactoryFixture.sol b/test/mocks/DefaultValidationFactoryFixture.sol index a4836ad8..9b5588cb 100644 --- a/test/mocks/DefaultValidationFactoryFixture.sol +++ b/test/mocks/DefaultValidationFactoryFixture.sol @@ -45,7 +45,10 @@ contract DefaultValidationFactoryFixture is OptimizedTest { * 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) { + function createAccount(address owner, uint256 salt) + public + returns (UpgradeableModularAccount account, bytes32 validationId) + { address addr = Create2.computeAddress(getSalt(owner, salt), _PROXY_BYTECODE_HASH); // short circuit if exists @@ -55,15 +58,16 @@ contract DefaultValidationFactoryFixture is OptimizedTest { new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); // point proxy to actual implementation and init plugins - UpgradeableModularAccount(payable(addr)).initializeDefaultValidation( + validationId = UpgradeableModularAccount(payable(addr)).initializeDefaultValidation( FunctionReferenceLib.pack( address(singleOwnerPlugin), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER) ), + true, pluginInstallData ); } - return UpgradeableModularAccount(payable(addr)); + account = UpgradeableModularAccount(payable(addr)); } /** diff --git a/test/mocks/EcdsaValidationFactoryFixture.sol b/test/mocks/EcdsaValidationFactoryFixture.sol new file mode 100644 index 00000000..584d2c8b --- /dev/null +++ b/test/mocks/EcdsaValidationFactoryFixture.sol @@ -0,0 +1,77 @@ +// 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 {FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; +import {EcdsaValidationPlugin} from "../../src/plugins/validation/EcdsaValidationPlugin.sol"; + +import {OptimizedTest} from "../utils/OptimizedTest.sol"; + +contract EcdsaValidationFactoryFixture is OptimizedTest { + UpgradeableModularAccount public accountImplementation; + EcdsaValidationPlugin public ecdsaValidationPlugin; + bytes32 private immutable _PROXY_BYTECODE_HASH; + + uint32 public constant UNSTAKE_DELAY = 1 weeks; + + IEntryPoint public entryPoint; + + address public self; + + constructor(IEntryPoint _entryPoint, EcdsaValidationPlugin _ecdsaValidationPlugin) { + entryPoint = _entryPoint; + accountImplementation = _deployUpgradeableModularAccount(_entryPoint); + _PROXY_BYTECODE_HASH = keccak256( + abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) + ); + ecdsaValidationPlugin = _ecdsaValidationPlugin; + self = address(this); + } + + /** + * 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 account, bytes32 validationId) + { + 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 + validationId = UpgradeableModularAccount(payable(addr)).initializeDefaultValidation( + FunctionReferenceLib.pack(address(ecdsaValidationPlugin), uint8(0)), true, pluginInstallData + ); + } + + account = 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/ComprehensivePlugin.sol b/test/mocks/plugins/ComprehensivePlugin.sol index 6ef654c7..6190c1a5 100644 --- a/test/mocks/plugins/ComprehensivePlugin.sol +++ b/test/mocks/plugins/ComprehensivePlugin.sol @@ -62,7 +62,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function validateUserOp(uint8 functionId, PackedUserOperation calldata, bytes32) + function validateUserOp(bytes32, uint8 functionId, PackedUserOperation calldata, bytes32) external pure override @@ -83,7 +83,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function validateRuntime(uint8 functionId, address, uint256, bytes calldata, bytes calldata) + function validateRuntime(bytes32, uint8 functionId, address, uint256, bytes calldata, bytes calldata) external pure override @@ -94,7 +94,7 @@ contract ComprehensivePlugin is IValidation, IValidationHook, IExecutionHook, Ba revert NotImplemented(); } - function validateSignature(uint8 functionId, address, bytes32, bytes calldata) + function validateSignature(bytes32, uint8 functionId, address, bytes32, bytes calldata) external pure returns (bytes4) diff --git a/test/mocks/plugins/ReturnDataPluginMocks.sol b/test/mocks/plugins/ReturnDataPluginMocks.sol index dae2c8e4..05232c9f 100644 --- a/test/mocks/plugins/ReturnDataPluginMocks.sol +++ b/test/mocks/plugins/ReturnDataPluginMocks.sol @@ -74,17 +74,24 @@ contract ResultConsumerPlugin is BasePlugin, IValidation { // Validation function implementations. We only care about the runtime validation function, to authorize // itself. - function validateUserOp(uint8, PackedUserOperation calldata, bytes32) external pure returns (uint256) { + function validateUserOp(bytes32, uint8, PackedUserOperation calldata, bytes32) + external + pure + returns (uint256) + { revert NotImplemented(); } - function validateRuntime(uint8, address sender, uint256, bytes calldata, bytes calldata) external view { + function validateRuntime(bytes32, uint8, address sender, uint256, bytes calldata, bytes calldata) + external + view + { if (sender != address(this)) { revert NotAuthorized(); } } - function validateSignature(uint8, address, bytes32, bytes calldata) external pure returns (bytes4) { + function validateSignature(bytes32, uint8, address, bytes32, bytes calldata) external pure returns (bytes4) { revert NotImplemented(); } diff --git a/test/mocks/plugins/ValidationPluginMocks.sol b/test/mocks/plugins/ValidationPluginMocks.sol index d5f75e99..0785561d 100644 --- a/test/mocks/plugins/ValidationPluginMocks.sol +++ b/test/mocks/plugins/ValidationPluginMocks.sol @@ -48,7 +48,7 @@ abstract contract MockBaseUserOpValidationPlugin is IValidation, IValidationHook revert NotImplemented(); } - function validateUserOp(uint8 functionId, PackedUserOperation calldata, bytes32) + function validateUserOp(bytes32, uint8 functionId, PackedUserOperation calldata, bytes32) external view override @@ -60,7 +60,12 @@ abstract contract MockBaseUserOpValidationPlugin is IValidation, IValidationHook revert NotImplemented(); } - function validateSignature(uint8, address, bytes32, bytes calldata) external pure override returns (bytes4) { + function validateSignature(bytes32, uint8, address, bytes32, bytes calldata) + external + pure + override + returns (bytes4) + { revert NotImplemented(); } @@ -71,7 +76,11 @@ abstract contract MockBaseUserOpValidationPlugin is IValidation, IValidationHook revert NotImplemented(); } - function validateRuntime(uint8, address, uint256, bytes calldata, bytes calldata) external pure override { + function validateRuntime(bytes32, uint8, address, uint256, bytes calldata, bytes calldata) + external + pure + override + { revert NotImplemented(); } } diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 41997591..1f37c1e6 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -114,11 +114,15 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(address(0), plugin.owner()); plugin.transferOwnership(owner1); assertEq(owner1, plugin.owner()); - plugin.validateRuntime(uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), owner1, 0, "", ""); + plugin.validateRuntime( + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), owner1, 0, "", "" + ); vm.startPrank(b); vm.expectRevert(ISingleOwnerPlugin.NotAuthorized.selector); - plugin.validateRuntime(uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), owner1, 0, "", ""); + plugin.validateRuntime( + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), owner1, 0, "", "" + ); } function testFuzz_validateUserOpSig(string memory salt, PackedUserOperation memory userOp) public { @@ -133,8 +137,9 @@ contract SingleOwnerPluginTest is OptimizedTest { userOp.signature = abi.encodePacked(r, s, v); // sig check should fail - uint256 success = - plugin.validateUserOp(uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), userOp, userOpHash); + uint256 success = plugin.validateUserOp( + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), userOp, userOpHash + ); assertEq(success, 1); // transfer ownership to signer @@ -142,7 +147,9 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(signer, plugin.owner()); // sig check should pass - success = plugin.validateUserOp(uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), userOp, userOpHash); + success = plugin.validateUserOp( + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), userOp, userOpHash + ); assertEq(success, 0); } @@ -156,6 +163,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should fail assertEq( plugin.validateSignature( + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), address(this), digest, @@ -171,6 +179,7 @@ contract SingleOwnerPluginTest is OptimizedTest { // sig check should pass assertEq( plugin.validateSignature( + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), address(this), digest, @@ -186,7 +195,7 @@ contract SingleOwnerPluginTest is OptimizedTest { bytes memory signature = contractOwner.sign(digest); assertEq( plugin.validateSignature( - uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), address(this), digest, signature + bytes32(0), uint8(ISingleOwnerPlugin.FunctionId.SIG_VALIDATION), address(this), digest, signature ), _1271_MAGIC_VALUE ); diff --git a/test/plugin/TokenReceiverPlugin.t.sol b/test/plugin/TokenReceiverPlugin.t.sol index 0e111020..91b42870 100644 --- a/test/plugin/TokenReceiverPlugin.t.sol +++ b/test/plugin/TokenReceiverPlugin.t.sol @@ -13,6 +13,7 @@ import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; import {MockERC721} from "../mocks/MockERC721.sol"; import {MockERC1155} from "../mocks/MockERC1155.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; +import {DefaultValidationFactoryFixture} from "../mocks/DefaultValidationFactoryFixture.sol"; contract TokenReceiverPluginTest is OptimizedTest, IERC1155Receiver { EntryPoint public entryPoint; @@ -27,6 +28,7 @@ contract TokenReceiverPluginTest is OptimizedTest, IERC1155Receiver { uint256[] public tokenIds; uint256[] public tokenAmts; uint256[] public zeroTokenAmts; + bytes32 public validationId; uint256 internal constant _TOKEN_AMOUNT = 1 ether; uint256 internal constant _TOKEN_ID = 0; @@ -34,9 +36,11 @@ contract TokenReceiverPluginTest is OptimizedTest, IERC1155Receiver { function setUp() public { entryPoint = new EntryPoint(); - MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, _deploySingleOwnerPlugin()); + // MSCAFactoryFixture factory = new MSCAFactoryFixture(entryPoint, _deploySingleOwnerPlugin()); + DefaultValidationFactoryFixture factory = + new DefaultValidationFactoryFixture(entryPoint, _deploySingleOwnerPlugin()); - acct = factory.createAccount(address(this), 0); + (acct, validationId) = factory.createAccount(address(this), 0); plugin = _deployTokenReceiverPlugin(); t0 = new MockERC721("t0", "t0"); diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 059e9cac..ad2cd587 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -10,6 +10,7 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; import {MSCAFactoryFixture} from "../mocks/MSCAFactoryFixture.sol"; +import {DefaultValidationFactoryFixture} from "../mocks/DefaultValidationFactoryFixture.sol"; /// @dev This contract handles common boilerplate setup for tests using UpgradeableModularAccount with /// SingleOwnerPlugin. @@ -17,11 +18,12 @@ abstract contract AccountTestBase is OptimizedTest { EntryPoint public entryPoint; address payable public beneficiary; SingleOwnerPlugin public singleOwnerPlugin; - MSCAFactoryFixture public factory; + DefaultValidationFactoryFixture public factory; address public owner1; uint256 public owner1Key; UpgradeableModularAccount public account1; + bytes32 public validationId; uint8 public constant SELECTOR_ASSOCIATED_VALIDATION = 0; uint8 public constant DEFAULT_VALIDATION = 1; @@ -32,9 +34,10 @@ abstract contract AccountTestBase is OptimizedTest { beneficiary = payable(makeAddr("beneficiary")); singleOwnerPlugin = _deploySingleOwnerPlugin(); - factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + // factory = new MSCAFactoryFixture(entryPoint, singleOwnerPlugin); + factory = new DefaultValidationFactoryFixture(entryPoint, singleOwnerPlugin); - account1 = factory.createAccount(owner1, 0); + (account1, validationId) = factory.createAccount(owner1, 0); vm.deal(address(account1), 100 ether); } @@ -50,11 +53,7 @@ abstract contract AccountTestBase is OptimizedTest { abi.encodeCall(SingleOwnerPlugin.transferOwnership, (address(this))) ) ), - abi.encodePacked( - address(singleOwnerPlugin), - ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER, - SELECTOR_ASSOCIATED_VALIDATION - ) + abi.encodePacked(validationId, DEFAULT_VALIDATION) ); } diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index f9431acc..1bf1df0c 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -7,6 +7,7 @@ import {IEntryPoint} from "@eth-infinitism/account-abstraction/interfaces/IEntry import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {EcdsaValidationPlugin} from "../../src/plugins/validation/EcdsaValidationPlugin.sol"; import {TokenReceiverPlugin} from "../../src/plugins/TokenReceiverPlugin.sol"; /// @dev This contract provides functions to deploy optimized (via IR) precompiled contracts. By compiling just @@ -50,6 +51,12 @@ abstract contract OptimizedTest is Test { : new SingleOwnerPlugin(); } + function _deployEcdsaValidationPlugin() internal returns (EcdsaValidationPlugin) { + return _isOptimizedTest() + ? EcdsaValidationPlugin(deployCode("out-optimized/EcdsaValidationPlugin.sol/EcdsaValidationPlugin.json")) + : new EcdsaValidationPlugin(); + } + function _deployTokenReceiverPlugin() internal returns (TokenReceiverPlugin) { return _isOptimizedTest() ? TokenReceiverPlugin(deployCode("out-optimized/TokenReceiverPlugin.sol/TokenReceiverPlugin.json")) diff --git a/test/validation/EcdsaValidationPlugin.t.sol b/test/validation/EcdsaValidationPlugin.t.sol new file mode 100644 index 00000000..f743fdd5 --- /dev/null +++ b/test/validation/EcdsaValidationPlugin.t.sol @@ -0,0 +1,80 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {console} from "forge-std/Test.sol"; +import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; +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 {EcdsaValidationPlugin} from "../../src/plugins/validation/EcdsaValidationPlugin.sol"; +import {FunctionReference, FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; + +import {OptimizedTest} from "../utils/OptimizedTest.sol"; +import {EcdsaValidationFactoryFixture} from "../mocks/EcdsaValidationFactoryFixture.sol"; + +contract EcdsaValidationPluginTest is OptimizedTest { + using MessageHashUtils for bytes32; + + EntryPoint public entryPoint; + EcdsaValidationPlugin public ecdsaValidationPlugin; + address payable public beneficiary; + address public ethRecipient; + + address public owner1; + uint256 public owner1Key; + EcdsaValidationFactoryFixture public factory; + UpgradeableModularAccount public account1; + bytes32 public validationId1; + + uint256 public constant CALL_GAS_LIMIT = 50000; + uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; + uint8 public constant SELECTOR_ASSOCIATED_VALIDATION = 0; + uint8 public constant DEFAULT_VALIDATION = 1; + + function setUp() public { + entryPoint = new EntryPoint(); + (owner1, owner1Key) = makeAddrAndKey("owner1"); + beneficiary = payable(makeAddr("beneficiary")); + ethRecipient = makeAddr("ethRecipient"); + + ecdsaValidationPlugin = _deployEcdsaValidationPlugin(); + factory = new EcdsaValidationFactoryFixture(entryPoint, ecdsaValidationPlugin); + + (account1, validationId1) = factory.createAccount(owner1, 0); + vm.deal(address(account1), 100 ether); + } + + function test_userOpValidation() public { + PackedUserOperation memory userOp = PackedUserOperation({ + sender: address(account1), + nonce: 0, + initCode: "", + 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(validationId1, DEFAULT_VALIDATION, r, s, v); + console.log("in test"); + console.logBytes32(validationId1); + + PackedUserOperation[] memory userOps = new PackedUserOperation[](1); + userOps[0] = userOp; + + entryPoint.handleOps(userOps, beneficiary); + + assertEq(ethRecipient.balance, 1 wei); + } + + // helper function to compress 2 gas values into a single bytes32 + function _encodeGas(uint256 g1, uint256 g2) internal pure returns (bytes32) { + return bytes32(uint256((g1 << 128) + uint128(g2))); + } +}