diff --git a/CHANGELOG.md b/CHANGELOG.md index 93d08b038..f71fb6bd1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * Upgraded the minimum compiler version to v0.5.2: this removes many Solidity warnings that were false positives. ([#1606](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1606)) * `Counter`'s API has been improved, and is now used by `ERC721` (though it is still in `drafts`). ([#1610](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1610)) * `ERC721`'s transfers are now more gas efficient due to removal of unnecessary `SafeMath` calls. ([#1610](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1610)) + * `ECDSA`: `recover` no longer accepts malleable signatures (those using upper-range values for `s`, or 0/1 for `v`). ([#1622](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1622)) * Fixed variable shadowing issues. ([#1606](https://github.com/OpenZeppelin/openzeppelin-solidity/pull/1606)) ### Bugfixes: diff --git a/contracts/cryptography/ECDSA.sol b/contracts/cryptography/ECDSA.sol index 39b6ad476..09e4e0c4e 100644 --- a/contracts/cryptography/ECDSA.sol +++ b/contracts/cryptography/ECDSA.sol @@ -14,16 +14,16 @@ library ECDSA { * @param signature bytes signature, the signature is generated using web3.eth.sign() */ function recover(bytes32 hash, bytes memory signature) internal pure returns (address) { - bytes32 r; - bytes32 s; - uint8 v; - // Check the signature length if (signature.length != 65) { return (address(0)); } // Divide the signature in r, s and v variables + bytes32 r; + bytes32 s; + uint8 v; + // ecrecover takes the signature parameters, and the only way to get them // currently is to use assembly. // solhint-disable-next-line no-inline-assembly @@ -33,17 +33,25 @@ library ECDSA { v := byte(0, mload(add(signature, 0x60))) } - // Version of signature should be 27 or 28, but 0 and 1 are also possible versions - if (v < 27) { - v += 27; + // EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature + // unique. Appendix F in the Ethereum Yellow paper (https://ethereum.github.io/yellowpaper/paper.pdf), defines + // the valid range for s in (281): 0 < s < secp256k1n ÷ 2 + 1, and for v in (282): v ∈ {27, 28}. Most + // signatures from current libraries generate a unique signature with an s-value in the lower half order. + // + // If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value + // with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or + // 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); } - // If the version is correct return the signer address if (v != 27 && v != 28) { - return (address(0)); - } else { - return ecrecover(hash, v, r, s); + return address(0); } + + // If the signature is valid (and not malleable), return the signer address + return ecrecover(hash, v, r, s); } /** diff --git a/test/cryptography/ECDSA.test.js b/test/cryptography/ECDSA.test.js index c37f405bd..573646159 100644 --- a/test/cryptography/ECDSA.test.js +++ b/test/cryptography/ECDSA.test.js @@ -1,5 +1,6 @@ -const { shouldFail } = require('openzeppelin-test-helpers'); -const { signMessage, toEthSignedMessageHash } = require('../helpers/sign'); +const { constants, shouldFail } = require('openzeppelin-test-helpers'); +const { ZERO_ADDRESS } = constants; +const { toEthSignedMessageHash, fixSignature } = require('../helpers/sign'); const ECDSAMock = artifacts.require('ECDSAMock'); @@ -19,10 +20,10 @@ contract('ECDSA', function ([_, anyone]) { const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892'; context('with 00 as version value', function () { - it('works', async function () { + it('returns 0', async function () { const version = '00'; const signature = signatureWithoutVersion + version; - (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer); + (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(ZERO_ADDRESS); }); }); @@ -40,8 +41,7 @@ contract('ECDSA', function ([_, anyone]) { // The only valid values are 0, 1, 27 and 28. const version = '02'; const signature = signatureWithoutVersion + version; - (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal( - '0x0000000000000000000000000000000000000000'); + (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(ZERO_ADDRESS); }); }); }); @@ -52,14 +52,14 @@ contract('ECDSA', function ([_, anyone]) { const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0'; context('with 01 as version value', function () { - it('works', async function () { + it('returns 0', async function () { const version = '01'; const signature = signatureWithoutVersion + version; - (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(signer); + (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(ZERO_ADDRESS); }); }); - context('with 28 signature', function () { + context('with 28 as version value', function () { it('works', async function () { const version = '1c'; // 28 = 1c. const signature = signatureWithoutVersion + version; @@ -73,17 +73,26 @@ contract('ECDSA', function ([_, anyone]) { // The only valid values are 0, 1, 27 and 28. const version = '02'; const signature = signatureWithoutVersion + version; - (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal( - '0x0000000000000000000000000000000000000000'); + (await this.ecdsa.recover(TEST_MESSAGE, signature)).should.equal(ZERO_ADDRESS); }); }); }); + context('with high-s value signature', function () { + it('returns 0', async function () { + const message = '0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9'; + // eslint-disable-next-line max-len + const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; + + (await this.ecdsa.recover(message, highSSignature)).should.equal(ZERO_ADDRESS); + }); + }); + context('using web3.eth.sign', function () { context('with correct signature', function () { it('returns signer address', async function () { // Create the signature - const signature = await signMessage(anyone, TEST_MESSAGE); + const signature = fixSignature(await web3.eth.sign(TEST_MESSAGE, anyone)); // Recover the signer address from the generated message and signature. (await this.ecdsa.recover( @@ -96,23 +105,23 @@ contract('ECDSA', function ([_, anyone]) { context('with wrong signature', function () { it('does not return signer address', async function () { // Create the signature - const signature = await signMessage(anyone, TEST_MESSAGE); + const signature = await web3.eth.sign(TEST_MESSAGE, anyone); // Recover the signer address from the generated message and wrong signature. (await this.ecdsa.recover(WRONG_MESSAGE, signature)).should.not.equal(anyone); }); }); }); - }); - 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 signMessage(anyone, TEST_MESSAGE); - await shouldFail.reverting( - this.ecdsa.recover(TEST_MESSAGE.substring(2), signature) - ); + 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, anyone); + await shouldFail.reverting( + this.ecdsa.recover(TEST_MESSAGE.substring(2), signature) + ); + }); }); }); diff --git a/test/helpers/sign.js b/test/helpers/sign.js index 1c60b6484..38b39a6f6 100644 --- a/test/helpers/sign.js +++ b/test/helpers/sign.js @@ -9,9 +9,21 @@ 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) -const signMessage = (signer, messageHex = '0x') => { - return web3.eth.sign(messageHex, signer); +async function signMessage (signer, messageHex = '0x') { + return fixSignature(await web3.eth.sign(messageHex, signer)); }; /** @@ -50,5 +62,6 @@ const getSignFor = (contract, signer) => (redeemer, methodName, methodArgs = []) module.exports = { signMessage, toEthSignedMessageHash, + fixSignature, getSignFor, };