From 6ee10eeafcfa1c6ce5ab14cd52ee0ab646ba0e75 Mon Sep 17 00:00:00 2001 From: t11s Date: Sun, 31 Jul 2022 01:47:54 -0700 Subject: [PATCH 1/6] ECDSA: Remove redundant constraint --- contracts/utils/cryptography/ECDSA.sol | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 38b2ea3598a..67d155957b0 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -16,8 +16,7 @@ library ECDSA { NoError, InvalidSignature, InvalidSignatureLength, - InvalidSignatureS, - InvalidSignatureV + InvalidSignatureS } function _throwError(RecoverError error) private pure { @@ -29,8 +28,6 @@ library ECDSA { revert("ECDSA: invalid signature length"); } else if (error == RecoverError.InvalidSignatureS) { revert("ECDSA: invalid signature 's' value"); - } else if (error == RecoverError.InvalidSignatureV) { - revert("ECDSA: invalid signature 'v' value"); } } @@ -163,9 +160,6 @@ library ECDSA { if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { return (address(0), RecoverError.InvalidSignatureS); } - if (v != 27 && v != 28) { - return (address(0), RecoverError.InvalidSignatureV); - } // If the signature is valid (and not malleable), return the signer address address signer = ecrecover(hash, v, r, s); From 073034070e2fdd1c9db9245842852fdc1867661e Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 16 Aug 2022 17:28:16 +0200 Subject: [PATCH 2/6] fix tests --- test/utils/cryptography/ECDSA.test.js | 28 ++++++++------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index fff3e2c30d7..11d2b3fcc91 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -117,11 +117,8 @@ contract('ECDSA', function (accounts) { it('reverts with 00 as version value', async function () { const version = '00'; const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); }); it('works with 27 as version value', async function () { @@ -137,11 +134,8 @@ contract('ECDSA', function (accounts) { // The only valid values are 0, 1, 27 and 28. const version = '02'; const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); }); it('works with short EIP2098 format', async function () { @@ -160,11 +154,8 @@ contract('ECDSA', function (accounts) { it('reverts with 01 as version value', async function () { const version = '01'; const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); }); it('works with 28 as version value', async function () { @@ -180,11 +171,8 @@ contract('ECDSA', function (accounts) { // The only valid values are 0, 1, 27 and 28. const version = '02'; const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); }); it('works with short EIP2098 format', async function () { From ae58a23484166a2fc78528345a156cb12118ba17 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 16 Aug 2022 17:30:11 +0200 Subject: [PATCH 3/6] add changelog entry --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1afa0ddf822..46cb906d75b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ * `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515)) * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) + * `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591)) ### Compatibility Note From 1917d94b48347f2877a941e7d89501131e317528 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 16 Aug 2022 17:48:21 +0200 Subject: [PATCH 4/6] Revert enum to previous values --- contracts/utils/cryptography/ECDSA.sol | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 52489697e18..2f46e532b61 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -16,7 +16,8 @@ library ECDSA { NoError, InvalidSignature, InvalidSignatureLength, - InvalidSignatureS + InvalidSignatureS, + InvalidSignatureV } function _throwError(RecoverError error) private pure { From 0174b979b05220c202a71f606f4e1570da675702 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 16 Aug 2022 18:18:41 +0200 Subject: [PATCH 5/6] refactor tests --- test/utils/cryptography/ECDSA.test.js | 84 +++++++++++++-------------- 1 file changed, 42 insertions(+), 42 deletions(-) diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index 9fd8083d24d..04374c4fe44 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -98,39 +98,39 @@ contract('ECDSA', function (accounts) { }); }); - context('with v0 signature', function () { + context('with v=27 signature', function () { // Signature generated outside ganache with method web3.eth.sign(signer, message) const signer = '0x2cc1166f6212628A0deEf2B33BEFB2187D35b86c'; // eslint-disable-next-line max-len - const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892'; + const signatureWithoutV = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892'; - it('reverts with 00 as version value', async function () { - const version = '00'; - const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); - await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); - }); - - it('works with 27 as version value', async function () { - const version = '1b'; // 27 = 1b. - const signature = signatureWithoutVersion + version; + it('works with correct v value', async function () { + const v = '1b'; // 27 = 1b. + const signature = signatureWithoutV + v; expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(signer); expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.equal(signer); expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.equal(signer); }); - it('reverts with wrong version', async function () { - // The last two hex digits are the signature version. - // The only valid values are 0, 1, 27 and 28. - const version = '02'; - const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); - await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); + it('rejects incorrect v value', async function () { + const v = '1c'; // 28 = 1c. + const signature = signatureWithoutV + v; + expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.not.equal(signer); + expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.not.equal(signer); + expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.not.equal(signer); + }); + + it('reverts wrong v values', async function () { + for (const v of ['00', '01']) { + const signature = signatureWithoutV + v; + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); + } }); it('rejects short EIP2098 format', async function () { - const version = '1b'; // 27 = 1b. - const signature = signatureWithoutVersion + version; + const v = '1b'; // 27 = 1b. + const signature = signatureWithoutV + v; await expectRevert( this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)), 'ECDSA: invalid signature length', @@ -138,38 +138,38 @@ contract('ECDSA', function (accounts) { }); }); - context('with v1 signature', function () { + context('with v=28 signature', function () { const signer = '0x1E318623aB09Fe6de3C9b8672098464Aeda9100E'; // eslint-disable-next-line max-len - const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0'; + const signatureWithoutV = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0'; - it('reverts with 01 as version value', async function () { - const version = '01'; - const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); - await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); - }); - - it('works with 28 as version value', async function () { - const version = '1c'; // 28 = 1c. - const signature = signatureWithoutVersion + version; + it('works with correct v value', async function () { + const v = '1c'; // 28 = 1c. + const signature = signatureWithoutV + v; expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(signer); expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.equal(signer); expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.equal(signer); }); - it('reverts with wrong version', async function () { - // The last two hex digits are the signature version. - // The only valid values are 0, 1, 27 and 28. - const version = '02'; - const signature = signatureWithoutVersion + version; - await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); - await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); + it('rejects incorrect v value', async function () { + const v = '1b'; // 27 = 1b. + const signature = signatureWithoutV + v; + expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.not.equal(signer); + expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.not.equal(signer); + expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.not.equal(signer); + }); + + it('reverts invalid v values', async function () { + for (const v of ['00', '01']) { + const signature = signatureWithoutV + v; + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature'); + } }); it('rejects short EIP2098 format', async function () { - const version = '1c'; // 27 = 1b. - const signature = signatureWithoutVersion + version; + const v = '1c'; // 27 = 1b. + const signature = signatureWithoutV + v; await expectRevert( this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)), 'ECDSA: invalid signature length', From 78b23b3fc873b5adc696d760e3e05508540f6bc1 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Tue, 16 Aug 2022 14:58:12 -0300 Subject: [PATCH 6/6] add deprecation comment --- contracts/utils/cryptography/ECDSA.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 2f46e532b61..32e8b7e761a 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -17,7 +17,7 @@ library ECDSA { InvalidSignature, InvalidSignatureLength, InvalidSignatureS, - InvalidSignatureV + InvalidSignatureV // Deprecated in v4.8 } function _throwError(RecoverError error) private pure {