Update EIP-7212: Clarifications and improvements #7676
Conversation
(in order of importance) - specify "Required Checks in Verification" more precisely - make choice of verification vs. recovery clearer - todo add links to implementation - one ??? that i didn't understand
github-actions
bot
added
c-update
|
✅ All reviewers have approved. |
eth-bot
changed the title
EIPS/eip-7212.md
Outdated
| - Verify that s is equal to or less than half of the order of the subgroup to prevent signature malleability. | ||
| - Verify that the point formed by (`x`, `y`) values is on the curve and both components are in between 0 and the `p` value of the curve. | ||
| - Verify that `r` and `s` values are valid scalar field elements, i.e. they are in `(0, n)` (exclusive) where `n` is the order of the subgroup. | ||
| - Verify that `s <= n // 2`, where `n` is the order of the subgroup to prevent signature malleability. |
There was a problem hiding this comment.
I would actually suggest that this be deleted from this EIP to match compatibility exactly with the NIST specs which does not require this. In particular, if I understand correctly, at the moment a valid Implementation of P256 signing according to the NIST spec can fail this verifier. This would not be ideal for the purpose of this precompile.
It is anyway relatively straightforward/cheap to add these checks separately in EVM byte code if a contract implementation does want them.
There was a problem hiding this comment.
I respect the idea of having compatibility between the standard and precompiled contract implementation, but I think that if we remove the malleability check, most of the applications using this code will require additional storage and checks. As in ecrecover, the malleability issue is not in the standards, but implemented in the precompiled contract. Is there any case which we need malleable signatures?
There was a problem hiding this comment.
https://github.com/daimo-eth/daimo as an example application uses this precompile with AA wallets and does not require non-malleability (since nonces/userops are unique).
The case that strongly concerns me is that making this check inside the precompile means we would disagree from the NIST spec, and by extension signatures that by default are marked as valid by the most popular P256 implementations like subtlecrypto and noble and all the test suites like wycheproof will be marked as invalid. Further there could exist device secure enclave’s that return signatures marked as invalid unless the user first runs a transform on them. This would be really confusing for users and annoying for implementation devs, and seems like unnecessary complexity.
Also, adding this check seperately as-needed in some sort of wrapper contract would not require any extra storage. As far as computation cost goes, users are likely to be calling a hash function before calling this precompile anyway, so the cost of one comparison seems trivial compared to that.
Overall, I think the point of this precompile should be to be able to onboard popular implementations to Ethereum (that all follow the NIST spec), so we should match the spec. If you would like, we can add an example usage or note to the EIP where we emphasise the malleability.
nalinbhardwaj
force-pushed
the
nibnalin/eip-7212
branch
from
440d975
to
0f8ba80
Compare
github-actions
bot
added
w-ci
nalinbhardwaj
force-pushed
the
nibnalin/eip-7212
branch
2 times, most recently
from
7cb196f
to
3254091
Compare
github-actions
bot
added
w-ci
nalinbhardwaj
force-pushed
the
nibnalin/eip-7212
branch
from
3254091
to
fd6dbb8
Compare
nalinbhardwaj
force-pushed
the
nibnalin/eip-7212
branch
from
fd6dbb8
to
93d855d
Compare
|
The commit 93d855d (as a parent of a3558d7) contains errors. |
nalinbhardwaj
force-pushed
the
nibnalin/eip-7212
branch
from
6393da5
to
66ffa73
Compare
(in order of importance)