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

fix: check y_parity value #1107

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+28 −0
Open

fix: check y_parity value #1107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 4 additions & 0 deletions src/ethereum/arrow_glacier/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
Comment on lines +244 to +245
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there cases where we'd want secp256k1_recover to accept y_parity not in (0, 1)?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's a boolean. It gets encoded in strange ways in LegacyTransaction, but that is weirdness inherited from Bitcoin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my understanding, no
Valid recovery id values are in (0, 1, 2, 3) (see coincurve) but ECDSA signatures are considered valid by the yellow paper only when v is in (0, 1).
image

This is confirmed by the fact that even for erecover, the input of secp256k1_recover will be 0 or 1 see ecrecover in exec spec and yellow paper

Capture d’écran 2025-02-04 à 16 46 40

Example with go-ethereum ValidateSignatureValues
https://github.com/ethereum/go-ethereum/blob/e6f3ce7b168b8f346de621a8f60d2fa57c2ebfb0/crypto/crypto.go#L274

and Sign function
https://github.com/ethereum/go-ethereum/blob/e6f3ce7b168b8f346de621a8f60d2fa57c2ebfb0/crypto/secp256k1/secp256.go#L68

Copy link
Contributor

Choose a reason for hiding this comment

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

So should we move the check to secp256k1_recover?

Copy link
Contributor Author

@obatirou obatirou Feb 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It is possible in this context if not mistaken but I think it would introduce a business need in secp256k1_recover which goal should only be to recover the public key from a given valid signature imo.
Shouldn't the validation of the signature happen before ?
The checks for r and s range are already outside the function

execution-specs/src/ethereum/cancun/transactions.py

Lines 260 to 265 in 9923823

r, s = tx.r, tx.s
if U256(0) >= r or r >= SECP256K1N:
raise InvalidSignatureError("bad r")
if U256(0) >= s or s > SECP256K1N // U256(2):
raise InvalidSignatureError("bad s")
and

execution-specs/src/ethereum/cancun/vm/precompiled_contracts/ecrecover.py

Lines 47 to 52 in 9923823

if v != U256(27) and v != U256(28):
return
if U256(0) >= r or r >= SECP256K1N:
return
if U256(0) >= s or s >= SECP256K1N:
return

Let me know what you think. Open to make the changes if you prefer of course.

public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
2 changes: 2 additions & 0 deletions src/ethereum/berlin/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,8 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
Expand Down
6 changes: 6 additions & 0 deletions src/ethereum/cancun/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,14 +280,20 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
elif isinstance(tx, BlobTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_4844(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/gray_glacier/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/london/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/paris/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
4 changes: 4 additions & 0 deletions src/ethereum/shanghai/transactions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,14 @@ def recover_sender(chain_id: U64, tx: Transaction) -> Address:
signing_hash_155(tx, chain_id),
)
elif isinstance(tx, AccessListTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_2930(tx)
)
elif isinstance(tx, FeeMarketTransaction):
if tx.y_parity not in (U256(0), U256(1)):
raise InvalidSignatureError("bad y_parity")
public_key = secp256k1_recover(
r, s, tx.y_parity, signing_hash_1559(tx)
)
Expand Down
Loading