From 405076274fd42fc0d0da84adc8c86f47cf712fed Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Fri, 5 Aug 2022 10:15:09 -0300 Subject: [PATCH 1/4] Remove support for compact signatures from ECDSA.recover --- CHANGELOG.md | 6 ++++++ contracts/utils/cryptography/ECDSA.sol | 20 +++----------------- test/utils/cryptography/ECDSA.test.js | 16 ++++++++++------ 3 files changed, 19 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1afa0ddf822..d1026a6d057 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,12 @@ ERC-721 integrators that interpret contract state from events should make sure that they implement the clearing of approval that is implicit in every transfer according to the EIP. Previous versions of OpenZeppellin Contracts emitted an explicit `Approval` event even though it was not required by the specification, and this is no longer the case. +## 4.7.3 + +### Breaking changes + + * `ECDSA`: `recover` and `tryRecover` no longer accept compact signatures to prevent malleability. + ## 4.7.2 * `LibArbitrumL2`, `CrossChainEnabledArbitrumL2`: Fixed detection of cross-chain calls for EOAs. Previously, calls from EOAs would be classified as cross-chain calls. ([#3578](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3578)) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 38b2ea3598a..066ac6571c1 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -55,10 +55,9 @@ library ECDSA { * _Available since v4.3._ */ 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._ - if (signature.length == 65) { + if (signature.length != 65) { + return (address(0), RecoverError.InvalidSignatureLength); + } else { bytes32 r; bytes32 s; uint8 v; @@ -71,19 +70,6 @@ library ECDSA { v := byte(0, mload(add(signature, 0x60))) } return tryRecover(hash, v, r, s); - } else if (signature.length == 64) { - bytes32 r; - bytes32 vs; - // ecrecover takes the signature parameters, and the only way to get them - // currently is to use assembly. - /// @solidity memory-safe-assembly - assembly { - r := mload(add(signature, 0x20)) - vs := mload(add(signature, 0x40)) - } - return tryRecover(hash, r, vs); - } else { - return (address(0), RecoverError.InvalidSignatureLength); } } diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index fff3e2c30d7..2e71174d0ee 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -144,11 +144,13 @@ contract('ECDSA', function (accounts) { ); }); - it('works with short EIP2098 format', async function () { + it('rejects short EIP2098 format', async function () { const version = '1b'; // 27 = 1b. const signature = signatureWithoutVersion + version; - expect(await this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature))).to.equal(signer); - expect(await this.ecdsa.recover(TEST_MESSAGE, from2098Format(to2098Format(signature)))).to.equal(signer); + await expectRevert( + this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)), + 'ECDSA: invalid signature length', + ); }); }); @@ -187,11 +189,13 @@ contract('ECDSA', function (accounts) { ); }); - it('works with short EIP2098 format', async function () { + it('rejects short EIP2098 format', async function () { const version = '1c'; // 27 = 1b. const signature = signatureWithoutVersion + version; - expect(await this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature))).to.equal(signer); - expect(await this.ecdsa.recover(TEST_MESSAGE, from2098Format(to2098Format(signature)))).to.equal(signer); + await expectRevert( + this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)), + 'ECDSA: invalid signature length', + ); }); }); From 6113e9c90058400b5842d31d9d7e62197f7a6de2 Mon Sep 17 00:00:00 2001 From: Francisco Date: Wed, 10 Aug 2022 11:02:30 -0300 Subject: [PATCH 2/4] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d1026a6d057..560dd258292 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,7 +21,7 @@ ERC-721 integrators that interpret contract state from events should make sure t ### Breaking changes - * `ECDSA`: `recover` and `tryRecover` no longer accept compact signatures to prevent malleability. + * `ECDSA`: `recover(bytes32,bytes)` and `tryRecover(bytes32,bytes)` no longer accept compact signatures to prevent malleability. Compact signature support remains available using `recover(bytes32,bytes32,bytes32)` and `tryRecover(bytes32,bytes32,bytes32)`. ## 4.7.2 From 992db5812c472fdf709f0773da1a1b8647fba4b8 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 10 Aug 2022 15:23:22 -0300 Subject: [PATCH 3/4] lint --- test/utils/cryptography/ECDSA.test.js | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/test/utils/cryptography/ECDSA.test.js b/test/utils/cryptography/ECDSA.test.js index 2e71174d0ee..3bcab192afd 100644 --- a/test/utils/cryptography/ECDSA.test.js +++ b/test/utils/cryptography/ECDSA.test.js @@ -22,16 +22,6 @@ function to2098Format (signature) { return web3.utils.bytesToHex(short); } -function from2098Format (signature) { - const short = web3.utils.hexToBytes(signature); - 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) { From 312609159161d7f6367ee982c685aaeead25a372 Mon Sep 17 00:00:00 2001 From: Francisco Giordano Date: Wed, 10 Aug 2022 15:26:58 -0300 Subject: [PATCH 4/4] invert if for simpler diff --- contracts/utils/cryptography/ECDSA.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/utils/cryptography/ECDSA.sol b/contracts/utils/cryptography/ECDSA.sol index 066ac6571c1..8e6e7bb3154 100644 --- a/contracts/utils/cryptography/ECDSA.sol +++ b/contracts/utils/cryptography/ECDSA.sol @@ -55,9 +55,7 @@ library ECDSA { * _Available since v4.3._ */ function tryRecover(bytes32 hash, bytes memory signature) internal pure returns (address, RecoverError) { - if (signature.length != 65) { - return (address(0), RecoverError.InvalidSignatureLength); - } else { + if (signature.length == 65) { bytes32 r; bytes32 s; uint8 v; @@ -70,6 +68,8 @@ library ECDSA { v := byte(0, mload(add(signature, 0x60))) } return tryRecover(hash, v, r, s); + } else { + return (address(0), RecoverError.InvalidSignatureLength); } }