sherlock-audit / 2024-04-titles-judging

10 stars 7 forks source link

ast3ros - TitlesGraph contract does not strictly follow EIP-712 standard #313

Closed sherlock-admin4 closed 5 months ago

sherlock-admin4 commented 5 months ago

ast3ros

medium

TitlesGraph contract does not strictly follow EIP-712 standard

Summary

The TitlesGraph contract does not adhere strictly to the EIP-712 standard during signature verification.

Vulnerability Detail

From the readme, the TitlesGraph contract strictly implements the EIP-712 standard for signing messages.

From EIP-712 about encodeData:

    The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

https://eips.ethereum.org/EIPS/eip-712

However, when constructing the digets for the signature, the data which is the bytes type is not keccak256 hashed, which not following the EIP-712 standards.

    modifier checkSignature(bytes32 edgeId, bytes calldata data, bytes calldata signature) {
        bytes32 digest = _hashTypedData(keccak256(abi.encode(ACK_TYPEHASH, edgeId, data))); // @audit the data is not keccak256 hashed
        if (
            !edges[edgeId].to.creator.target.isValidSignatureNowCalldata(digest, signature)
                || _isUsed[keccak256(signature)]
        ) {
            revert Unauthorized();
        }
        _;
        _isUsed[keccak256(signature)] = true;
    }

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

Impact

The current implementation does not follow the EIP-712 standard, which could lead to potential issues with signature verification.

Code Snippet

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

Tool used

Manual Review

Recommendation

modify the checkSignature modifier to hash the data parameter before encoding it

    modifier checkSignature(bytes32 edgeId, 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, keccak256(data))));
        if (
            !edges[edgeId].to.creator.target.isValidSignatureNowCalldata(digest, signature)
                || _isUsed[keccak256(signature)]
        ) {
            revert Unauthorized();
        }
        _;
        _isUsed[keccak256(signature)] = true;
    }

Duplicate of #74