xiaoming90 - Malicious EDITION_MANAGER_ROLE can front-run victims to increase royalty

[sherlock-admin4]

xiaoming90

medium

Malicious EDITION_MANAGER_ROLE can front-run victims to increase royalty

Summary

Malicious EDITION_MANAGER_ROLE can front-run victims to increase royalty, leading to a loss of assets for the victim.

Vulnerability Detail

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) =>
On an Edition, this role can:

Set the Edition's ERC2981 royalty receiver (setRoyaltyTarget). This is the only way to change the royalty receiver for the Edition.

Note

About ERC2981

ERC2981 known as "NFT Royalty Standard." It introduces a standardized way to handle royalty payments for non-fungible tokens (NFTs) on the Ethereum blockchain, providing a mechanism to ensure creators receive a share of the proceeds when their NFTs are resold

Assume that the current royalty for work/collection X ($Collection_X$) is 5%. When secondary marketplaces resell the TITLES NFT, they will retrieve the NFT's royalty information by calling the Edition.royaltyInfo function. The royalty payments are calculated, collected, and transferred automatically to the creator's wallet whenever their NFT is resold.

The issue is that the EDITION_MANAGER_ROLE is restricted in the context of this audit and is considered not fully trusted.

Assume Bob submits a transaction to purchase an NFT from $Collection_X$​ to the mempool. A malicious editor manager could front-run the purchase transaction and attempt to increase the royalty to be increased from 5% to 95%, resulting in more royalty fees being collected from Bob, and routed to the creators. This leads to a loss of assets for Bob.

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

File: Edition.sol
386:     /// @notice Set the ERC2981 royalty target for the given work.
387:     /// @param tokenId The ID of the work.
388:     /// @param target The address to receive royalties.
389:     function setRoyaltyTarget(uint256 tokenId, address target)
390:         external
391:         onlyRoles(EDITION_MANAGER_ROLE)
392:     {
393:         _setTokenRoyalty(tokenId, target, works[tokenId].strategy.royaltyBps);
394:     }

Impact

Loss of assets for the victim, as shown in the above scenario

Code Snippet

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

Tool used

Manual Review

Recommendation

Ensure that only trusted users can update the royalty information, as this is a sensitive value that could affect the fee being charged when TITLES NFT is resold in the secondary market.

[xiaoming9090]

Escalate

1. Per the contest's README, it states that the EDITION_MANAGER_ROLE is restricted.

EDITION_MANAGER_ROLE (Restricted) =>

Any issue related to EDITION_MANAGER_ROLE that could affect TITLES protocol/users negatively will be considered valid in this audit contest. This report demonstrates the negative impacts that a malicious editor manager could cause to the users. Thus, this issue should be valid.

Per Sherlock Judging rules, note that Contest README supersedes anything else, including protocol answers on the contest public Discord channel. This Sherlock rule must be adhered to strictly.

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

2. The following issues are not duplicated of this issue as they did not demonstrate how a malicious editor manager can front-run victims to increase royalty, causing a negative impact on the users.

• nisedo - Works with multiple attributions will result in royalty payments only for the first attribution #252
• ZdravkoHr. - Edition does not have way to set default royalties #298
• nisedo - Broken Role Checking Mechanism #337

[sherlock-admin3]

Escalate

1. Per the contest's README, it states that the EDITION_MANAGER_ROLE is restricted.

EDITION_MANAGER_ROLE (Restricted) =>

Any issue related to EDITION_MANAGER_ROLE that could affect TITLES protocol/users negatively will be considered valid in this audit contest. This report demonstrates the negative impacts that a malicious editor manager could cause to the users. Thus, this issue should be valid.

Per Sherlock Judging rules, note that Contest README supersedes anything else, including protocol answers on the contest public Discord channel. This Sherlock rule must be adhered to strictly.

Hierarchy of truth: Contest README > Sherlock rules for valid issues > protocol documentation (including code comments) > protocol answers on the contest public Discord channel.

2. The following issues are not duplicated of this issue as they did not demonstrate how a malicious editor manager can front-run victims to increase royalty, causing a negative impact on the users.

• nisedo - Works with multiple attributions will result in royalty payments only for the first attribution #252
• ZdravkoHr. - Edition does not have way to set default royalties #298
• nisedo - Broken Role Checking Mechanism #337

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]

Agree with the escalation. Planning to accept it and validate issue to unique medium.

[Evert0x]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• xiaoming9090: accepted

[ccashwell]

Why is this accepted? It's a generic informational issue at best, the finding literally applies to any modifiable configuration that affects pricing in any contract. This is not a valid logic issue.

[WangSecurity]

Based on the README, we have to consider that Edition_Manager_Role to be restricted, hence, untrusted. In that sense, there is a possibility it will harm users as shown in this report, thus, exploiting the current logic of the protocol.

[ccashwell]

I think the word "exploiting" is wrong here, what's reported is that an NFT's creator can change pricing. Literally anytime this occurs is valid. Moreover, it would assume that the royalty payer (i.e. secondary market buyer) somehow sent an amount of assets beyond the expected amount due.

There are many many ways to modify configs such that users end up paying more, and "front running" is irrelevant to that. For example, if the team changed core protocol fees, could that not also qualify as malicious under such a broad interpretation? Fee changes are a normal part of the protocol (and editions), and all users are aware of the exact quoted amount at the time they click "mint". If they send more than this amount, that's user error.