Revenue split inconsistency in `_getFees`

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L460-L474

Vulnerability details

[PNM-002] Revenue split inconsistency in _getFees

Links

• https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L460-L474

Description

_getFees is an internal function used to designate how the revenue from selling an NFT drop collection is split. In all cases, revenue is given to two types of users: seller (1) and creatorRecipients (many), usually, the creator is designated as creatorRecipient[0].

There exists multiple pathways that affect how the revenue is paid out, and in this scenario, assume that there is more than one creatorRecipient and assumePrimarySale is false.

There are then two possibilities: either the creator is the seller, or the creator is not the seller, and these two options make up the else if and else branches on lines 460 and 467 respectively.

If the creator is not the seller, or during the else branch, a new value is computed, sellerRev representing the revenue earned by the seller. However, this is not present in the else if branch, or when the creator is the seller.

In both cases, the remaining revenue is split amongst the creatorRecipients.

In the else branch, if the seller is also part of the creatorRecipients, he or she may be paid twice: first from the sellerRev, and second from the creatorRecipient.

However, in the else if branch, the seller is only paid once by only the creatorRecipient share.

This appears to be inconsistant, as when the creator acts as the seller, he or she may recieve less than if someone who wasn't the creator acted as a seller for the same amount of revenue.

This can be taken advantage of by the creator using another account as a seller to reap more profits from other creatorRecipients.

It is important to note that the seller must add themselves to the creatorRecipients array; this is not done automatically.

PoC / Attack Scenario

Case A (creator is seller)

• Alice is the creator of an NFT drop collection with multiple creatorRecipients.
• Alice decides to activate a sale, and generates revenue.
• Alice recieves an amount dependant on her share in the creatorRecipients array.

Case B (creator is not the seller)

• Bob is not the creatorof the NFT drop collection mentioned above, but he has aquired permision to sell it.
• Bob activates a sale, and generates revenue
• Bob recieves his share in the creatorRecipients array, and also the sellerRev.

Suggested Fix

Provide the creator with a sellerRev so any profits are transparent to other potential creatorRecipients.

[HardlyDifficult]

The core concern here appears to be how numbers are attributed in events, which seems like a non-critical issue - however I argue below that it's correct as is.

Case B identifies a scenario that may not align with the creator's expectations. This could qualify as a Low risk issue.

This submission refers to risk to drops when assumePrimarySale is not set, however it's hard-coded to true.

MarketFees was in scope for this contest and in our other (out of scope) market implementation, assumePrimarySale will be false -- so lets explore the submission from that point of view:

• Case A: The seller is the creator, so 95% of the sale price is distributed to the creator recipients and the seller may be one of those receiving a portion of the revenue. This is by design and as expected.
• Case B: The seller is not identified as the creator but IS one of the creator recipients. As the seller they get 85% of the sale price; and as a creator recipient they get x% of 10% of the sale price (for a total > 85% and < 95%). This is valid but may not be what was expected by the creator/seller.

Case B is potentially confusing. It's possible a more appropriate action in this scenario is to check all the recipients and if the seller is found, attribute all the revenue to the recipients as we would have when normally sold by the creator.

For the events themselves - creatorRev always represents how much the creator recipients received. This makes it easier to parse in scenarios where there is more than a single recipient. If we were to use sellerRev instead as suggested in the rec, consumers would need to parse the sale scenario in order to determine if that amount was split amongst recipients or if it went to a single seller address.

[HickupHH3]

I agree with the sponsor. Took me a while to understand the problem.

At first glance, it seems that the creator is better off with Case B: seller != creator, but is one of the recipients. As the warden mentioned, "when the creator acts as the seller, he or she may recieve less than if someone who wasn't the creator acted as a seller for the same amount of revenue.". The rationale is because the creator gets paid twice, whereby he recieves his share in the creatorRecipients array, and also the sellerRev.

However, as the sponsor rightly points out, if we were to examine the amounts, it would actually be better for the seller to specify himself as the creator (ie. option A):

• Option A nets him 95% of the proceeds:

  totalFees = price / PROTOCOL_FEE_DENOMINATOR; // 5%
  creatorRev = price - totalFees; // 100% - 5% = 95%

• Option B nets him between 85% to 95% of the proceeds:

  creatorRev = price / CREATOR_ROYALTY_DENOMINATOR; // x% of 10% of price
  sellerRev = price - totalFees - creatorRev; // 100% - 5% - 10% = 85%

Thus, since option A is incentivised, I agree that the creator is rightfully incentivised. Downgrading to low-severity QA because of Case B potentially being "a scenario that may not align with the creator's expectations".

[HickupHH3]

Warden has no QA, marking as primary issue.