ECDSA: add parse and tryParse #5814

Amxx · 2025-07-23T21:34:13Z

Following discussion with @frangio, we believe that this could be usefull for someone that wants to re-implement isValidSignatureNow in a way taht supports both 65 and 64 bytes signatures.

We should add this support nativelly in v6.0, but for the same reason as the one mentioned in this GHSA this is not a change that should happen in a minor release.

function isValidSignatureNow64or65(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
        if (signer.code.length == 0) {
            (uint8 v, bytes32 r, bytes32 s) = ECDSA.parse(signature);
            (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, v, r, s);
            return err == ECDSA.RecoverError.NoError && recovered == signer;
        } else {
            return isValidERC1271SignatureNow(signer, hash, signature);
        }
    }

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

changeset-bot · 2025-07-23T21:34:17Z

🦋 Changeset detected

Latest commit: 23c393f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

frangio · 2025-07-24T22:14:32Z

contracts/utils/cryptography/ECDSA.sol

+     * @dev Parse a signature into its `v`, `r` and `s` components. Supports both 65 bytes and 64 bytes (eip-2098)
+     * signature formats. Returns 0, 0, 0 is the signature is not in a proper format.
+     */
+    function parse(bytes memory signature) internal pure returns (int8 v, bytes32 r, bytes32 s) {


For discussion (I'm undecided): should this have a special name given that, unlike the rest of this library, it allows 64-65 byte malleability? E.g., parseMalleable.

Would that note be good enough ?

IMO not using signatures as unique identifier is basic good practices, and I'm not sure we should complexify the name of the functions (creating possible confusion?) as a way to "help enforce" these basic good practices.

not using signatures as unique identifier is basic good practices

Yeah I agree and we should move towards this assumption. My point is only about how this breaks from the existing library design.

How about deprecating malleability protection in all of the ECDSA library?

How about deprecating malleability protection in all of the ECDSA library?

YES !

@frangio how does that look ?
144b0e2

Why is v a signed integer?

Why is v a signed integer?

Because I made a mistake. Thanks for spotting it !

frangio · 2025-07-24T22:15:50Z

contracts/utils/cryptography/ECDSA.sol

@@ -196,6 +196,63 @@ library ECDSA {
        return recovered;
    }

+    /**
+     * @dev Parse a signature into its `v`, `r` and `s` components. Supports both 65 bytes and 64 bytes (eip-2098)
+     * signature formats. Returns 0, 0, 0 is the signature is not in a proper format.


If one wants to check if decoding succeeded, is it necessary to check all three return values for 0, or is there one of them that could be checked for 0 to provide indication that decoding failed? It would be useful to document this if so.

Maybe v is enough?

v should be enough.

According to https://www.evm.codes/precompiled, any v that is NOT 27 or 28 will be rejected by the precompile. Similarly, r and s are expected to be in range ]0, secp256k1n[, so either of those being 0 would also mean "obviously invalid signature".

That mean any individual value being 0 is a good enough check

Isn't possible to pass in an invalid signature but only a valid v value? I think it's not enough to make sure the parsing succeeded. Also, what does it mean that decoding failed?

I agree we should note it if it's useful

@ernestognw it should be possible to pass as invalid signature but with a valid v or r for example, so only checking if one of them is 0 could lead to false positives

Right, I see what you mean, thanks @luiz-lvj. Imo deserves a note:

* NOTE: A returned `v == 0` means the signature couldn't be parsed. Consider avoid calling `tryRecover` unnecessarily

A few comments:

False positive on the "parsing success" would only lead to tryVerify being called with bad params, which would be identified as an invalid signature, so IMO there is no security risk.

In case of 65 bytes long signatures, the decoding parsing might be "correct" and v could be 0. That would happen if the signature encodes invalid data (a v=0). I don't think that would quality as a parsing error.

If we are in a situation where we parse a bytes (of arbitray length) and we get v, r, s returned to us, we have two options: inspect them to try and exit "early", or pass them to tryRecover/recover and let them handle the invalid signatures. The first options includes check that everyone will pay, including valid sigantures ! The second option makes failure more expensive (they happen later) but doesn't add any cost to the "happy path". IMO we should optimise the "happy path" cost, which means I wouldn't add any v,r,s check between he parsing and the recovery.

we should optimise the "happy path" cost, which means I wouldn't add any v,r,s check between he parsing and the recovery.

Yes, agree with this. Just echoing fran's comment:

It would be useful to document this if so.

Yes, agree with this. Just echoing fran's comment:

It would be useful to document this if so.

👍🏻

So what do we want to do? I would personally not encourage devs to do such a check. Does anyone feel differently?

If we feel like 0,0,0 being an invalid signature for all hashes is enough, then what about:

Suggested change

* signature formats. Returns 0, 0, 0 is the signature is not in a proper format.

* signature formats. If the signature format is not recognised, this function returns (0,0,0), which both

* {recover} and {tryRecover} will see as invalid (for every hash).

The block started to sound repetitive. See 23c393f

contracts/utils/cryptography/ECDSA.sol

ernestognw · 2025-07-28T15:59:59Z

contracts/utils/cryptography/ECDSA.sol

@@ -196,6 +196,63 @@ library ECDSA {
        return recovered;
    }

+    /**
+     * @dev Parse a signature into its `v`, `r` and `s` components. Supports both 65 bytes and 64 bytes (eip-2098)
+     * signature formats. Returns 0, 0, 0 is the signature is not in a proper format.


Isn't possible to pass in an invalid signature but only a valid v value? I think it's not enough to make sure the parsing succeeded. Also, what does it mean that decoding failed?

I agree we should note it if it's useful

ECDSA: add parse and tryParse

f839be4

Amxx requested a review from a team as a code owner July 23, 2025 21:34

Amxx requested review from arr00, james-toussaint, ernestognw, luiz-lvj and gonzaotc July 23, 2025 21:34

Amxx added this to the 5.5 milestone Jul 23, 2025

frangio reviewed Jul 24, 2025

View reviewed changes

Amxx commented Jul 25, 2025

View reviewed changes

contracts/utils/cryptography/ECDSA.sol Show resolved Hide resolved

add notice about malleability protection

144b0e2

Amxx commented Jul 25, 2025

View reviewed changes