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

[Amxx]

Fixes #????

PR Checklist

• Tests
• Documentation
• Changeset entry (run npx changeset add)

[changeset-bot]

🦋 Changeset detected

Latest commit: 7dd8cbd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
openzeppelin-solidity	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[ernestognw]

We should consider a calldata version for the ERC-7913 isValidSignatureNow function. That would allow replacing them in various signers and 7913 verifiers.

[ernestognw]

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

[Amxx]

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.

[Amxx]

We should consider a calldata version for the ERC-7913 isValidSignatureNow function. That would allow replacing them in various signers and 7913 verifiers.

That function has 2 bytes input (signer and signature). Which one should be calldata. signer ? signature ? both ? We can easily get to 4 different variants of that functions, which I don't think we want.

Should we discuss that in a separate issue/PR ?

[Amxx]

Approved during today's call with @frangio and @luiz-lvj

[afa7789]

@Amxx any preview on when this is going to be released, since it's merged ? thnks