sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

KupiaSec - `checkSignature` modifier does not consider `block.chainid` #402

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 2 months ago

KupiaSec

medium

checkSignature modifier does not consider block.chainid

Summary

In the TitlesGraph.checkSignature modifier, it checks the signature without the block.chainID, which means an attacker can use the same signature on another chain.

Vulnerability Detail

In the TitlesGraph.checkSignature modifier, it checks the signature with edgeId, data and signature parameters.

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/graph/TitlesGraph.sol#L40

File: wallflower-contract-v2\src\graph\TitlesGraph.sol
40:     modifier checkSignature(bytes32 edgeId, bytes calldata data, bytes calldata signature) {
41:         bytes32 digest = _hashTypedData(keccak256(abi.encode(ACK_TYPEHASH, edgeId, data)));
42:         if (
43:             !edges[edgeId].to.creator.target.isValidSignatureNowCalldata(digest, signature)
44:                 || _isUsed[keccak256(signature)]
45:         ) {
46:             revert Unauthorized();
47:         }
48:         _;
49:         _isUsed[keccak256(signature)] = true;
50:     }

This modifier does not consider block.chainID, so attacker can pass this modifier with variables: edgeId, data and signature which is used in another chain.

Impact

The attacker, who is not the creator or the entity of edges[edgeId_].to can change the edge's acknowledged status.

Tool used

Manual Review

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/graph/TitlesGraph.sol#L34C1-L50C6

Recommendation

File: wallflower-contract-v2\src\graph\TitlesGraph.sol
-   bytes32 public constant ACK_TYPEHASH = keccak256("Ack(bytes32 edgeId,bytes data)");
+   bytes32 public constant ACK_TYPEHASH = keccak256("Ack(bytes32 edgeId,uint256 chainID,bytes data)");

    // @notice The EIP-712 domain type hash. (Exposed here for convenience.)
    bytes32 public constant DOMAIN_TYPEHASH = _DOMAIN_TYPEHASH;

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

Duplicate of #284

ccashwell commented 2 months ago

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

image