sherlock-audit / 2024-04-titles-judging

9 stars 6 forks source link

sammy - Updating the fee strategy using `setFeeStrategy()` does not update the royalty info, resulting in inconsistent royalty information #258

Closed sherlock-admin3 closed 4 months ago

sherlock-admin3 commented 4 months ago

sammy

medium

Updating the fee strategy using setFeeStrategy() does not update the royalty info, resulting in inconsistent royalty information

Summary

A publication creator can update the fee collection strategy of their work by invoking the setFeeStrategy() function in Edition.sol. However, doing so does not update the token royalty information of the publication, therefore breaking ERC2981 compliance and resulting in inconsistent values.

Vulnerability Detail

When a publication is created using publish() in TitlesCore.sol, the royalty info is updated using the fee strategy inputted by the creator at the time of creation :

function _publish(Edition edition_, WorkPayload memory work_, address referrer_)
        internal
        returns (uint256 tokenId)
    {
        // Publish the new Work in the Edition
        // wake-disable-next-line reentrancy
        tokenId = edition_.publish(
            work_.creator.target,
            work_.maxSupply,
            work_.opensAt,
            work_.closesAt,
            work_.attributions,
            work_.strategy,
            work_.metadata
        );

        // Collect the creation fee
        // wake-disable-next-line reentrancy
        feeManager.collectCreationFee{value: msg.value}(edition_, tokenId, msg.sender);

        // Create the fee route for the new Work
        // wake-disable-next-line reentrancy
        Target memory feeReceiver = feeManager.createRoute(
            edition_, tokenId, _attributionTargets(work_.attributions), referrer_
        );

        // Set the royalty target for the new Work
        // wake-disable-next-line reentrancy
@>        edition_.setRoyaltyTarget(tokenId, feeReceiver.target);
    }

The creator has the option to update this strategy later on by invoking the setFeeStrategy() function in Edition.sol, but this does not update the royalty info of the work :

 function setFeeStrategy(uint256 tokenId_, Strategy calldata strategy_) external {
        if (msg.sender != works[tokenId_].creator) revert Unauthorized();
        works[tokenId_].strategy = FEE_MANAGER.validateStrategy(strategy_);
    }

The setRoyaltyTarget() can only be called by the EDITION_MANAGER_ROLE , i.e, the TitlesCore.sol contract, thus the creator has no way of updating the royalty information. The contract can call the _setTokenRoyalty() internal function and update it, but this is not currently implemented in the code.

Impact

Breaking ERC2981 compliance, loss of protocol functionality

Code Snippet

Tool used

Manual Review

Recommendation

Alter setFeeStrategy() as follows :

     function setFeeStrategy(uint256 tokenId_, Strategy calldata strategy_) external {
         if (msg.sender != works[tokenId_].creator) revert Unauthorized();
         works[tokenId_].strategy = FEE_MANAGER.validateStrategy(strategy_);
+        _setTokenRoyalty(tokenId_, works[tokenId_].creator, strategy_.royaltyBps);
     }

Duplicate of #144