sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

nisedo - Cross-Chain Signature Replay Attack in `TitlesGraph.checkSignature()` #256

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

nisedo

medium

Cross-Chain Signature Replay Attack in TitlesGraph.checkSignature()

Summary

TitlesGraph.checkSignature() doesn't check for chainId allowing for cross-chain signature replay attacks.

Vulnerability Detail

The protocol is meant to be deployed on multiples chains according to the README:

Q: On what chains are the smart contracts going to be deployed?

Ethereum, Base, OP, Zora, Blast, Arbitrum, zkSync, Degen

TitlesGraph has a checkSignature modifier used to check the signature for a proxied acknowledgment in TitlesGraph.acknowledgeEdge() and TitlesGraph.unacknowledgeEdge().

    /// @notice Modified to check the signature for a proxied acknowledgment.
    modifier checkSignature(bytes32 edgeId, bytes calldata data, bytes calldata signature) {
        bytes32 digest = _hashTypedData(keccak256(abi.encode(ACK_TYPEHASH, edgeId, data)));
        if (
            !edges[edgeId].to.creator.target.isValidSignatureNowCalldata(digest, signature)
                || _isUsed[keccak256(signature)]
        ) {
            revert Unauthorized();
        }
        _;
        _isUsed[keccak256(signature)] = true;
    }

However, it is vulnerable to cross-chain replay attacks because it doesn't check for chainId in the current implementation as specified by the Solady SignatureCheckerLib doc:

/// WARNING! Do NOT use signatures as unique identifiers: /// - Use a nonce in the digest to prevent replay attacks on the same contract. /// - Use EIP-712 for the digest to prevent replay attacks across different chains and contracts. /// EIP-712 also enables readable signing of typed data for better user safety. /// This implementation does NOT check if a signature is non-malleable.

Impact

Signature replay attack leading to unfair authorized calls to acknowledgeEdge() / unacknowledgeEdge().

Code Snippet

Tool used

Manual Review

Recommendation

Add a chainId check in the checkSignature modifier.

Duplicate of #284

ccashwell commented 4 months ago

This is not valid. Indeed, the chainId is being validated as part of the EIP712 domain.

image