From 7ec8d445371557eaa9f608f59616dbd09964fd20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 9 Mar 2020 18:24:03 -0300 Subject: [PATCH 1/6] Make ECDSA.recover revert on error --- contracts/cryptography/ECDSA.sol | 15 ++++++------- test/cryptography/ECDSA.test.js | 37 ++++++++++++++++---------------- 2 files changed, 26 insertions(+), 26 deletions(-) diff --git a/contracts/cryptography/ECDSA.sol b/contracts/cryptography/ECDSA.sol index 4651cb421be..46d412c7140 100644 --- a/contracts/cryptography/ECDSA.sol +++ b/contracts/cryptography/ECDSA.sol @@ -15,10 +15,6 @@ 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 call _does not revert_ if the signature is invalid, or - * if the signer is otherwise unable to be retrieved. In those scenarios, - * the zero address is returned. - * * 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 @@ -28,7 +24,7 @@ library ECDSA { function recover(bytes32 hash, bytes memory signature) internal pure returns (address) { // Check the signature length if (signature.length != 65) { - return (address(0)); + revert("ECDSA: invalid signature length"); } // Divide the signature in r, s and v variables @@ -55,15 +51,18 @@ library ECDSA { // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept // these malleable signatures as well. if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { - return address(0); + revert("ECDSA: invalid signature 's' value"); } if (v != 27 && v != 28) { - return address(0); + revert("ECDSA: invalid signature 'v' value"); } // If the signature is valid (and not malleable), return the signer address - return ecrecover(hash, v, r, s); + address signer = ecrecover(hash, v, r, s); + require(signer != address(0), "ECDSA: invalid signature"); + + return signer; } /** diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index 9e3b24e9a3c..2abbbaeb3e2 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -1,7 +1,6 @@ const { accounts, contract, web3 } = require('@openzeppelin/test-environment'); -const { constants, expectRevert } = require('@openzeppelin/test-helpers'); -const { ZERO_ADDRESS } = constants; +const { expectRevert } = require('@openzeppelin/test-helpers'); const { toEthSignedMessageHash, fixSignature } = require('../helpers/sign'); const { expect } = require('chai'); @@ -20,13 +19,15 @@ describe('ECDSA', function () { context('recover with invalid signature', function () { it('with short signature', async function () { - expect(await this.ecdsa.recover(TEST_MESSAGE, '0x1234')).to.equal(ZERO_ADDRESS); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, '0x1234'), 'ECDSA: invalid signature length'); }); it('with long signature', async function () { - // eslint-disable-next-line max-len - expect(await this.ecdsa.recover(TEST_MESSAGE, '0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789')) - .to.equal(ZERO_ADDRESS); + await expectRevert( + // eslint-disable-next-line max-len + this.ecdsa.recover(TEST_MESSAGE, '0x01234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789'), + 'ECDSA: invalid signature length' + ); }); }); @@ -38,10 +39,10 @@ describe('ECDSA', function () { const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892'; context('with 00 as version value', function () { - it('returns 0', async function () { + it('reverts', async function () { const version = '00'; const signature = signatureWithoutVersion + version; - expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(ZERO_ADDRESS); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); }); }); @@ -54,12 +55,12 @@ describe('ECDSA', function () { }); context('with wrong version', function () { - it('returns 0', async function () { + it('reverts', 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; - expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(ZERO_ADDRESS); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); }); }); }); @@ -70,10 +71,10 @@ describe('ECDSA', function () { const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0'; context('with 01 as version value', function () { - it('returns 0', async function () { + it('reverts', async function () { const version = '01'; const signature = signatureWithoutVersion + version; - expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(ZERO_ADDRESS); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); }); }); @@ -86,23 +87,23 @@ describe('ECDSA', function () { }); context('with wrong version', function () { - it('returns 0', async function () { + it('reverts', 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; - expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(ZERO_ADDRESS); + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); }); }); }); context('with high-s value signature', function () { - it('returns 0', async function () { + it('reverts', async function () { const message = '0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9'; // eslint-disable-next-line max-len const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; - expect(await this.ecdsa.recover(message, highSSignature)).to.equal(ZERO_ADDRESS); + await expectRevert(this.ecdsa.recover(message, highSSignature), 'ECDSA: invalid signature \'s\' value'); }); }); @@ -121,9 +122,9 @@ describe('ECDSA', function () { }); context('with wrong signature', function () { - it('does not return signer address', async function () { + it('reverts', async function () { // Create the signature - const signature = await web3.eth.sign(TEST_MESSAGE, other); + const signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, other)); // Recover the signer address from the generated message and wrong signature. expect(await this.ecdsa.recover(WRONG_MESSAGE, signature)).to.not.equal(other); From 4121b0b647193917bec33fd9bad36a015ed70a04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 9 Mar 2020 18:51:11 -0300 Subject: [PATCH 2/6] Removed unused test --- test/cryptography/ECDSA.test.js | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index 2abbbaeb3e2..a155e8b2a9b 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -131,18 +131,6 @@ describe('ECDSA', function () { }); }); }); - - context('with small hash', function () { - // @TODO - remove `skip` once we upgrade to solc^0.5 - it.skip('reverts', async function () { - // Create the signature - const signature = await web3.eth.sign(TEST_MESSAGE, other); - await expectRevert( - this.ecdsa.recover(TEST_MESSAGE.substring(2), signature), - 'Failure message' - ); - }); - }); }); context('toEthSignedMessage', function () { From f7d958a389223a2bbf20e24e95f48e4a639e4ecc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 9 Mar 2020 18:51:36 -0300 Subject: [PATCH 3/6] Remove duplicate line --- test/cryptography/ECDSA.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index a155e8b2a9b..aa369e74df1 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -136,7 +136,6 @@ describe('ECDSA', function () { context('toEthSignedMessage', function () { it('should prefix hashes correctly', async function () { expect(await this.ecdsa.toEthSignedMessageHash(TEST_MESSAGE)).to.equal(toEthSignedMessageHash(TEST_MESSAGE)); - expect(await this.ecdsa.toEthSignedMessageHash(TEST_MESSAGE)).to.equal(toEthSignedMessageHash(TEST_MESSAGE)); }); }); }); From c37c9741d04c9708430676a73b2eef78691b73a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Mon, 9 Mar 2020 19:00:42 -0300 Subject: [PATCH 4/6] Add tests for invalid signatures --- test/cryptography/ECDSA.test.js | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index aa369e74df1..350bbf71b52 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -121,15 +121,19 @@ describe('ECDSA', function () { }); }); - context('with wrong signature', function () { - it('reverts', async function () { - // Create the signature + context('with wrong message', function () { + it('returns a different address', async function () { const signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, other)); - - // Recover the signer address from the generated message and wrong signature. expect(await this.ecdsa.recover(WRONG_MESSAGE, signature)).to.not.equal(other); }); }); + + context('with invalid signature', function () { + it('reverts', async function () { + const signature = '0x332ce75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e01c' + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + }); + }); }); }); From dea4790e33fd7087d098ae05ce10f672f6c33945 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 10 Mar 2020 14:53:50 -0300 Subject: [PATCH 5/6] Fix linter errors --- test/cryptography/ECDSA.test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index 350bbf71b52..8c6ec91f6a8 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -130,7 +130,8 @@ describe('ECDSA', function () { context('with invalid signature', function () { it('reverts', async function () { - const signature = '0x332ce75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e01c' + // eslint-disable-next-line max-len + const signature = '0x332ce75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e01c'; await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); }); }); From 6a259d7a9f832471b84aa31e682759a1a7dd346b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Venturo?= Date: Tue, 10 Mar 2020 20:12:57 -0300 Subject: [PATCH 6/6] Add changelog entry --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b446128f402..85e0cf6d5ce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,10 @@ # Changelog +## 3.0.0 (unreleased) + +### Breaking Changes + * `ECDSA`: when receiving an invalid signature, `recover` now reverts instead of returning the zero address. ([#2114](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2114)) + ## 2.5.0 (2020-02-04) ### New features