Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove redundant ECDSA constraint #3591

Merged
merged 7 commits into from
Aug 16, 2022
Merged

Remove redundant ECDSA constraint #3591

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6ee10ee
ECDSA: Remove redundant constraint
transmissions11 Jul 31, 2022
0730340
fix tests
Amxx Aug 16, 2022
ae58a23
add changelog entry
Amxx Aug 16, 2022
3106862
Merge branch 'master' into patch-3
Amxx Aug 16, 2022
1917d94
Revert enum to previous values
Amxx Aug 16, 2022
0174b97
refactor tests
Amxx Aug 16, 2022
78b23b3
add deprecation comment
frangio Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,13 @@
* `Address`: optimize `functionCall` functions by checking contract size only if there is no returned data. ([#3469](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3469))
* `GovernorCompatibilityBravo`: remove unused `using` statements ([#3506](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3506))
* `ERC20`: optimize `_transfer`, `_mint` and `_burn` by using `unchecked` arithmetic when possible. ([#3513](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3513))
* `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551))
* `ERC721`: optimize transfers by making approval clearing implicit instead of emitting an event. ([#3481](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3481))
* `ERC721`: optimize burn by making approval clearing implicit instead of emitting an event. ([#3538](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3538))
* `ReentrancyGuard`: Reduce code size impact of the modifier by using internal functions. ([#3515](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3515))
* `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565))
* `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551))
* `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605))
* `ECDSA`: Remove redundant check on the `v` value. ([#3591](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3591))

### Compatibility Note

Expand Down
7 changes: 1 addition & 6 deletions contracts/utils/cryptography/ECDSA.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ library ECDSA {
InvalidSignature,
InvalidSignatureLength,
InvalidSignatureS,
InvalidSignatureV
Amxx marked this conversation as resolved.
Show resolved Hide resolved
InvalidSignatureV // Deprecated in v4.8
}

function _throwError(RecoverError error) private pure {
Expand All @@ -29,8 +29,6 @@ library ECDSA {
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");
}
}

Expand Down Expand Up @@ -149,9 +147,6 @@ library ECDSA {
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);
Expand Down
96 changes: 42 additions & 54 deletions test/utils/cryptography/ECDSA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,90 +98,78 @@ contract('ECDSA', function (accounts) {
});
});

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

it('reverts with 00 as version value', async function () {
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;
it('works with correct v value', async function () {
const v = '1b'; // 27 = 1b.
const signature = signatureWithoutV + v;
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 () {
// The last two hex digits are the signature version.
// The only valid values are 0, 1, 27 and 28.
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('rejects incorrect v value', async function () {
const v = '1c'; // 28 = 1c.
const signature = signatureWithoutV + v;
expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.not.equal(signer);
expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.not.equal(signer);
expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.not.equal(signer);
});

it('reverts wrong v values', async function () {
for (const v of ['00', '01']) {
const signature = signatureWithoutV + v;
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature');
await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature');
}
});

it('rejects short EIP2098 format', async function () {
const version = '1b'; // 27 = 1b.
const signature = signatureWithoutVersion + version;
const v = '1b'; // 27 = 1b.
const signature = signatureWithoutV + v;
await expectRevert(
this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)),
'ECDSA: invalid signature length',
);
});
});

context('with v1 signature', function () {
context('with v=28 signature', function () {
const signer = '0x1E318623aB09Fe6de3C9b8672098464Aeda9100E';
// eslint-disable-next-line max-len
const signatureWithoutVersion = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';
const signatureWithoutV = '0x331fe75a821c982f9127538858900d87d3ec1f9f737338ad67cad133fa48feff48e6fa0c18abc62e42820f05943e47af3e9fbe306ce74d64094bdf1691ee53e0';

it('reverts with 01 as version value', async function () {
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;
it('works with correct v value', async function () {
const v = '1c'; // 28 = 1c.
const signature = signatureWithoutV + v;
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 () {
// The last two hex digits are the signature version.
// The only valid values are 0, 1, 27 and 28.
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('rejects incorrect v value', async function () {
const v = '1b'; // 27 = 1b.
const signature = signatureWithoutV + v;
expect(await this.ecdsa.recover(TEST_MESSAGE, signature)).to.not.equal(signer);
expect(await this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature))).to.not.equal(signer);
expect(await this.ecdsa.recover_r_vs(TEST_MESSAGE, ...split(to2098Format(signature)))).to.not.equal(signer);
});

it('reverts invalid v values', async function () {
for (const v of ['00', '01']) {
const signature = signatureWithoutV + v;
await expectRevert(this.ecdsa.recover(TEST_MESSAGE, signature), 'ECDSA: invalid signature');
await expectRevert(this.ecdsa.recover_v_r_s(TEST_MESSAGE, ...split(signature)), 'ECDSA: invalid signature');
}
});

it('rejects short EIP2098 format', async function () {
const version = '1c'; // 27 = 1b.
const signature = signatureWithoutVersion + version;
const v = '1c'; // 27 = 1b.
const signature = signatureWithoutV + v;
await expectRevert(
this.ecdsa.recover(TEST_MESSAGE, to2098Format(signature)),
'ECDSA: invalid signature length',
Expand Down