Skip to content

Add Calldata variants of ECDSA.recover, ECDSA.tryRecover and SignatureChecker.isValidSignatureNow #5788

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

Merged
merged 4 commits into from
Jul 11, 2025
Merged

Add Calldata variants of ECDSA.recover, ECDSA.tryRecover and SignatureChecker.isValidSignatureNow #5788

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
efb021b
Add Calldata variants of ECDSA.recover, ECDSA.tryRecover and Signatur…
Amxx Jul 7, 2025
8ee758d
creating slices has a cost, using assembly saves ~400gas
Amxx Jul 7, 2025
de6f559
up
Amxx Jul 7, 2025
7dd8cbd
rewrite isValidERC1271SignatureNow in assembly to avoid memory alloca…
Amxx Jul 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/violet-turtles-like.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`ECDSA`: Add `recoverCalldata` and `tryRecoverCalldata`, variants of `recover` and `tryRecover` that are more efficient when signatures are in calldata.
5 changes: 5 additions & 0 deletions .changeset/whole-plums-speak.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'openzeppelin-solidity': minor
---

`SignatureChecker`: Add `isValidSignatureNowCalldata(address,bytes32,bytes calldata)` for efficient processing of calldata signatures.
33 changes: 33 additions & 0 deletions contracts/utils/cryptography/ECDSA.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,30 @@ library ECDSA {
}
}

/**
* @dev Variant of {tryRecover} that takes a signature in calldata
*/
function tryRecoverCalldata(
bytes32 hash,
bytes calldata signature
) internal pure returns (address recovered, RecoverError err, bytes32 errArg) {
if (signature.length == 65) {
bytes32 r;
bytes32 s;
uint8 v;
// ecrecover takes the signature parameters, calldata slices would work here, but are
// significantly more expensive (length check) than using calldataload in assembly.
assembly ("memory-safe") {
r := calldataload(signature.offset)
s := calldataload(add(signature.offset, 0x20))
v := byte(0, calldataload(add(signature.offset, 0x40)))
}
return tryRecover(hash, v, r, s);
} else {
return (address(0), RecoverError.InvalidSignatureLength, bytes32(signature.length));
}
}
Copy link
Member

@ernestognw ernestognw Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to skip the 65 length check as long as we explicitly note that out of bounds calldata is treated as 0, and also mentioning that the function doesn't return RecoverError.InvalidSignatureLength?

Suggested change
}
function tryRecoverCalldata(
bytes32 hash,
bytes calldata signature
) internal pure returns (address recovered, bytes32 errArg) {
bytes32 r;
bytes32 s;
uint8 v;
// ecrecover takes the signature parameters, calldata slices would work here, but are
// significantly more expensive (length check) than using calldataload in assembly.
assembly ("memory-safe") {
r := calldataload(signature.offset)
s := calldataload(add(signature.offset, 0x20))
v := byte(0, calldataload(add(signature.offset, 0x40)))
}
return tryRecover(hash, v, r, s);
}

To keep consistency and avoid backwards compatibility issues in the original tryRecover memory, I would keep the current code and add a tryUnsafeRecover (or tryUnboundedRecover) along with its calldata variant. Maybe for another PR

Copy link
Collaborator Author

@Amxx Amxx Jul 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially I was worried that if the length is not 65, it could be 64 (which is the length of compact signature) and we would possibly accept these. We removed short signature support because of issues with "uniqueness/maleability" and I would not want to reintroduce that.

If there is nothing after the signature (its the last object in calldata) and if the signature is <65bytes long, then indeed, the rest will be zeros ... and that would not be verified since v is expected to be 27/28. But there is also the risk that the signature is not the end of the calldata. In that case, and if we don't check the length, that will get loaded.

Lets say we don't check the length and we process the following calldata (an ERC1271 isValidSignature call)

