Security considerations regarding `SignatureChecker.isValidERC1271SignatureNow`

📝 Details

After a small discussion I wanted to collect some thoughts on SignatureChecker.isValidERC1271SignatureNow. It largely assumes that the signer contract is trusted when verifying ERC-1271 signatures.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4e7e6e54daedf091d91f2f2df024cbb8f253e2ef/contracts/utils/cryptography/SignatureChecker.sol#L29-L47

Signatures are often used by relayers to perform actions on behalf of users with their permission. Therefore it should be assumed that signers can be malicious and additional security considerations might come into play. Two observations are:

1. signer.staticcall does not include a gas limit

2. (bool success, bytes memory result) = signer.staticcall copies the entirety of the returned data, making it susceptible to returndata bomb attacks

3. is partially addressed in the EIP:

Since there are no gas-limit expected for calling the isValidSignature() function, it is possible that some implementation will consume a large amount of gas. It is therefore important to not hardcode an amount of gas sent when calling this method on an external contract as it could prevent the validation of certain signatures.

However, general security concerns for relayers still apply (call to the unknown): Relayers might not be aware that a fixed gas limit should be set or that relayed transactions should be discarded if they cost more than X. This could be a bad scenario if relayers are sponsored, use a fixed fee, or if the maximum gas is not debited upfront.

2. is not much of a concern when there is no fixed gas limit in the call to the signer. The worst scenario might be confusion if the call reverts in the caller's context instead of the signer contract.

Potential solutions:

1. Add general security consideration advice to SignatureChecker.isValidERC1271SignatureNow.
2. Only load 32 bytes from returndata via assembly.
3. Add gasLimit parameter to SignatureChecker.isValidERC1271SignatureNow and implement 2.
4. Do nothing.

Considering that the most simple solution might be the best, 4. or 1. might be the most sensible, though I'm interested if anyone has some more thoughts on this.

Read the linked SignatureChecker.sol implementation and the ERC-1271 security guidance cited in the issue. Compare the four proposed options, including returndata handling and gas-limit implications; done requires a maintainer decision on the intended behavior and corresponding implementation or security documentation.