Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881
base: master
Are you sure you want to change the base?
Implement P256 verification via RIP-7212 precompile with Solidity fallback #4881
Changes from 70 commits
5e82076
da0f27e
aa59c67
9512947
a60bf48
9185026
025e360
803e735
57fcecd
4dae298
6cf039d
c094fa1
20a03df
15f1a6b
e2040e4
695b732
41aaf71
3bf4557
3cbf426
bba7fa3
2812ed8
e0ef63b
61a244d
910bc71
2e9d04d
9062633
a44bb71
3a6e1f5
3e71fad
887272b
433548f
4f80ca0
be69f5c
fcde23f
5828566
9362936
921745b
2c113f4
fb7dc6f
f264dae
2c9a137
f4cbf51
0227656
e3a8338
cbd2ff5
194f19a
704a12e
61d52a5
242c796
787834d
d8f4f7e
fc54017
5a7887b
cc82c17
a67e5a2
4c93009
046463c
e4df1d1
1bddcf5
e5ba358
2eecacf
ced4fb8
b82af11
c6a86d9
9b24014
3616771
be078b1
ecd3aa2
d83e707
fbc11f5
9c88101
b5e6bd7
db76353
0722d93
306a5f6
e67a456
1a8cb63
3c3fa27
2fe4a16
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this would address general concerns about the availability of the precompile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if Ethereum puts another precompile at
0x100
in the future that also returns a0x20
length return data? Might be an edge case, but EIP and RIP must not align in the long run! Or another example is, if an L2 already put a precompile on0x100
which is different but also returns0x20
length (haven't done research on this tbh).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were told that EIP-7212 is not up to date, that RIP-7212 is the one to follow, and that precompile address allocation would be organised at the RIP level to avoid conflicts.
If the community can't figure that out, wtf are we (library devs) expected to do ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"would be organised at the RIP level to avoid conflicts." -> talk is cheap ;) so idk, I just don't trust this really. The reason why I haven't provided a native way to call the
P256
precompile in my snekmate (Vyper) library is that I leave this decision to the devs deploying it. I.e. they plan to deploy on 5 chains, 3 of them have the precompile and 2 not, so they write their own wrapper around it using e.g.0x100
as target since it's ensured that it exists there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you don't want to risk it, just use
verifySolidity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you know how people are: "hey let's gas optimise everything and native looks sweet in our tests since we run it on a
P256
-supported fork." I think a good compromise is to put a warning in the docs about the current situation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I never said this. My main point here is that the precompile check is not future proof as the return data of length 32 bytes could also come from other precompiles in the future on that address. I'm not saying this will happen, but I have seen too much shit over the last years to be paranoid enough about this edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out @pcaversaccio. I agree with the concern and now I think more about it I also agree we should explain the situation at the very minimum. We raised a similar concern for Math.modexp that ended up in an explicit documentation mention, so I would consider adding a note here.
Regardless, both RIP-7212 and EIP-7212 agree that the precompile "returns 1 in 32 bytes format". The current implementation reverts if the returndata can't be decoded as a boolean. In my opinion, it's unlikely that a precompile at
address(0x100)
will randomly returntrue
but I recognize the possibility. Would you agree the decoding constrains the possibility?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall yes, but I want to raise that this might result in an unexpected behaviour since you don't expect a revert from a precompile usually (but no return data to indicate an error).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree it's unexpected behavior, but it's a manageable risk.
I hear you with this and think an "IMPORTANT" block should be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sgtm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for a public key to be valid, it should:
on property one: the public key is used in$(0,0)$ represents the point of infinity, so in this case, the conversion in $(0,0)$ via
_preComputeJacobianPoints
bypoints[0x01] = JPoint(px, py, 1); // 1,0 (p)
, which would never be the point of infinity._affineFromJacobian
uses the convention that_preComputeJacobianPoints
(L278) would be incorrect, but given that the point of infinity is not valid anyways, it's not an issue as we instead reject the public keyisOnCurve
.on property two: x and y coordinates of the public key get reduced to modulo p in calculations, so missing a check for property two means that the verify function will check the signature for the public key with coordinates$(x \bmod p, y \bmod p)$ . This would allow for public keys $(x,y)$ , where $x<2^{256} - p$ or $y<2^{256} - p$ , which there would be another pair $(x^\prime,y^\prime)$ — such as $(x+p,y)$ — that can be used as a public key and for which signatures made for $(x,y)$ would also verify.
on property three: we are checking that it lies on the curve via the Weierstrass equation (
isOnCurve
).i think properties 1 and 3 are ok, but missing 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
B != 0
in Sepc256r1, then(0, 0)
is not on the curve ... that should be okuint256(qx) >= P || uint256(qy) >= P
? Also, is it P or N?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah that should be good. it's P because we are using that for the modulus operations on calculations like
isOnCurve
tests pass with this change, but i think it's useful to add a unit test for this property specifically, i can add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it also makes sense to have this in
_tryVerifyNative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the check directly in
isOnCurve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the public key is unlikelly to be arbitrary user input. It will be recorded/whitelisted onchain, by some admin. So this attack vector is not really critical. But still its good to add the (cheap) check in
isOnCurve
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although this is true, it's unclear to me whether you can create a signature for public keys$(x,y)$ if it's not on the curve. I still think checking is valuable and pretty much agree with @Amxx's conclusion that mostly public keys won't be arbitrary and the check is cheap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's not on the curve
: the problem is that you can findThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, so it's the other way around. Still it's relevant that public keys are unlikely to be arbitrary, but let's keep the check regardless.