diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 9282977de..10a36a2df 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/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 diff --git a/test/helpers/sign.js b/test/helpers/sign.js index 7fc21cdd1..4c14d1f1d 100644 --- a/test/helpers/sign.js +++ b/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, }; diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index 468e36a19..b3152f397 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/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 () { diff --git a/test/utils/cryptography/SignatureChecker.test.js b/test/utils/cryptography/SignatureChecker.test.js index 4364d150e..6c99e3cf4 100644 --- a/test/utils/cryptography/SignatureChecker.test.js +++ b/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 () {