CodeWasp - TitlesGraph does not check chainId

[sherlock-admin3]

CodeWasp

high

TitlesGraph does not check chainId

Summary

TitlesGraph does not check chainId

Vulnerability Detail

TitlesGraph.{_isCreator,_isEntity} does not check if chainId matches the one recorded in node.entity / node.creator.

Impact

Insufficient auth on chains with Ethereum-incompatible addresses.

Code Snippet

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

Tool used

Manual Review

Recommendation

Check chain id.

[thpani]

Escalate

Let me elaborate:

Vulnerability Detail

TitlesGraph.createEdge and the non-ECDSA versions of TitlesGraph.{acknowledgeEdge,unacknowledgeEdge} call into _isCreator and/or _isEntity for authorization. These check the target property of the respective Node's creator or entity:

    function _isCreator(Node memory node, address guy) internal pure returns (bool) {
        return node.creator.target == guy;
    }
[...]
    function _isEntity(Node memory node, address guy) internal pure returns (bool) {
        return node.entity.target == guy;
    }

creator and entity are Targets, which are identified not just by their address (target), but also by their chainId:

struct Node {
    NodeType nodeType;
    Target entity;
    Target creator;
    bytes data;
}
[...]
struct Target {
    uint256 chainId;
    address target;
}

However, the chainId is never checked in the authorization code in _isCreator and _isEntity.

Impact

Assume that Titles is depolyed on mainnet. Assume that chain ID 1234 points to a chain with Ethereum-incompatible addresses, and that a 1234-entity adam = { chainId: 1234, target: someAddr } is recorded in the graph as the target of an edge.

eve = { chainId: 1, target: someAddr } who owns a private key for which their mainnet-address someAddr collides with the 1234-address someAddr can call TitlesGraph.{acknowledgeEdge,unacknowledgeEdge} although they are not the entity identified in the graph.

The sponsor opted-in on findings about future integrations and the implementation of TitlesGraph is clearly prepared to record entities on various chains.

Recommendation

Check the chain ID:

diff --git a/wallflower-contract-v2/src/graph/TitlesGraph.sol b/wallflower-contract-v2/src/graph/TitlesGraph.sol
index 47def9f..9d6305c 100644
--- a/wallflower-contract-v2/src/graph/TitlesGraph.sol
+++ b/wallflower-contract-v2/src/graph/TitlesGraph.sol
@@ -236,7 +236,7 @@ contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgr
     /// @return True if the address is the creator of the node, false otherwise.
     function _isCreator(Node memory node, address guy) internal pure returns (bool) {
         // @audit-issue does not check chainId
-        return node.creator.target == guy;
+        return node.creator.target == guy && node.creator.chainId == block.chainid;
     }

     /// @notice Checks if the given address is the on-chain entity represented by a node.
@@ -245,7 +245,7 @@ contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgr
     /// @return True if the address is the entity of the node, false otherwise.
     function _isEntity(Node memory node, address guy) internal pure returns (bool) {
         // @audit-issue does not check chainId
-        return node.entity.target == guy;
+        return node.entity.target == guy && node.entity.chainId == block.chainid;
     }

     /// @notice Checks if the given address is either the creator or on-chain entity represented by a node.

[sherlock-admin3]

Escalate

Let me elaborate:

Vulnerability Detail

TitlesGraph.createEdge and the non-ECDSA versions of TitlesGraph.{acknowledgeEdge,unacknowledgeEdge} call into _isCreator and/or _isEntity for authorization. These check the target property of the respective Node's creator or entity:

    function _isCreator(Node memory node, address guy) internal pure returns (bool) {
        return node.creator.target == guy;
    }
[...]
    function _isEntity(Node memory node, address guy) internal pure returns (bool) {
        return node.entity.target == guy;
    }

creator and entity are Targets, which are identified not just by their address (target), but also by their chainId:

struct Node {
    NodeType nodeType;
    Target entity;
    Target creator;
    bytes data;
}
[...]
struct Target {
    uint256 chainId;
    address target;
}

However, the chainId is never checked in the authorization code in _isCreator and _isEntity.

Impact

Assume that Titles is depolyed on mainnet. Assume that chain ID 1234 points to a chain with Ethereum-incompatible addresses, and that a 1234-entity adam = { chainId: 1234, target: someAddr } is recorded in the graph as the target of an edge.

eve = { chainId: 1, target: someAddr } who owns a private key for which their mainnet-address someAddr collides with the 1234-address someAddr can call TitlesGraph.{acknowledgeEdge,unacknowledgeEdge} although they are not the entity identified in the graph.

The sponsor opted-in on findings about future integrations and the implementation of TitlesGraph is clearly prepared to record entities on various chains.

Recommendation

Check the chain ID:

diff --git a/wallflower-contract-v2/src/graph/TitlesGraph.sol b/wallflower-contract-v2/src/graph/TitlesGraph.sol
index 47def9f..9d6305c 100644
--- a/wallflower-contract-v2/src/graph/TitlesGraph.sol
+++ b/wallflower-contract-v2/src/graph/TitlesGraph.sol
@@ -236,7 +236,7 @@ contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgr
     /// @return True if the address is the creator of the node, false otherwise.
     function _isCreator(Node memory node, address guy) internal pure returns (bool) {
         // @audit-issue does not check chainId
-        return node.creator.target == guy;
+        return node.creator.target == guy && node.creator.chainId == block.chainid;
     }

     /// @notice Checks if the given address is the on-chain entity represented by a node.
@@ -245,7 +245,7 @@ contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgr
     /// @return True if the address is the entity of the node, false otherwise.
     function _isEntity(Node memory node, address guy) internal pure returns (bool) {
         // @audit-issue does not check chainId
-        return node.entity.target == guy;
+        return node.entity.target == guy && node.entity.chainId == block.chainid;
     }

     /// @notice Checks if the given address is either the creator or on-chain entity represented by a node.

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]

Unfortunately, we have to judge the original report rather than the escalation itself. The report doesn't explain any impact or the risk:

Insufficient auth on chains with Ethereum-incompatible addresses.

It sounds only as a recommendation, hence, invalid. Planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result: Invalid Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• thpani: rejected