set ecdsa_raw_recover to accept 0 or 1 as valid v values#100
Conversation
2054e03 to
db30752
Compare
There was a problem hiding this comment.
Where does the range 0-3 come from with "other implementations"? If we're to keep to Ethereum and not accounting for EIP-155, should we keep this check to 27 and 28 only? This may be different for py-ecc (not ethereum-specific) but I do think it's also going to be 0 or 1 only.
The error message also throws me off as a reader. I'd rather have the check 0-1 inclusive (if we choose 27-28) before we add 27 - so raising the BadSignature at line 155. Otherwise, we should keep 27-28 in the error message.
db30752 to
36c76c4
Compare
|
@pacrob I'm not convinced on OpenZeppelin ECDSA.sol (0 or 1): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L103 |
|
@fselmo It comes from other more generalized implementations of ecdsa. The second two links are examples. That, combined with what I dook as the intent of the original error message, are why I decided to keep it allowing 0-3 instead of just 0-1. |
|
@pacrob the second link seems to reinforce the original implementation more than this one to me (27-34). Unless we are wanting to change this to the uncompressed key range only (27-30). But I'm not sure why we'd take that path either. I'd prefer an approach where we consider the context of the library itself and the bounds within which the method should be used.
If we take the second approach, I guess I'm confused why we're going from validating the whole range of uncompresssed + compressed key cases for the "recid" example in that comment (27-34) to validating only the uncompressed cases (27-30), when all we really care about, and all that Solidity and other implementations check for afaict, is |
36c76c4 to
1e843aa
Compare
What was wrong?
In
ecdsa_raw_recover, the value check forvlooked for `27 <= v <=34, while the error message indicated 27-31.Closes #67
How was it fixed?
I believe the intended check should be looking for avvalue of 27-30 inclusive, meaning the argument to the function should be in [0, 1, 2, 3]. This would be consistent with the error message, taking the python sense of range of lower number inclusive, higher number exclusive. I've clarified the error message range and used0-3instead of27-30, as that is what the arg passed to the function would be.On the actual value check, I'm unable to find any reference to a
vvalue in [27-34]. Pre-EIP155 Ethereum used [0-1], other "standard" implementations use [0-3], and post-EIP155 Ethereum uses{0,1} + CHAIN_ID * 2 + 35. This function looks like it was written to accept [0-3], but in testing other functions in the same file, I'm not able to generate avother that 0 or 1.It would be great to update to work with post-EIP155
vvalues, but that would need more research to see what else is affected. Out of scope for this PR.Updated: Setting to accept only values of 0 or 1, in line with the pre-EIP155 Ethereum-specific standard.
This same bug exists in
py_ecc- https://github.com/ethereum/py_ecc/blob/93edf6d654c06fff017ba77a103d59aae48edcd7/py_ecc/secp256k1/secp256k1.py#L262Once merged, I'll make the same fix there.
Todo:
Cute Animal Picture