From 1412f9275ee2a6032f5b8247c3ec551a57b84473 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 31 Jul 2025 16:56:09 +0200 Subject: [PATCH 1/3] ECDSA: add parse and tryParse (#5814) Co-authored-by: ernestognw --- .changeset/plain-times-itch.md | 5 + CHANGELOG.md | 4 + contracts/utils/cryptography/ECDSA.sol | 65 ++++++++++++ test/utils/cryptography/ECDSA.test.js | 139 +++++++++++++++++++------ 4 files changed, 183 insertions(+), 30 deletions(-) create mode 100644 .changeset/plain-times-itch.md 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, + ]); + }); + }); }); From 3790c59623e99cb0272ddf84e6a17a5979d06b35 Mon Sep 17 00:00:00 2001 From: Daejun Park Date: Thu, 31 Jul 2025 08:58:01 -0700 Subject: [PATCH 2/3] Enable some math tests to be run by halmos (#5824) Co-authored-by: ernestognw --- .github/workflows/formal-verification.yml | 2 ++ test/utils/Bytes.t.sol | 4 ++-- test/utils/math/Math.t.sol | 12 +++++++----- test/utils/math/SignedMath.t.sol | 5 +++-- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/.github/workflows/formal-verification.yml b/.github/workflows/formal-verification.yml index 86acca7f32b..b26fcef003b 100644 --- a/.github/workflows/formal-verification.yml +++ b/.github/workflows/formal-verification.yml @@ -84,3 +84,5 @@ jobs: run: pip install -r fv-requirements.txt - name: Run Halmos run: halmos --match-test '^symbolic|^testSymbolic' -vv + env: + HALMOS_ALLOW_DOWNLOAD: 1 diff --git a/test/utils/Bytes.t.sol b/test/utils/Bytes.t.sol index 9fdcd47c2d1..e01d933460d 100644 --- a/test/utils/Bytes.t.sol +++ b/test/utils/Bytes.t.sol @@ -14,7 +14,7 @@ contract BytesTest is Test { } // INDEX OF - function testIndexOf(bytes memory buffer, bytes1 s) public pure { + function testSymbolicIndexOf(bytes memory buffer, bytes1 s) public pure { uint256 result = Bytes.indexOf(buffer, s); if (buffer.length == 0) { @@ -48,7 +48,7 @@ contract BytesTest is Test { } } - function testLastIndexOf(bytes memory buffer, bytes1 s) public pure { + function testSymbolicLastIndexOf(bytes memory buffer, bytes1 s) public pure { uint256 result = Bytes.lastIndexOf(buffer, s); if (buffer.length == 0) { diff --git a/test/utils/math/Math.t.sol b/test/utils/math/Math.t.sol index b25db92de8d..4284e44a64b 100644 --- a/test/utils/math/Math.t.sol +++ b/test/utils/math/Math.t.sol @@ -12,7 +12,7 @@ contract MathTest is Test { } // ADD512 & MUL512 - function testAdd512(uint256 a, uint256 b) public pure { + function testSymbolicAdd512(uint256 a, uint256 b) public pure { (uint256 high, uint256 low) = Math.add512(a, b); // test against tryAdd @@ -60,7 +60,8 @@ contract MathTest is Test { } // CEILDIV - function testCeilDiv(uint256 a, uint256 b) public pure { + /// @custom:halmos --solver cvc5-int + function testSymbolicCeilDiv(uint256 a, uint256 b) public pure { vm.assume(b > 0); uint256 result = Math.ceilDiv(a, b); @@ -112,17 +113,18 @@ contract MathTest is Test { _testInvMod(value, p, true); } - function testInvMod2(uint256 seed) public pure { + function testSymbolicInvMod2(uint256 seed) public pure { uint256 p = 2; // prime _testInvMod(bound(seed, 1, p - 1), p, false); } - function testInvMod17(uint256 seed) public pure { + function testSymbolicInvMod17(uint256 seed) public pure { uint256 p = 17; // prime _testInvMod(bound(seed, 1, p - 1), p, false); } - function testInvMod65537(uint256 seed) public pure { + /// @custom:halmos --solver bitwuzla-abs + function testSymbolicInvMod65537(uint256 seed) public pure { uint256 p = 65537; // prime _testInvMod(bound(seed, 1, p - 1), p, false); } diff --git a/test/utils/math/SignedMath.t.sol b/test/utils/math/SignedMath.t.sol index 98a40a13998..aa567d61a6f 100644 --- a/test/utils/math/SignedMath.t.sol +++ b/test/utils/math/SignedMath.t.sol @@ -47,8 +47,9 @@ contract SignedMathTest is Test { assertEq(result, (a + b) / 2); } - // 2. more complex test, full int256 range - function testAverage2(int256 a, int256 b) public pure { + // 2. more complex test, full int256 range (solver timeout 0 = no timeout) + /// @custom:halmos --solver-timeout-assertion 0 + function testSymbolicAverage2(int256 a, int256 b) public pure { (int256 result, int256 min, int256 max) = ( SignedMath.average(a, b), SignedMath.min(a, b), From 341782b8a484c03d301b48785975033fbadac31f Mon Sep 17 00:00:00 2001 From: Maximilian Hubert <64627729+gap-editor@users.noreply.github.com> Date: Sun, 3 Aug 2025 12:45:24 +0200 Subject: [PATCH 3/3] Update account-abstraction.adoc --- docs/modules/ROOT/pages/account-abstraction.adoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/modules/ROOT/pages/account-abstraction.adoc b/docs/modules/ROOT/pages/account-abstraction.adoc index 82fc65e295a..a2e070dc4a1 100644 --- a/docs/modules/ROOT/pages/account-abstraction.adoc +++ b/docs/modules/ROOT/pages/account-abstraction.adoc @@ -95,6 +95,6 @@ To process a bundle of `UserOperations`, bundlers call xref:api:account.adoc#Acc These rules outline the requirements for operations to be processed by the canonical mempool. -Accounts can access its own storage during the validation phase, they might easily violate ERC-7562 storage access rules in undirect ways. For example, most accounts access their public keys from storage when validating a signature, limiting the ability of having accounts that validate operations for other accounts (e.g. via ERC-1271) +Accounts can access its own storage during the validation phase, they might easily violate ERC-7562 storage access rules in undirect ways. For example, most accounts access their public keys from storage when validating a signature, limiting the ability of having accounts that validate operations for other accounts (e.g. via https://eips.ethereum.org/EIPS/eip-1271[ERC-1271]) TIP: Although any Account that breaks such rules may still be processed by a private bundler, developers should keep in mind the centralization tradeoffs of relying on private infrastructure instead of _permissionless_ execution.