ECDSA: add parse and tryParse by Amxx · Pull Request #5814 · OpenZeppelin/openzeppelin-contracts

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

+     * @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 !

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, ernestognw, gonzaotc, james-toussaint and luiz-lvj July 23, 2025 21:34

Amxx added this to the 5.5 milestone Jul 23, 2025

frangio reviewed Jul 24, 2025

View reviewed changes