openzeppelin-contracts

openzeppelin-contracts copied to clipboard

Motivation All client implementations of the precompile ecrecover apparently check if the value v is 27 or 28. The references for the different client implementations can be found here.

There are two ways to deal with this IMHO:

1. Either we remove the check here, or
2. we keep it with a custom error for better error reporting. In this case, we could even optimise it like that (if you are open to assembly):

assembly {
   ...
    vIsInvalid := iszero(byte(v, 0x0000000000000000000000000000000000000000000000000000000101000000))
}

The check is useful for two reasons:

1. Better error reporting
2. Saving gas in the failure case (i.e. when this signature check is preceding an EIP-1271 verification -- this is the case in the Seaport use case)

(More context in this tweet).

@axic the fundamental question I think here is whether the better error reporting should come from a library or the client implementation directly. I see your point re EIP-1271 tho.

Checking a condition twice (library & pre-compile implementation) is still not right IMHO.

2. we keep it with a custom error for better error reporting. In this case, we could even optimise it like that (if you are open to assembly):

We want to generalize the use of custom errors. So, I'd keep the check like we have it today, and just replace the revert reason wit a custom error.

We want to generalize the use of custom errors. So, I'd keep the check like we have it today, and just replace the revert reason wit a custom error.

If we do this we should add a small natspec section that informs the dev that the client implementations actually make this check as well. It's important to be informative about that IMO. We could link to the now merged Yello Paper PR #860 from @axic as well.