From 0c98b9de6ee150c47339093066d2ddab7f754f6f Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Tue, 12 Mar 2024 10:21:43 -0400 Subject: [PATCH] feat: use nested eip-712 approach for isValidSignature --- ext/solady/EIP712.sol | 208 ++++++++++++++++++++++++++++++ src/LightAccount.sol | 77 ++++------- src/MultiOwnerLightAccount.sol | 77 ++++------- src/common/BaseLightAccount.sol | 8 +- src/common/ERC1271.sol | 119 +++++++++++++++++ test/LightAccount.t.sol | 118 +++++++++++++++-- test/MultiOwnerLightAccount.t.sol | 117 +++++++++++++++-- 7 files changed, 594 insertions(+), 130 deletions(-) create mode 100644 ext/solady/EIP712.sol create mode 100644 src/common/ERC1271.sol diff --git a/ext/solady/EIP712.sol b/ext/solady/EIP712.sol new file mode 100644 index 0000000..2b6c1a3 --- /dev/null +++ b/ext/solady/EIP712.sol @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; + +/// @notice Contract for EIP-712 typed structured data hashing and signing. +/// @author Solady (https://github.com/vectorized/solady/blob/main/src/utils/EIP712.sol) +/// @author Modified from Solbase (https://github.com/Sol-DAO/solbase/blob/main/src/utils/EIP712.sol) +/// @author Modified from OpenZeppelin (https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/EIP712.sol) +/// +/// @dev Note, this implementation: +/// - Uses `address(this)` for the `verifyingContract` field. +/// - Does NOT use the optional EIP-712 salt. +/// - Does NOT use any EIP-712 extensions. +/// This is for simplicity and to save gas. +/// If you need to customize, please fork / modify accordingly. +abstract contract EIP712 { + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* CONSTANTS AND IMMUTABLES */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /// @dev `keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)")`. + bytes32 internal constant _DOMAIN_TYPEHASH = + 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f; + + uint256 private immutable _cachedThis; + uint256 private immutable _cachedChainId; + bytes32 private immutable _cachedNameHash; + bytes32 private immutable _cachedVersionHash; + bytes32 private immutable _cachedDomainSeparator; + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* CONSTRUCTOR */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /// @dev Cache the hashes for cheaper runtime gas costs. + /// In the case of upgradeable contracts (i.e. proxies), + /// or if the chain id changes due to a hard fork, + /// the domain separator will be seamlessly calculated on-the-fly. + constructor() { + _cachedThis = uint256(uint160(address(this))); + _cachedChainId = block.chainid; + + string memory name; + string memory version; + if (!_domainNameAndVersionMayChange()) (name, version) = _domainNameAndVersion(); + bytes32 nameHash = _domainNameAndVersionMayChange() ? bytes32(0) : keccak256(bytes(name)); + bytes32 versionHash = + _domainNameAndVersionMayChange() ? bytes32(0) : keccak256(bytes(version)); + _cachedNameHash = nameHash; + _cachedVersionHash = versionHash; + + bytes32 separator; + if (!_domainNameAndVersionMayChange()) { + /// @solidity memory-safe-assembly + assembly { + let m := mload(0x40) // Load the free memory pointer. + mstore(m, _DOMAIN_TYPEHASH) + mstore(add(m, 0x20), nameHash) + mstore(add(m, 0x40), versionHash) + mstore(add(m, 0x60), chainid()) + mstore(add(m, 0x80), address()) + separator := keccak256(m, 0xa0) + } + } + _cachedDomainSeparator = separator; + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* FUNCTIONS TO OVERRIDE */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /// @dev Please override this function to return the domain name and version. + /// ``` + /// function _domainNameAndVersion() + /// internal + /// pure + /// virtual + /// returns (string memory name, string memory version) + /// { + /// name = "Solady"; + /// version = "1"; + /// } + /// ``` + /// + /// Note: If the returned result may change after the contract has been deployed, + /// you must override `_domainNameAndVersionMayChange()` to return true. + function _domainNameAndVersion() + internal + view + virtual + returns (string memory name, string memory version); + + /// @dev Returns if `_domainNameAndVersion()` may change + /// after the contract has been deployed (i.e. after the constructor). + /// Default: false. + function _domainNameAndVersionMayChange() internal pure virtual returns (bool result) {} + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* HASHING OPERATIONS */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /// @dev Returns the EIP-712 domain separator. + function _domainSeparator() internal view virtual returns (bytes32 separator) { + if (_domainNameAndVersionMayChange()) { + separator = _buildDomainSeparator(); + } else { + separator = _cachedDomainSeparator; + if (_cachedDomainSeparatorInvalidated()) separator = _buildDomainSeparator(); + } + } + + /// @dev Returns the hash of the fully encoded EIP-712 message for this domain, + /// given `structHash`, as defined in + /// https://eips.ethereum.org/EIPS/eip-712#definition-of-hashstruct. + /// + /// The hash can be used together with {ECDSA-recover} to obtain the signer of a message: + /// ``` + /// bytes32 digest = _hashTypedData(keccak256(abi.encode( + /// keccak256("Mail(address to,string contents)"), + /// mailTo, + /// keccak256(bytes(mailContents)) + /// ))); + /// address signer = ECDSA.recover(digest, signature); + /// ``` + function _hashTypedData(bytes32 structHash) internal view virtual returns (bytes32 digest) { + // We will use `digest` to store the domain separator to save a bit of gas. + if (_domainNameAndVersionMayChange()) { + digest = _buildDomainSeparator(); + } else { + digest = _cachedDomainSeparator; + if (_cachedDomainSeparatorInvalidated()) digest = _buildDomainSeparator(); + } + /// @solidity memory-safe-assembly + assembly { + // Compute the digest. + mstore(0x00, 0x1901000000000000) // Store "\x19\x01". + mstore(0x1a, digest) // Store the domain separator. + mstore(0x3a, structHash) // Store the struct hash. + digest := keccak256(0x18, 0x42) + // Restore the part of the free memory slot that was overwritten. + mstore(0x3a, 0) + } + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* EIP-5267 OPERATIONS */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /// @dev See: https://eips.ethereum.org/EIPS/eip-5267 + function eip712Domain() + public + view + virtual + returns ( + bytes1 fields, + string memory name, + string memory version, + uint256 chainId, + address verifyingContract, + bytes32 salt, + uint256[] memory extensions + ) + { + fields = hex"0f"; // `0b01111`. + (name, version) = _domainNameAndVersion(); + chainId = block.chainid; + verifyingContract = address(this); + salt = salt; // `bytes32(0)`. + extensions = extensions; // `new uint256[](0)`. + } + + /*´:°•.°+.*•´.*:˚.°*.˚•´.°:°•.°•.*•´.*:˚.°*.˚•´.°:°•.°+.*•´.*:*/ + /* PRIVATE HELPERS */ + /*.•°:°.´+˚.*°.˚:*.´•*.+°.•°:´*.´•*.•°.•°:°.´:•˚°.*°.˚:*.´+°.•*/ + + /// @dev Returns the EIP-712 domain separator. + function _buildDomainSeparator() private view returns (bytes32 separator) { + // We will use `separator` to store the name hash to save a bit of gas. + bytes32 versionHash; + if (_domainNameAndVersionMayChange()) { + (string memory name, string memory version) = _domainNameAndVersion(); + separator = keccak256(bytes(name)); + versionHash = keccak256(bytes(version)); + } else { + separator = _cachedNameHash; + versionHash = _cachedVersionHash; + } + /// @solidity memory-safe-assembly + assembly { + let m := mload(0x40) // Load the free memory pointer. + mstore(m, _DOMAIN_TYPEHASH) + mstore(add(m, 0x20), separator) // Name hash. + mstore(add(m, 0x40), versionHash) + mstore(add(m, 0x60), chainid()) + mstore(add(m, 0x80), address()) + separator := keccak256(m, 0xa0) + } + } + + /// @dev Returns if the cached domain separator has been invalidated. + function _cachedDomainSeparatorInvalidated() private view returns (bool result) { + uint256 cachedChainId = _cachedChainId; + uint256 cachedThis = _cachedThis; + /// @solidity memory-safe-assembly + assembly { + result := iszero(and(eq(chainid(), cachedChainId), eq(address(), cachedThis))) + } + } +} \ No newline at end of file diff --git a/src/LightAccount.sol b/src/LightAccount.sol index fa78170..b2ece15 100644 --- a/src/LightAccount.sol +++ b/src/LightAccount.sol @@ -5,7 +5,6 @@ pragma solidity ^0.8.23; /* solhint-disable no-inline-assembly */ /* solhint-disable reason-string */ -import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; @@ -47,11 +46,6 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { // keccak256(abi.encode(uint256(keccak256("light_account_v1.initializable")) - 1)) & ~bytes32(uint256(0xff)); bytes32 internal constant _INITIALIZABLE_STORAGE_POSITION = 0x33e4b41198cc5b8053630ed667ea7c0c4c873f7fc8d9a478b5d7259cec0a4a00; - bytes32 internal constant _DOMAIN_SEPARATOR_TYPEHASH = - keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - bytes32 internal constant _LA_MSG_TYPEHASH = keccak256("LightAccountMessage(bytes message)"); - bytes32 internal constant _NAME_HASH = keccak256("LightAccount"); - bytes32 internal constant _VERSION_HASH = keccak256("1"); struct LightAccountStorage { address owner; @@ -102,48 +96,6 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { return _getStorage().owner; } - /// @notice Returns the domain separator for this contract, as defined in the EIP-712 standard. - /// @return bytes32 The domain separator hash. - function domainSeparator() public view returns (bytes32) { - return keccak256( - abi.encode( - _DOMAIN_SEPARATOR_TYPEHASH, - _NAME_HASH, // name - _VERSION_HASH, // version - block.chainid, // chainId - address(this) // verifying contract - ) - ); - } - - /// @notice Returns the pre-image of the message hash. - /// @param message Message that should be encoded. - /// @return Encoded message. - function encodeMessageData(bytes memory message) public view returns (bytes memory) { - bytes32 messageHash = keccak256(abi.encode(_LA_MSG_TYPEHASH, keccak256(message))); - return abi.encodePacked("\x19\x01", domainSeparator(), messageHash); - } - - /// @notice Returns hash of a message that can be signed by owners. - /// @param message Message that should be hashed. - /// @return Message hash. - function getMessageHash(bytes memory message) public view returns (bytes32) { - return keccak256(encodeMessageData(message)); - } - - /// @inheritdoc IERC1271 - /// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a - /// valid ERC-1271 signature from the owner (if the owner is a contract). Note that unlike the signature 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 isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4) { - bytes32 messageHash = getMessageHash(abi.encode(hash)); - if (SignatureChecker.isValidSignatureNow(owner(), messageHash, signature)) { - return _1271_MAGIC_VALUE; - } - return 0xffffffff; - } - function _initialize(address owner_) internal virtual { if (owner_ == address(0)) { revert InvalidOwner(address(0)); @@ -172,19 +124,42 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { override returns (uint256 validationData) { - address _owner = owner(); + address owner_ = owner(); bytes32 signedHash = userOpHash.toEthSignedMessageHash(); bytes memory signature = userOp.signature; (address recovered, ECDSA.RecoverError error,) = signedHash.tryRecover(signature); if ( - (error == ECDSA.RecoverError.NoError && recovered == _owner) - || SignatureChecker.isValidERC1271SignatureNow(_owner, userOpHash, signature) + (error == ECDSA.RecoverError.NoError && recovered == owner_) + || SignatureChecker.isValidERC1271SignatureNow(owner_, userOpHash, signature) ) { return 0; } return SIG_VALIDATION_FAILED; } + /// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a + /// valid ERC-1271 signature from the owner (if the owner is a contract). + function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) + internal + view + virtual + override + returns (bool) + { + return SignatureChecker.isValidSignatureNow(owner(), derivedHash, trimmedSignature); + } + + function _domainNameAndVersion() + internal + view + virtual + override + returns (string memory name, string memory version) + { + name = "LightAccount"; + version = "2"; + } + function _isFromOwner() internal view virtual override returns (bool) { return msg.sender == owner(); } diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index d660eec..bb7b950 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; -import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {ECDSA} from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import {MessageHashUtils} from "@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol"; import {SignatureChecker} from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; @@ -30,11 +29,6 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { // keccak256(abi.encode(uint256(keccak256("multi_owner_light_account_v1.initializable")) - 1)) & ~bytes32(uint256(0xff)); bytes32 internal constant _INITIALIZABLE_STORAGE_POSITION = 0xaa296a366a62f6551d3ddfceae892d1791068a359a0d3461aab99dfc6c5fd700; - bytes32 internal constant _DOMAIN_SEPARATOR_TYPEHASH = - keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"); - bytes32 internal constant _LA_MSG_TYPEHASH = keccak256("MultiOwnerLightAccountMessage(bytes message)"); - bytes32 internal constant _NAME_HASH = keccak256("MultiOwnerLightAccount"); - bytes32 internal constant _VERSION_HASH = keccak256("1"); struct LightAccountStorage { LinkedListSet owners; @@ -91,52 +85,6 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { return _getStorage().owners.getAll().toAddressArray(); } - /// @notice Returns the domain separator for this contract, as defined in the EIP-712 standard. - /// @return bytes32 The domain separator hash. - function domainSeparator() public view returns (bytes32) { - return keccak256( - abi.encode( - _DOMAIN_SEPARATOR_TYPEHASH, - _NAME_HASH, // name - _VERSION_HASH, // version - block.chainid, // chainId - address(this) // verifying contract - ) - ); - } - - /// @notice Returns the pre-image of the message hash. - /// @param message Message that should be encoded. - /// @return Encoded message. - function encodeMessageData(bytes memory message) public view returns (bytes memory) { - bytes32 messageHash = keccak256(abi.encode(_LA_MSG_TYPEHASH, keccak256(message))); - return abi.encodePacked("\x19\x01", domainSeparator(), messageHash); - } - - /// @notice Returns hash of a message that can be signed by owners. - /// @param message Message that should be hashed. - /// @return Message hash. - function getMessageHash(bytes memory message) public view returns (bytes32) { - return keccak256(encodeMessageData(message)); - } - - /// @inheritdoc IERC1271 - /// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a - /// valid ERC-1271 signature from the owner (if the owner is a contract). Note that unlike the signature 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 isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4) { - bytes32 messageHash = getMessageHash(abi.encode(hash)); - (address recovered, ECDSA.RecoverError error,) = messageHash.tryRecover(signature); - if (error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(CastLib.toSetValue(recovered))) { - return _1271_MAGIC_VALUE; - } - if (_isValidERC1271SignatureNow(messageHash, signature)) { - return _1271_MAGIC_VALUE; - } - return 0xffffffff; - } - function _initialize(address[] calldata owners_) internal virtual { emit LightAccountInitialized(_ENTRY_POINT, owners_); _updateOwners(owners_, new address[](0)); @@ -210,6 +158,31 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { return false; } + /// @dev The signature is valid if it is signed by the owner's private key (if the owner is an EOA) or if it is a + /// valid ERC-1271 signature from the owner (if the owner is a contract). + function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) + internal + view + virtual + override + returns (bool) + { + (address recovered, ECDSA.RecoverError error,) = derivedHash.tryRecover(trimmedSignature); + return (error == ECDSA.RecoverError.NoError && _getStorage().owners.contains(CastLib.toSetValue(recovered))) + || _isValidERC1271SignatureNow(derivedHash, trimmedSignature); + } + + function _domainNameAndVersion() + internal + view + virtual + override + returns (string memory name, string memory version) + { + name = "MultiOwnerLightAccount"; + version = "1"; + } + function _isFromOwner() internal view virtual override returns (bool) { return _getStorage().owners.contains(msg.sender.toSetValue()); } diff --git a/src/common/BaseLightAccount.sol b/src/common/BaseLightAccount.sol index 6c35112..e48e577 100644 --- a/src/common/BaseLightAccount.sol +++ b/src/common/BaseLightAccount.sol @@ -1,7 +1,6 @@ // SPDX-License-Identifier: GPL-3.0 pragma solidity ^0.8.23; -import {IERC1271} from "@openzeppelin/contracts/interfaces/IERC1271.sol"; import {BaseAccount} from "account-abstraction/core/BaseAccount.sol"; import {SIG_VALIDATION_FAILED} from "account-abstraction/core/Helpers.sol"; import {IEntryPoint} from "account-abstraction/interfaces/IEntryPoint.sol"; @@ -9,8 +8,9 @@ import {PackedUserOperation} from "account-abstraction/interfaces/PackedUserOper import {TokenCallbackHandler} from "account-abstraction/samples/callback/TokenCallbackHandler.sol"; import {UUPSUpgradeable} from "../../ext/solady/UUPSUpgradeable.sol"; +import {ERC1271} from "./ERC1271.sol"; -abstract contract BaseLightAccount is BaseAccount, TokenCallbackHandler, UUPSUpgradeable, IERC1271 { +abstract contract BaseLightAccount is BaseAccount, TokenCallbackHandler, UUPSUpgradeable, ERC1271 { bytes4 internal constant _1271_MAGIC_VALUE = bytes4(keccak256("isValidSignature(bytes32,bytes)")); // 0x1626ba7e IEntryPoint internal immutable _ENTRY_POINT; @@ -92,10 +92,6 @@ abstract contract BaseLightAccount is BaseAccount, TokenCallbackHandler, UUPSUpg return entryPoint().balanceOf(address(this)); } - /// @inheritdoc IERC1271 - /// @dev Must override to support ERC-1271 signature validation. - function isValidSignature(bytes32 hash, bytes memory signature) public view virtual override returns (bytes4); - /// @dev Must override to support user op validation. function _validateSignature(PackedUserOperation calldata userOp, bytes32 userOpHash) internal diff --git a/src/common/ERC1271.sol b/src/common/ERC1271.sol new file mode 100644 index 0000000..8a9ac65 --- /dev/null +++ b/src/common/ERC1271.sol @@ -0,0 +1,119 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.23; + +import {EIP712} from "../../ext/solady/EIP712.sol"; + +/// @title ERC-1271 implementation using nested EIP-712 for replay protection. +/// @dev Identical to Solady's ERC1271, with a minor change to support overriding the signature verification logic. +/// @author Solady (https://github.com/vectorized/solady/blob/main/src/accounts/ERC1271.sol) +/// @author Alchemy +abstract contract ERC1271 is EIP712 { + /// @dev Validates the signature with ERC1271 return, + /// so that this account can also be used as a signer. + /// + /// This implementation uses ECDSA recovery. It also uses a nested EIP-712 approach to + /// prevent signature replays when a single EOA owns multiple smart contract accounts, + /// while still enabling wallet UIs (e.g. Metamask) to show the EIP-712 values. + /// + /// For the nested EIP-712 workflow, the final hash will be: + /// ``` + /// keccak256(\x19\x01 || DOMAIN_SEP_A || + /// hashStruct(Parent({ + /// childHash: keccak256(\x19\x01 || DOMAIN_SEP_B || hashStruct(originalStruct)), + /// child: hashStruct(originalStruct) + /// })) + /// ) + /// ``` + /// where `||` denotes the concatenation operator for bytes. + /// The signature will be `r || s || v || PARENT_TYPEHASH || DOMAIN_SEP_B || child`. + /// + /// The `DOMAIN_SEP_B` and `child` will be used to verify if `childHash` is indeed correct. + /// + /// For the `personal_sign` workflow, the final hash will be: + /// ``` + /// keccak256(\x19\x01 || DOMAIN_SEP_A || + /// hashStruct(Parent({ + /// childHash: personalSign(someBytes) + /// })) + /// ) + /// ``` + /// where `||` denotes the concatenation operator for bytes. + /// The signature will be `r || s || v || PARENT_TYPEHASH`. + /// + /// For demo and typescript code, see: + /// - https://github.com/junomonster/nested-eip-712 + /// - https://github.com/frangio/eip712-wrapper-for-eip1271 + /// + /// Of course, if you are a wallet app maker and can update your app's UI at will, + /// you can choose a more minimalistic signature scheme like + /// `keccak256(abi.encode(address(this), hash))` instead of all these acrobatics. + /// All these are just for widespead out-of-the-box compatibility with other wallet apps. + /// + /// The `hash` parameter is the `childHash`. + function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4 result) { + /// @solidity memory-safe-assembly + assembly { + let m := mload(0x40) // Cache the free memory pointer. + let o := add(signature.offset, sub(signature.length, 0x60)) + calldatacopy(0x00, o, 0x60) // Copy the `DOMAIN_SEP_B` and child's structHash. + mstore(0x00, 0x1901) // Store the "\x19\x01" prefix, overwriting 0x00. + for {} 1 {} { + // Use the nested EIP-712 workflow if the reconstructed childHash matches, + // and the signature is at least 96 bytes long. + if iszero(or(xor(keccak256(0x1e, 0x42), hash), lt(signature.length, 0x60))) { + // Truncate the `signature.length` by 3 words (96 bytes). + signature.length := sub(signature.length, 0x60) + mstore(0x00, calldataload(o)) // Store the `PARENT_TYPEHASH`. + mstore(0x20, hash) // Store the `childHash`. + // The child's structHash is already at 0x40. + hash := keccak256(0x00, 0x60) // Compute the parent's structHash. + break + } + // Else, use the `personal_sign` workflow. + // Truncate the `signature.length` by 1 word (32 bytes), until zero. + signature.length := mul(gt(signature.length, 0x20), sub(signature.length, 0x20)) + // The `PARENT_TYPEHASH` is already at 0x40. + mstore(0x60, hash) // Store the `childHash`. + hash := keccak256(0x40, 0x40) // Compute the parent's structHash. + mstore(0x60, 0) // Restore the zero pointer. + break + } + mstore(0x40, m) // Restore the free memory pointer. + } + bool success = _isValidSignature(_hashTypedData(hash), signature); + /// @solidity memory-safe-assembly + assembly { + // `success ? bytes4(keccak256("isValidSignature(bytes32,bytes)")) : 0xffffffff`. + result := shl(224, or(0x1626ba7e, sub(0, iszero(success)))) + } + } + + /// @dev Must override to provide the signature verification logic. + /// For the nested EIP-712 workflow, the final hash will be: + /// ``` + /// keccak256(\x19\x01 || DOMAIN_SEP_A || + /// hashStruct(Parent({ + /// childHash: keccak256(\x19\x01 || DOMAIN_SEP_B || hashStruct(originalStruct)), + /// child: hashStruct(originalStruct) + /// })) + /// ) + /// ``` + /// + /// For the `personal_sign` workflow, the final hash will be: + /// ``` + /// keccak256(\x19\x01 || DOMAIN_SEP_A || + /// hashStruct(Parent({ + /// childHash: personalSign(someBytes) + /// })) + /// ) + /// ``` + /// @param derivedHash The final hash that is derived from the original hash and signature passed to + /// `isValidSignature`. + /// @param trimmedSignature The actual signature component of the signature passed to `isValidSignature`. + /// @return Whether the signature is valid. + function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) + internal + view + virtual + returns (bool); +} diff --git a/test/LightAccount.t.sol b/test/LightAccount.t.sol index 72bf6c4..0694d1f 100644 --- a/test/LightAccount.t.sol +++ b/test/LightAccount.t.sol @@ -23,9 +23,10 @@ contract LightAccountTest is Test { uint256 public constant EOA_PRIVATE_KEY = 1; address payable public constant BENEFICIARY = payable(address(0xbe9ef1c1a2ee)); + bytes32 internal constant _PARENT_TYPEHASH = keccak256("Parent(bytes32 childHash,Mail child)Mail(string contents)"); + bytes32 internal constant _CHILD_TYPEHASH = keccak256("Mail(string contents)"); address public eoaAddress; LightAccount public account; - LightAccount public contractOwnedAccount; EntryPoint public entryPoint; LightSwitch public lightSwitch; Owner public contractOwner; @@ -225,22 +226,76 @@ contract LightAccountTest is Test { } function testIsValidSignatureForEoaOwner() public { - bytes32 digest = keccak256("digest"); - bytes memory signature = _sign(EOA_PRIVATE_KEY, account.getMessageHash(abi.encode(digest))); - assertEq(account.isValidSignature(digest, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = abi.encodePacked( + _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child + ); + assertEq( + account.isValidSignature(_toChildHash(child), signature), + bytes4(keccak256("isValidSignature(bytes32,bytes)")) + ); } function testIsValidSignatureForContractOwner() public { _useContractOwner(); - bytes32 digest = keccak256("digest"); - bytes memory signature = contractOwner.sign(account.getMessageHash(abi.encode(digest))); - assertEq(account.isValidSignature(digest, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = + abi.encodePacked(contractOwner.sign(_toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child); + assertEq( + account.isValidSignature(_toChildHash(child), signature), + bytes4(keccak256("isValidSignature(bytes32,bytes)")) + ); } function testIsValidSignatureRejectsInvalid() public { - bytes32 digest = keccak256("digest"); - bytes memory signature = _sign(123, account.getMessageHash(abi.encode(digest))); - assertEq(account.isValidSignature(digest, signature), bytes4(0xffffffff)); + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = + abi.encodePacked(_sign(123, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child); + assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); + + signature = abi.encodePacked( + _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorA(), child + ); + assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); + + assertEq(account.isValidSignature(_toChildHash(child), ""), bytes4(0xffffffff)); + } + + function testIsValidSignaturePersonalSign() public { + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = + abi.encodePacked(_sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + } + + function testIsValidSignaturePersonalSignForContractOwner() public { + _useContractOwner(); + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = + abi.encodePacked(contractOwner.sign(_toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + } + + function testIsValidSignaturePersonalSignRejectsInvalid() public { + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = abi.encodePacked(_sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); + + signature = abi.encodePacked( + _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + childHash + ); + assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); + + assertEq(account.isValidSignature(childHash, ""), bytes4(0xffffffff)); } function testOwnerCanUpgrade() public { @@ -288,7 +343,7 @@ contract LightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x67451f36b4201f5ba07b252318016021456072e6ac603ba84431630cfc7ecac3 + 0xc3bc748e3b230102604330f3f7926d87f837e7efab3a3af8b0a0ac0133bf248c ); } @@ -333,6 +388,47 @@ contract LightAccountTest is Test { (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, digest); return abi.encodePacked(r, s, v); } + + function _toERC1271Hash(bytes32 child) internal view returns (bytes32) { + bytes32 parentStructHash = keccak256(abi.encode(_PARENT_TYPEHASH, _toChildHash(child), child)); + return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorA(), parentStructHash)); + } + + function _toERC1271HashPersonalSign(bytes32 childHash) internal view returns (bytes32) { + bytes32 parentStructHash = keccak256(abi.encode(_PARENT_TYPEHASH, childHash)); + return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorA(), parentStructHash)); + } + + function _toChildHash(bytes32 child) internal view returns (bytes32) { + return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorB(), child)); + } + + /// @dev Domain separator for the parent struct. + function _domainSeparatorA() internal view returns (bytes32) { + (, string memory name, string memory version,,,,) = account.eip712Domain(); + return keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256(bytes(name)), + keccak256(bytes(version)), + block.chainid, + address(account) + ) + ); + } + + /// @dev Domain separator for the child struct. + function _domainSeparatorB() internal view returns (bytes32) { + return keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("Mail"), + keccak256("1"), + block.chainid, + address(1) + ) + ); + } } contract LightSwitch { diff --git a/test/MultiOwnerLightAccount.t.sol b/test/MultiOwnerLightAccount.t.sol index fc24657..e4d38ca 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -25,6 +25,8 @@ contract MultiOwnerLightAccountTest is Test { uint256 public constant EOA_PRIVATE_KEY = 1; address payable public constant BENEFICIARY = payable(address(0xbe9ef1c1a2ee)); + bytes32 internal constant _PARENT_TYPEHASH = keccak256("Parent(bytes32 childHash,Mail child)Mail(string contents)"); + bytes32 internal constant _CHILD_TYPEHASH = keccak256("Mail(string contents)"); address public eoaAddress; MultiOwnerLightAccount public account; MultiOwnerLightAccount public contractOwnedAccount; @@ -266,22 +268,76 @@ contract MultiOwnerLightAccountTest is Test { } function testIsValidSignatureForEoaOwner() public { - bytes32 digest = keccak256("digest"); - bytes memory signature = _sign(EOA_PRIVATE_KEY, account.getMessageHash(abi.encode(digest))); - assertEq(account.isValidSignature(digest, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = abi.encodePacked( + _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child + ); + assertEq( + account.isValidSignature(_toChildHash(child), signature), + bytes4(keccak256("isValidSignature(bytes32,bytes)")) + ); } function testIsValidSignatureForContractOwner() public { _useContractOwner(); - bytes32 digest = keccak256("digest"); - bytes memory signature = contractOwner.sign(account.getMessageHash(abi.encode(digest))); - assertEq(account.isValidSignature(digest, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = + abi.encodePacked(contractOwner.sign(_toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child); + assertEq( + account.isValidSignature(_toChildHash(child), signature), + bytes4(keccak256("isValidSignature(bytes32,bytes)")) + ); } function testIsValidSignatureRejectsInvalid() public { - bytes32 digest = keccak256("digest"); - bytes memory signature = _sign(123, account.getMessageHash(abi.encode(digest))); - assertEq(account.isValidSignature(digest, signature), bytes4(0xffffffff)); + bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes memory signature = + abi.encodePacked(_sign(123, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child); + assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); + + signature = abi.encodePacked( + _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorA(), child + ); + assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); + + assertEq(account.isValidSignature(_toChildHash(child), ""), bytes4(0xffffffff)); + } + + function testIsValidSignaturePersonalSign() public { + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = + abi.encodePacked(_sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + } + + function testIsValidSignaturePersonalSignForContractOwner() public { + _useContractOwner(); + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = + abi.encodePacked(contractOwner.sign(_toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); + } + + function testIsValidSignaturePersonalSignRejectsInvalid() public { + string memory message = "hello world"; + bytes32 childHash = + keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n", bytes(message).length, message)); + bytes memory signature = abi.encodePacked(_sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH); + assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); + + signature = abi.encodePacked( + _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), + _PARENT_TYPEHASH, + _domainSeparatorB(), + childHash + ); + assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); + + assertEq(account.isValidSignature(childHash, ""), bytes4(0xffffffff)); } function testOwnerCanUpgrade() public { @@ -330,7 +386,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0x4c8a83770489550019a83bd0af91c7577ea978131b78945f87c63d1ec36036e0 + 0x60712973c68a63453f650d73d8eab281ae1293370703c13aaaaa76595a769661 ); } @@ -389,6 +445,47 @@ contract MultiOwnerLightAccountTest is Test { storageStruct.slot := position } } + + function _toERC1271Hash(bytes32 child) internal view returns (bytes32) { + bytes32 parentStructHash = keccak256(abi.encode(_PARENT_TYPEHASH, _toChildHash(child), child)); + return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorA(), parentStructHash)); + } + + function _toERC1271HashPersonalSign(bytes32 childHash) internal view returns (bytes32) { + bytes32 parentStructHash = keccak256(abi.encode(_PARENT_TYPEHASH, childHash)); + return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorA(), parentStructHash)); + } + + function _toChildHash(bytes32 child) internal view returns (bytes32) { + return keccak256(abi.encodePacked("\x19\x01", _domainSeparatorB(), child)); + } + + /// @dev Domain separator for the parent struct. + function _domainSeparatorA() internal view returns (bytes32) { + (, string memory name, string memory version,,,,) = account.eip712Domain(); + return keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256(bytes(name)), + keccak256(bytes(version)), + block.chainid, + address(account) + ) + ); + } + + /// @dev Domain separator for the child struct. + function _domainSeparatorB() internal view returns (bytes32) { + return keccak256( + abi.encode( + keccak256("EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"), + keccak256("Mail"), + keccak256("1"), + block.chainid, + address(1) + ) + ); + } } contract LightSwitch {