Skip to content

ECDSA: add parse and tryParse #5814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Amxx merged 11 commits into from
Jul 31, 2025
Merged

ECDSA: add parse and tryParse #5814

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f839be4
ECDSA: add parse and tryParse
Amxx Jul 23, 2025
144b0e2
add notice about malleability protection
Amxx Jul 25, 2025
e9d04b3
Apply suggestions from code review
Amxx Jul 25, 2025
65f0059
Apply suggestions from code review
Amxx Jul 25, 2025
7b80596
update
Amxx Jul 25, 2025
d409599
rewrite deprecation notice + add to changelog
Amxx Jul 25, 2025
ec112c7
Merge branch 'master' into feature/ecdsa-parse
Amxx Jul 25, 2025
069e9d4
int8 v -> uint8 v
Amxx Jul 29, 2025
a63d6a0
address PR comments
Amxx Jul 29, 2025
4ead44c
Update plain-times-itch.md
Amxx Jul 29, 2025
23c393f
Update parse NatSpec
ernestognw Jul 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/plain-times-itch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`ECDSA`: Add `parse` and `parseCalldata` to parse bytes signatures of length 65 or 64 (eip-2098) into its v,r,s components.
57 changes: 57 additions & 0 deletions contracts/utils/cryptography/ECDSA.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
function parse(bytes memory signature) internal pure returns (int8 v, bytes32 r, bytes32 s) {
Copy link
Contributor

@frangio frangio Jul 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Amxx Amxx Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@frangio frangio Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@Amxx Amxx Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

YES !

Copy link
Collaborator Author

@Amxx Amxx Jul 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@frangio how does that look ?
144b0e2

Copy link
Contributor

@arr00 arr00 Jul 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is v a signed integer?

Copy link
Collaborator Author

@Amxx Amxx Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is v a signed integer?

Because I made a mistake. Thanks for spotting it !

assembly ("memory-safe") {
// Check the signature length
switch mload(signature)
// - case 65: r,s,v signature (standard)
case 65 {
r := mload(add(signature, 0x20))
s := mload(add(signature, 0x40))
v := byte(0, mload(add(signature, 0x60)))
}
// - case 64: r,vs signature (cf https://eips.ethereum.org/EIPS/eip-2098)
case 64 {
let vs := mload(add(signature, 0x40))
r := mload(add(signature, 0x20))
s := and(vs, shr(1, not(0)))
v := add(shr(255, vs), 27)
}
default {
r := 0
s := 0
v := 0
}
}
}

/**
* @dev Variant of {parse} that takes a signature in calldata
*/
function parseCalldata(bytes calldata signature) internal pure returns (int8 v, bytes32 r, bytes32 s) {
assembly ("memory-safe") {
// Check the signature length
switch signature.length
// - case 65: r,s,v signature (standard)
case 65 {
r := calldataload(signature.offset)
s := calldataload(add(signature.offset, 0x20))
v := byte(0, calldataload(add(signature.offset, 0x40)))
}
// - case 64: r,vs signature (cf https://eips.ethereum.org/EIPS/eip-2098)
case 64 {
let vs := calldataload(add(signature.offset, 0x20))
r := calldataload(signature.offset)
s := and(vs, shr(1, not(0)))
v := add(shr(255, vs), 27)
}
default {
r := 0
s := 0
v := 0
}
}
}

/**
* @dev Optionally reverts with the corresponding custom error according to the `error` argument provided.
*/
Expand Down
139 changes: 109 additions & 30 deletions test/utils/cryptography/ECDSA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,26 @@ describe('ECDSA', function () {

describe('recover with invalid signature', function () {
it('with short signature', async function () {
await expect(this.mock.$recover(TEST_MESSAGE, '0x1234'))
const signature = '0x1234';

await expect(this.mock.$recover(TEST_MESSAGE, signature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(2);

await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(2);
});

it('with long signature', async function () {
await expect(
this.mock.$recover(
TEST_MESSAGE,
'0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789',
),
)
const signature =
'0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789';

await expect(this.mock.$recover(TEST_MESSAGE, signature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(85);

await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(85);
});
Expand All @@ -43,23 +51,29 @@ describe('ECDSA', function () {
const signature = await this.signer.signMessage(TEST_MESSAGE);

// Recover the signer address from the generated message and signature.
expect(await this.mock.$recover(ethers.hashMessage(TEST_MESSAGE), signature)).to.equal(this.signer);
expect(await this.mock.$recoverCalldata(ethers.hashMessage(TEST_MESSAGE), signature)).to.equal(this.signer);
await expect(this.mock.$recover(ethers.hashMessage(TEST_MESSAGE), signature)).to.eventually.equal(this.signer);
await expect(this.mock.$recoverCalldata(ethers.hashMessage(TEST_MESSAGE), signature)).to.eventually.equal(
this.signer,
);
});

it('returns signer address with correct signature for arbitrary length message', async function () {
// Create the signature
const signature = await this.signer.signMessage(NON_HASH_MESSAGE);

// Recover the signer address from the generated message and signature.
expect(await this.mock.$recover(ethers.hashMessage(NON_HASH_MESSAGE), signature)).to.equal(this.signer);
expect(await this.mock.$recoverCalldata(ethers.hashMessage(NON_HASH_MESSAGE), signature)).to.equal(this.signer);
await expect(this.mock.$recover(ethers.hashMessage(NON_HASH_MESSAGE), signature)).to.eventually.equal(
this.signer,
);
await expect(this.mock.$recoverCalldata(ethers.hashMessage(NON_HASH_MESSAGE), signature)).to.eventually.equal(
this.signer,
);
});

it('returns a different address', async function () {
const signature = await this.signer.signMessage(TEST_MESSAGE);
expect(await this.mock.$recover(WRONG_MESSAGE, signature)).to.not.be.equal(this.signer);
expect(await this.mock.$recoverCalldata(WRONG_MESSAGE, signature)).to.not.be.equal(this.signer);
await expect(this.mock.$recover(WRONG_MESSAGE, signature)).to.eventually.not.equal(this.signer);
await expect(this.mock.$recoverCalldata(WRONG_MESSAGE, signature)).to.eventually.not.equal(this.signer);
});

it('reverts with invalid signature', async function () {
Expand All @@ -85,22 +99,24 @@ describe('ECDSA', function () {
it('works with correct v value', async function () {
const v = '0x1b'; // 27 = 1b.
const signature = ethers.concat([signatureWithoutV, v]);
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.equal(signer);
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.equal(signer);
await expect(this.mock.$recover(TEST_MESSAGE, signature)).to.eventually.equal(signer);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.eventually.equal(signer);

const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
expect(await this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s)).to.equal(
signer,
);
await expect(
this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s),
).to.eventually.equal(signer);

expect(await this.mock.getFunction('$recover(bytes32,bytes32,bytes32)')(TEST_MESSAGE, r, vs)).to.equal(signer);
await expect(
this.mock.getFunction('$recover(bytes32,bytes32,bytes32)')(TEST_MESSAGE, r, vs),
).to.eventually.equal(signer);
});

it('rejects incorrect v value', async function () {
const v = '0x1c'; // 28 = 1c.
const signature = ethers.concat([signatureWithoutV, v]);
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.not.equal(signer);
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.not.equal(signer);
await expect(this.mock.$recover(TEST_MESSAGE, signature)).to.eventually.not.equal(signer);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.eventually.not.equal(signer);

const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
expect(
Expand Down Expand Up @@ -154,29 +170,31 @@ describe('ECDSA', function () {
it('works with correct v value', async function () {
const v = '0x1c'; // 28 = 1c.
const signature = ethers.concat([signatureWithoutV, v]);
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.equal(signer);
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.equal(signer);
await expect(this.mock.$recover(TEST_MESSAGE, signature)).to.eventually.equal(signer);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.eventually.equal(signer);

const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
expect(await this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s)).to.equal(
signer,
);
await expect(
this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s),
).to.eventually.equal(signer);

expect(await this.mock.getFunction('$recover(bytes32,bytes32,bytes32)')(TEST_MESSAGE, r, vs)).to.equal(signer);
await expect(
this.mock.getFunction('$recover(bytes32,bytes32,bytes32)')(TEST_MESSAGE, r, vs),
).to.eventually.equal(signer);
});

it('rejects incorrect v value', async function () {
const v = '0x1b'; // 27 = 1b.
const signature = ethers.concat([signatureWithoutV, v]);
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.not.equal(signer);
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.not.equal(signer);
await expect(this.mock.$recover(TEST_MESSAGE, signature)).to.not.equal(signer);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.not.equal(signer);

const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
expect(
await this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s),
).to.not.equal(signer);

expect(await this.mock.getFunction('$recover(bytes32,bytes32,bytes32)')(TEST_MESSAGE, r, vs)).to.not.equal(
await expect(this.mock.getFunction('$recover(bytes32,bytes32,bytes32)')(TEST_MESSAGE, r, vs)).to.not.equal(
signer,
);
});
Expand Down Expand Up @@ -236,4 +254,65 @@ describe('ECDSA', function () {
expect(() => ethers.Signature.from(highSSignature)).to.throw('non-canonical s');
});
});

describe('parse signature', function () {
it('65 and 64 bytes signatures', async function () {
// Create the signature
const signature = await this.signer.signMessage(TEST_MESSAGE).then(ethers.Signature.from);

await expect(this.mock.$parse(signature.serialized)).to.eventually.deep.equal([
signature.v,
signature.r,
signature.s,
]);
await expect(this.mock.$parse(signature.compactSerialized)).to.eventually.deep.equal([
signature.v,
signature.r,
signature.s,
]);
await expect(this.mock.$parseCalldata(signature.serialized)).to.eventually.deep.equal([
signature.v,
signature.r,
signature.s,
]);
await expect(this.mock.$parseCalldata(signature.compactSerialized)).to.eventually.deep.equal([
signature.v,
signature.r,
signature.s,
]);
});

it('with short signature', async function () {
const signature = '0x1234';

await expect(this.mock.$parse(signature)).to.eventually.deep.equal([0n, ethers.ZeroHash, ethers.ZeroHash]);

await expect(this.mock.$parseCalldata(signature)).to.eventually.deep.equal([
0n,
ethers.ZeroHash,
ethers.ZeroHash,
]);
});

it('with long signature', async function () {
const signature =
'0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789';

await expect(this.mock.$recover(TEST_MESSAGE, signature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(85);

await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(85);

await expect(this.mock.$parse(signature)).to.eventually.deep.equal([0n, ethers.ZeroHash, ethers.ZeroHash]);

await expect(this.mock.$parseCalldata(signature)).to.eventually.deep.equal([
0n,
ethers.ZeroHash,
ethers.ZeroHash,
]);
});
});
});