signature is not validated

[hats-bug-reporter]

Github username: @0xRizwan
Twitter username: 0xRizwann
Submission hash (on-chain): 0xe458bcaaf6e4d0173186894301d6a2213144dc069174e3805c03629524149211
Severity: low

Description:
Description\

SignatureValidator.sol is base contract supporting ERC1271 differentisValidSignature versions.

_verifySignature() is overridden and has been used in SafeWebAuthnSharedSigner.sol and SafeWebAuthnSignerSingleton.sol.

    function _verifySignature(bytes32 message, bytes calldata signature) internal view virtual returns (bool success);

Then, this _verifySignature() is being called in isValidSignature()

    function isValidSignature(bytes32 message, bytes calldata signature) external view returns (bytes4 magicValue) {
        if (_verifySignature(message, signature)) {
            magicValue = ERC1271.MAGIC_VALUE;
        }
    }

The issue is that, _verifySignature() as internal function does not validate the signature length passed with it. To protetct from malicious signatures, the signature length should be checked.

If we see, EIP-1271 the reference implementation has checked the signature length thereby validating the signature input explicitely.

    require(_signature.length == 65, "SignatureValidator#recoverSigner: invalid signature length");

Reference- Reference Implementation

The means the signature.length == 65 considers, signature inputs, v as bytes1, r and s is bytes32 of signature.

Recommendations\

For signature inputs in functions, Check the signature length.

For example:

    require(signature.length == 65, "invalid signature length");

[nlordell]

I am marking this as invalid as the signature bytes are checked in WebAuthn.sol (which is ultimately called by all paths of isValidSignature and includes checks on upper bounds of signature length encoding:

Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/modules/passkey/contracts/libraries/WebAuthn.sol

Line 121 in 2c03e35

isValid = signature.length < 193 + authenticatorDataAlignedLength + clientDataFieldsAlignedLength;

Note that this is an upper bound check by design:

Safe-0x2909fdefd24a1ced675cb1444918fa766d76bdac/modules/passkey/contracts/libraries/WebAuthn.sol

Lines 88 to 91 in 2c03e35

	// Read the lengths of the dynamic byte arrays in assembly. This is done because
	// Solidity generates calldata bounds checks which aren't required for the security of
	// the signature verification, as it can only lead to _shorter_ signatures which are
	// are less gas expensive anyway.

The proposed solution is not valid for this project - WebAuthn signatures are not 65 bytes, they include two 32 byte values for the ECDSA signature R and S components, as well as bytes for authenticator and client data that is required for computing the signing message.