1626ba7e                                                         // selector
8144a6fa26be252b86456491fbcd43c1de7e022241845ffea1c3df066f7cfede // hash
0000000000000000000000000000000000000000000000000000000000000040 // offset
0000000000000000000000000000000000000000000000000000000000000041 // length (65)
083c44ecf9fc16fb648c8a23565de49e66d56ff89f7eff7c52d587f6acb3d241 // r
5e83c3744b28a072842748e573d13fa050089e0851e9e96566da8e8b547ede21 // s
1b                                                               // v

You could change the length to refactor it as

1626ba7e                                                         // selector
8144a6fa26be252b86456491fbcd43c1de7e022241845ffea1c3df066f7cfede // hash
0000000000000000000000000000000000000000000000000000000000000040 // offset
0000000000000000000000000000000000000000000000000000000000000040 // length (64)
083c44ecf9fc16fb648c8a23565de49e66d56ff89f7eff7c52d587f6acb3d241 // r
5e83c3744b28a072842748e573d13fa050089e0851e9e96566da8e8b547ede21 // s
1b                                                               // v

this would cause the bytes calldata signature to have a shorter length. If solidity starts manipulating it (to hash it for example) the v will not be considered part of it. But still, it would be seen as valid by tryRecover. Basically, you could have the length part be any value. As long as you have everything behind it formated correctly, tryRecover would still work even through the signature object would be different from solidity's point of view.

IMO, checking the length is not that expensive, and there is just too much that can go wrong if we dont do it.


/**
* @dev Returns the address that signed a hashed message (`hash`) with
* `signature`. This address can then be used for verification purposes.
Expand All @@ -94,6 +118,15 @@ library ECDSA {
return recovered;
}

/**
* @dev Variant of {recover} that takes a signature in calldata
*/
function recoverCalldata(bytes32 hash, bytes calldata signature) internal pure returns (address) {
(address recovered, RecoverError error, bytes32 errorArg) = tryRecoverCalldata(hash, signature);
_throwError(error, errorArg);
return recovered;
}

