From 541e82144f691aa171c53ba8c3b32ef7f05b99a5 Mon Sep 17 00:00:00 2001 From: Anton Bukov Date: Fri, 6 Aug 2021 16:47:52 +0300 Subject: [PATCH] Optimize EOA signature verification (#2661) Co-authored-by: Hadrien Croubois Co-authored-by: Francisco Giordano --- CHANGELOG.md | 4 + contracts/mocks/ECDSAMock.sol | 19 +++ contracts/utils/cryptography/ECDSA.sol | 126 ++++++++++++++---- .../utils/cryptography/SignatureChecker.sol | 16 +-- test/utils/cryptography/ECDSA.test.js | 56 +++++++- 5 files changed, 187 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ad1a3067..a01b37a0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ * `EnumerableSet`: add `values()` functions that returns an array containing all values in a single call. ([#2768](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2768)) * `Governor`: added a modular system of `Governor` contracts based on `GovernorAlpha` and `GovernorBravo`. ([#2672](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2672)) * Add an `interface` folder containing solidity interfaces to final ERCs. ([#2517](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/2517)) + * `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)) + * `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/mocks/ECDSAMock.sol b/contracts/mocks/ECDSAMock.sol index 99f6a7983..e1296a3f0 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/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 2baaebfb5..0f8a3a56d 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -9,9 +9,31 @@ 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 error: do 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`. 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 @@ -26,8 +48,10 @@ 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 recover(bytes32 hash, bytes memory signature) internal pure returns (address) { + 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._ @@ -42,7 +66,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,17 +76,56 @@ library ECDSA { r := mload(add(signature, 0x20)) vs := mload(add(signature, 0x40)) } - return recover(hash, r, vs); + return tryRecover(hash, r, vs); } else { - revert("ECDSA: invalid signature length"); + return (address(0), RecoverError.InvalidSignatureLength); } } /** - * @dev Overload of {ECDSA-recover} that receives the `r` and `vs` short-signature fields separately. + * @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, RecoverError error) = tryRecover(hash, signature); + _throwError(error); + return recovered; + } + + /** + * @dev Overload of {ECDSA-tryRecover} that receives the `r` and `vs` short-signature fields separately. * * See https://eips.ethereum.org/EIPS/eip-2098[EIP-2098 short signatures] * + * _Available since v4.3._ + */ + function tryRecover( + bytes32 hash, + bytes32 r, + bytes32 vs + ) internal pure returns (address, RecoverError) { + bytes32 s; + uint8 v; + assembly { + s := and(vs, 0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) + v := add(shr(255, vs), 27) + } + return tryRecover(hash, v, r, s); + } + + /** + * @dev Overload of {ECDSA-recover} that receives the `r and `vs` short-signature fields separately. + * * _Available since v4.2._ */ function recover( @@ -70,24 +133,23 @@ library ECDSA { bytes32 r, bytes32 vs ) internal pure returns (address) { - bytes32 s; - uint8 v; - assembly { - s := and(vs, 0x7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff) - v := add(shr(255, vs), 27) - } - return recover(hash, v, r, s); + (address recovered, RecoverError error) = tryRecover(hash, r, vs); + _throwError(error); + return recovered; } /** - * @dev Overload of {ECDSA-recover} that receives the `v`, `r` and `s` signature fields separately. + * @dev Overload of {ECDSA-tryRecover} that receives the `v`, + * `r` and `s` signature fields separately. + * + * _Available since v4.3._ */ - function recover( + function tryRecover( bytes32 hash, uint8 v, bytes32 r, bytes32 s - ) internal pure returns (address) { + ) 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 (301): 0 < s < secp256k1n ÷ 2 + 1, and for v in (302): v ∈ {27, 28}. Most @@ -97,17 +159,35 @@ library ECDSA { // 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. - require( - uint256(s) <= 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0, - "ECDSA: invalid signature 's' value" - ); - require(v == 27 || v == 28, "ECDSA: invalid signature 'v' value"); + if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) { + return (address(0), RecoverError.InvalidSignatureS); + } + if (v != 27 && v != 28) { + return (address(0), RecoverError.InvalidSignatureV); + } // 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), RecoverError.InvalidSignature); + } - return signer; + return (signer, RecoverError.NoError); + } + + /** + * @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, 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 5040ff1bf..71e30eb5b 100644 --- a/contracts/utils/cryptography/SignatureChecker.sol +++ b/contracts/utils/cryptography/SignatureChecker.sol @@ -22,14 +22,14 @@ library SignatureChecker { 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.isValidSignature.selector; - } catch { - return false; - } - } else { - return ECDSA.recover(hash, signature) == signer; + (address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(hash, signature); + if (error == ECDSA.RecoverError.NoError && recovered == signer) { + return true; } + + (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); } } diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index b3152f397..c8a2da550 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,12 +23,33 @@ 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); } +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 +106,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 +126,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 +149,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 +169,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 () { @@ -141,8 +187,12 @@ contract('ECDSA', function (accounts) { const message = '0xb94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9'; // eslint-disable-next-line max-len 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', + ); + expect(() => to2098Format(highSSignature)).to.throw('invalid signature \'s\' value'); }); });