Skip to content

Commit 324eda2

Browse files
transmissions11Amxxfrangio
authored
Remove redundant ECDSA constraint (#3591)
Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com> Co-authored-by: Francisco Giordano <frangio.1@gmail.com>
1 parent 2dc0865 commit 324eda2

File tree

3 files changed

+45
-61
lines changed

3 files changed

+45
-61
lines changed

‎CHANGELOG.md

+2-1
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@
77
* `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469))
88
* `GovernorCompatibilityBravo`: remove unused `using` statements ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506))
99
* `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513))
10+
* `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551))
1011
* `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481))
1112
* `ERC721`: optimize burn by making approval clearing implicit instead of emitting an event. ([#3538](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3538))
1213
* `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515))
1314
* `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565))
14-
* `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551))
1515
* `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605))
16+
* `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591))
1617

1718
### Compatibility Note
1819

‎contracts/utils/cryptography/ECDSA.sol

+1-6
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ library ECDSA {
1717
InvalidSignature,
1818
InvalidSignatureLength,
1919
InvalidSignatureS,
20-
InvalidSignatureV
20+
InvalidSignatureV // Deprecated in v4.8
2121
}
2222

2323
function _throwError(RecoverError error) private pure {
@@ -29,8 +29,6 @@ library ECDSA {
2929
revert("ECDSA: invalid signature length");
3030
} else if (error == RecoverError.InvalidSignatureS) {
3131
revert("ECDSA: invalid signature 's' value");
32-
} else if (error == RecoverError.InvalidSignatureV) {
33-
revert("ECDSA: invalid signature 'v' value");
3432
}
3533
}
3634

@@ -149,9 +147,6 @@ library ECDSA {
149147
if (uint256(s) > 0x7FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF5D576E7357A4501DDFE92F46681B20A0) {
150148
return (address(0), RecoverError.InvalidSignatureS);
151149
}
152-
if (v != 27 && v != 28) {
153-
return (address(0), RecoverError.InvalidSignatureV);
154-
}
155150

156151
// If the signature is valid (and not malleable), return the signer address
157152
address signer = ecrecover(hash, v, r, s);

‎test/utils/cryptography/ECDSA.test.js

+42-54
Original file line numberDiff line numberDiff line change
@@ -98,90 +98,78 @@ contract('ECDSA', function (accounts) {
9898
});
9999
});
100100

101-
context('with v0 signature', function () {
101+
context('with v=27 signature', function () {
102102
// Signature generated outside ganache with method web3.eth.sign(signer, message)
103103
const signer = '0x2cc1166f6212628A0deEf2B33BEFB2187D35b86c';
104104
// eslint-disable-next-line max-len
105-
const signatureWithoutVersion = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
105+
const signatureWithoutV = '0x5d99b6f7f6d1f73d1a26497f2b1c89b24c0993913f86e9a2d02cd69887d9c94f3c880358579d811b21dd1b7fd9bb01c1d81d10e69f0384e675c32b39643be892';
106106

107-
it('reverts with 00 as version value', async function () {
108-
const version = '00';
109-
const signature = signatureWithoutVersion + version;
110-
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value');
111-
await expectRevert(
112-
this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)),
113-
'ECDSA: invalid signature \'v\' value',
114-
);
115-
});
116-
117-
it('works with 27 as version value', async function () {
118-
const version = '1b'; // 27 = 1b.
119-
const signature = signatureWithoutVersion + version;
107+
it('works with correct v value', async function () {
108+
const v = '1b'; // 27 = 1b.
109+
const signature = signatureWithoutV + v;
120110
expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(signer);
121111
expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.equal(signer);
122112
expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.equal(signer);
123113
});
124114

125-
it('reverts with wrong version', async function () {
126-
// The last two hex digits are the signature version.
127-
// The only valid values are 0, 1, 27 and 28.
128-
const version = '02';
129-
const signature = signatureWithoutVersion + version;
130-
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value');
131-
await expectRevert(
132-
this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)),
133-
'ECDSA: invalid signature \'v\' value',
134-
);
115+
it('rejects incorrect v value', async function () {
116+
const v = '1c'; // 28 = 1c.
117+
const signature = signatureWithoutV + v;
118+
expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.not.equal(signer);
119+
expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.not.equal(signer);
120+
expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.not.equal(signer);
121+
});
122+
123+
it('reverts wrong v values', async function () {
124+
for (const v of ['00', '01']) {
125+
const signature = signatureWithoutV + v;
126+
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature');
127+
await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature');
128+
}
135129
});
136130

137131
it('rejects short EIP2098 format', async function () {
138-
const version = '1b'; // 27 = 1b.
139-
const signature = signatureWithoutVersion + version;
132+
const v = '1b'; // 27 = 1b.
133+
const signature = signatureWithoutV + v;
140134
await expectRevert(
141135
this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)),
142136
'ECDSA: invalid signature length',
143137
);
144138
});
145139
});
146140

147-
context('with v1 signature', function () {
141+
context('with v=28 signature', function () {
148142
const signer = '0x1E318623aB09Fe6de3C9b8672098464Aeda9100E';
149143
// eslint-disable-next-line max-len
150-
const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';
144+
const signatureWithoutV = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';
151145

152-
it('reverts with 01 as version value', async function () {
153-
const version = '01';
154-
const signature = signatureWithoutVersion + version;
155-
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value');
156-
await expectRevert(
157-
this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)),
158-
'ECDSA: invalid signature \'v\' value',
159-
);
160-
});
161-
162-
it('works with 28 as version value', async function () {
163-
const version = '1c'; // 28 = 1c.
164-
const signature = signatureWithoutVersion + version;
146+
it('works with correct v value', async function () {
147+
const v = '1c'; // 28 = 1c.
148+
const signature = signatureWithoutV + v;
165149
expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.equal(signer);
166150
expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.equal(signer);
167151
expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.equal(signer);
168152
});
169153

170-
it('reverts with wrong version', async function () {
171-
// The last two hex digits are the signature version.
172-
// The only valid values are 0, 1, 27 and 28.
173-
const version = '02';
174-
const signature = signatureWithoutVersion + version;
175-
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature \'v\' value');
176-
await expectRevert(
177-
this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)),
178-
'ECDSA: invalid signature \'v\' value',
179-
);
154+
it('rejects incorrect v value', async function () {
155+
const v = '1b'; // 27 = 1b.
156+
const signature = signatureWithoutV + v;
157+
expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.not.equal(signer);
158+
expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.not.equal(signer);
159+
expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.not.equal(signer);
160+
});
161+
162+
it('reverts invalid v values', async function () {
163+
for (const v of ['00', '01']) {
164+
const signature = signatureWithoutV + v;
165+
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature');
166+
await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature');
167+
}
180168
});
181169

182170
it('rejects short EIP2098 format', async function () {
183-
const version = '1c'; // 27 = 1b.
184-
const signature = signatureWithoutVersion + version;
171+
const v = '1c'; // 27 = 1b.
172+
const signature = signatureWithoutV + v;
185173
await expectRevert(
186174
this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)),
187175
'ECDSA: invalid signature length',

0 commit comments

Comments
 (0)