Implicit Return Value for Invalid Signatures in ERC-1271 Compliance

[hats-bug-reporter[bot]]

Github username: @Jelev123 Twitter username: zhulien_zhelev Submission hash (on-chain): 0xd306e5855512ecf725281c3dbd0de5eb81c0cd7672ee5f218c847d782dadbf9a Severity: medium

Description: Description\ In the smart contract provided, the functions isValidSignature(bytes memory data, bytes calldata signature) and isValidSignature(bytes32 message, bytes calldata signature) are designed to validate signatures according to the ERC-1271 standard. However, these functions rely on the implicit default return value of 0x00000000 when the signature verification fails. According to the ERC-1271 standard, invalid signatures should explicitly return 0x00000000.

This issue arises because the functions only explicitly return the valid magic value (ERC1271.LEGACY_MAGIC_VALUE or ERC1271.MAGIC_VALUE) if the signature is valid. If the signature is invalid, the functions implicitly return 0x00000000, which may not be immediately clear to developers or integrators relying on explicit contract behavior.

Impact:

Standards Compliance: The contract does not fully comply with the ERC-1271 standard, potentially leading to interoperability issues. Interoperability: Systems and dApps integrating with this contract may expect an explicit return of 0x00000000 for invalid signatures. The implicit return could cause unexpected behavior and integration failures.

1. Proof of Concept (PoC) File

https://github.com/hats-finance/Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/blob/2c03e35cd1f93de704136ab6e54ae42971a69465/modules/passkey/contracts/base/SignatureValidator.sol#L18-L34

Recommendation

 /**
     * @dev Validates the signature for the given data.
     * @param data The signed data bytes.
     * @param signature The signature to be validated.
     * @return magicValue The magic value indicating the validity of the signature.
     */
    function isValidSignature(bytes memory data, bytes calldata signature) external view returns (bytes4 magicValue) {
        if (_verifySignature(keccak256(data), signature)) {
            return ERC1271.LEGACY_MAGIC_VALUE;
        } else {
            return 0x00000000; // Explicitly return invalid signature magic value
        }
    }

    /**
     * @dev Validates the signature for a given data hash.
     * @param message The signed message hash.
     * @param signature The signature to be validated.
     * @return magicValue The magic value indicating the validity of the signature.
     */
    function isValidSignature(bytes32 message, bytes calldata signature) external view returns (bytes4 magicValue) {
        if (_verifySignature(message, signature)) {
            return ERC1271.MAGIC_VALUE;
        } else {
            return 0x00000000; // Explicitly return invalid signature magic value
        }
    }

[nlordell]

I read through the standard and the only requirement around the return value is:

MUST return the bytes4 magic value 0x1626ba7e when function passes.

Which we do follow. Also note that in Solidity, it is explicitly documented to initialize variables to the 0 value [1] and [2]:

The concept of “undefined” or “null” values does not exist in Solidity, but newly declared variables always have a default value dependent on its type.

and

A variable which is declared will have an initial default value whose byte-representation is all zeros. The “default values” of variables are the typical “zero-state” of whatever the type is. For example, the default value for a bool is false. The default value for the uint or int types is 0. For statically-sized arrays and bytes1 to bytes32, each individual element will be initialized to the default value corresponding to its type. For dynamically-sized arrays, bytes and string, the default value is an empty array or string. For the enum type, the default value is its first member.

Which means that your proposed solution and the current code are functionally equivalent.

I will mark this submission as invalid. If you disagree, can you please provide a code sample demonstrating interoperability issues?