EIP-1271 Signature may contains 64 or 65 bytes but isn't EIP-2098 signature. But it may be reverted if it isn't EIP-2098 signature.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-opensea-seaport/blob/4140473b1f85d0df602548ad260b1739ddd734a5/contracts/lib/SignatureVerification.sol#L39-L100

Vulnerability details

Impact

EIP-1271 Signature may contains 64 or 65 bytes but isn't EIP-2098 signature. But it may be reverted if it isn't EIP-2098 signature.

EIP-1271 Signature that contains 64 or 65 bytes may be custom kind of signature for example it may be keccak256 of some value or merkle tree root.

In the case that 64 or 65 bytes EIP-1271 Signature is not EIP-2098 signature. It will be reverted with InvalidSignature() or BadSignatureV(v) because it is checked with EIP-2098 verification algorithm first and recoveredSigner may be address(0)

Let assume signer contract accept this signature (return correct magicValue)

In reality, this signature should be valid as calling isValidSignature return correct magicValue due to assumption above.

Proof of Concept

Assumption

Assume current signature is a valid EIP-1271 signature (calling isValidSignature on signer return correct magicValue).

Case signature length 64

        if (signature.length == 64) {
            // Declare temporary vs that will be decomposed into s and v.
            bytes32 vs;

            (r, vs) = abi.decode(signature, (bytes32, bytes32));

            s = vs & EIP2098_allButHighestBitMask;

            v = uint8(uint256(vs >> 255)) + 27;
        }

This code block will be passed. But ecrecover may return 0

If ecrecover return 0

        if (recoveredSigner == address(0)) {
            revert InvalidSignature();
            // Should a signer be recovered, but it doesn't match the signer...
        }

It will be reverted with InvalidSignature().

But in fact it should be valid due to assumption above. ( must perform _assertValidEIP1271Signature(signer, digest, signature); )

If ecrecover return non-zero address

        } else if (recoveredSigner != signer) {
            // Attempt EIP-1271 static call to signer in case it's a contract.
            _assertValidEIP1271Signature(signer, digest, signature);
        }

This case is valid as it is calling _assertValidEIP1271Signature(signer, digest, signature); as expected

Case signature length 65

        } else if (signature.length == 65) {
            (r, s) = abi.decode(signature, (bytes32, bytes32));
            v = uint8(signature[64]);

            // Ensure v value is properly formatted.
            if (v != 27 && v != 28) {
                revert BadSignatureV(v);
            }
        }

In this case, It may be reverted with revert BadSignatureV(v); if length is 65 but uint8(signature[64]) is not 27 or 28.

But in fact it should be valid due to assumption above. ( must perform _assertValidEIP1271Signature(signer, digest, signature); )

Other length

Signature of other length is verified directly into signer contract by calling _assertValidEIP1271Signature(signer, digest, signature);, so it will be passed.

        } else {
            // For all other signature lengths, try verification via EIP-1271.
            // Attempt EIP-1271 static call to signer in case it's a contract.
            _assertValidEIP1271Signature(signer, digest, signature);

            // Return early if the ERC-1271 signature check succeeded.
            return;
        }

Tools Used

Manual code review

Recommended Mitigation Steps

Check isContract on signer first, if it is contract only verify EIP-1271 as contract never sign EIP-2098 under his address.

If signer is an EOA, only check for EIP-2098 as EOA never use EIP-1271.

If signer is an EOA but signature is malformed or recovered to different signer, revert with InvalidSignature() or BadSignatureV(v).

Change code to be like this

    function _assertValidSignature(
        address signer,
        bytes32 digest,
        bytes memory signature
    ) internal view {
        // If signer is a contract only check EIP-1271 signature.
        if (signer.isContract()) {
            // Attempt EIP-1271 static call to signer in case it's a contract.
            _assertValidEIP1271Signature(signer, digest, signature);

            // Please don't check EIP-2098 signature again for contract signer!
            return;
        }

        // EOA accounts must sign EIP-2098

        // Declare r, s, and v signature parameters.
        bytes32 r;
        bytes32 s;
        uint8 v;

        // If signature contains 64 bytes, parse as EIP-2098 signature (r+s&v).
        if (signature.length == 64) {
            // Declare temporary vs that will be decomposed into s and v.
            bytes32 vs;

            // Read each parameter directly from the signature's memory region.
            assembly {
                // Put the first word from the signature onto the stack as r.
                r := mload(add(signature, OneWord))

                // Put the second word from the signature onto the stack as vs.
                vs := mload(add(signature, TwoWords))

                // Extract canonical s from vs (all but the highest bit).
                s := and(vs, EIP2098_allButHighestBitMask)

                // Extract yParity from highest bit of vs and add 27 to get v.
                v := add(shr(255, vs), 27)
            }
        } else if (signature.length == 65) {
            // If signature is 65 bytes, parse as a standard signature (r+s+v).
            // Read each parameter directly from the signature's memory region.
            assembly {
                // Place first word on the stack at r.
                r := mload(add(signature, OneWord))

                // Place second word on the stack at s.
                s := mload(add(signature, TwoWords))

                // Place final byte on the stack at v.
                v := byte(0, mload(add(signature, ThreeWords)))
            }

            // Ensure v value is properly formatted.
            if (v != 27 && v != 28) {
                revert BadSignatureV(v);
            }
        } else {
            // Neither EIP-1271 nor EIP-2098
            revert InvalidSignature();
        }

        // Disallow invalid signers.
        if (ecrecover(digest, v, r, s) != signer) {
            revert InvalidSignature();
        }
    }

[0age]

Always checking isContract adds significant overhead in the standard case where it's not 1271 — this is a known limitation and would be a highly unusual application of eip-1271 (i.e. passing in something that isn't a signature, but has the exact length as a signature). This should be documented but is not a significant issue in our view.

[HardlyDifficult]

EIP-1271 is pretty open ended in terms of what may be considered acceptable, supporting signature schemes other than just ECDSA. The current implementation assumes that the signature is a valid ECDSA signature so when the length happens to match but a different scheme was used, it's possible that verification will fail before the call to _assertValidEIP1271Signature.

Lowering to a Low severity since the impact is presumably rare. It does have the potential to prevent the contract from interacting with seaport but doesn't lead to other negative consequences.

Grouping with the warden's QA report #188