Skip to content

Commit bc8f775

Browse files
AmxxAmxx
authored
Add Calldata variants of ECDSA.recover, ECDSA.tryRecover and SignatureChecker.isValidSignatureNow (#5788)
1 parent 667bb9b commit bc8f775

File tree

6 files changed

+114
-8
lines changed

6 files changed

+114
-8
lines changed

‎.changeset/violet-turtles-like.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': minor
3+
---
4+
5+
`ECDSA`: Add `recoverCalldata` and `tryRecoverCalldata`, variants of `recover` and `tryRecover` that are more efficient when signatures are in calldata.

‎.changeset/whole-plums-speak.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'openzeppelin-solidity': minor
3+
---
4+
5+
`SignatureChecker`: Add `isValidSignatureNowCalldata(address,bytes32,bytes calldata)` for efficient processing of calldata signatures.

‎contracts/utils/cryptography/ECDSA.sol

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,30 @@ library ECDSA {
7474
}
7575
}
7676

77+
/**
78+
* @dev Variant of {tryRecover} that takes a signature in calldata
79+
*/
80+
function tryRecoverCalldata(
81+
bytes32 hash,
82+
bytes calldata signature
83+
) internal pure returns (address recovered, RecoverError err, bytes32 errArg) {
84+
if (signature.length == 65) {
85+
bytes32 r;
86+
bytes32 s;
87+
uint8 v;
88+
// ecrecover takes the signature parameters, calldata slices would work here, but are
89+
// significantly more expensive (length check) than using calldataload in assembly.
90+
assembly ("memory-safe") {
91+
r := calldataload(signature.offset)
92+
s := calldataload(add(signature.offset, 0x20))
93+
v := byte(0, calldataload(add(signature.offset, 0x40)))
94+
}
95+
return tryRecover(hash, v, r, s);
96+
} else {
97+
return (address(0), RecoverError.InvalidSignatureLength, bytes32(signature.length));
98+
}
99+
}
100+
77101
/**
78102
* @dev Returns the address that signed a hashed message (`hash`) with
79103
* `signature`. This address can then be used for verification purposes.
@@ -94,6 +118,15 @@ library ECDSA {
94118
return recovered;
95119
}
96120

121+
/**
122+
* @dev Variant of {recover} that takes a signature in calldata
123+
*/
124+
function recoverCalldata(bytes32 hash, bytes calldata signature) internal pure returns (address) {
125+
(address recovered, RecoverError error, bytes32 errorArg) = tryRecoverCalldata(hash, signature);
126+
_throwError(error, errorArg);
127+
return recovered;
128+
}
129+
97130
/**
98131
* @dev Overload of {ECDSA-tryRecover} that receives the `r` and `vs` short-signature fields separately.
99132
*

‎contracts/utils/cryptography/SignatureChecker.sol

Lines changed: 36 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,22 @@ library SignatureChecker {
3838
}
3939
}
4040

41+
/**
42+
* @dev Variant of {isValidSignatureNow} that takes a signature in calldata
43+
*/
44+
function isValidSignatureNowCalldata(
45+
address signer,
46+
bytes32 hash,
47+
bytes calldata signature
48+
) internal view returns (bool) {
49+
if (signer.code.length == 0) {
50+
(address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecoverCalldata(hash, signature);
51+
return err == ECDSA.RecoverError.NoError && recovered == signer;
52+
} else {
53+
return isValidERC1271SignatureNow(signer, hash, signature);
54+
}
55+
}
56+
4157
/**
4258
* @dev Checks if a signature is valid for a given signer and data hash. The signature is validated
4359
* against the signer smart contract using ERC-1271.
@@ -49,13 +65,26 @@ library SignatureChecker {
4965
address signer,
5066
bytes32 hash,
5167
bytes memory signature
52-
) internal view returns (bool) {
53-
(bool success, bytes memory result) = signer.staticcall(
54-
abi.encodeCall(IERC1271.isValidSignature, (hash, signature))
55-
);
56-
return (success &&
57-
result.length >= 32 &&
58-
abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
68+
) internal view returns (bool result) {
69+
bytes4 selector = IERC1271.isValidSignature.selector;
70+
uint256 length = signature.length;
71+
72+
assembly ("memory-safe") {
73+
// Encoded calldata is :
74+
// [ 0x00 - 0x03 ] <selector>
75+
// [ 0x04 - 0x23 ] <hash>
76+
// [ 0x24 - 0x44 ] <signature offset> (0x40)
77+
// [ 0x44 - 0x64 ] <signature length>
78+
// [ 0x64 - ... ] <signature data>
79+
let ptr := mload(0x40)
80+
mstore(ptr, selector)
81+
mstore(add(ptr, 0x04), hash)
82+
mstore(add(ptr, 0x24), 0x40)
83+
mcopy(add(ptr, 0x44), signature, add(length, 0x20))
84+
85+
let success := staticcall(gas(), signer, ptr, add(length, 0x64), 0, 0x20)
86+
result := and(success, and(gt(returndatasize(), 0x19), eq(mload(0x00), selector)))
87+
}
5988
}
6089

6190
/**

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ describe('ECDSA', function () {
4444

4545
// Recover the signer address from the generated message and signature.
4646
expect(await this.mock.$recover(ethers.hashMessage(TEST_MESSAGE), signature)).to.equal(this.signer);
47+
expect(await this.mock.$recoverCalldata(ethers.hashMessage(TEST_MESSAGE), signature)).to.equal(this.signer);
4748
});
4849

4950
it('returns signer address with correct signature for arbitrary length message', async function () {
@@ -52,11 +53,13 @@ describe('ECDSA', function () {
5253

5354
// Recover the signer address from the generated message and signature.
5455
expect(await this.mock.$recover(ethers.hashMessage(NON_HASH_MESSAGE), signature)).to.equal(this.signer);
56+
expect(await this.mock.$recoverCalldata(ethers.hashMessage(NON_HASH_MESSAGE), signature)).to.equal(this.signer);
5557
});
5658

5759
it('returns a different address', async function () {
5860
const signature = await this.signer.signMessage(TEST_MESSAGE);
5961
expect(await this.mock.$recover(WRONG_MESSAGE, signature)).to.not.be.equal(this.signer);
62+
expect(await this.mock.$recoverCalldata(WRONG_MESSAGE, signature)).to.not.be.equal(this.signer);
6063
});
6164

6265
it('reverts with invalid signature', async function () {
@@ -66,6 +69,10 @@ describe('ECDSA', function () {
6669
this.mock,
6770
'ECDSAInvalidSignature',
6871
);
72+
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.be.revertedWithCustomError(
73+
this.mock,
74+
'ECDSAInvalidSignature',
75+
);
6976
});
7077
});
7178

@@ -79,6 +86,7 @@ describe('ECDSA', function () {
7986
const v = '0x1b'; // 27 = 1b.
8087
const signature = ethers.concat([signatureWithoutV, v]);
8188
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.equal(signer);
89+
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.equal(signer);
8290

8391
const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
8492
expect(await this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s)).to.equal(
@@ -92,6 +100,7 @@ describe('ECDSA', function () {
92100
const v = '0x1c'; // 28 = 1c.
93101
const signature = ethers.concat([signatureWithoutV, v]);
94102
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.not.equal(signer);
103+
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.not.equal(signer);
95104

96105
const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
97106
expect(
@@ -110,6 +119,10 @@ describe('ECDSA', function () {
110119
this.mock,
111120
'ECDSAInvalidSignature',
112121
);
122+
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.be.revertedWithCustomError(
123+
this.mock,
124+
'ECDSAInvalidSignature',
125+
);
113126

114127
const { r, s } = ethers.Signature.from(signature);
115128
await expect(
@@ -126,6 +139,9 @@ describe('ECDSA', function () {
126139
await expect(this.mock.$recover(TEST_MESSAGE, compactSerialized))
127140
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
128141
.withArgs(64);
142+
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, compactSerialized))
143+
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
144+
.withArgs(64);
129145
});
130146
});
131147

@@ -139,6 +155,7 @@ describe('ECDSA', function () {
139155
const v = '0x1c'; // 28 = 1c.
140156
const signature = ethers.concat([signatureWithoutV, v]);
141157
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.equal(signer);
158+
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.equal(signer);
142159

143160
const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
144161
expect(await this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s)).to.equal(
@@ -152,6 +169,7 @@ describe('ECDSA', function () {
152169
const v = '0x1b'; // 27 = 1b.
153170
const signature = ethers.concat([signatureWithoutV, v]);
154171
expect(await this.mock.$recover(TEST_MESSAGE, signature)).to.not.equal(signer);
172+
expect(await this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.not.equal(signer);
155173

156174
const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
157175
expect(
@@ -170,6 +188,10 @@ describe('ECDSA', function () {
170188
this.mock,
171189
'ECDSAInvalidSignature',
172190
);
191+
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.be.revertedWithCustomError(
192+
this.mock,
193+
'ECDSAInvalidSignature',
194+
);
173195

174196
const { r, s } = ethers.Signature.from(signature);
175197
await expect(
@@ -186,6 +208,9 @@ describe('ECDSA', function () {
186208
await expect(this.mock.$recover(TEST_MESSAGE, compactSerialized))
187209
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
188210
.withArgs(64);
211+
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, compactSerialized))
212+
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
213+
.withArgs(64);
189214
});
190215
});
191216

@@ -202,6 +227,9 @@ describe('ECDSA', function () {
202227
await expect(this.mock.$recover(message, highSSignature))
203228
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureS')
204229
.withArgs(s);
230+
await expect(this.mock.$recoverCalldata(message, highSSignature))
231+
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureS')
232+
.withArgs(s);
205233
await expect(this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s))
206234
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureS')
207235
.withArgs(s);

‎test/utils/cryptography/SignatureChecker.test.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,23 +36,29 @@ describe('SignatureChecker (ERC1271)', function () {
3636
await expect(
3737
this.mock.$isValidSignatureNow(ethers.Typed.address(this.signer.address), TEST_MESSAGE_HASH, this.signature),
3838
).to.eventually.be.true;
39+
await expect(this.mock.$isValidSignatureNowCalldata(this.signer.address, TEST_MESSAGE_HASH, this.signature)).to
40+
.eventually.be.true;
3941
});
4042

4143
it('with invalid signer', async function () {
4244
await expect(
4345
this.mock.$isValidSignatureNow(ethers.Typed.address(this.other.address), TEST_MESSAGE_HASH, this.signature),
4446
).to.eventually.be.false;
47+
await expect(this.mock.$isValidSignatureNowCalldata(this.other.address, TEST_MESSAGE_HASH, this.signature)).to
48+
.eventually.be.false;
4549
});
4650

4751
it('with invalid signature', async function () {
4852
await expect(
4953
this.mock.$isValidSignatureNow(ethers.Typed.address(this.signer.address), WRONG_MESSAGE_HASH, this.signature),
5054
).to.eventually.be.false;
55+
await expect(this.mock.$isValidSignatureNowCalldata(this.signer.address, WRONG_MESSAGE_HASH, this.signature)).to
56+
.eventually.be.false;
5157
});
5258
});
5359

5460
describe('ERC1271 wallet', function () {
55-
for (const fn of ['isValidERC1271SignatureNow', 'isValidSignatureNow']) {
61+
for (const fn of ['isValidERC1271SignatureNow', 'isValidSignatureNow', 'isValidSignatureNowCalldata']) {
5662
describe(fn, function () {
5763
it('with matching signer and signature', async function () {
5864
await expect(

0 commit comments

Comments
 (0)