Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: revert to previous 1271 implementation #55

Merged
merged 3 commits into from
Apr 22, 2024
Merged

feat: revert to previous 1271 implementation #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
72d4aea
feat: revert to previous 1271 implementation
jaypaik Apr 18, 2024
74963c1
test: update tests for 1271
jaypaik Apr 19, 2024
d220011
docs: remove ERC1271 dependency from README
jaypaik Apr 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) |
14 changes: 7 additions & 7 deletions src/LightAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
16 changes: 8 additions & 8 deletions src/MultiOwnerLightAccount.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
132 changes: 25 additions & 107 deletions src/common/ERC1271.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
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 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)");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(No change needed) Do you know what the rationale was for the field being a bytes instead of a bytes32? I might have something a little hazy here, but I believe this field is always containing the hash that's being asked to be signed, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

I was thinking I might want to change that too, but left it as is. This is also what MultiOwnerPlugin has.

I believe the original reason was because that's what Safe does. I'm happy to change it if it won't cause SDK headaches.

Cc @howydev

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I think it's fine to stick to what Safe does, I was just curious.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, we inherited that conversion of bytes32 digest -> encoded into bytes -> hashed back to a bytes32 from Safe. It added a little overhead that's probably unnecessary, but I think the SDK already accounts for this. 100% should address this in the next iteration though

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks guys


/// @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 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;
}
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);
}
Loading
Loading