From f8ce9f67ad90a6963169019a370d6ca0fecaf59d Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Wed, 5 May 2021 10:33:49 +0200 Subject: [PATCH 01/10] Optimize EOA signature verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Prev gas:
EOA: 2000 + 3500
SC: 2000

Now gas:
EOA: 3500
SC: (signature.length is 64 or 65 ? 3500 : 0) + 2000 --- .../utils/cryptography/SignatureChecker.sol | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index c678a52b618..d0af5756d5a 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -18,14 +18,14 @@ import "../../interfaces/IERC1271.sol"; */ library SignatureChecker { function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { - if (Address.isContract(signer)) { - try IERC1271(signer).isValidSignature(hash, signature) returns (bytes4 magicValue) { - return magicValue == IERC1271(signer).isValidSignature.selector; - } catch { - return false; - } - } else { - return ECDSA.recover(hash, signature) == signer; + if ((signature.length == 65 || signature.length == 64) && ECDSA.recover(hash, signature) == signer) { + return true; + } + + try IERC1271(signer).isValidSignature(hash, signature) returns (bytes4 magicValue) { + return magicValue == IERC1271(signer).isValidSignature.selector; + } catch { + return false; } } } From 791de63fc6f06135715a9a6bab5aa2fd82e16d41 Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Wed, 5 May 2021 11:56:14 +0200 Subject: [PATCH 02/10] Fix EOA address call revert --- contracts/utils/cryptography/SignatureChecker.sol | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index d0af5756d5a..d8fa1434c9f 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -22,10 +22,7 @@ library SignatureChecker { return true; } - try IERC1271(signer).isValidSignature(hash, signature) returns (bytes4 magicValue) { - return magicValue == IERC1271(signer).isValidSignature.selector; - } catch { - return false; - } + (bool success, bytes memory result) = signer.staticcall(abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)); + return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector); } } From 98091c2c87982c15e551287cfcafe6f3ca4e7ae0 Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Tue, 13 Jul 2021 00:49:26 +0300 Subject: [PATCH 03/10] Introduce non-reverting ECDSA-tryRecover methods --- contracts/utils/cryptography/ECDSA.sol | 56 ++++++++++++++++--- .../utils/cryptography/SignatureChecker.sol | 7 ++- 2 files changed, 52 insertions(+), 11 deletions(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 9282977de13..ef9f0d2140b 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -11,7 +11,7 @@ pragma solidity ^0.8.0; library ECDSA { /** * @dev Returns the address that signed a hashed message (`hash`) with - * `signature`. This address can then be used for verification purposes. + * `signature` or error string. This address can then be used for verification purposes. * * The `ecrecover` EVM opcode allows for malleable (non-unique) signatures: * this function rejects them by requiring the `s` value to be in the lower @@ -23,7 +23,7 @@ library ECDSA { * this is by receiving a hash of the original message (which may otherwise * be too long), and then calling {toEthSignedMessageHash} on it. */ - function recover(bytes32 hash, bytes memory signature) internal pure returns (address) { + function tryRecover(bytes32 hash, bytes memory signature) internal pure returns (address, string memory) { // Divide the signature in r, s and v variables bytes32 r; bytes32 s; @@ -52,17 +52,39 @@ library ECDSA { v := add(shr(255, vs), 27) } } else { - revert("ECDSA: invalid signature length"); + return (address(0), "ECDSA: invalid signature length"); } - return recover(hash, v, r, s); + return (recover(hash, v, r, s), ""); } /** - * @dev Overload of {ECDSA-recover} that receives the `v`, + * @dev Returns the address that signed a hashed message (`hash`) with + * `signature`. This address can then be used for verification purposes. + * + * The `ecrecover` EVM opcode allows for malleable (non-unique) signatures: + * 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. + * + * 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 + * this is by receiving a hash of the original message (which may otherwise + * be too long), and then calling {toEthSignedMessageHash} on it. + */ + function recover(bytes32 hash, bytes memory signature) internal pure returns (address) { + (address recovered, string memory error) = tryRecover(hash, signature); + if (bytes(error).length > 0) { + revert(error); + } + return recovered; + } + + /** + * @dev Overload of {ECDSA-tryRecover} that receives the `v`, * `r` and `s` signature fields separately. */ - function recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) internal pure returns (address) { + function tryRecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) internal pure returns (address, string memory) { // 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 @@ -73,13 +95,29 @@ 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. require(uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0, "ECDSA: invalid signature 's' value"); - require(v == 27 || v == 28, "ECDSA: invalid signature 'v' value"); + if (v != 27 && v != 28) { + return (address(0), "ECDSA: invalid signature 'v' value"); + } // If the signature is valid (and not malleable), return the signer address address signer = ecrecover(hash, v, r, s); - require(signer != address(0), "ECDSA: invalid signature"); + if (signer == address(0)) { + return (address(0), "ECDSA: invalid signature"); + } - return signer; + return (signer, ""); + } + + /** + * @dev Overload of {ECDSA-recover} that receives the `v`, + * `r` and `s` signature fields separately. + */ + function recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) internal pure returns (address) { + (address recovered, string memory error) = tryRecover(hash, v, r, s); + if (bytes(error).length > 0) { + revert(error); + } + return recovered; } /** diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index d8fa1434c9f..7fea6065343 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -18,8 +18,11 @@ import "../../interfaces/IERC1271.sol"; */ library SignatureChecker { function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { - if ((signature.length == 65 || signature.length == 64) && ECDSA.recover(hash, signature) == signer) { - return true; + if (signature.length == 64 || signature.length == 65) { + (address recovered, string memory error) = ECDSA.tryRecover(hash, signature); + if (bytes(error).length == 0 && recovered == signer) { + return true; + } } (bool success, bytes memory result) = signer.staticcall(abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)); From 3ebc72219ea39cf352474a2ae609d6eed0f6c551 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 13 Jul 2021 00:24:58 +0200 Subject: [PATCH 04/10] fix lint --- contracts/utils/cryptography/ECDSA.sol | 26 ++++++++++++++++--- .../utils/cryptography/SignatureChecker.sol | 10 +++++-- 2 files changed, 30 insertions(+), 6 deletions(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 4363a104e34..27ac9a7b22f 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -87,7 +87,11 @@ library ECDSA { * * _Available since v4.2._ */ - function tryRecover(bytes32 hash, bytes32 r, bytes32 vs) internal pure returns (address, string memory) { + function tryRecover( + bytes32 hash, + bytes32 r, + bytes32 vs + ) internal pure returns (address, string memory) { bytes32 s; uint8 v; assembly { @@ -100,7 +104,11 @@ library ECDSA { /** * @dev Overload of {ECDSA-recover} that receives the `r and `vs` short-signature fields separately. */ - function recover(bytes32 hash, bytes32 r, bytes32 vs) internal pure returns (address) { + function recover( + bytes32 hash, + bytes32 r, + bytes32 vs + ) internal pure returns (address) { (address recovered, string memory error) = tryRecover(hash, r, vs); if (bytes(error).length > 0) { revert(error); @@ -112,7 +120,12 @@ library ECDSA { * @dev Overload of {ECDSA-tryRecover} that receives the `v`, * `r` and `s` signature fields separately. */ - function tryRecover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) internal pure returns (address, string memory) { + function tryRecover( + bytes32 hash, + uint8 v, + bytes32 r, + bytes32 s + ) internal pure returns (address, string memory) { // 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 @@ -142,7 +155,12 @@ library ECDSA { * @dev Overload of {ECDSA-recover} that receives the `v`, * `r` and `s` signature fields separately. */ - function recover(bytes32 hash, uint8 v, bytes32 r, bytes32 s) internal pure returns (address) { + function recover( + bytes32 hash, + uint8 v, + bytes32 r, + bytes32 s + ) internal pure returns (address) { (address recovered, string memory error) = tryRecover(hash, v, r, s); if (bytes(error).length > 0) { revert(error); diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index c8d208e7cea..fec692c698c 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -17,13 +17,19 @@ import "../../interfaces/IERC1271.sol"; * _Available since v4.1._ */ library SignatureChecker { - function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) { + function isValidSignatureNow( + address signer, + bytes32 hash, + bytes memory signature + ) internal view returns (bool) { (address recovered, string memory error) = ECDSA.tryRecover(hash, signature); if (bytes(error).length == 0 && recovered == signer) { return true; } - (bool success, bytes memory result) = signer.staticcall(abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature)); + (bool success, bytes memory result) = signer.staticcall( + abi.encodeWithSelector(IERC1271.isValidSignature.selector, hash, signature) + ); return (success && result.length == 32 && abi.decode(result, (bytes4)) == IERC1271.isValidSignature.selector); } } From 3fa6ca4588dc760f79e0aee4f82819f65a2e9668 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 13 Jul 2021 00:27:36 +0200 Subject: [PATCH 05/10] use tryrecover instead of recover to avoid string allocation --- contracts/utils/cryptography/ECDSA.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 27ac9a7b22f..28a9820d809 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -42,7 +42,7 @@ library ECDSA { s := mload(add(signature, 0x40)) v := byte(0, mload(add(signature, 0x60))) } - return (recover(hash, v, r, s), ""); + return tryRecover(hash, v, r, s); } else if (signature.length == 64) { bytes32 r; bytes32 vs; @@ -52,7 +52,7 @@ library ECDSA { r := mload(add(signature, 0x20)) vs := mload(add(signature, 0x40)) } - return (recover(hash, r, vs), ""); + return tryRecover(hash, r, vs); } else { return (address(0), "ECDSA: invalid signature length"); } From cae49cbf1758ffecbc542111e013c7593b52920a Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Tue, 13 Jul 2021 01:18:30 +0200 Subject: [PATCH 06/10] fix coverage of ECDSA's tryRecover --- contracts/mocks/ECDSAMock.sol | 19 ++++++++++ test/utils/cryptography/ECDSA.test.js | 51 ++++++++++++++++++++++++++- 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/contracts/mocks/ECDSAMock.sol b/contracts/mocks/ECDSAMock.sol index 99f6a7983e0..e1296a3f0cb 100644 --- a/contracts/mocks/ECDSAMock.sol +++ b/contracts/mocks/ECDSAMock.sol @@ -11,6 +11,25 @@ contract ECDSAMock { return hash.recover(signature); } + // solhint-disable-next-line func-name-mixedcase + function recover_v_r_s( + bytes32 hash, + uint8 v, + bytes32 r, + bytes32 s + ) public pure returns (address) { + return hash.recover(v, r, s); + } + + // solhint-disable-next-line func-name-mixedcase + function recover_r_vs( + bytes32 hash, + bytes32 r, + bytes32 vs + ) public pure returns (address) { + return hash.recover(r, vs); + } + function toEthSignedMessageHash(bytes32 hash) public pure returns (bytes32) { return hash.toEthSignedMessageHash(); } diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index b3152f3977c..ba20cdee4e7 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -24,6 +24,25 @@ function from2098Format (signature) { return web3.utils.bytesToHex(short); } +function split (signature) { + const raw = web3.utils.hexToBytes(signature); + switch (raw.length) { + case 64: + return [ + web3.utils.bytesToHex(raw.slice(0, 32)), // r + web3.utils.bytesToHex(raw.slice(32, 64)), // vs + ]; + case 65: + return [ + raw[64], // v + web3.utils.bytesToHex(raw.slice(0, 32)), // r + web3.utils.bytesToHex(raw.slice(32, 64)), // s + ]; + default: + expect.fail('Invalid siganture length, cannot split'); + } +} + contract('ECDSA', function (accounts) { const [ other ] = accounts; @@ -80,12 +99,18 @@ contract('ECDSA', function (accounts) { const version = '00'; const signature = signatureWithoutVersion + version; await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); + await expectRevert( + this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), + 'ECDSA: invalid signature \'v\' value', + ); }); it('works with 27 as version value', async function () { const version = '1b'; // 27 = 1b. const signature = signatureWithoutVersion + version; expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(signer); + expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.equal(signer); + expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.equal(signer); }); it('reverts with wrong version', async function () { @@ -94,6 +119,10 @@ contract('ECDSA', function (accounts) { const version = '02'; const signature = signatureWithoutVersion + version; await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); + await expectRevert( + this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), + 'ECDSA: invalid signature \'v\' value', + ); }); it('works with short EIP2098 format', async function () { @@ -113,12 +142,18 @@ contract('ECDSA', function (accounts) { const version = '01'; const signature = signatureWithoutVersion + version; await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); + await expectRevert( + this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), + 'ECDSA: invalid signature \'v\' value', + ); }); it('works with 28 as version value', async function () { const version = '1c'; // 28 = 1c. const signature = signatureWithoutVersion + version; expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(signer); + expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.equal(signer); + expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.equal(signer); }); it('reverts with wrong version', async function () { @@ -127,6 +162,10 @@ contract('ECDSA', function (accounts) { const version = '02'; const signature = signatureWithoutVersion + version; await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value'); + await expectRevert( + this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), + 'ECDSA: invalid signature \'v\' value', + ); }); it('works with short EIP2098 format', async function () { @@ -140,9 +179,19 @@ contract('ECDSA', function (accounts) { it('reverts with high-s value signature', async function () { const message = '0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9'; // eslint-disable-next-line max-len - const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; + // const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; + // eslint-disable-next-line max-len + const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bd7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a11b'; await expectRevert(this.ecdsa.recover(message, highSSignature), 'ECDSA: invalid signature \'s\' value'); + await expectRevert( + this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(highSSignature)), + 'ECDSA: invalid signature \'s\' value', + ); + await expectRevert( + this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(highSSignature))), + 'ECDSA: invalid signature \'s\' value', + ); }); }); From 2a8ac50ab289d267477afda0c9416d908b73c3b3 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 15:03:24 +0200 Subject: [PATCH 07/10] use an enum for error management in ECDSA --- contracts/utils/cryptography/ECDSA.sol | 56 ++++++++++++------- .../utils/cryptography/SignatureChecker.sol | 4 +- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 28a9820d809..4ad6b35dfbe 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -9,6 +9,28 @@ pragma solidity ^0.8.0; * of the private keys of a given address. */ library ECDSA { + enum RecoverError { + NoError, + InvalidSignature, + InvalidSignatureLength, + InvalidSignatureS, + InvalidSignatureV + } + + function _throwError(RecoverError error) private pure { + if (error == RecoverError.NoError) { + return; // no nothing + } else if (error == RecoverError.InvalidSignature) { + revert("ECDSA: invalid signature"); + } else if (error == RecoverError.InvalidSignatureLength) { + revert("ECDSA: invalid signature length"); + } else if (error == RecoverError.InvalidSignatureS) { + revert("ECDSA: invalid signature 's' value"); + } else if (error == RecoverError.InvalidSignatureV) { + revert("ECDSA: invalid signature 'v' value"); + } + } + /** * @dev Returns the address that signed a hashed message (`hash`) with * `signature` or error string. This address can then be used for verification purposes. @@ -27,7 +49,7 @@ library ECDSA { * - 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 tryRecover(bytes32 hash, bytes memory signature) internal pure returns (address, string memory) { + function tryRecover(bytes32 hash, bytes memory signature) internal pure returns (address, RecoverError) { // Check the signature length // - case 65: r,s,v signature (standard) // - case 64: r,vs signature (cf https://eips.ethereum.org/EIPS/eip-2098) _Available since v4.1._ @@ -54,7 +76,7 @@ library ECDSA { } return tryRecover(hash, r, vs); } else { - return (address(0), "ECDSA: invalid signature length"); + return (address(0), RecoverError.InvalidSignatureLength); } } @@ -73,10 +95,8 @@ library ECDSA { * be too long), and then calling {toEthSignedMessageHash} on it. */ function recover(bytes32 hash, bytes memory signature) internal pure returns (address) { - (address recovered, string memory error) = tryRecover(hash, signature); - if (bytes(error).length > 0) { - revert(error); - } + (address recovered, RecoverError error) = tryRecover(hash, signature); + _throwError(error); return recovered; } @@ -91,7 +111,7 @@ library ECDSA { bytes32 hash, bytes32 r, bytes32 vs - ) internal pure returns (address, string memory) { + ) internal pure returns (address, RecoverError) { bytes32 s; uint8 v; assembly { @@ -109,10 +129,8 @@ library ECDSA { bytes32 r, bytes32 vs ) internal pure returns (address) { - (address recovered, string memory error) = tryRecover(hash, r, vs); - if (bytes(error).length > 0) { - revert(error); - } + (address recovered, RecoverError error) = tryRecover(hash, r, vs); + _throwError(error); return recovered; } @@ -125,7 +143,7 @@ library ECDSA { uint8 v, bytes32 r, bytes32 s - ) internal pure returns (address, string memory) { + ) internal pure returns (address, RecoverError) { // 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 @@ -136,19 +154,19 @@ 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), "ECDSA: invalid signature 's' value"); + return (address(0), RecoverError.InvalidSignatureS); } if (v != 27 && v != 28) { - return (address(0), "ECDSA: invalid signature 'v' value"); + return (address(0), RecoverError.InvalidSignatureV); } // If the signature is valid (and not malleable), return the signer address address signer = ecrecover(hash, v, r, s); if (signer == address(0)) { - return (address(0), "ECDSA: invalid signature"); + return (address(0), RecoverError.InvalidSignature); } - return (signer, ""); + return (signer, RecoverError.NoError); } /** @@ -161,10 +179,8 @@ library ECDSA { bytes32 r, bytes32 s ) internal pure returns (address) { - (address recovered, string memory error) = tryRecover(hash, v, r, s); - if (bytes(error).length > 0) { - revert(error); - } + (address recovered, RecoverError error) = tryRecover(hash, v, r, s); + _throwError(error); return recovered; } diff --git a/contracts/utils/cryptography/SignatureChecker.sol b/contracts/utils/cryptography/SignatureChecker.sol index fec692c698c..71e30eb5b54 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -22,8 +22,8 @@ library SignatureChecker { bytes32 hash, bytes memory signature ) internal view returns (bool) { - (address recovered, string memory error) = ECDSA.tryRecover(hash, signature); - if (bytes(error).length == 0 && recovered == signer) { + (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature); + if (error == ECDSA.RecoverError.NoError && recovered == signer) { return true; } From 9cd67aa0b6e5f9c15c4b943fc11022528b667634 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Wed, 14 Jul 2021 16:19:23 +0200 Subject: [PATCH 08/10] add changelog entry --- CHANGELOG.md | 2 ++ contracts/utils/cryptography/ECDSA.sol | 10 ++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ae86dcc3b..0eafdd69122 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,8 @@ ## Unreleased * `ERC2771Context`: use private variable from storage to store the forwarder address. Fixes issues where `_msgSender()` was not callable from constructors. ([#2754](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2754)) + * `ECDSA`: add `tryRecover` functions that will not throw if the signature is invalid, and will return an error flag instead. ([#2661](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2661)) + * `SignatureChecker`: Reduce gas usage of the `isValidSignatureNow` function for the "signature by EOA" case. ([#2661](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2661)) ## 4.2.0 (2021-06-30) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 4ad6b35dfbe..d2af1e2061e 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -19,7 +19,7 @@ library ECDSA { function _throwError(RecoverError error) private pure { if (error == RecoverError.NoError) { - return; // no nothing + return; // no error: do nothing } else if (error == RecoverError.InvalidSignature) { revert("ECDSA: invalid signature"); } else if (error == RecoverError.InvalidSignatureLength) { @@ -48,6 +48,8 @@ library ECDSA { * 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] + * + * _Available since v4.3._ */ function tryRecover(bytes32 hash, bytes memory signature) internal pure returns (address, RecoverError) { // Check the signature length @@ -105,7 +107,7 @@ library ECDSA { * * See https://eips.ethereum.org/EIPS/eip-2098[EIP-2098 short signatures] * - * _Available since v4.2._ + * _Available since v4.3._ */ function tryRecover( bytes32 hash, @@ -123,6 +125,8 @@ library ECDSA { /** * @dev Overload of {ECDSA-recover} that receives the `r and `vs` short-signature fields separately. + * + * _Available since v4.2._ */ function recover( bytes32 hash, @@ -137,6 +141,8 @@ library ECDSA { /** * @dev Overload of {ECDSA-tryRecover} that receives the `v`, * `r` and `s` signature fields separately. + * + * _Available since v4.3._ */ function tryRecover( bytes32 hash, From c430bafce5723c1dd0c4318d4a8f1ca844c3d39f Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 6 Aug 2021 10:32:52 -0300 Subject: [PATCH 09/10] revert to previous high s signature --- test/utils/cryptography/ECDSA.test.js | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index ba20cdee4e7..14d86e97782 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -179,19 +179,13 @@ contract('ECDSA', function (accounts) { it('reverts with high-s value signature', async function () { const message = '0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9'; // eslint-disable-next-line max-len - // const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; - // eslint-disable-next-line max-len - const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bd7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a11b'; + const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; await expectRevert(this.ecdsa.recover(message, highSSignature), 'ECDSA: invalid signature \'s\' value'); await expectRevert( this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(highSSignature)), 'ECDSA: invalid signature \'s\' value', ); - await expectRevert( - this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(highSSignature))), - 'ECDSA: invalid signature \'s\' value', - ); }); }); From 61e702b1a3fba379285a46b67cd617bca05bda20 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Fri, 6 Aug 2021 15:41:31 +0200 Subject: [PATCH 10/10] sanity check in convertion functions --- test/utils/cryptography/ECDSA.test.js | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index ba20cdee4e7..c8a2da55093 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -10,7 +10,12 @@ const WRONG_MESSAGE = web3.utils.sha3('Nope'); function to2098Format (signature) { const long = web3.utils.hexToBytes(signature); - expect(long.length).to.be.equal(65); + if (long.length !== 65) { + throw new Error('invalid signature length (expected long format)'); + } + if (long[32] >> 7 === 1) { + throw new Error('invalid signature \'s\' value'); + } const short = long.slice(0, 64); short[32] |= (long[64] % 27) << 7; // set the first bit of the 32nd byte to the v parity bit return web3.utils.bytesToHex(short); @@ -18,7 +23,9 @@ function to2098Format (signature) { function from2098Format (signature) { const short = web3.utils.hexToBytes(signature); - expect(short.length).to.be.equal(64); + if (short.length !== 64) { + throw new Error('invalid signature length (expected short format)'); + } short.push((short[32] >> 7) + 27); short[32] &= (1 << 7) - 1; // zero out the first bit of 1 the 32nd byte return web3.utils.bytesToHex(short); @@ -179,19 +186,13 @@ contract('ECDSA', function (accounts) { it('reverts with high-s value signature', async function () { const message = '0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9'; // eslint-disable-next-line max-len - // const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; - // eslint-disable-next-line max-len - const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bd7fffffffffffffffffffffffffffffff5d576e7357a4501ddfe92f46681b20a11b'; - + const highSSignature = '0xe742ff452d41413616a5bf43fe15dd88294e983d3d36206c2712f39083d638bde0a0fc89be718fbc1033e1d30d78be1c68081562ed2e97af876f286f3453231d1b'; await expectRevert(this.ecdsa.recover(message, highSSignature), 'ECDSA: invalid signature \'s\' value'); await expectRevert( this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(highSSignature)), 'ECDSA: invalid signature \'s\' value', ); - await expectRevert( - this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(highSSignature))), - 'ECDSA: invalid signature \'s\' value', - ); + expect(() => to2098Format(highSSignature)).to.throw('invalid signature \'s\' value'); }); });