Consider checking code length of the signer before attempting an ecrecover in SignatureChecker.isValidSignatureNow

[Amxx]

The current implementation of SignatureChecker.isValidSignatureNow is

function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
    (address recovered, ECDSA.RecoverError error, ) = ECDSA.tryRecover(hash, signature);
    return
        (error == ECDSA.RecoverError.NoError && recovered == signer) ||
        isValidERC1271SignatureNow(signer, hash, signature);
}

This implementation assumes that ecrecover will not identify the correct signer if the signer is a smart contract. This comes from the assumption that there contract addresses is generated in such a way that there is no (known) private key that derivates to the same address.

These assumption could be challenged by EIP-7377.

If EIP-7377 is deployed, a private key would be able to deploy code at its own address. In that case, there would be a known private key for the contract. This causes a governance issue. Some people may expect the contract to have some shared ownership (Multisig), be controlled by another wallet (Ownable), or just trustless. But using the private key that was used to deploy it, the deployer could generate a Permit signature (or similar) that could result in funds being drained out of the contract.

This attack will be feasable on historical implementations if the ecrecover precompile does not check the presence of code at the recovered location!

It as been proposed to not rely on the precompile possibly being changed, and change our implementation toward:

function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
    if (signer.code.length == 0) {
        (address recovered, ECDSA.RecoverError error, ) = ECDSA.tryRecover(hash, signature);
        return (error == ECDSA.RecoverError.NoError && recovered == signer);
    } else {
        return isValidERC1271SignatureNow(signer, hash, signature);
    }
}

However, doing this would significantly increase the gas cost when the signer is an EOA:

• If the signer is an EOA, then it is likelly that the signature is used in a transaction that is not performed by the signer itself. In that case, the signer account is cold, and the signer.code.length == 0 checks costs 2600 gas! With signer being an EOA, it is unlikelly that it is called at any point, so "warming up" the account it not reused, and we should not anticipate saving elsewere as a result of this.
• If the signer is not an EOA, then we are going to call IERC1271.isValidSignature on it anyway. In that case the cost of the check is only really 100gas. The new version would also not try the unecessary ecrecover, with result is some savings. Overall this new version would be cheaper.

We expect the proposed version to be cheaper when signer is a contract, and signature verification is done through ERC-1271. However the proposed version should also be significantly more expensive (+2600gas) when the signer is an EOA, which is the vast majority of cases.

[apbendi]

Hey @Amxx, we're curious if you intend to include this update in a future version of the OZ, or if you don't see it as a worthwhile mitigation given the gas costs. Thanks!

[Amxx]

Thank you @apbendi for the reminder.

We indeed need to measure costs of the different options.