Validate s value in signature

[carver]

• Version: dd4f00a

What was wrong?

It appears that s is validated against secpk1_n but should be validated against s < secpk1_n / 2 + 1.

How can it be fixed?

1. Verify our understanding of the yellow paper
2. Add a test with an s == secpk1_n / 2 + 1 that should fail validation, for the test to pass.
3. Add another test with s == secpk1_n // 2 + 1 that should succeed validation.
4. Validate r and s separately, since they have different constraints. Instead of validating them the same way:

eth-keys/eth_keys/validation.py

Lines 108 to 111 in dd4f00a

	def validate_signature_r_or_s(value: int) -> None:
	validate_integer(value)
	validate_gte(value, 0)
	validate_lt_secpk1n(value)

[kclowes]

After looking into this some, it looks like disallowing s > secpk1_n / 2 + 1 from transaction signatures was changed in the Homestead hard fork. The ecrecover precompile should still accept s-values greater than secp256k1n / 2 + 1.

EIP-2 (the Homestead hard fork EIP) states:

All transaction signatures whose s-value is greater than secp256k1n/2 are now considered invalid. The ECDSA recover precompiled contract remains unchanged and will keep accepting high s-values; this is useful e.g. if a contract recovers old Bitcoin signatures.

Further elaboration in EIP-2:

Allowing transactions with any s value with 0 < s < secp256k1n, as is currently the case, opens a transaction malleability concern, as one can take any transaction, flip the s value from s to secp256k1n - s, flip the v value (27 -> 28, 28 -> 27), and the resulting signature would still be valid. This is not a serious security flaw, especially since Ethereum uses addresses and not transaction hashes as the input to an ether value transfer or other transaction, but it nevertheless creates a UI inconvenience as an attacker can cause the transaction that gets confirmed in a block to have a different hash from the transaction that any user sends, interfering with user interfaces that use transaction hashes as tracking IDs. Preventing high s values removes this problem.

[carver]

Thanks for hunting this down.

[kclowes]

I think this should stay open because this library is used outside of the context of the ecrecover precompile, right?

[carver]

Hm. So maybe you're arguing that there should be an API specifically for signatures that follow the ethereum transaction signature rules?

My presumption was that the normal, basic rule is correct for this base library. Then tighter rules, like the transaction signature, should be applied at other layers (like eth-account and/or py-evm).

[kclowes]

Ah, okay. Applying at higher levels makes sense to me!