Improve ECDSA tests and docs (#2619)

contracts/utils/cryptography/ECDSA.sol

@@ -22,6 +22,10 @@ library ECDSA {
      * recover to arbitrary addresses for non-hashed data. A safe way to ensure
      * this is by receiving a hash of the original message (which may otherwise
      * be too long), and then calling {toEthSignedMessageHash} on it.
+     *
+     * Documentation for signature generation:
+     * - with https://web3js.readthedocs.io/en/v1.3.4/web3-eth-accounts.html#sign[Web3.js]
+     * - with https://docs.ethers.io/v5/api/signer/#Signer-signMessage[ethers]
      */
     function recover(bytes32 hash, bytes memory signature) internal pure returns (address) {
         // Divide the signature in r, s and v variables

test/helpers/sign.js

@@ -4,23 +4,6 @@ function toEthSignedMessageHash (messageHex) {
   return web3.utils.sha3(Buffer.concat([prefix, messageBuffer]));
 }
 
-function fixSignature (signature) {
-  // in geth its always 27/28, in ganache its 0/1. Change to 27/28 to prevent
-  // signature malleability if version is 0/1
-  // see https://github.com/ethereum/go-ethereum/blob/v1.8.23/internal/ethapi/api.go#L465
-  let v = parseInt(signature.slice(130, 132), 16);
-  if (v < 27) {
-    v += 27;
-  }
-  const vHex = v.toString(16);
-  return signature.slice(0, 130) + vHex;
-}
-
-// signs message in node (ganache auto-applies "Ethereum Signed Message" prefix)
-async function signMessage (signer, messageHex = '0x') {
-  return fixSignature(await web3.eth.sign(messageHex, signer));
-};
-
 /**
  * Create a signer between a contract and a signer for a voucher of method, args, and redeemer
  * Note that `method` is the web3 method, not the truffle-contract method
@@ -55,12 +38,10 @@ const getSignFor = (contract, signer) => (redeemer, methodName, methodArgs = [])
 
   // return the signature of the "Ethereum Signed Message" hash of the hash of `parts`
   const messageHex = web3.utils.soliditySha3(...parts);
-  return signMessage(signer, messageHex);
+  return web3.eth.sign(messageHex, signer);
 };
 
 module.exports = {
-  signMessage,
   toEthSignedMessageHash,
-  fixSignature,
   getSignFor,
 };

test/utils/cryptography/ECDSA.test.js

@@ -1,5 +1,5 @@
 const { expectRevert } = require('@openzeppelin/test-helpers');
-const { toEthSignedMessageHash, fixSignature } = require('../../helpers/sign');
+const { toEthSignedMessageHash } = require('../../helpers/sign');
 
 const { expect } = require('chai');
 
@@ -46,6 +46,30 @@ contract('ECDSA', function (accounts) {
   });
 
   context('recover with valid signature', function () {
+    context('using web3.eth.sign', function () {
+      it('returns signer address with correct signature', async function () {
+        // Create the signature
+        const signature = await web3.eth.sign(TEST_MESSAGE, other);
+
+        // Recover the signer address from the generated message and signature.
+        expect(await this.ecdsa.recover(
+          toEthSignedMessageHash(TEST_MESSAGE),
+          signature,
+        )).to.equal(other);
+      });
+
+      it('returns a different address', async function () {
+        const signature = await web3.eth.sign(TEST_MESSAGE, other);
+        expect(await this.ecdsa.recover(WRONG_MESSAGE, signature)).to.not.equal(other);
+      });
+
+      it('reverts with invalid signature', async function () {
+        // eslint-disable-next-line max-len
+        const signature = '0x332ce75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e01c';
+        await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature');
+      });
+    });
+
     context('with v0 signature', function () {
       // Signature generated outside ganache with method web3.eth.sign(signer, message)
       const signer = '0x2cc1166f6212628A0deEf2B33BEFB2187D35b86c';
@@ -120,30 +144,6 @@ contract('ECDSA', function (accounts) {
 
       await expectRevert(this.ecdsa.recover(message, highSSignature), 'ECDSA: invalid signature \'s\' value');
     });
-
-    context('using web3.eth.sign', function () {
-      it('returns signer address with correct signature', async function () {
-        // Create the signature
-        const signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, other));
-
-        // Recover the signer address from the generated message and signature.
-        expect(await this.ecdsa.recover(
-          toEthSignedMessageHash(TEST_MESSAGE),
-          signature,
-        )).to.equal(other);
-      });
-
-      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);
-      });
-
-      it('reverts with invalid signature', 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 () {

test/utils/cryptography/SignatureChecker.test.js

@@ -1,4 +1,4 @@
-const { toEthSignedMessageHash, fixSignature } = require('../../helpers/sign');
+const { toEthSignedMessageHash } = require('../../helpers/sign');
 
 const { expect } = require('chai');
 
@@ -14,7 +14,7 @@ contract('SignatureChecker (ERC1271)', function (accounts) {
   before('deploying', async function () {
     this.signaturechecker = await SignatureCheckerMock.new();
     this.wallet = await ERC1271WalletMock.new(signer);
-    this.signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, signer));
+    this.signature = await web3.eth.sign(TEST_MESSAGE, signer);
   });
 
   context('EOA account', function () {