From c817eada7f0af7d1cd744c0c05e0ac866d812312 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Tue, 4 Jun 2024 13:35:38 -0400 Subject: [PATCH 1/8] feat: [v0.8-develop, experimental] composable validation --- src/plugins/owner/ISingleOwnerPlugin.sol | 19 +++++------ src/plugins/owner/SingleOwnerPlugin.sol | 40 +++++++++++------------- src/validators/ERC1271Validator.sol | 29 +++++++++++++++++ src/validators/ISignatureValidator.sol | 16 ++++++++++ src/validators/Secp256k1Validator.sol | 29 +++++++++++++++++ 5 files changed, 102 insertions(+), 31 deletions(-) create mode 100644 src/validators/ERC1271Validator.sol create mode 100644 src/validators/ISignatureValidator.sol create mode 100644 src/validators/Secp256k1Validator.sol diff --git a/src/plugins/owner/ISingleOwnerPlugin.sol b/src/plugins/owner/ISingleOwnerPlugin.sol index 57bcac80..72b93325 100644 --- a/src/plugins/owner/ISingleOwnerPlugin.sol +++ b/src/plugins/owner/ISingleOwnerPlugin.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8.25; import {IValidation} from "../../interfaces/IValidation.sol"; +import {Signer} from "../../validators/ISignatureValidator.sol"; interface ISingleOwnerPlugin is IValidation { enum FunctionId { @@ -11,27 +12,27 @@ interface ISingleOwnerPlugin is IValidation { /// @notice This event is emitted when ownership of the account changes. /// @param account The account whose ownership changed. - /// @param previousOwner The address of the previous owner. - /// @param newOwner The address of the new owner. - event OwnershipTransferred(address indexed account, address indexed previousOwner, address indexed newOwner); + /// @param previousOwner The details of the previous owner. + /// @param newOwner The details of the new owner. + event OwnershipTransferred(address indexed account, Signer previousOwner, Signer newOwner); error NotAuthorized(); /// @notice Transfer ownership of the account to `newOwner`. /// @dev This function is installed on the account as part of plugin installation, and should /// only be called from an account. - /// @param newOwner The address of the new owner. - function transferOwnership(address newOwner) external; + /// @param newOwner The details of the new owner. + function transferOwnership(Signer calldata newOwner) external; /// @notice Get the owner of the account. /// @dev This function is installed on the account as part of plugin installation, and should /// only be called from an account. - /// @return The address of the owner. - function owner() external view returns (address); + /// @return The details of the owner. + function owner() external view returns (Signer memory); /// @notice Get the owner of `account`. /// @dev This function is not installed on the account, and can be called by anyone. /// @param account The account to get the owner of. - /// @return The address of the owner. - function ownerOf(address account) external view returns (address); + /// @return The details of the owner. + function ownerOf(address account) external view returns (Signer memory); } diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index dbdd41b2..1d4ed9a5 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -1,13 +1,10 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.25; -import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; - import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; -import {IPluginManager} from "../../interfaces/IPluginManager.sol"; +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; + import { ManifestFunction, ManifestAssociatedFunctionType, @@ -16,9 +13,11 @@ import { PluginMetadata, SelectorPermission } from "../../interfaces/IPlugin.sol"; -import {IStandardExecutor} from "../../interfaces/IStandardExecutor.sol"; import {IPlugin} from "../../interfaces/IPlugin.sol"; +import {IPluginManager} from "../../interfaces/IPluginManager.sol"; +import {IStandardExecutor} from "../../interfaces/IStandardExecutor.sol"; import {IValidation} from "../../interfaces/IValidation.sol"; +import {Signer} from "../../validators/ISignatureValidator.sol"; import {BasePlugin, IERC165} from "../BasePlugin.sol"; import {ISingleOwnerPlugin} from "./ISingleOwnerPlugin.sol"; @@ -39,9 +38,6 @@ import {ISingleOwnerPlugin} from "./ISingleOwnerPlugin.sol"; /// owner of a modular account may not be another modular account if you want to /// send user operations through a bundler. contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { - using ECDSA for bytes32; - using MessageHashUtils for bytes32; - string public constant NAME = "Single Owner Plugin"; string public constant VERSION = "1.0.0"; string public constant AUTHOR = "ERC-6900 Authors"; @@ -53,14 +49,14 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; bytes4 internal constant _1271_INVALID = 0xffffffff; - mapping(address => address) internal _owners; + mapping(address => Signer) internal _owners; // ┏━━━━━━━━━━━━━━━━━━━━━━━━━━━┓ // ┃ Execution functions ┃ // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ /// @inheritdoc ISingleOwnerPlugin - function transferOwnership(address newOwner) external { + function transferOwnership(Signer calldata newOwner) external { _transferOwnership(newOwner); } @@ -70,12 +66,13 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { /// @inheritdoc IPlugin function onInstall(bytes calldata data) external override { - _transferOwnership(abi.decode(data, (address))); + _transferOwnership(abi.decode(data, (Signer))); } /// @inheritdoc IPlugin function onUninstall(bytes calldata) external override { - _transferOwnership(address(0)); + Signer memory empty; + _transferOwnership(empty); } /// @inheritdoc IValidation @@ -86,7 +83,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { { if (functionId == uint8(FunctionId.VALIDATION_OWNER)) { // Validate that the sender is the owner of the account or self. - if (sender != _owners[msg.sender] && sender != msg.sender) { + if (sender != abi.decode(_owners[msg.sender].data, (address)) && sender != msg.sender) { revert NotAuthorized(); } return; @@ -130,16 +127,15 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { returns (bytes4) { if (functionId == uint8(FunctionId.SIG_VALIDATION)) { - if (SignatureChecker.isValidSignatureNow(_owners[msg.sender], digest, signature)) { - return _1271_MAGIC_VALUE; - } - return _1271_INVALID; + Signer memory signer = _owners[msg.sender]; + (bool isValid,) = signer.validator.validate(msg.sender, signer.data, digest, signature); + return isValid ? _1271_MAGIC_VALUE : _1271_INVALID; } revert NotImplemented(); } /// @inheritdoc ISingleOwnerPlugin - function owner() external view returns (address) { + function owner() external view returns (Signer memory) { return _owners[msg.sender]; } @@ -148,7 +144,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ /// @inheritdoc ISingleOwnerPlugin - function ownerOf(address account) external view returns (address) { + function ownerOf(address account) external view returns (Signer memory) { return _owners[account]; } @@ -222,8 +218,8 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { // ┃ Internal / Private functions ┃ // ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛ - function _transferOwnership(address newOwner) internal { - address previousOwner = _owners[msg.sender]; + function _transferOwnership(Signer memory newOwner) internal { + Signer memory previousOwner = _owners[msg.sender]; _owners[msg.sender] = newOwner; emit OwnershipTransferred(msg.sender, previousOwner, newOwner); } diff --git a/src/validators/ERC1271Validator.sol b/src/validators/ERC1271Validator.sol new file mode 100644 index 00000000..00e694d7 --- /dev/null +++ b/src/validators/ERC1271Validator.sol @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; +import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; + +import {ISignatureValidator} from "./ISignatureValidator.sol"; + +contract ERC1271Validator is ISignatureValidator { + /// @dev Code inherited from function `isValidERC1271SignatureNow` in + /// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v5.0/contracts/utils/cryptography/SignatureChecker.sol + function validate(address, bytes memory signerData, bytes32 hash, bytes memory signature) + external + view + returns (bool isValid, bytes memory result) + { + address signer = abi.decode(signerData, (address)); + + (isValid, result) = signer.staticcall(abi.encodeCall(IERC1271.isValidSignature, (hash, signature))); + isValid = ( + isValid && result.length >= 32 + && abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector) + ); + } + + function encodeSignerData(address signer) external pure returns (bytes memory data) { + data = abi.encode(signer); + } +} diff --git a/src/validators/ISignatureValidator.sol b/src/validators/ISignatureValidator.sol new file mode 100644 index 00000000..615e442b --- /dev/null +++ b/src/validators/ISignatureValidator.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {IPlugin} from "../interfaces/IPlugin.sol"; + +struct Signer { + ISignatureValidator validator; + bytes data; +} + +interface ISignatureValidator { + function validate(address account, bytes memory signerData, bytes32 hash, bytes memory signature) + external + view + returns (bool isValid, bytes memory result); +} diff --git a/src/validators/Secp256k1Validator.sol b/src/validators/Secp256k1Validator.sol new file mode 100644 index 00000000..e7fd0976 --- /dev/null +++ b/src/validators/Secp256k1Validator.sol @@ -0,0 +1,29 @@ +// 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 {ISignatureValidator} from "./ISignatureValidator.sol"; + +contract Secp256k1Validator is ISignatureValidator { + using ECDSA for bytes32; + using MessageHashUtils for bytes32; + + function validate(address, bytes memory signerData, bytes32 hash, bytes memory signature) + external + pure + returns (bool isValid, bytes memory result) + { + bytes32 messageHash = hash.toEthSignedMessageHash(); + address expectedSigner = abi.decode(signerData, (address)); + + address signer = messageHash.recover(signature); + isValid = signer == expectedSigner; + result = abi.encode(signer); + } + + function encodeSignerData(address signer) external pure returns (bytes memory data) { + data = abi.encode(signer); + } +} From 50e2f44783ebd945eb13d4e9451036a0ea3f47e0 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Thu, 20 Jun 2024 17:27:19 -0700 Subject: [PATCH 2/8] feat: add three basic stateless validators and update singleOwnerPlugin --- src/plugins/owner/ISingleOwnerPlugin.sol | 2 +- src/plugins/owner/SingleOwnerPlugin.sol | 17 +- src/validators/Base64URL.sol | 32 ++++ src/validators/ERC1271Validator.sol | 17 +- ...p256k1Validator.sol => EcdsaValidator.sol} | 14 +- src/validators/ISignatureValidator.sol | 16 -- src/validators/IStatelessValidator.sol | 15 ++ src/validators/WebAuthn.sol | 171 ++++++++++++++++++ src/validators/WebAuthnValidator.sol | 51 ++++++ 9 files changed, 292 insertions(+), 43 deletions(-) create mode 100644 src/validators/Base64URL.sol rename src/validators/{Secp256k1Validator.sol => EcdsaValidator.sol} (52%) delete mode 100644 src/validators/ISignatureValidator.sol create mode 100644 src/validators/IStatelessValidator.sol create mode 100644 src/validators/WebAuthn.sol create mode 100644 src/validators/WebAuthnValidator.sol diff --git a/src/plugins/owner/ISingleOwnerPlugin.sol b/src/plugins/owner/ISingleOwnerPlugin.sol index 72b93325..534a1ac1 100644 --- a/src/plugins/owner/ISingleOwnerPlugin.sol +++ b/src/plugins/owner/ISingleOwnerPlugin.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.25; import {IValidation} from "../../interfaces/IValidation.sol"; -import {Signer} from "../../validators/ISignatureValidator.sol"; +import {Signer} from "../../validators/IStatelessValidator.sol"; interface ISingleOwnerPlugin is IValidation { enum FunctionId { diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 1d4ed9a5..3f066f67 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.25; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; import {UUPSUpgradeable} from "@openzeppelin/contracts/proxy/utils/UUPSUpgradeable.sol"; -import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import { ManifestFunction, @@ -17,7 +17,7 @@ import {IPlugin} from "../../interfaces/IPlugin.sol"; import {IPluginManager} from "../../interfaces/IPluginManager.sol"; import {IStandardExecutor} from "../../interfaces/IStandardExecutor.sol"; import {IValidation} from "../../interfaces/IValidation.sol"; -import {Signer} from "../../validators/ISignatureValidator.sol"; +import {Signer} from "../../validators/IStatelessValidator.sol"; import {BasePlugin, IERC165} from "../BasePlugin.sol"; import {ISingleOwnerPlugin} from "./ISingleOwnerPlugin.sol"; @@ -38,6 +38,8 @@ import {ISingleOwnerPlugin} from "./ISingleOwnerPlugin.sol"; /// owner of a modular account may not be another modular account if you want to /// send user operations through a bundler. contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { + using MessageHashUtils for bytes32; + string public constant NAME = "Single Owner Plugin"; string public constant VERSION = "1.0.0"; string public constant AUTHOR = "ERC-6900 Authors"; @@ -100,11 +102,14 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { { if (functionId == uint8(FunctionId.VALIDATION_OWNER)) { // Validate the user op signature against the owner. - (address signer,,) = (userOpHash.toEthSignedMessageHash()).tryRecover(userOp.signature); - if (signer == address(0) || signer != _owners[msg.sender]) { + Signer memory signer = _owners[msg.sender]; + + if (address(signer.validator) == address(0)) { return _SIG_VALIDATION_FAILED; } - return _SIG_VALIDATION_PASSED; + (bool isValid,) = + signer.validator.validate(signer.data, userOpHash.toEthSignedMessageHash(), userOp.signature); + return isValid ? _SIG_VALIDATION_PASSED : _SIG_VALIDATION_FAILED; } revert NotImplemented(); } @@ -128,7 +133,7 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { { if (functionId == uint8(FunctionId.SIG_VALIDATION)) { Signer memory signer = _owners[msg.sender]; - (bool isValid,) = signer.validator.validate(msg.sender, signer.data, digest, signature); + (bool isValid,) = signer.validator.validate(signer.data, digest, signature); return isValid ? _1271_MAGIC_VALUE : _1271_INVALID; } revert NotImplemented(); diff --git a/src/validators/Base64URL.sol b/src/validators/Base64URL.sol new file mode 100644 index 00000000..f8a498a7 --- /dev/null +++ b/src/validators/Base64URL.sol @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import "openzeppelin-contracts/contracts/utils/Base64.sol"; + +/// TODO this library is not necessary once Openzeppelin 5.0.2 Base64 is supported +library Base64URL { + function encode(bytes memory data) internal pure returns (string memory) { + string memory strb64 = Base64.encode(data); + bytes memory b64 = bytes(strb64); + + // Base64 can end with "=" or "=="; Base64URL has no padding. + uint256 equalsCount = 0; + if (b64.length > 2 && b64[b64.length - 2] == "=") equalsCount = 2; + else if (b64.length > 1 && b64[b64.length - 1] == "=") equalsCount = 1; + + uint256 len = b64.length - equalsCount; + bytes memory result = new bytes(len); + + for (uint256 i = 0; i < len; i++) { + if (b64[i] == "+") { + result[i] = "-"; + } else if (b64[i] == "/") { + result[i] = "_"; + } else { + result[i] = b64[i]; + } + } + + return string(result); + } +} diff --git a/src/validators/ERC1271Validator.sol b/src/validators/ERC1271Validator.sol index 00e694d7..d935b527 100644 --- a/src/validators/ERC1271Validator.sol +++ b/src/validators/ERC1271Validator.sol @@ -1,26 +1,19 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.25; -import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; -import {ISignatureValidator} from "./ISignatureValidator.sol"; +import {IStatelessValidator} from "./IStatelessValidator.sol"; -contract ERC1271Validator is ISignatureValidator { - /// @dev Code inherited from function `isValidERC1271SignatureNow` in - /// https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v5.0/contracts/utils/cryptography/SignatureChecker.sol - function validate(address, bytes memory signerData, bytes32 hash, bytes memory signature) +contract Erc1271Validator is IStatelessValidator { + function validate(bytes memory signerData, bytes32 hash, bytes memory signature) external view returns (bool isValid, bytes memory result) { - address signer = abi.decode(signerData, (address)); + address expectedSigner = abi.decode(signerData, (address)); - (isValid, result) = signer.staticcall(abi.encodeCall(IERC1271.isValidSignature, (hash, signature))); - isValid = ( - isValid && result.length >= 32 - && abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector) - ); + isValid = SignatureChecker.isValidERC1271SignatureNow(expectedSigner, hash, signature); } function encodeSignerData(address signer) external pure returns (bytes memory data) { diff --git a/src/validators/Secp256k1Validator.sol b/src/validators/EcdsaValidator.sol similarity index 52% rename from src/validators/Secp256k1Validator.sol rename to src/validators/EcdsaValidator.sol index e7fd0976..25e51642 100644 --- a/src/validators/Secp256k1Validator.sol +++ b/src/validators/EcdsaValidator.sol @@ -2,23 +2,21 @@ pragma solidity ^0.8.25; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; -import {ISignatureValidator} from "./ISignatureValidator.sol"; +import {IStatelessValidator} from "./IStatelessValidator.sol"; -contract Secp256k1Validator is ISignatureValidator { +contract EcdsaValidator is IStatelessValidator { using ECDSA for bytes32; - using MessageHashUtils for bytes32; - function validate(address, bytes memory signerData, bytes32 hash, bytes memory signature) + /// @dev result always returns the correct singer of the signature. + function validate(bytes memory signerData, bytes32 hash, bytes memory signature) external - pure + view returns (bool isValid, bytes memory result) { - bytes32 messageHash = hash.toEthSignedMessageHash(); address expectedSigner = abi.decode(signerData, (address)); - address signer = messageHash.recover(signature); + address signer = hash.recover(signature); isValid = signer == expectedSigner; result = abi.encode(signer); } diff --git a/src/validators/ISignatureValidator.sol b/src/validators/ISignatureValidator.sol deleted file mode 100644 index 615e442b..00000000 --- a/src/validators/ISignatureValidator.sol +++ /dev/null @@ -1,16 +0,0 @@ -// SPDX-License-Identifier: GPL-3.0 -pragma solidity ^0.8.25; - -import {IPlugin} from "../interfaces/IPlugin.sol"; - -struct Signer { - ISignatureValidator validator; - bytes data; -} - -interface ISignatureValidator { - function validate(address account, bytes memory signerData, bytes32 hash, bytes memory signature) - external - view - returns (bool isValid, bytes memory result); -} diff --git a/src/validators/IStatelessValidator.sol b/src/validators/IStatelessValidator.sol new file mode 100644 index 00000000..0441c26c --- /dev/null +++ b/src/validators/IStatelessValidator.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +struct Signer { + IStatelessValidator validator; + /// data is passed as signedData to the validator + bytes data; +} + +interface IStatelessValidator { + function validate(bytes memory signerData, bytes32 hash, bytes memory signature) + external + view + returns (bool isValid, bytes memory result); +} diff --git a/src/validators/WebAuthn.sol b/src/validators/WebAuthn.sol new file mode 100644 index 00000000..008aaee0 --- /dev/null +++ b/src/validators/WebAuthn.sol @@ -0,0 +1,171 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.25; + +import "./Base64URL.sol"; + +/** + * Helper library for external contracts to verify WebAuthn signatures. Only support RIP-7212 precompiled signature + * check. + * /// @author Alchemy + * /// @author Daimo (https://github.com/daimo-eth/p256-verifier/blob/master/src/WebAuthn.sol) + */ +library WebAuthn { + /// Checks whether substr occurs in str starting at a given byte offset. + function contains(string memory substr, string memory str, uint256 location) internal pure returns (bool) { + bytes memory substrBytes = bytes(substr); + bytes memory strBytes = bytes(str); + + uint256 substrLen = substrBytes.length; + uint256 strLen = strBytes.length; + + for (uint256 i = 0; i < substrLen; i++) { + if (location + i >= strLen) { + return false; + } + + if (substrBytes[i] != strBytes[location + i]) { + return false; + } + } + + return true; + } + + bytes1 constant _AUTH_DATA_FLAGS_UP = 0x01; // Bit 0 + bytes1 constant _AUTH_DATA_FLAGS_UV = 0x04; // Bit 2 + bytes1 constant _AUTH_DATA_FLAGS_BE = 0x08; // Bit 3 + bytes1 constant _AUTH_DATA_FLAGS_BS = 0x10; // Bit 4 + + /// @dev Secp256r1 curve order / 2 used as guard to prevent signature malleability issue. + /// 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551 / 2 + uint256 constant _P256_N_DIV_2 = 57896044605178124381348723474703786764998477612067880171211129530534256022184; + /// @dev The precompiled contract address to use for signature verification in the “secp256r1” elliptic curve. + /// See https://github.com/ethereum/RIPs/blob/master/RIPS/rip-7212.md. + address constant _PRECOMPILED_VERIFIER = 0x0000000000000000000000000000000000000100; + + /// Verifies the authFlags in authenticatorData. Numbers in inline comment + /// correspond to the same numbered bullets in + /// https://www.w3.org/TR/webauthn-2/#sctn-verifying-assertion. + function checkAuthFlags(bytes1 flags, bool requireUserVerification) internal pure returns (bool) { + // 17. Verify that the UP bit of the flags in authData is set. + if (flags & _AUTH_DATA_FLAGS_UP != _AUTH_DATA_FLAGS_UP) { + return false; + } + + // 18. If user verification was determined to be required, verify that + // the UV bit of the flags in authData is set. Otherwise, ignore the + // value of the UV flag. + if (requireUserVerification && (flags & _AUTH_DATA_FLAGS_UV) != _AUTH_DATA_FLAGS_UV) { + return false; + } + + // 19. If the BE bit of the flags in authData is not set, verify that + // the BS bit is not set. + if (flags & _AUTH_DATA_FLAGS_BE != _AUTH_DATA_FLAGS_BE) { + if (flags & _AUTH_DATA_FLAGS_BS == _AUTH_DATA_FLAGS_BS) { + return false; + } + } + + return true; + } + + /** + * Verifies a Webauthn P256 signature (Authentication Assertion) as described + * in https://www.w3.org/TR/webauthn-2/#sctn-verifying-assertion. We do not + * verify all the steps as described in the specification, only ones relevant + * to our context. Please carefully read through this list before usage. + * Specifically, we do verify the following: + * - Verify that authenticatorData (which comes from the authenticator, + * such as iCloud Keychain) indicates a well-formed assertion. If + * requireUserVerification is set, checks that the authenticator enforced + * user verification. User verification should be required if, + * and only if, options.userVerification is set to required in the request + * - Verifies that the client JSON is of type "webauthn.get", i.e. the client + * was responding to a request to assert authentication. + * - Verifies that the client JSON contains the requested challenge. + * - Finally, verifies that (r, s) constitute a valid signature over both + * the authenicatorData and client JSON, for public key (x, y). + * + * We make some assumptions about the particular use case of this verifier, + * so we do NOT verify the following: + * - Does NOT verify that the origin in the clientDataJSON matches the + * Relying Party's origin: It is considered the authenticator's + * responsibility to ensure that the user is interacting with the correct + * RP. This is enforced by most high quality authenticators properly, + * particularly the iCloud Keychain and Google Password Manager were + * tested. + * - Does NOT verify That c.topOrigin is well-formed: We assume c.topOrigin + * would never be present, i.e. the credentials are never used in a + * cross-origin/iframe context. The website/app set up should disallow + * cross-origin usage of the credentials. This is the default behaviour for + * created credentials in common settings. + * - Does NOT verify that the rpIdHash in authData is the SHA-256 hash of an + * RP ID expected by the Relying Party: This means that we rely on the + * authenticator to properly enforce credentials to be used only by the + * correct RP. This is generally enforced with features like Apple App Site + * Association and Google Asset Links. To protect from edge cases in which + * a previously-linked RP ID is removed from the authorised RP IDs, + * we recommend that messages signed by the authenticator include some + * expiry mechanism. + * - Does NOT verify the credential backup state: This assumes the credential + * backup state is NOT used as part of Relying Party business logic or + * policy. + * - Does NOT verify the values of the client extension outputs: This assumes + * that the Relying Party does not use client extension outputs. + * - Does NOT verify the signature counter: Signature counters are intended + * to enable risk scoring for the Relying Party. This assumes risk scoring + * is not used as part of Relying Party business logic or policy. + * - Does NOT verify the attestation object: This assumes that + * response.attestationObject is NOT present in the response, i.e. the + * RP does not intend to verify an attestation. + */ + function verifySignature( + bytes memory challenge, + bytes memory authenticatorData, + bool requireUserVerification, + string memory clientDataJSON, + uint256 challengeLocation, + uint256 responseTypeLocation, + uint256 r, + uint256 s, + uint256 x, + uint256 y + ) internal view returns (bool) { + // check for signature malleability + if (s > _P256_N_DIV_2) { + return false; + } + + // Check that authenticatorData has good flags + if (authenticatorData.length < 37 || !checkAuthFlags(authenticatorData[32], requireUserVerification)) { + return false; + } + + // Check that response is for an authentication assertion + // solhint-disable-next-line quotes + string memory responseType = '"type":"webauthn.get"'; + if (!contains(responseType, clientDataJSON, responseTypeLocation)) { + return false; + } + + // Check that challenge is in the clientDataJSON + string memory challengeB64url = Base64URL.encode(challenge); + // solhint-disable-next-line quotes + string memory challengeProperty = string.concat('"challenge":"', challengeB64url, '"'); + + if (!contains(challengeProperty, clientDataJSON, challengeLocation)) { + return false; + } + + // Check that the public key signed sha256(authenticatorData || sha256(clientDataJSON)) + bytes32 clientDataJSONHash = sha256(bytes(clientDataJSON)); + bytes32 messageHash = sha256(abi.encodePacked(authenticatorData, clientDataJSONHash)); + bytes memory args = abi.encode(messageHash, r, s, x, y); + (bool success, bytes memory ret) = _PRECOMPILED_VERIFIER.staticcall(args); + if (success == false || ret.length == 0) { + return false; + } + return abi.decode(ret, (uint256)) == 1; + } +} diff --git a/src/validators/WebAuthnValidator.sol b/src/validators/WebAuthnValidator.sol new file mode 100644 index 00000000..fc9e5ef3 --- /dev/null +++ b/src/validators/WebAuthnValidator.sol @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-3.0 +pragma solidity ^0.8.25; + +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; + +import {IStatelessValidator} from "./IStatelessValidator.sol"; +import {WebAuthn} from "./WebAuthn.sol"; + +contract WebAuthnValidator is IStatelessValidator { + using MessageHashUtils for bytes32; + + struct SignerData { + /// The x coordinate of the public key. + uint256 x; + /// The y coordinate of the public key. + uint256 y; + } + + function validate(bytes memory signerData, bytes32 hash, bytes memory signature) + external + view + returns (bool isValid, bytes memory result) + { + ( + bytes memory authenticatorData, + string memory clientDataJSON, + uint256 challengeLocation, + uint256 responseTypeLocation, + uint256 r, + uint256 s + ) = abi.decode(signature, (bytes, string, uint256, uint256, uint256, uint256)); + SignerData memory signerDataDecoded = abi.decode(signerData, (SignerData)); + + isValid = WebAuthn.verifySignature( + abi.encodePacked(hash), + authenticatorData, + true, + clientDataJSON, + challengeLocation, + responseTypeLocation, + r, + s, + signerDataDecoded.x, + signerDataDecoded.y + ); + } + + function encodeSignerData(SignerData calldata signerData) external pure returns (bytes memory data) { + data = abi.encode(signerData); + } +} From 10ac17da5be6af50f828ab55ea52aabcab9c8792 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 21 Jun 2024 18:00:18 -0700 Subject: [PATCH 3/8] fix tests with new validation system --- test/account/UpgradeableModularAccount.t.sol | 13 ++- .../mocks/DefaultValidationFactoryFixture.sol | 6 +- test/mocks/MSCAFactoryFixture.sol | 6 +- test/plugin/SingleOwnerPlugin.t.sol | 102 ++++++++++-------- test/utils/AccountTestBase.sol | 9 +- test/utils/OptimizedTest.sol | 8 ++ 6 files changed, 93 insertions(+), 51 deletions(-) diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index e3d09e7f..9ee7f2e8 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -24,6 +24,9 @@ import {ComprehensivePlugin} from "../mocks/plugins/ComprehensivePlugin.sol"; import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; +import {Signer} from "../../src/validators/IStatelessValidator.sol"; +import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; + contract UpgradeableModularAccountTest is AccountTestBase { using ECDSA for bytes32; using MessageHashUtils for bytes32; @@ -31,6 +34,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { TokenReceiverPlugin public tokenReceiverPlugin; // A separate account and owner that isn't deployed yet, used to test initcode + Signer public signer2; address public owner2; uint256 public owner2Key; UpgradeableModularAccount public account2; @@ -52,6 +56,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { tokenReceiverPlugin = _deployTokenReceiverPlugin(); (owner2, owner2Key) = makeAddrAndKey("owner2"); + signer2 = Signer(ecdsaValidator, abi.encode(address(owner2))); // Compute counterfactual address account2 = UpgradeableModularAccount(payable(factory.getAddress(owner2, 0))); @@ -104,7 +109,7 @@ contract UpgradeableModularAccountTest is AccountTestBase { initCode: abi.encodePacked(address(factory), abi.encodeCall(factory.createAccount, (owner2, 0))), callData: abi.encodeCall( UpgradeableModularAccount.execute, - (address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (owner2))) + (address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (signer2))) ), accountGasLimits: _encodeGas(VERIFICATION_GAS_LIMIT, CALL_GAS_LIMIT), preVerificationGas: 0, @@ -431,14 +436,14 @@ contract UpgradeableModularAccountTest is AccountTestBase { } function test_transferOwnership() public { - assertEq(singleOwnerPlugin.ownerOf(address(account1)), owner1); + assertEq(_getAccountOwnerAddress(singleOwnerPlugin, address(account1)), owner1); vm.prank(address(entryPoint)); account1.execute( - address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (owner2)) + address(singleOwnerPlugin), 0, abi.encodeCall(SingleOwnerPlugin.transferOwnership, (signer2)) ); - assertEq(singleOwnerPlugin.ownerOf(address(account1)), owner2); + assertEq(_getAccountOwnerAddress(singleOwnerPlugin, address(account1)), owner2); } function test_isValidSignature() public { diff --git a/test/mocks/DefaultValidationFactoryFixture.sol b/test/mocks/DefaultValidationFactoryFixture.sol index a4836ad8..ddb186e7 100644 --- a/test/mocks/DefaultValidationFactoryFixture.sol +++ b/test/mocks/DefaultValidationFactoryFixture.sol @@ -9,6 +9,8 @@ import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAcc import {FunctionReferenceLib} from "../../src/helpers/FunctionReferenceLib.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {Signer} from "../../src/validators/IStatelessValidator.sol"; +import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; @@ -20,6 +22,7 @@ contract DefaultValidationFactoryFixture is OptimizedTest { uint32 public constant UNSTAKE_DELAY = 1 weeks; IEntryPoint public entryPoint; + EcdsaValidator public ecdsaValidator; address public self; @@ -27,6 +30,7 @@ contract DefaultValidationFactoryFixture is OptimizedTest { constructor(IEntryPoint _entryPoint, SingleOwnerPlugin _singleOwnerPlugin) { entryPoint = _entryPoint; + ecdsaValidator = new EcdsaValidator(); accountImplementation = _deployUpgradeableModularAccount(_entryPoint); _PROXY_BYTECODE_HASH = keccak256( abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) @@ -50,7 +54,7 @@ contract DefaultValidationFactoryFixture is OptimizedTest { // short circuit if exists if (addr.code.length == 0) { - bytes memory pluginInstallData = abi.encode(owner); + bytes memory pluginInstallData = abi.encode(Signer(ecdsaValidator, abi.encode(address(owner)))); // not necessary to check return addr since next call will fail if so new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); diff --git a/test/mocks/MSCAFactoryFixture.sol b/test/mocks/MSCAFactoryFixture.sol index cc8ae60a..8b8fbe83 100644 --- a/test/mocks/MSCAFactoryFixture.sol +++ b/test/mocks/MSCAFactoryFixture.sol @@ -7,6 +7,8 @@ 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 {Signer} from "../../src/validators/IStatelessValidator.sol"; +import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; @@ -18,6 +20,7 @@ import {OptimizedTest} from "../utils/OptimizedTest.sol"; contract MSCAFactoryFixture is OptimizedTest { UpgradeableModularAccount public accountImplementation; SingleOwnerPlugin public singleOwnerPlugin; + EcdsaValidator public ecdsaValidator; bytes32 private immutable _PROXY_BYTECODE_HASH; uint32 public constant UNSTAKE_DELAY = 1 weeks; @@ -35,6 +38,7 @@ contract MSCAFactoryFixture is OptimizedTest { abi.encodePacked(type(ERC1967Proxy).creationCode, abi.encode(address(accountImplementation), "")) ); singleOwnerPlugin = _singleOwnerPlugin; + ecdsaValidator = new EcdsaValidator(); 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. @@ -58,7 +62,7 @@ contract MSCAFactoryFixture is OptimizedTest { bytes32[] memory pluginManifestHashes = new bytes32[](1); pluginManifestHashes[0] = keccak256(abi.encode(singleOwnerPlugin.pluginManifest())); bytes[] memory pluginInstallData = new bytes[](1); - pluginInstallData[0] = abi.encode(owner); + pluginInstallData[0] = abi.encode(Signer(ecdsaValidator, abi.encode(address(owner)))); // not necessary to check return addr since next call will fail if so new ERC1967Proxy{salt: getSalt(owner, salt)}(address(accountImplementation), ""); diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 41997591..954e333f 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -11,12 +11,18 @@ import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol import {ContractOwner} from "../mocks/ContractOwner.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; +import {Signer, IStatelessValidator} from "../../src/validators/IStatelessValidator.sol"; +import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; +import {Erc1271Validator} from "../../src/validators/Erc1271Validator.sol"; + contract SingleOwnerPluginTest is OptimizedTest { using ECDSA for bytes32; using MessageHashUtils for bytes32; SingleOwnerPlugin public plugin; EntryPoint public entryPoint; + EcdsaValidator public ecdsaValidator; + Erc1271Validator public erc1271Validator; bytes4 internal constant _1271_MAGIC_VALUE = 0x1626ba7e; address public a; @@ -24,20 +30,27 @@ contract SingleOwnerPluginTest is OptimizedTest { address public owner1; address public owner2; + Signer public signer1; + Signer public signer2; ContractOwner public contractOwner; // Event declarations (needed for vm.expectEmit) - event OwnershipTransferred(address indexed account, address indexed previousOwner, address indexed newOwner); + event OwnershipTransferred(address indexed account, Signer indexed previousOwner, Signer indexed newOwner); function setUp() public { plugin = _deploySingleOwnerPlugin(); entryPoint = new EntryPoint(); + ecdsaValidator = new EcdsaValidator(); + erc1271Validator = new Erc1271Validator(); a = makeAddr("a"); b = makeAddr("b"); owner1 = makeAddr("owner1"); owner2 = makeAddr("owner2"); contractOwner = new ContractOwner(); + + signer1 = Signer(ecdsaValidator, abi.encode(owner1)); + signer2 = Signer(ecdsaValidator, abi.encode(owner2)); } // Tests: @@ -51,69 +64,70 @@ contract SingleOwnerPluginTest is OptimizedTest { function test_uninitializedOwner() public { vm.startPrank(a); - assertEq(address(0), plugin.owner()); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); } function test_ownerInitialization() public { vm.startPrank(a); - assertEq(address(0), plugin.owner()); - plugin.transferOwnership(owner1); - assertEq(owner1, plugin.owner()); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + plugin.transferOwnership(signer1); + assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); } - function test_ownerInitializationEvent() public { - vm.startPrank(a); - assertEq(address(0), plugin.owner()); + // function test_ownerInitializationEvent() public { + // vm.startPrank(a); + // assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); - vm.expectEmit(true, true, true, true); - emit OwnershipTransferred(a, address(0), owner1); + // vm.expectEmit(true, true, true, true); + // emit OwnershipTransferred(a, Signer(IStatelessValidator(address(0)), ""), + // signer1); - plugin.transferOwnership(owner1); - assertEq(owner1, plugin.owner()); - } + // plugin.transferOwnership(signer1); + // assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); + // } function test_ownerMigration() public { vm.startPrank(a); - assertEq(address(0), plugin.owner()); - plugin.transferOwnership(owner1); - assertEq(owner1, plugin.owner()); - plugin.transferOwnership(owner2); - assertEq(owner2, plugin.owner()); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + plugin.transferOwnership(signer1); + assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); + plugin.transferOwnership(signer2); + assertEq(owner2, _getAccountOwnerAddress(plugin, address(a))); } - function test_ownerMigrationEvents() public { - vm.startPrank(a); - assertEq(address(0), plugin.owner()); + // function test_ownerMigrationEvents() public { + // vm.startPrank(a); + // assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); - vm.expectEmit(true, true, true, true); - emit OwnershipTransferred(a, address(0), owner1); + // vm.expectEmit(true, true, true, true); + // emit OwnershipTransferred(a, Signer(IStatelessValidator(address(0)), abi.encode(address(0))), signer1); - plugin.transferOwnership(owner1); - assertEq(owner1, plugin.owner()); + // plugin.transferOwnership(signer1); + // assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); - vm.expectEmit(true, true, true, true); - emit OwnershipTransferred(a, owner1, owner2); + // vm.expectEmit(true, true, true, true); + // emit OwnershipTransferred(a, signer1, signer2); - plugin.transferOwnership(owner2); - assertEq(owner2, plugin.owner()); - } + // plugin.transferOwnership(signer2); + // assertEq(owner2, _getAccountOwnerAddress(plugin, address(a))); + // } function test_ownerForSender() public { vm.startPrank(a); - assertEq(address(0), plugin.owner()); - plugin.transferOwnership(owner1); - assertEq(owner1, plugin.owner()); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + plugin.transferOwnership(signer1); + assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); vm.startPrank(b); - assertEq(address(0), plugin.owner()); - plugin.transferOwnership(owner2); - assertEq(owner2, plugin.owner()); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(b))); + plugin.transferOwnership(signer2); + assertEq(owner2, _getAccountOwnerAddress(plugin, address(b))); } function test_requireOwner() public { vm.startPrank(a); - assertEq(address(0), plugin.owner()); - plugin.transferOwnership(owner1); - assertEq(owner1, plugin.owner()); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + plugin.transferOwnership(signer1); + assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); plugin.validateRuntime(uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), owner1, 0, "", ""); vm.startPrank(b); @@ -138,8 +152,8 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(success, 1); // transfer ownership to signer - plugin.transferOwnership(signer); - assertEq(signer, plugin.owner()); + plugin.transferOwnership(Signer(ecdsaValidator, abi.encode(signer))); + assertEq(signer, _getAccountOwnerAddress(plugin, address(a))); // sig check should pass success = plugin.validateUserOp(uint8(ISingleOwnerPlugin.FunctionId.VALIDATION_OWNER), userOp, userOpHash); @@ -165,8 +179,8 @@ contract SingleOwnerPluginTest is OptimizedTest { ); // transfer ownership to signer - plugin.transferOwnership(signer); - assertEq(signer, plugin.owner()); + plugin.transferOwnership(Signer(ecdsaValidator, abi.encode(signer))); + assertEq(signer, _getAccountOwnerAddress(plugin, address(a))); // sig check should pass assertEq( @@ -182,7 +196,7 @@ contract SingleOwnerPluginTest is OptimizedTest { function testFuzz_isValidSignatureForContractOwner(bytes32 digest) public { vm.startPrank(a); - plugin.transferOwnership(address(contractOwner)); + plugin.transferOwnership(Signer(erc1271Validator, abi.encode(address(contractOwner)))); bytes memory signature = contractOwner.sign(digest); assertEq( plugin.validateSignature( diff --git a/test/utils/AccountTestBase.sol b/test/utils/AccountTestBase.sol index 059e9cac..06191914 100644 --- a/test/utils/AccountTestBase.sol +++ b/test/utils/AccountTestBase.sol @@ -6,6 +6,8 @@ import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.so import {UpgradeableModularAccount} from "../../src/account/UpgradeableModularAccount.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {Signer} from "../../src/validators/IStatelessValidator.sol"; +import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; import {OptimizedTest} from "./OptimizedTest.sol"; @@ -17,6 +19,7 @@ abstract contract AccountTestBase is OptimizedTest { EntryPoint public entryPoint; address payable public beneficiary; SingleOwnerPlugin public singleOwnerPlugin; + EcdsaValidator public ecdsaValidator; MSCAFactoryFixture public factory; address public owner1; @@ -27,6 +30,8 @@ abstract contract AccountTestBase is OptimizedTest { uint8 public constant DEFAULT_VALIDATION = 1; constructor() { + ecdsaValidator = new EcdsaValidator(); + entryPoint = new EntryPoint(); (owner1, owner1Key) = makeAddrAndKey("owner1"); beneficiary = payable(makeAddr("beneficiary")); @@ -47,7 +52,9 @@ abstract contract AccountTestBase is OptimizedTest { ( address(singleOwnerPlugin), 0, - abi.encodeCall(SingleOwnerPlugin.transferOwnership, (address(this))) + abi.encodeCall( + SingleOwnerPlugin.transferOwnership, (Signer(ecdsaValidator, abi.encode(address(this)))) + ) ) ), abi.encodePacked( diff --git a/test/utils/OptimizedTest.sol b/test/utils/OptimizedTest.sol index f9431acc..535d58d3 100644 --- a/test/utils/OptimizedTest.sol +++ b/test/utils/OptimizedTest.sol @@ -55,4 +55,12 @@ abstract contract OptimizedTest is Test { ? TokenReceiverPlugin(deployCode("out-optimized/TokenReceiverPlugin.sol/TokenReceiverPlugin.json")) : new TokenReceiverPlugin(); } + + function _getAccountOwnerAddress(SingleOwnerPlugin singleOwnerPlugin, address account) + internal + returns (address) + { + bytes memory signerData = singleOwnerPlugin.ownerOf(account).data; + return signerData.length > 0 ? abi.decode(singleOwnerPlugin.ownerOf(account).data, (address)) : address(0); + } } From ae7bacb2ceae32cdb5e54cca392f1349bd96cfea Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 21 Jun 2024 18:38:07 -0700 Subject: [PATCH 4/8] fix signer not set errors --- src/plugins/owner/SingleOwnerPlugin.sol | 12 +++++++++++- src/validators/ERC1271Validator.sol | 10 +++++++--- src/validators/EcdsaValidator.sol | 12 ++++++++---- 3 files changed, 26 insertions(+), 8 deletions(-) diff --git a/src/plugins/owner/SingleOwnerPlugin.sol b/src/plugins/owner/SingleOwnerPlugin.sol index 3f066f67..b0942feb 100644 --- a/src/plugins/owner/SingleOwnerPlugin.sol +++ b/src/plugins/owner/SingleOwnerPlugin.sol @@ -85,7 +85,13 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { { if (functionId == uint8(FunctionId.VALIDATION_OWNER)) { // Validate that the sender is the owner of the account or self. - if (sender != abi.decode(_owners[msg.sender].data, (address)) && sender != msg.sender) { + if ( + sender != msg.sender + && ( + _owners[msg.sender].data.length == 0 + || sender != abi.decode(_owners[msg.sender].data, (address)) + ) + ) { revert NotAuthorized(); } return; @@ -133,6 +139,10 @@ contract SingleOwnerPlugin is ISingleOwnerPlugin, BasePlugin { { if (functionId == uint8(FunctionId.SIG_VALIDATION)) { Signer memory signer = _owners[msg.sender]; + if (address(signer.validator) == address(0)) { + return _1271_INVALID; + } + (bool isValid,) = signer.validator.validate(signer.data, digest, signature); return isValid ? _1271_MAGIC_VALUE : _1271_INVALID; } diff --git a/src/validators/ERC1271Validator.sol b/src/validators/ERC1271Validator.sol index d935b527..b008e1d6 100644 --- a/src/validators/ERC1271Validator.sol +++ b/src/validators/ERC1271Validator.sol @@ -9,11 +9,15 @@ contract Erc1271Validator is IStatelessValidator { function validate(bytes memory signerData, bytes32 hash, bytes memory signature) external view - returns (bool isValid, bytes memory result) + returns (bool isValid, bytes memory) { - address expectedSigner = abi.decode(signerData, (address)); + if (signerData.length == 0) { + isValid = false; + } else { + address expectedSigner = abi.decode(signerData, (address)); - isValid = SignatureChecker.isValidERC1271SignatureNow(expectedSigner, hash, signature); + isValid = SignatureChecker.isValidERC1271SignatureNow(expectedSigner, hash, signature); + } } function encodeSignerData(address signer) external pure returns (bytes memory data) { diff --git a/src/validators/EcdsaValidator.sol b/src/validators/EcdsaValidator.sol index 25e51642..a001e4f3 100644 --- a/src/validators/EcdsaValidator.sol +++ b/src/validators/EcdsaValidator.sol @@ -14,11 +14,15 @@ contract EcdsaValidator is IStatelessValidator { view returns (bool isValid, bytes memory result) { - address expectedSigner = abi.decode(signerData, (address)); + if (signerData.length == 0) { + isValid = false; + } else { + address expectedSigner = abi.decode(signerData, (address)); - address signer = hash.recover(signature); - isValid = signer == expectedSigner; - result = abi.encode(signer); + address signer = hash.recover(signature); + isValid = signer == expectedSigner; + result = abi.encode(signer); + } } function encodeSignerData(address signer) external pure returns (bytes memory data) { From 085bc7831e29213ac3b2ac22bf02f1c1603b936b Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 21 Jun 2024 18:54:54 -0700 Subject: [PATCH 5/8] fix single owner plugin events tests --- test/plugin/SingleOwnerPlugin.t.sol | 45 +++++++++++++++-------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 954e333f..0379156d 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -35,7 +35,7 @@ contract SingleOwnerPluginTest is OptimizedTest { ContractOwner public contractOwner; // Event declarations (needed for vm.expectEmit) - event OwnershipTransferred(address indexed account, Signer indexed previousOwner, Signer indexed newOwner); + event OwnershipTransferred(address indexed account, Signer previousOwner, Signer newOwner); function setUp() public { plugin = _deploySingleOwnerPlugin(); @@ -74,17 +74,17 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); } - // function test_ownerInitializationEvent() public { - // vm.startPrank(a); - // assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + function test_ownerInitializationEvent() public { + vm.startPrank(a); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); - // vm.expectEmit(true, true, true, true); - // emit OwnershipTransferred(a, Signer(IStatelessValidator(address(0)), ""), - // signer1); + Signer memory emptySigner; + vm.expectEmit(true, true, true, true); + emit OwnershipTransferred(a, emptySigner, signer1); - // plugin.transferOwnership(signer1); - // assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); - // } + plugin.transferOwnership(signer1); + assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); + } function test_ownerMigration() public { vm.startPrank(a); @@ -95,22 +95,23 @@ contract SingleOwnerPluginTest is OptimizedTest { assertEq(owner2, _getAccountOwnerAddress(plugin, address(a))); } - // function test_ownerMigrationEvents() public { - // vm.startPrank(a); - // assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + function test_ownerMigrationEvents() public { + vm.startPrank(a); + assertEq(address(0), _getAccountOwnerAddress(plugin, address(a))); + Signer memory emptySigner; - // vm.expectEmit(true, true, true, true); - // emit OwnershipTransferred(a, Signer(IStatelessValidator(address(0)), abi.encode(address(0))), signer1); + vm.expectEmit(true, true, true, true); + emit OwnershipTransferred(a, emptySigner, signer1); - // plugin.transferOwnership(signer1); - // assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); + plugin.transferOwnership(signer1); + assertEq(owner1, _getAccountOwnerAddress(plugin, address(a))); - // vm.expectEmit(true, true, true, true); - // emit OwnershipTransferred(a, signer1, signer2); + vm.expectEmit(true, true, true, true); + emit OwnershipTransferred(a, signer1, signer2); - // plugin.transferOwnership(signer2); - // assertEq(owner2, _getAccountOwnerAddress(plugin, address(a))); - // } + plugin.transferOwnership(signer2); + assertEq(owner2, _getAccountOwnerAddress(plugin, address(a))); + } function test_ownerForSender() public { vm.startPrank(a); From 8026eecac8ded1a82da6ae6cbfd3be950b87a620 Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Fri, 21 Jun 2024 19:17:38 -0700 Subject: [PATCH 6/8] fix remaining tests and clean up interface function overrides --- src/validators/EcdsaValidator.sol | 1 + ...ERC1271Validator.sol => Erc1271Validator.sol} | 1 + src/validators/WebAuthnValidator.sol | 3 ++- test/account/MultiValidation.t.sol | 16 +++++++++++++--- test/account/UpgradeableModularAccount.t.sol | 1 - test/plugin/SingleOwnerPlugin.t.sol | 2 +- 6 files changed, 18 insertions(+), 6 deletions(-) rename src/validators/{ERC1271Validator.sol => Erc1271Validator.sol} (98%) diff --git a/src/validators/EcdsaValidator.sol b/src/validators/EcdsaValidator.sol index a001e4f3..f84b1cac 100644 --- a/src/validators/EcdsaValidator.sol +++ b/src/validators/EcdsaValidator.sol @@ -12,6 +12,7 @@ contract EcdsaValidator is IStatelessValidator { function validate(bytes memory signerData, bytes32 hash, bytes memory signature) external view + override returns (bool isValid, bytes memory result) { if (signerData.length == 0) { diff --git a/src/validators/ERC1271Validator.sol b/src/validators/Erc1271Validator.sol similarity index 98% rename from src/validators/ERC1271Validator.sol rename to src/validators/Erc1271Validator.sol index b008e1d6..eb9f464a 100644 --- a/src/validators/ERC1271Validator.sol +++ b/src/validators/Erc1271Validator.sol @@ -9,6 +9,7 @@ contract Erc1271Validator is IStatelessValidator { function validate(bytes memory signerData, bytes32 hash, bytes memory signature) external view + override returns (bool isValid, bytes memory) { if (signerData.length == 0) { diff --git a/src/validators/WebAuthnValidator.sol b/src/validators/WebAuthnValidator.sol index fc9e5ef3..8463e40d 100644 --- a/src/validators/WebAuthnValidator.sol +++ b/src/validators/WebAuthnValidator.sol @@ -19,7 +19,8 @@ contract WebAuthnValidator is IStatelessValidator { function validate(bytes memory signerData, bytes32 hash, bytes memory signature) external view - returns (bool isValid, bytes memory result) + override + returns (bool isValid, bytes memory) { ( bytes memory authenticatorData, diff --git a/test/account/MultiValidation.t.sol b/test/account/MultiValidation.t.sol index 9b22f5a0..8afa4b73 100644 --- a/test/account/MultiValidation.t.sol +++ b/test/account/MultiValidation.t.sol @@ -15,6 +15,7 @@ import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; +import {Signer} from "../../src/validators/IStatelessValidator.sol"; contract MultiValidationTest is AccountTestBase { using ECDSA for bytes32; @@ -24,6 +25,7 @@ contract MultiValidationTest is AccountTestBase { address public owner2; uint256 public owner2Key; + Signer public signer2; uint256 public constant CALL_GAS_LIMIT = 50000; uint256 public constant VERIFICATION_GAS_LIMIT = 1200000; @@ -31,13 +33,15 @@ contract MultiValidationTest is AccountTestBase { function setUp() public { validator2 = new SingleOwnerPlugin(); + (owner1, owner1Key) = makeAddrAndKey("owner1"); (owner2, owner2Key) = makeAddrAndKey("owner2"); + signer2 = Signer(ecdsaValidator, abi.encode(address(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)); + account1.installPlugin(address(validator2), manifestHash, abi.encode(signer2), new FunctionReference[](0)); FunctionReference[] memory validations = new FunctionReference[](2); validations[0] = FunctionReferenceLib.pack( @@ -123,8 +127,14 @@ contract MultiValidationTest is AccountTestBase { userOp.nonce = 1; (v, r, s) = vm.sign(owner1Key, 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 + ); userOps[0] = userOp; vm.expectRevert(abi.encodeWithSelector(IEntryPoint.FailedOp.selector, 0, "AA24 signature error")); diff --git a/test/account/UpgradeableModularAccount.t.sol b/test/account/UpgradeableModularAccount.t.sol index 9ee7f2e8..18cf9567 100644 --- a/test/account/UpgradeableModularAccount.t.sol +++ b/test/account/UpgradeableModularAccount.t.sol @@ -25,7 +25,6 @@ import {MockPlugin} from "../mocks/MockPlugin.sol"; import {AccountTestBase} from "../utils/AccountTestBase.sol"; import {Signer} from "../../src/validators/IStatelessValidator.sol"; -import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; contract UpgradeableModularAccountTest is AccountTestBase { using ECDSA for bytes32; diff --git a/test/plugin/SingleOwnerPlugin.t.sol b/test/plugin/SingleOwnerPlugin.t.sol index 0379156d..d6ba120f 100644 --- a/test/plugin/SingleOwnerPlugin.t.sol +++ b/test/plugin/SingleOwnerPlugin.t.sol @@ -11,7 +11,7 @@ import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol import {ContractOwner} from "../mocks/ContractOwner.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; -import {Signer, IStatelessValidator} from "../../src/validators/IStatelessValidator.sol"; +import {Signer} from "../../src/validators/IStatelessValidator.sol"; import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; import {Erc1271Validator} from "../../src/validators/Erc1271Validator.sol"; From 9aee83bc23691a3badc9b9be181e224fc686000a Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Mon, 24 Jun 2024 15:42:13 -0700 Subject: [PATCH 7/8] add tests for validators --- test/validator/EcdsaAndErc1271Validator.t.sol | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 test/validator/EcdsaAndErc1271Validator.t.sol diff --git a/test/validator/EcdsaAndErc1271Validator.t.sol b/test/validator/EcdsaAndErc1271Validator.t.sol new file mode 100644 index 00000000..7834e3f9 --- /dev/null +++ b/test/validator/EcdsaAndErc1271Validator.t.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.19; + +import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; +import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; +import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; +import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; + +import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; +import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; +import {ContractOwner} from "../mocks/ContractOwner.sol"; +import {OptimizedTest} from "../utils/OptimizedTest.sol"; + +import {Signer} from "../../src/validators/IStatelessValidator.sol"; +import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; +import {Erc1271Validator} from "../../src/validators/Erc1271Validator.sol"; +import {WebAuthnValidator} from "../../src/validators/WebAuthnValidator.sol"; + +contract EcdsaAndErc1271ValidatorTest is OptimizedTest { + EcdsaValidator public ecdsaValidator; + Erc1271Validator public erc1271Validator; + + ContractOwner public contractOwner; + + function setUp() public { + ecdsaValidator = new EcdsaValidator(); + erc1271Validator = new Erc1271Validator(); + + contractOwner = new ContractOwner(); + } + + function testFuzz_EcdsaValidator(string memory salt, bytes32 digest) public { + (address signer, uint256 privateKey) = makeAddrAndKey(salt); + (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); + + (bool isValid, bytes memory result) = + ecdsaValidator.validate(abi.encode(signer), digest, abi.encodePacked(r, s, v)); + assertTrue(isValid); + assertEq(abi.decode(result, (address)), signer); + } + + function testFuzz_Erc1271Validator(bytes32 digest) public { + bytes memory signature = contractOwner.sign(digest); + + (bool isValid, bytes memory result) = + erc1271Validator.validate(abi.encode(address(contractOwner)), digest, signature); + assertTrue(isValid); + assertEq(result, ""); + } +} From 03f49ae723abdf5004e39346197bcd4ae5ff12eb Mon Sep 17 00:00:00 2001 From: Fangting Liu Date: Mon, 24 Jun 2024 15:44:41 -0700 Subject: [PATCH 8/8] add tests for validators --- .gitmodules | 3 +++ lib/FreshCryptoLib | 1 + src/validators/Base64URL.sol | 2 +- test/validator/EcdsaAndErc1271Validator.t.sol | 9 --------- 4 files changed, 5 insertions(+), 10 deletions(-) create mode 160000 lib/FreshCryptoLib diff --git a/.gitmodules b/.gitmodules index 813d955e..ee306f9e 100644 --- a/.gitmodules +++ b/.gitmodules @@ -7,3 +7,6 @@ [submodule "lib/forge-std"] path = lib/forge-std url = https://github.com/foundry-rs/forge-std +[submodule "lib/FreshCryptoLib"] + path = lib/FreshCryptoLib + url = https://github.com/rdubois-crypto/FreshCryptoLib diff --git a/lib/FreshCryptoLib b/lib/FreshCryptoLib new file mode 160000 index 00000000..fd2a0e6e --- /dev/null +++ b/lib/FreshCryptoLib @@ -0,0 +1 @@ +Subproject commit fd2a0e6e64609ade0b0c165e4d66de8106a70db6 diff --git a/src/validators/Base64URL.sol b/src/validators/Base64URL.sol index f8a498a7..8b9d5c94 100644 --- a/src/validators/Base64URL.sol +++ b/src/validators/Base64URL.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.25; -import "openzeppelin-contracts/contracts/utils/Base64.sol"; +import {Base64} from "@openzeppelin/contracts/utils/Base64.sol"; /// TODO this library is not necessary once Openzeppelin 5.0.2 Base64 is supported library Base64URL { diff --git a/test/validator/EcdsaAndErc1271Validator.t.sol b/test/validator/EcdsaAndErc1271Validator.t.sol index 7834e3f9..df5d1a89 100644 --- a/test/validator/EcdsaAndErc1271Validator.t.sol +++ b/test/validator/EcdsaAndErc1271Validator.t.sol @@ -1,20 +1,11 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.19; -import {EntryPoint} from "@eth-infinitism/account-abstraction/core/EntryPoint.sol"; -import {PackedUserOperation} from "@eth-infinitism/account-abstraction/interfaces/PackedUserOperation.sol"; -import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; -import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; - -import {SingleOwnerPlugin} from "../../src/plugins/owner/SingleOwnerPlugin.sol"; -import {ISingleOwnerPlugin} from "../../src/plugins/owner/ISingleOwnerPlugin.sol"; import {ContractOwner} from "../mocks/ContractOwner.sol"; import {OptimizedTest} from "../utils/OptimizedTest.sol"; -import {Signer} from "../../src/validators/IStatelessValidator.sol"; import {EcdsaValidator} from "../../src/validators/EcdsaValidator.sol"; import {Erc1271Validator} from "../../src/validators/Erc1271Validator.sol"; -import {WebAuthnValidator} from "../../src/validators/WebAuthnValidator.sol"; contract EcdsaAndErc1271ValidatorTest is OptimizedTest { EcdsaValidator public ecdsaValidator;