sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

BengalCatBalu - Broken TitlesGraph behavior when changing work.creator #341

Closed sherlock-admin4 closed 2 months ago

sherlock-admin4 commented 2 months ago

BengalCatBalu

medium

Broken TitlesGraph behavior when changing work.creator

Summary

The work in the graph is stored as a Node data structure. Which stores the creator of the work in one of the fields (Target creator). Only the creator of the work or entity can change the edge's knowledge. However, in the Edition.sol::transferWork function, when the creator of the work is changed, the state of the node in the edge does not change. That is, only the original owner who was specified when creating the edges (that is, when creating the work) will be able to change the edge's acknowledge field

Vulnerability Detail

Validation of msg.sender when changing the acknowledge field occurs in the function TitlesGraph::_isСreatOrEntity

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

    /// @notice Checks if the given address is the on-chain entity represented by a node.
    /// @param node The node to check.
    /// @param guy The address to check.
    /// @return True if the address is the entity of the node, false otherwise.
    function _isEntity(Node memory node, address guy) internal pure returns (bool) {
        return node.entity.target == guy; // always address of edition contract
    }

    /// @notice Checks if the given address is either the creator or on-chain entity represented by a node.
    /// @param node The node to check.
    /// @param guy The address to check.
    /// @return True if the address is the creator or entity of the node, false otherwise.
    function _isCreatorOrEntity(Node memory node, address guy) internal pure returns (bool) {
        return _isCreator(node, guy) || _isEntity(node, guy);
    }

We see that the _isCreator check relies solely on the node stored in the edge

For example, when calling aknowledgeEdge(bytes 32 edgeId, bytes сalldata data), the edge is edges[edgeId], and the field being checked is edges[edgeId].to - this field is saved in the edge when it is created, the possibility of modification is not provided within the protocol. That is, if you change the creator of the work, the old creator will retain control over the necessary edges, but the new one will not.

Impact

This problem does not lead to a loss of funds. However, here is a quote from Readme::Additional Audit Information - "In addition to the security of funds, we would also like there to be focus on the sanctity of the data in the TitlesGraph and the permissioning around it (only the appropriate people/contracts can signal reference and acknowledgement of reference)."

This problem can lead to incorrect behavior and manipulation of TitlesGraph data.

Code Snippet

function transferWork(address to_, uint256 tokenId_) external {
        Work storage work = works[tokenId_];
        if (msg.sender != work.creator) revert Unauthorized();

        // Transfer the work to the new creator
        work.creator = to_;

        emit WorkTransferred(address(this), tokenId_, to_);
 }

Tool used

Manual Review

Recommendation

Add the ability to change the nodes in the edge. And also, when transferring the creator status, change the creator in the node.

Duplicate of #283