code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L477 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L478 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L496 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L497

Vulnerability details

Impact

Detailed description of the impact of this finding.

The creatorRecipient and creatorShares can be forcefully trancated to a fixed length. And the truncated creator is not paid.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

When calculating fees,

we have

      // Cap the max number of recipients supported
      creatorRecipients.capLength(MAX_ROYALTY_RECIPIENTS);
      creatorShares.capLength(MAX_ROYALTY_RECIPIENTS);

if we have more than MAX_ROYALTY_RECIPIENT length of recipient, the truncated recipient is not paid.

Tools Used

Recommended Mitigation Steps

instead of truncating creator recipient when calculating fees, we can restrict the number of creator when setting creator recipient and not truncating the creator recipient array when calculating fees.

HardlyDifficult commented 2 years ago

This is by design and important for our creators to understand.

It's true that the number of royalty recipients is capped to 5 and if more than 5 were defined, then we would simply ignore all the entries other than the first 5 listed.

We cannot restrict the number as suggested in the recommendation because our code works with all ERC721 NFTs, not just those created via our own NFTCollectionFactory. Our factory will always return exactly 1 royalty recipient. However industry standard APIs allow for returning arbitrarily sized arrays.

We have yet to see any examples in the wild of more than 5 royalty recipients defined, but this code is being defensive since it's bound to happen at some point.

We need a cap because the creator defines the list of recipients, but it's collectors that pay the gas in order to process all these payments. The cap here aims to ensure the worst case scenario does not get to be outrageous.

HickupHH3 commented 2 years ago

Agree with sponsor. Processing uncapped arrays usually means potential DoS of the functionality due to the block gas limit size. One can argue 5 is too little and should be raised, but it's ultimately up to the team to decide what the reasonable number is.

code-423n4 / 2022-08-foundation-findings