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 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..8c6ec91f6a8 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'); }); }); @@ -120,26 +121,19 @@ describe('ECDSA', function () { }); }); - context('with wrong signature', function () { - it('does not return signer address', async function () { - // Create the signature - const signature = await web3.eth.sign(TEST_MESSAGE, other); - - // Recover the signer address from the generated message and wrong signature. + context('with wrong message', function () { + it('returns a different address', async function () { + const signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, other)); expect(await this.ecdsa.recover(WRONG_MESSAGE, signature)).to.not.equal(other); }); }); - }); - 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('with invalid signature', function () { + it('reverts', async function () { + // eslint-disable-next-line max-len + const signature = '0x332ce75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e01c'; + await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature'); + }); }); }); }); @@ -147,7 +141,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)); }); }); });