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

Remove invalid signature check on v in ECDSA #3457

Closed
pcaversaccio opened this issue Jun 8, 2022 · 5 comments · Fixed by #3591
Closed

Remove invalid signature check on v in ECDSA #3457

pcaversaccio opened this issue Jun 8, 2022 · 5 comments · Fixed by #3591
Milestone

Comments

@pcaversaccio
Copy link
Contributor

pcaversaccio commented Jun 8, 2022

Motivation
All client implementations of the precompile ecrecover apparently check if the value v is 27 or 28. The references for the different client implementations can be found here.

There are two ways to deal with this IMHO:

  1. Either we remove the check here, or
  2. we keep it with a custom error for better error reporting. In this case, we could even optimise it like that (if you are open to assembly):
assembly {
   ...
    vIsInvalid := iszero(byte(v, 0x0000000000000000000000000000000000000000000000000000000101000000))
}
@axic
Copy link
Contributor

axic commented Jun 8, 2022

The check is useful for two reasons:

  1. Better error reporting
  2. Saving gas in the failure case (i.e. when this signature check is preceding an EIP-1271 verification -- this is the case in the Seaport use case)

(More context in this tweet).

@pcaversaccio
Copy link
Contributor Author

@axic the fundamental question I think here is whether the better error reporting should come from a library or the client implementation directly. I see your point re EIP-1271 tho.

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Jun 8, 2022

Checking a condition twice (library & pre-compile implementation) is still not right IMHO.

@Amxx
Copy link
Collaborator

Amxx commented Jun 8, 2022

2. we keep it with a custom error for better error reporting. In this case, we could even optimise it like that (if you are open to assembly):

We want to generalize the use of custom errors. So, I'd keep the check like we have it today, and just replace the revert reason wit a custom error.

@pcaversaccio
Copy link
Contributor Author

pcaversaccio commented Jun 8, 2022

We want to generalize the use of custom errors. So, I'd keep the check like we have it today, and just replace the revert reason wit a custom error.

If we do this we should add a small natspec section that informs the dev that the client implementations actually make this check as well. It's important to be informative about that IMO. We could link to the now merged Yello Paper PR #860 from @axic as well.

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 a pull request may close this issue.

4 participants