/**
* @dev Overload of {ECDSA-tryRecover} that receives the `r` and `vs` short-signature fields separately.
*
Expand Down
43 changes: 36 additions & 7 deletions contracts/utils/cryptography/SignatureChecker.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,22 @@ library SignatureChecker {
}
}

/**
* @dev Variant of {isValidSignatureNow} that takes a signature in calldata
*/
function isValidSignatureNowCalldata(
address signer,
bytes32 hash,
bytes calldata signature
) internal view returns (bool) {
if (signer.code.length == 0) {
(address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecoverCalldata(hash, signature);
return err == ECDSA.RecoverError.NoError && recovered == signer;
} else {
return isValidERC1271SignatureNow(signer, hash, signature);
}
}

/**
* @dev Checks if a signature is valid for a given signer and data hash. The signature is validated
* against the signer smart contract using ERC-1271.
Expand All @@ -49,13 +65,26 @@ library SignatureChecker {
address signer,
bytes32 hash,
bytes memory signature
) internal view returns (bool) {
(bool success, bytes memory result) = signer.staticcall(
abi.encodeCall(IERC1271.isValidSignature, (hash, signature))
);
return (success &&
result.length >= 32 &&
abi.decode(result, (bytes32)) == bytes32(IERC1271.isValidSignature.selector));
) internal view returns (bool result) {
bytes4 selector = IERC1271.isValidSignature.selector;
uint256 length = signature.length;

assembly ("memory-safe") {
// Encoded calldata is :
// [ 0x00 - 0x03 ] <selector>
// [ 0x04 - 0x23 ] <hash>
// [ 0x24 - 0x44 ] <signature offset> (0x40)
// [ 0x44 - 0x64 ] <signature length>
// [ 0x64 - ... ] <signature data>
let ptr := mload(0x40)
mstore(ptr, selector)
mstore(add(ptr, 0x04), hash)
mstore(add(ptr, 0x24), 0x40)
mcopy(add(ptr, 0x44), signature, add(length, 0x20))

let success := staticcall(gas(), signer, ptr, add(length, 0x64), 0, 0x20)
result := and(success, and(gt(returndatasize(), 0x19), eq(mload(0x00), selector)))
}
}

/**
Expand Down
28 changes: 28 additions & 0 deletions test/utils/cryptography/ECDSA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ describe('ECDSA', function () {

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

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

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

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

it('reverts with invalid signature', async function () {
Expand All @@ -66,6 +69,10 @@ describe('ECDSA', function () {
this.mock,
'ECDSAInvalidSignature',
);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.be.revertedWithCustomError(
this.mock,
'ECDSAInvalidSignature',
);
});
});

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

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

const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
expect(
Expand All @@ -110,6 +119,10 @@ describe('ECDSA', function () {
this.mock,
'ECDSAInvalidSignature',
);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.be.revertedWithCustomError(
this.mock,
'ECDSAInvalidSignature',
);

const { r, s } = ethers.Signature.from(signature);
await expect(
Expand All @@ -126,6 +139,9 @@ describe('ECDSA', function () {
await expect(this.mock.$recover(TEST_MESSAGE, compactSerialized))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(64);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, compactSerialized))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(64);
});
});

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

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

const { r, s, yParityAndS: vs } = ethers.Signature.from(signature);
expect(
Expand All @@ -170,6 +188,10 @@ describe('ECDSA', function () {
this.mock,
'ECDSAInvalidSignature',
);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, signature)).to.be.revertedWithCustomError(
this.mock,
'ECDSAInvalidSignature',
);

const { r, s } = ethers.Signature.from(signature);
await expect(
Expand All @@ -186,6 +208,9 @@ describe('ECDSA', function () {
await expect(this.mock.$recover(TEST_MESSAGE, compactSerialized))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(64);
await expect(this.mock.$recoverCalldata(TEST_MESSAGE, compactSerialized))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureLength')
.withArgs(64);
});
});

Expand All @@ -202,6 +227,9 @@ describe('ECDSA', function () {
await expect(this.mock.$recover(message, highSSignature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureS')
.withArgs(s);
await expect(this.mock.$recoverCalldata(message, highSSignature))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureS')
.withArgs(s);
await expect(this.mock.getFunction('$recover(bytes32,uint8,bytes32,bytes32)')(TEST_MESSAGE, v, r, s))
.to.be.revertedWithCustomError(this.mock, 'ECDSAInvalidSignatureS')
.withArgs(s);
Expand Down
8 changes: 7 additions & 1 deletion test/utils/cryptography/SignatureChecker.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,23 +36,29 @@ describe('SignatureChecker (ERC1271)', function () {
await expect(
this.mock.$isValidSignatureNow(ethers.Typed.address(this.signer.address), TEST_MESSAGE_HASH, this.signature),
).to.eventually.be.true;
await expect(this.mock.$isValidSignatureNowCalldata(this.signer.address, TEST_MESSAGE_HASH, this.signature)).to
.eventually.be.true;
});

it('with invalid signer', async function () {
await expect(
this.mock.$isValidSignatureNow(ethers.Typed.address(this.other.address), TEST_MESSAGE_HASH, this.signature),
).to.eventually.be.false;
await expect(this.mock.$isValidSignatureNowCalldata(this.other.address, TEST_MESSAGE_HASH, this.signature)).to
.eventually.be.false;
});

it('with invalid signature', async function () {
await expect(
this.mock.$isValidSignatureNow(ethers.Typed.address(this.signer.address), WRONG_MESSAGE_HASH, this.signature),
).to.eventually.be.false;
await expect(this.mock.$isValidSignatureNowCalldata(this.signer.address, WRONG_MESSAGE_HASH, this.signature)).to
.eventually.be.false;
});
});

describe('ERC1271 wallet', function () {
for (const fn of ['isValidERC1271SignatureNow', 'isValidSignatureNow']) {
for (const fn of ['isValidERC1271SignatureNow', 'isValidSignatureNow', 'isValidSignatureNowCalldata']) {
describe(fn, function () {
it('with matching signer and signature', async function () {
await expect(
Expand Down
Loading