From 18614d4e3a71d5ac1e85bcec0a9ebf4741c3d5c8 Mon Sep 17 00:00:00 2001 From: ronhuafeng Date: Fri, 9 Sep 2022 17:09:17 +0100 Subject: [PATCH] Remove redundant ECDSA constraint (#3591) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- CHANGELOG.md | 3 +- contracts/utils/cryptography/ECDSA.sol | 7 +- test/utils/cryptography/ECDSA.test.js | 96 +++++++++++--------------- 3 files changed, 45 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25facc7b655..77a2e7ec28e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,13 @@ * `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469)) * `GovernorCompatibilityBravo`: remove unused `using` statements ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506)) * `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513)) + * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481)) * `ERC721`: optimize burn by making approval clearing implicit instead of emitting an event. ([#3538](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3538)) * `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)) * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) + * `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591)) ### Compatibility Note diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 8e6e7bb3154..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 { @@ -29,8 +29,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"); } } @@ -149,9 +147,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); diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index 3bcab192afd..04374c4fe44 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -98,45 +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 \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); - }); - - 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 \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); + 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', @@ -144,44 +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 \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); - }); - - 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 \'v\' value'); - await expectRevert( - this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), - 'ECDSA: invalid signature \'v\' value', - ); + 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',