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.

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

feat(v2): Add signature enum to user operation signature validation #41

Merged
merged 3 commits into from
Mar 15, 2024

Conversation

adam-alchemy
Copy link
Contributor

Motivation

  • When we validate owner signatures for both user op validation and 1271 signature validation, if the owner is a contract account and there is more than one owner, then the signature verification must loop over all the owners and attempt calling isValidSignature. The problem this causes is gas inefficiency and a potential DOS attack a contract owner can perform, where it wastes all gas available in the call to prevent future signature checking.
  • When the looping happens during user op validation, the owner addresses will all be warmed by the dummy signature, despite not being warmed when ECDSA verification succeeds. This leads to incorrect gas estimation when the execution step of the user operation interacts with an owner EOA, due to the account being warmed during estimation but not execution.

Solution

Two pieces of information may be useful - which owner the signature is for, and whether or not the owner is a contract or an EOA. However, the looping behavior might actually be useful for calldata-constrained networks like Optimism, Base, Arbitrum, etc.

So, the proposed solution is to define a 1-byte enum in the front of the signature to allow selecting between the different signing mechanisms .

  • Case 0: EOA signature
    • Signature format: 1 byte enum || 65-byte EOA signature
  • Case 1: Contract signature, no address
    • Signature format: 1 byte enum || arbitrary-length contract signature
  • Case 2: Contract signature with address
    • Signature format: 1 byte enum || 20-byte contract address || arbitrary-length contract signature

This PR only handles the user operation validation checks. A stacked PR will handle the signature verification for the 1271 case. Enum values for the 1271 case have already been included here.

src/MultiOwnerLightAccount.sol Outdated Show resolved Hide resolved
empty_2, // skip 6 to align bitmap
TRANSPARENT_CONTRACT_WITH_ADDR // 7

}
Copy link
Collaborator

@jaypaik jaypaik Mar 15, 2024

Choose a reason for hiding this comment

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

Are we going to be using the bits for checks later?

If not, maybe it's better to just go with regularly incrementing enums.

If we are, maintaining enums might get messy as we add on new signature types. So maybe just maintain these flags in a library and introduce helper functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was planning on using the bits during the 1271 signature checking, but I'll take a look and see if it makes sense to just drop this down to an enum.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok! Edited my earlier comment to suggest a library instead if we're using the bits.

/// @return validationData The validation data value. 0 if success is true, 1 (SIG_VALIDATION_FAILED) if
/// success is false.
function _successToValidationData(bool success) internal pure returns (uint256 validationData) {
return success ? SIG_VALIDATION_SUCCESS : SIG_VALIDATION_FAILED;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I previously had this as just iszero in assembly, but I figured it might make sense to keep this simple and readable. The gas difference is about 20, so not very significant, but also OK to switch back.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Was wondering the same thing. I think this reads better!

@adam-alchemy adam-alchemy merged commit 816168b into develop Mar 15, 2024
2 checks passed
@adam-alchemy adam-alchemy deleted the adam/enum-uo-sig branch March 15, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants