diff --git a/.changeset/plain-times-itch.md b/.changeset/plain-times-itch.md new file mode 100644 index 00000000000..2fc84ffbe5e --- /dev/null +++ b/.changeset/plain-times-itch.md @@ -0,0 +1,5 @@ +--- +'openzeppelin-solidity': minor +--- + +`ECDSA`: Add `parse` and `parseCalldata` to parse bytes signatures of length 65 or 64 (erc-2098) into its v,r,s components. diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a6a4fb8354..74e0952bedf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,10 @@ - Update minimum pragma to 0.8.24 in `Votes`, `VotesExtended`, `ERC20Votes`, `Strings`, `ERC1155URIStorage`, `MessageHashUtils`, `ERC721URIStorage`, `ERC721Votes`, `ERC721Wrapper`, `ERC721Burnable`, `ERC721Consecutive`, `ERC721Enumerable`, `ERC721Pausable`, `ERC721Royalty`, `ERC721Wrapper`, `EIP712`, and `ERC7739`. ([#5726](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/5726)) +### Deprecation + +- `ECDSA` signature malleability protection is partly deprecated. See documentation for more details. + ## 5.4.0 (2025-07-17) ### Breaking changes diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 334c44bd721..22544c4a66b 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -43,6 +43,10 @@ library ECDSA { * this function rejects them by requiring the `s` value to be in the lower * half order, and the `v` value to be either 27 or 28. * + * NOTE: This function only supports 65-byte signatures. ERC-2098 short signatures are rejected. This restriction + * is DEPRECATED and will be removed in v6.0. Developers SHOULD NOT use signatures as unique identifiers; use hash + * invalidation or nonces for replay protection. + * * IMPORTANT: `hash` _must_ be the result of a hash operation for the * verification to be secure: it is possible to craft signatures that * recover to arbitrary addresses for non-hashed data. A safe way to ensure @@ -106,6 +110,10 @@ library ECDSA { * this function rejects them by requiring the `s` value to be in the lower * half order, and the `v` value to be either 27 or 28. * + * NOTE: This function only supports 65-byte signatures. ERC-2098 short signatures are rejected. This restriction + * is DEPRECATED and will be removed in v6.0. Developers SHOULD NOT use signatures as unique identifiers; use hash + * invalidation or nonces for replay protection. + * * IMPORTANT: `hash` _must_ be the result of a hash operation for the * verification to be secure: it is possible to craft signatures that * recover to arbitrary addresses for non-hashed data. A safe way to ensure @@ -196,6 +204,63 @@ library ECDSA { return recovered; } + /** + * @dev Parse a signature into its `v`, `r` and `s` components. Supports 65-byte and 64-byte (ERC-2098) + * formats. Returns (0,0,0) for invalid signatures. Consider skipping {tryRecover} or {recover} if so. + */ + function parse(bytes memory signature) internal pure returns (uint8 v, bytes32 r, bytes32 s) { + 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 (uint8 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. */ diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index fa40f597c94..513841c2fdc 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -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); }); @@ -43,8 +51,10 @@ 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 () { @@ -52,14 +62,18 @@ describe('ECDSA', function () { 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 () { @@ -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( @@ -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, ); }); @@ -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, + ]); + }); + }); });