xiaoming90 - New creators unable to update the royalty target and the fee route for their works

[sherlock-admin4]

xiaoming90

medium

New creators unable to update the royalty target and the fee route for their works

Summary

The new creators are unable to update the royalty target and the fee route for their works. As a result, it could lead to a loss of assets for the new creator due to the following:

• The work's royalty target still points to the previous creator, so the royalty fee is routed to the previous creator instead of the current creator.
• The work's fee is still routed to the recipients, which included the previous creator instead of the current creator.

Vulnerability Detail

The creator can call the transferWork to transfer the work to another creator. However, it was observed that after the transfer, there is still much information about the work pointing to the previous creator instead of the new creator.

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

File: Edition.sol
412:     function transferWork(address to_, uint256 tokenId_) external {
413:         Work storage work = works[tokenId_];
414:         if (msg.sender != work.creator) revert Unauthorized();
415: 
416:         // Transfer the work to the new creator
417:         work.creator = to_;
418: 
419:         emit WorkTransferred(address(this), tokenId_, to_);
420:     }

Following are some instances of the issues:

• The work's royalty target still points to the previous creator, so the royalty fee is routed to the previous creator instead of the current creator.
• The work's fee is still routed to the recipients, which included the previous creator instead of the current creator.

To aggravate the issues, creators cannot call the Edition.setRoyaltyTarget and FeeManager.createRoute as these functions can only executed by EDITION_MANAGER_ROLE and [Owner, Admin], respectively.

Specifically for the Edition.setRoyaltyTarget function that can only be executed by EDITION_MANAGER_ROLE, which is restricted and not fully trusted in the context of this audit. The new creator could have purchased the work from the previous creator, but only to find out that the malicious edition manager decided not to update the royalty target to point to the new creator's address for certain reasons, leading to the royalty fee continuing to be routed to the previous creator. In this case, it negatively impacts the new creator as it leads to a loss of royalty fee for the new creator.

Note

The following is an extract from the contest's README stating that the EDITION_MANAGER_ROLE is restricted. This means that any issue related to EDITION_MANAGER_ROLE that could affect TITLES protocol/users negatively will be considered valid in this audit contest.

EDITION_MANAGER_ROLE (Restricted) =>

Impact

Loss of assets for the new creator due to the following:

• The work's royalty target still points to the previous creator, so the royalty fee is routed to the previous creator instead of the current creator.
• The work's fee is still routed to the recipients, which included the previous creator instead of the current creator.

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider allowing the creators themselves to have the right to update the royalty target and the fee route for their works.

[pqseags]

While this is unintuitive, we do not intend to update the fee splits when ownership of a work is changed after publishing

[cducrest]

Escalate

Should be low/info as this is just un-intuitive behaviour of the protocol and does not represent a real threat / loss of funds.

The team does not intend to update the behaviour confirming that this is intended.

The problem arises when users transfer a work to another user while also believing it will transfer the ownership of the fees. This is a user mistake / misuse of the protocol and not an attack / exploit of a vulnerability.

[sherlock-admin3]

Escalate

Should be low/info as this is just un-intuitive behaviour of the protocol and does not represent a real threat / loss of funds.

The team does not intend to update the behaviour confirming that this is intended.

The problem arises when users transfer a work to another user while also believing it will transfer the ownership of the fees. This is a user mistake / misuse of the protocol and not an attack / exploit of a vulnerability.

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.

[Hash01011122]

@pqseags @cducrest can you please elaborate on how this issue just un-intuitive behavior of the protocol? Also where was it mentioned at the time of contest that protocol does not intend to update the fee splits when ownership of a work is changed after publishing

[pqseags]

It was not documented, but there isn't any logic in the code that makes an attempt to modify the fee route. In our minds, changing the creator on a work is more for the sake of changing who has admin responsibility. As this wasn't documented clearly in the read-me, I'll leave it to the judges to determine severity level given the context.

[cducrest]

It is not possible to change the fee split whether before or after ownership of a work is changed. To me, this is an optional feature request. It is not like it was possible to change the fee / royalty target before the work was transferred to a new creator and it becomes no longer possible. It is never possible to update it.

Also it makes little sense to update the "creator" of a work. If a "creator" (in the English literal understanding) publishes a work (a book, a picture, an NFT, anything) they remain the "creator" forever. Shakespeare cannot transfer authorship of Hamlet. As mentioned by @pqseags the transferWork() function serves more administrative purpose and delegating admin rights. It does not necessarily need to transfer fee/royalty rights.

[WangSecurity]

Thank you for these responses and insightful examples, but I agree that this design is actuall counter-intuitive and it wasn't documented anywhere. Also, I believe examples of how it works somewhere else are irrelevent. The watsons couldn't know what was the correct design for this function or it's known that when you transfer ownership the royalty is sent to the previous owner, hence, I believe it's fair to keep it valid.

Planning to reject the escalation and leave the issue as it is.

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• cducrest: rejected

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/titlesnyc/wallflower-contract-v2/pull/1

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.