Make ECDSA.recover revert on error. (#2114)

* Make ECDSA.recover revert on error * Removed unused test * Remove duplicate line * Add tests for invalid signatures * Fix linter errors * Add changelog entry
OpenZeppelin · Mar 10, 2020 · 65e4ffd · 65e4ffd
1 parent e2813df
commit 65e4ffd
Show file tree

Hide file tree

Showing 3 changed files with 38 additions and 41 deletions.
diff --git 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
@@ -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
@@ -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,34 +121,26 @@ 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');
+        });
       });
     });
   });
 
   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));
     });
   });
 });