QA report

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketCreators.sol#L94-L101

Vulnerability details

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketCreators.sol#L94-L101

for (uint256 i = 0; i < _recipients.length; ++i) {
    if (_recipients[i] != address(0)) {
    hasRecipient = true;
    if (_recipients[i] == seller) {
        return (_recipients, recipientBasisPoints, true);
    }
    }
}

In the current implementation of NFTMarketCreators.sol#_getCreatorPaymentInfo(), when the seller is one of the royalty recipients, isCreator will be set to true.

In NFTMarketFees.sol#_getFees(), when isCreator is true, in spite of there may be other creatorRecipients, all revenue is split, and ownerRev will be left as the deafult value of 0.

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketFees.sol#L197-L200

 if (isCreator) {
    // When sold by the creator, all revenue is split if applicable.
    creatorRev = price - foundationFee;
  }

AS a result, in NFTMarketFees.sol#_distributeFunds() the creatorFee will be distributed to all creatorRecipients based on creatorShares.

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketFees.sol#L100-L105

uint256 totalDistributed;
for (uint256 i = 1; i <= maxCreatorIndex; ++i) {
  uint256 share = (creatorFee * creatorShares[i]) / totalShares;
  totalDistributed += share;
  _sendValueWithFallbackWithdraw(creatorRecipients[i], share, SEND_VALUE_GAS_LIMIT_MULTIPLE_RECIPIENTS);
}

If the owner/seller of the NFT is one of the creatorRecipients, they can not get the expected revenue, instead, most of the revenue can be distributed to the other _recipients.

PoC

Given:

• NFT-Token1 Royalties:

  • Alice = 9,000
  • Bob = 1,000

• Bob owns the NFT-Token1

1. Bob setBuyPrice() and listed NFT-Token1 for 10 ETH;

2. Charle buy() NFT-Token1 with 10 ETH:

   • foundationFee = 1.5 ETH
   • creatorFee = 8.5 ETH

   as a result:

   • Alice received 7.65 ETH
   • Bob received 0.85 ETH

Recommendation

Change to:

if (creatorRecipients.length > 0) {
  if (isCreator && creatorRecipients.length == 1) {
    ownerRevTo = seller;
    ownerRev = price - foundationFee;
  } else {
    // Rounding favors the owner first, then creator, and foundation last.
    creatorRev = (price * CREATOR_ROYALTY_BASIS_POINTS) / BASIS_POINTS;
    ownerRevTo = seller;
    ownerRev = price - foundationFee - creatorRev;
  }
} else {
  // No royalty recipients found.
  ownerRevTo = seller;
  ownerRev = price - foundationFee;
}

[HardlyDifficult]

This is a fair concern as some users would not expect the market to pay royalties in this way, but it is how we intended it to work.

The thinking behind this is we wanted any rev share defined by the NFT to be respected in a way that collectors might expect. An example is if the NFT has declared that 50% of the revenue will go to a charity. It's not immediately obvious if all the revenue from the initial sale, for example, should be split with the charity or if this is only referring to the 10% royalties. We have chosen to lean in favor of splitting anywhere it seems reasonable to do so to try and minimize misleading deals like this. In order for that to continue to work as expected, we did not want to pursue a change like the suggested && creatorRecipients.length == 1.

We added a comment to help clarify the expected behavior here.

However another related issue that was raised lead to a significant change in the logic here. It mitigates some of the concern raised here as well: https://github.com/code-423n4/2022-02-foundation-findings/issues/30

[alcueca]

Downgrading this to a code clarity issue.

[alcueca]

Score as a QA Report: 5

[CloudEllie]

Since this issue was downgraded to a QA level, and the warden did not submit a separate QA report, we've renamed this one to "QA report" for consistency.

The original title, for the record, was "Inappropriate implementation of NFTMarketFees#_getFees() can result in incorrect distribution when the seller is one of the royalty recipients."