Security considerations regarding SignatureChecker.isValidERC1271SignatureNow

[0xPhaze]

📝 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.

openzeppelin-contracts/contracts/utils/cryptography/SignatureChecker.sol

Lines 29 to 47 in 4e7e6e5

	/**
	* @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.
	*
	* NOTE: Unlike ECDSA signatures, contract signatures are revocable, and the outcome of this function can thus
	* change through time. It could return true at block N and false at block N+1 (or the opposite).
	*/
	function isValidERC1271SignatureNow(
	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));
	}

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.

[ernestognw]

Thanks for flagging @0xPhaze.

The fix for this should be easy if we replicate what's done in ERC165Checker:

assembly {
    success := staticcall(30000, account, add(encodedParams, 0x20), mload(encodedParams), 0x00, 0x20)
    returnSize := returndatasize()
    returnValue := mload(0x00)
}

I wouldn't say it's a breaking change because contracts won't stop working if they adhere to the standard.

[0xPhaze]

Adding a fixed gas limit technically would be breaking though.
Thanks for pointing out the similarities to ERC-165.
A perhaps relevant discussion is #1750.