`SignatureChecker::isValidSignatureNow` is broken for EIP7702 accounts

[devdacian]

SignatureChecker::isValidSignatureNow is broken for EIP7702 accounts as:

• these accounts have code
• they sign using their private key as normal EOAs, so validation needs to occur via ECDSA.tryRecover

The current code ensures this wont happen, but the verification will be attempted only via isValidERC1271SignatureNow which is not correct for EIP7702 accounts:

    function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
        // @audit EIP7702 accounts have code, so `if` branch won't be executed
        if (signer.code.length == 0) {
            (address recovered, ECDSA.RecoverError err, ) = ECDSA.tryRecover(hash, signature);
            return err == ECDSA.RecoverError.NoError && recovered == signer;
        } else {
            return isValidERC1271SignatureNow(signer, hash, signature);
        }
    }

A correct implementation would first try ECDSA.tryRecover, then if that didn't work try isValidERC1271SignatureNow ? Something like:

    function isValidSignatureNow(address signer, bytes32 hash, bytes memory signature) internal view returns (bool) {
        // try this first for EOA & EIP7702 accounts
        (address recovered, ECDSA.RecoverError err,) = ECDSA.tryRecover(hash, signature);
        if (err == ECDSA.RecoverError.NoError && recovered == signer) return true;

        // if first method failed, try EIP1271 smart wallet
        else return isValidERC1271SignatureNow(signer, hash, signature);
    }

[ernestognw]

Users are signing a transaction to delegate the execution to a contract that's incapable of signing. It's not SignatureChecker's fault that users delegate to an smart contract that can't sign!

If users have delegated to an account, it's assumed they'll do it through a UI or wallet provider that most likely will implement ERC-1271 already. Users delegating to custom contracts may already have enough knowledge to redelegate to an accout using SignerERC7702: https://wizard.openzeppelin.com#account

Here's more context: #4855

[d10r]

With EIP-7377 abandoned, the reason to not try ecrecover for accounts with code has gone away, no?
As for ERC-7702, I couldn't find any reference saying that EOAs using it shall use ERC-1271. I just tried doing an EIP-712 signature with a Metamask account upgraded to smart account, and it did not use ERC-1271 (despite the delegated contract supporting it).

So what to do as a Dapp developer?
Currently it seems to me that the best option is to check signature the way OZ previously did: try ecrecover and fall back to 1271 if that fails and the account has code.
Is there any good reason not to do that? And if not, shouldn't OZ also go back to that logic?

My context for looking into this: superfluid-org/protocol-monorepo#2142