sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

Varun_05 - transferWork can change the creator of a work to different address but the mint fees is still transferred to the old address. #336

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 2 months ago

Varun_05

high

transferWork can change the creator of a work to different address but the mint fees is still transferred to the old address.

Summary

transferWork can change the creator of a work to different address but the mint fees is still transferred to the old address. This can cause loss of funds for the creator as there can be scenario where the creator wants to call transfer work maybe because it old address is not safe anymore or can be any other reason.

Vulnerability Detail

Following is transferWork function which changes the creator of an existing work.

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_);
    }

Now as can be seen from the following createRoute function which sets the fee receivers of a particular work.

 function createRoute(
        IEdition edition_,
        uint256 tokenId_,
        Target[] calldata attributions_,
        address referrer_
    ) external onlyOwnerOrRoles(ADMIN_ROLE) returns (Target memory receiver) {
        Target memory creator = edition_.node(tokenId_).creator;

        if (attributions_.length == 0) {
            // No attributions, pay the creator directly
            receiver = creator;
        } else {
            // Distribute the fee among the creator and attributions
            (address[] memory targets, uint256[] memory revshares) = _buildSharesAndTargets(
                creator, attributions_, edition_.feeStrategy(tokenId_).revshareBps
            );

            // Create the split. The protocol retains "ownership" to enable future use cases.
            receiver = Target({
                target: splitFactory.createSplit(
                    SplitV2Lib.Split({
                        recipients: targets,
                        allocations: revshares,
                        totalAllocation: 1e6,
                        distributionIncentive: 0
                    }),
                    address(this),
                    creator.target
                    ),
                chainId: creator.chainId
            });
        }

   ==>     _feeReceivers[getRouteId(edition_, tokenId_)] = receiver;
        referrers[edition_] = referrer_;
    }

Now feeReceivers[getRouteId(edition, tokenId_)] = receiver; is only sets once and can't be changed and receiver is calculated as follows

receiver = Target({
                target: splitFactory.createSplit(
                    SplitV2Lib.Split({
                        recipients: targets,
                        allocations: revshares,
                        totalAllocation: 1e6,
                        distributionIncentive: 0
                    }),
                    address(this),
                    creator.target
                    ),
                chainId: creator.chainId
            });
        }

And it is clear from the following peice of code ''creator.target'' is used for calculating who receives the mint fees. So now if trasnfer work is called then the creator of that work is changed but the old address of creator continues to receives the fees, which can even cause loss of funds for the user who would have thought that the new address would now receive the fees.

This is a valid issue because there is no clear documentation where it is written what is the exact purpose for which the transfer work function is called, so the above scenario is valid.

Impact

Doesn't updates the new address which should receive the fees from now onwards thus can cause loss of fees for the creator.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/d7f60952df22da00b772db5d3a8272a988546089/wallflower-contract-v2/src/editions/Edition.sol#L412

Tool used

Manual Review

Recommendation

Add a function to update the value of feeReceivers[getRouteId(edition, tokenId_)] when trasnfer work function is called.

Duplicate of #283

ccashwell commented 2 months ago

Won't fix, this is expected behavior