From 72d4aea810b1c49d129c47293563165c5bb02ce4 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Thu, 18 Apr 2024 18:01:32 -0400 Subject: [PATCH 1/3] feat: revert to previous 1271 implementation --- src/LightAccount.sol | 14 ++-- src/MultiOwnerLightAccount.sol | 16 ++-- src/common/ERC1271.sol | 130 ++++++--------------------------- 3 files changed, 39 insertions(+), 121 deletions(-) diff --git a/src/LightAccount.sol b/src/LightAccount.sol index 1baced2..b962dc1 100644 --- a/src/LightAccount.sol +++ b/src/LightAccount.sol @@ -159,25 +159,25 @@ contract LightAccount is BaseLightAccount, CustomSlotInitializable { /// @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). Reverts if the signature is malformed. - function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) + /// Note that unlike the signature validation used in `validateUserOp`, this does **not** wrap the hash in an + /// "Ethereum Signed Message" envelope before checking the signature in the EOA-owner case. + function _isValidSignature(bytes32 replaySafeHash, bytes calldata signature) internal view virtual override returns (bool) { - if (trimmedSignature.length < 1) { + if (signature.length < 1) { revert InvalidSignatureType(); } - uint8 signatureType = uint8(trimmedSignature[0]); + uint8 signatureType = uint8(signature[0]); if (signatureType == uint8(SignatureType.EOA)) { // EOA signature - bytes memory signature = trimmedSignature[1:]; - return _isValidEOAOwnerSignature(derivedHash, signature); + return _isValidEOAOwnerSignature(replaySafeHash, signature[1:]); } else if (signatureType == uint8(SignatureType.CONTRACT)) { // Contract signature without address - bytes memory signature = trimmedSignature[1:]; - return _isValidContractOwnerSignatureNow(derivedHash, signature); + return _isValidContractOwnerSignatureNow(replaySafeHash, signature[1:]); } revert InvalidSignatureType(); } diff --git a/src/MultiOwnerLightAccount.sol b/src/MultiOwnerLightAccount.sol index 771a59d..c7e1882 100644 --- a/src/MultiOwnerLightAccount.sol +++ b/src/MultiOwnerLightAccount.sol @@ -181,26 +181,26 @@ contract MultiOwnerLightAccount is BaseLightAccount, CustomSlotInitializable { /// @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). Reverts if the signature is malformed. - function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) + /// Note that unlike the signature validation used in `validateUserOp`, this does **not** wrap the hash in an + /// "Ethereum Signed Message" envelope before checking the signature in the EOA-owner case. + function _isValidSignature(bytes32 replaySafeHash, bytes calldata signature) internal view virtual override returns (bool) { - if (trimmedSignature.length < 1) { + if (signature.length < 1) { revert InvalidSignatureType(); } - uint8 signatureType = uint8(trimmedSignature[0]); + uint8 signatureType = uint8(signature[0]); if (signatureType == uint8(SignatureType.EOA)) { // EOA signature - bytes memory signature = trimmedSignature[1:]; - return _isValidEOAOwnerSignature(derivedHash, signature); + return _isValidEOAOwnerSignature(replaySafeHash, signature[1:]); } else if (signatureType == uint8(SignatureType.CONTRACT_WITH_ADDR)) { // Contract signature with address - address contractOwner = address(bytes20(trimmedSignature[1:21])); - bytes memory signature = trimmedSignature[21:]; - return _isValidContractOwnerSignatureNow(contractOwner, derivedHash, signature); + address contractOwner = address(bytes20(signature[1:21])); + return _isValidContractOwnerSignatureNow(contractOwner, replaySafeHash, signature[21:]); } revert InvalidSignatureType(); } diff --git a/src/common/ERC1271.sol b/src/common/ERC1271.sol index 1b31d68..5429a26 100644 --- a/src/common/ERC1271.sol +++ b/src/common/ERC1271.sol @@ -3,117 +3,35 @@ pragma solidity ^0.8.23; import {EIP712} from "../external/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`. + /// @dev bytes4(keccak256("isValidSignature(bytes32,bytes)")) + bytes4 internal constant _1271_MAGIC_VALUE_SUCCESS = 0x1626ba7e; + bytes4 internal constant _1271_MAGIC_VALUE_FAILURE = 0xffffffff; + bytes32 internal constant _MESSAGE_TYPEHASH = keccak256("LightAccountMessage(bytes message)"); + + /// @notice Returns the replay-safe hash of a message that can be signed by owners. + /// @param message Message that should be hashed. + /// @return The replay-safe message hash. + function getMessageHash(bytes memory message) public view returns (bytes32) { + bytes32 structHash = keccak256(abi.encode(_MESSAGE_TYPEHASH, keccak256(message))); + return _hashTypedData(structHash); + } + + /// @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). + /// @param hash Hash of the data to be signed. + /// @param signature Signature byte array associated with the data. + /// @return result Magic value `0x1626ba7e` if validation succeeded, else `0xffffffff`. 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)))) + if (_isValidSignature(getMessageHash(abi.encode(hash)), signature)) { + return _1271_MAGIC_VALUE_SUCCESS; } + return _1271_MAGIC_VALUE_FAILURE; } /// @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`. + /// @param replaySafeHash The replay-safe hash that is derived from the original message. + /// @param signature The signature passed to `isValidSignature`. /// @return Whether the signature is valid. - function _isValidSignature(bytes32 derivedHash, bytes calldata trimmedSignature) - internal - view - virtual - returns (bool); + function _isValidSignature(bytes32 replaySafeHash, bytes calldata signature) internal view virtual returns (bool); } From 74963c142a057ea5ca8879a76739f6f6dbfdd858 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Thu, 18 Apr 2024 20:06:40 -0400 Subject: [PATCH 2/3] test: update tests for 1271 --- src/common/ERC1271.sol | 4 +- test/LightAccount.t.sol | 156 +++++------------------ test/MultiOwnerLightAccount.t.sol | 201 +++++++----------------------- 3 files changed, 80 insertions(+), 281 deletions(-) diff --git a/src/common/ERC1271.sol b/src/common/ERC1271.sol index 5429a26..3378385 100644 --- a/src/common/ERC1271.sol +++ b/src/common/ERC1271.sol @@ -21,8 +21,8 @@ abstract contract ERC1271 is EIP712 { /// a valid ERC-1271 signature from the owner (if the owner is a contract). /// @param hash Hash of the data to be signed. /// @param signature Signature byte array associated with the data. - /// @return result Magic value `0x1626ba7e` if validation succeeded, else `0xffffffff`. - function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4 result) { + /// @return Magic value `0x1626ba7e` if validation succeeded, else `0xffffffff`. + function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4) { if (_isValidSignature(getMessageHash(abi.encode(hash)), signature)) { return _1271_MAGIC_VALUE_SUCCESS; } diff --git a/test/LightAccount.t.sol b/test/LightAccount.t.sol index 2258b73..948ddcd 100644 --- a/test/LightAccount.t.sol +++ b/test/LightAccount.t.sol @@ -22,8 +22,7 @@ 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)"); + bytes32 internal constant _MESSAGE_TYPEHASH = keccak256("LightAccountMessage(bytes message)"); address public eoaAddress; LightAccount public account; EntryPoint public entryPoint; @@ -349,110 +348,43 @@ contract LightAccountTest is Test { } function testIsValidSignatureForEoaOwner() public { - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes32 message = keccak256("hello world"); bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child - ); - assertEq( - account.isValidSignature(_toChildHash(child), signature), - bytes4(keccak256("isValidSignature(bytes32,bytes)")) + BaseLightAccount.SignatureType.EOA, _sign(EOA_PRIVATE_KEY, _getMessageHash(abi.encode(message))) ); + assertEq(account.isValidSignature(message, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); } function testIsValidSignatureForContractOwner() public { _useContractOwner(); - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes32 message = keccak256("hello world"); bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.CONTRACT, - contractOwner.sign(_toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child - ); - assertEq( - account.isValidSignature(_toChildHash(child), signature), - bytes4(keccak256("isValidSignature(bytes32,bytes)")) + BaseLightAccount.SignatureType.CONTRACT, contractOwner.sign(_getMessageHash(abi.encode(message))) ); + assertEq(account.isValidSignature(message, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); } function testIsValidSignatureRejectsInvalid() public { - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); - bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(123, _toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child - ); - assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); - - signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorA(), - child - ); - - // ERC1271.isValidSignature only truncates 32 bytes (since the wrong domain separator was used) before passing it on to _isValidSignature - // _isValidSignature truncates the SignatureType byte before ecrecover - vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); - account.isValidSignature(_toChildHash(child), signature); - - vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA)); - } - - function testIsValidSignaturePersonalSign() public { - string memory message = "hello world"; - bytes32 childHash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n11", message)); - bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _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:\n11", message)); - bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.CONTRACT, - 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:\n11", message)); - bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, _sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH - ); - assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); - - signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - childHash - ); - - // ERC1271.isValidSignature only truncates 32 bytes for personal_sign before passing it on to _isValidSignature - // _isValidSignature truncates the SignatureType byte before ecrecover - vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); - account.isValidSignature(childHash, signature); - + bytes32 message = keccak256("hello world"); + bytes memory signature = + abi.encodePacked(BaseLightAccount.SignatureType.EOA, _sign(123, _getMessageHash(abi.encode(message)))); + assertEq(account.isValidSignature(message, signature), bytes4(0xffffffff)); + + // Invalid length + signature = + abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex"1234567890abcdef1234567890abcdef1234567890abcdef"); + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, 24)); + account.isValidSignature(message, signature); + + // 0 length + signature = abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex""); + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, 0)); + account.isValidSignature(message, signature); + + // Missing SignatureType prefix + signature = _sign(EOA_PRIVATE_KEY, _getMessageHash(abi.encode(message))); vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(childHash, ""); + account.isValidSignature(message, signature); } function testOwnerCanUpgrade() public { @@ -553,7 +485,7 @@ contract LightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0xd163ff9f4127df29ad58e4686d0f5367607e32fe777a91a8f4d86fdab6e23640 + 0x5ad3bccf602cb277e15f7bcac8cd88873618c6f038cbcd490610d91be26fcf34 ); } @@ -597,22 +529,15 @@ contract LightAccountTest is Test { 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)); + /// @dev Purposefully redefined here to surface any necessary updates to client-side message preparation for + /// signing, in case `account.getMessageHash()` is updated. + function _getMessageHash(bytes memory message) public view returns (bytes32) { + bytes32 structHash = keccak256(abi.encode(_MESSAGE_TYPEHASH, keccak256(message))); + return keccak256(abi.encodePacked("\x19\x01", _domainSeparator(), structHash)); } - 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) { + /// @dev Domain separator for the account. + function _domainSeparator() internal view returns (bytes32) { (, string memory name, string memory version,,,,) = account.eip712Domain(); return keccak256( abi.encode( @@ -624,19 +549,6 @@ contract LightAccountTest is Test { ) ); } - - /// @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 dc076cc..22fc57e 100644 --- a/test/MultiOwnerLightAccount.t.sol +++ b/test/MultiOwnerLightAccount.t.sol @@ -25,8 +25,7 @@ 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)"); + bytes32 internal constant _MESSAGE_TYPEHASH = keccak256("LightAccountMessage(bytes message)"); address public eoaAddress; MultiOwnerLightAccount public account; MultiOwnerLightAccount public contractOwnedAccount; @@ -460,168 +459,76 @@ contract MultiOwnerLightAccountTest is Test { } function testIsValidSignatureForEoaOwner() public { - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes32 message = keccak256("hello world"); bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child - ); - assertEq( - account.isValidSignature(_toChildHash(child), signature), - bytes4(keccak256("isValidSignature(bytes32,bytes)")) + BaseLightAccount.SignatureType.EOA, _sign(EOA_PRIVATE_KEY, _getMessageHash(abi.encode(message))) ); + assertEq(account.isValidSignature(message, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); } function testIsValidSignatureForContractOwnerSpecified() public { _useContractOwner(); - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes32 message = keccak256("hello world"); bytes memory signature = abi.encodePacked( BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR, contractOwner, - contractOwner.sign(_toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child - ); - assertEq( - account.isValidSignature(_toChildHash(child), signature), - bytes4(keccak256("isValidSignature(bytes32,bytes)")) + contractOwner.sign(_getMessageHash(abi.encode(message))) ); + assertEq(account.isValidSignature(message, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); } function testIsValidSignatureRejectsContractOwnerUnspecified() public { _useContractOwner(); - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes32 message = keccak256("hello world"); bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.CONTRACT, - contractOwner.sign(_toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child + BaseLightAccount.SignatureType.CONTRACT, contractOwner.sign(_getMessageHash(abi.encode(message))) ); vm.expectRevert(abi.encodePacked(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(_toChildHash(child), signature); + account.isValidSignature(message, signature); } function testIsValidSignatureRejectsInvalidEOA() public { - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); - bytes memory signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(123, _toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child - ); - assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); - - signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorA(), - child - ); - - // ERC1271.isValidSignature only truncates 32 bytes (since the wrong domain separator was used) before passing it on to _isValidSignature - // _isValidSignature truncates the SignatureType byte before ecrecover - vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); - account.isValidSignature(_toChildHash(child), signature); - + bytes32 message = keccak256("hello world"); + bytes memory signature = + abi.encodePacked(BaseLightAccount.SignatureType.EOA, _sign(123, _getMessageHash(abi.encode(message)))); + assertEq(account.isValidSignature(message, signature), bytes4(0xffffffff)); + + // Invalid length + signature = + abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex"1234567890abcdef1234567890abcdef1234567890abcdef"); + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, 24)); + account.isValidSignature(message, signature); + + // 0 length + signature = abi.encodePacked(BaseLightAccount.SignatureType.EOA, hex""); + vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, 0)); + account.isValidSignature(message, signature); + + // Missing SignatureType prefix + signature = _sign(EOA_PRIVATE_KEY, _getMessageHash(abi.encode(message))); vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(_toChildHash(child), abi.encodePacked(BaseLightAccount.SignatureType.EOA)); + account.isValidSignature(message, signature); } function testIsValidSignatureRejectsInvalidContractOwner() public { // Signature should fail, because the contract owner is not an owner - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); + bytes32 message = keccak256("hello world"); bytes memory signature = abi.encodePacked( BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR, contractOwner, - contractOwner.sign(_toERC1271Hash(child)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - child + contractOwner.sign(_getMessageHash(abi.encode(message))) ); - assertEq(account.isValidSignature(_toChildHash(child), signature), bytes4(0xffffffff)); + assertEq(account.isValidSignature(message, signature), bytes4(0xffffffff)); } function testFuzz_isValidSignatureRejectsInvalidSignatureType(uint8 signatureType) public { signatureType = uint8(bound(signatureType, 3, type(uint8).max)); - bytes32 child = keccak256(abi.encode(_CHILD_TYPEHASH, "hello world")); - bytes memory signature = abi.encodePacked( - signatureType, _sign(EOA_PRIVATE_KEY, _toERC1271Hash(child)), _PARENT_TYPEHASH, _domainSeparatorB(), child - ); - vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(_toChildHash(child), signature); - } - - 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( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), - _PARENT_TYPEHASH - ); - assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); - } - - function testIsValidSignaturePersonalSignForContractOwnerSpecified() 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( - BaseLightAccount.SignatureType.CONTRACT_WITH_ADDR, - contractOwner, - contractOwner.sign(_toERC1271HashPersonalSign(childHash)), - _PARENT_TYPEHASH - ); - assertEq(account.isValidSignature(childHash, signature), bytes4(keccak256("isValidSignature(bytes32,bytes)"))); - } - - function testIsValidSignaturePersonalSignRejectsContractOwnerUnspecified() 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( - BaseLightAccount.SignatureType.CONTRACT, - contractOwner.sign(_toERC1271HashPersonalSign(childHash)), - _PARENT_TYPEHASH - ); - vm.expectRevert(abi.encodePacked(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(childHash, signature); - } - - 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( - BaseLightAccount.SignatureType.EOA, _sign(123, _toERC1271HashPersonalSign(childHash)), _PARENT_TYPEHASH - ); - assertEq(account.isValidSignature(childHash, signature), bytes4(0xffffffff)); - - signature = abi.encodePacked( - BaseLightAccount.SignatureType.EOA, - _sign(EOA_PRIVATE_KEY, _toERC1271HashPersonalSign(childHash)), - _PARENT_TYPEHASH, - _domainSeparatorB(), - childHash - ); - - // ERC1271.isValidSignature only truncates 32 bytes for personal_sign before passing it on to _isValidSignature - // _isValidSignature truncates the SignatureType byte before ecrecover - vm.expectRevert(abi.encodeWithSelector(ECDSA.ECDSAInvalidSignatureLength.selector, (signature.length - 32 - 1))); - account.isValidSignature(childHash, signature); - + bytes32 message = keccak256("hello world"); + bytes memory signature = + abi.encodePacked(signatureType, _sign(EOA_PRIVATE_KEY, _getMessageHash(abi.encode(message)))); vm.expectRevert(abi.encodeWithSelector(BaseLightAccount.InvalidSignatureType.selector)); - account.isValidSignature(childHash, abi.encodePacked(BaseLightAccount.SignatureType.EOA)); + account.isValidSignature(message, signature); } function testOwnerCanUpgrade() public { @@ -723,7 +630,7 @@ contract MultiOwnerLightAccountTest is Test { bytes32(uint256(uint160(0x0000000071727De22E5E9d8BAf0edAc6f37da032))) ) ), - 0xd5c53db2178734fbfa4566d827f4af4dbb897979875488640713b7b7d4689d1a + 0x80c58908b2149ff4b21996454a1ad577de59738d9f23637fe3f01506a8836767 ); } @@ -781,22 +688,15 @@ contract MultiOwnerLightAccountTest is Test { } } - 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)); + /// @dev Purposefully redefined here to surface any necessary updates to client-side message preparation for + /// signing, in case `account.getMessageHash()` is updated. + function _getMessageHash(bytes memory message) public view returns (bytes32) { + bytes32 structHash = keccak256(abi.encode(_MESSAGE_TYPEHASH, keccak256(message))); + return keccak256(abi.encodePacked("\x19\x01", _domainSeparator(), structHash)); } - 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) { + /// @dev Domain separator for the account. + function _domainSeparator() internal view returns (bytes32) { (, string memory name, string memory version,,,,) = account.eip712Domain(); return keccak256( abi.encode( @@ -808,19 +708,6 @@ contract MultiOwnerLightAccountTest is Test { ) ); } - - /// @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 { From d22001141e81af039310bf3d9732218421fd4456 Mon Sep 17 00:00:00 2001 From: Jay Paik Date: Thu, 18 Apr 2024 20:12:03 -0400 Subject: [PATCH 3/3] docs: remove ERC1271 dependency from README --- README.md | 1 - 1 file changed, 1 deletion(-) diff --git a/README.md b/README.md index 25ed6c0..cb5c2ba 100644 --- a/README.md +++ b/README.md @@ -73,7 +73,6 @@ Light Account uses dependencies via git submodules, pinned to release branches. | File | Description | Source | | ----------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | [CustomSlotInitializable.sol](./src/common/CustomSlotInitializable.sol) | A fork of OpenZeppelin's `Initializable` contract that allows custom storage slots to be used. | [Initializable.sol (932fddf)](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/932fddf69a699a9a80fd2396fd1a2ab91cdda123/contracts/proxy/utils/Initializable.sol) | -| [ERC1271.sol](./src/common/ERC1271.sol) | A fork of Solady's `ERC1271` contract that allows for more flexibility in signature checks. | [ERC1271.sol (7a2c4af)](https://github.com/Vectorized/solady/blob/7a2c4afcc7328908ddd3f6eae076d277b2b5da23/src/accounts/ERC1271.sol) | | [EIP712.sol](./src/external/solady/EIP712.sol) | Copied from Solady. | [EIP712.sol (eac17da)](https://github.com/Vectorized/solady/blob/eac17da6d57d864f179a6d81e02127cabe3b77d9/src/utils/EIP712.sol) | | [LibClone.sol](./src/external/solady/LibClone.sol) | Copied from Solady. | [LibClone.sol (7a1f591)](https://github.com/Vectorized/solady/blob/7a1f591fe53487bd6952c4df23d3bed26a4b678d/src/utils/LibClone.sol) | | [UUPSUpgradeable.sol](./src/external/solady/UUPSUpgradeable.sol) | Copied from Solady. | [UUPSUpgradeable.sol (a061f38)](https://github.com/Vectorized/solady/blob/a061f38f27cd7ae330a86d42d3f15b4e7237f064/src/utils/UUPSUpgradeable.sol) |