Foundation Drop contest - KIntern_NA's results

Lines of code

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

Vulnerability details

Impact

Wrong return of cretorShares and creatorRecipients can make real royalties party can't gain the revenue of sale.

Proof of concept

Function getFees() firstly call to function internalGetImmutableRoyalties to get the list of creatorRecipients and creatorShares if the nftContract define ERC2981 royalties.

try implementationAddress.internalGetImmutableRoyalties(nftContract, tokenId) returns (
  address payable[] memory _recipients,
  uint256[] memory _splitPerRecipientInBasisPoints
) {
  (creatorRecipients, creatorShares) = (_recipients, _splitPerRecipientInBasisPoints);
} catch // solhint-disable-next-line no-empty-blocks
{
  // Fall through
}

---

In the 1st priority it check the nftContract define the function royaltyInfo or not. If yes, it get the return value receiver and royaltyAmount. In some manifold contracts of erc2981, it return (address(this), 0) when royalties are not defined. So we ignore it when the royaltyAmount = 0

  try IRoyaltyInfo(nftContract).royaltyInfo{ gas: READ_ONLY_GAS_LIMIT }(tokenId, BASIS_POINTS) returns (
    address receiver,
    uint256 royaltyAmount
  ) {
    // Manifold contracts return (address(this), 0) when royalties are not defined
    // - so ignore results when the amount is 0
    if (royaltyAmount > 0) {
      recipients = new address payable[](1);
      recipients[0] = payable(receiver);
      splitPerRecipientInBasisPoints = new uint256[](1);
      // The split amount is assumed to be 100% when only 1 recipient is returned
      return (recipients, splitPerRecipientInBasisPoints);
    }

---

In the same sense, the 3rd priority (it can reach to 3rd priority when function internalGetImmutableRoyalies fail to return some royalties) should check same as the 1st priority with the royaltyRegistry.getRoyaltyLookupAddress. But the 3rd priority forget to check the case when royaltyAmount == 0.

  try IRoyaltyInfo(nftContract).royaltyInfo{ gas: READ_ONLY_GAS_LIMIT }(tokenId, BASIS_POINTS) returns (
    address receiver,
    uint256 /* royaltyAmount */
  ) {
    recipients = new address payable[](1);
    recipients[0] = payable(receiver);
    splitPerRecipientInBasisPoints = new uint256[](1);
    // The split amount is assumed to be 100% when only 1 recipient is returned
    return (recipients, splitPerRecipientInBasisPoints);
  } 

It will make function _distributeFunds() transfer to wrong creatorRecipients (for example erc2981 return (address(this), 0), market will transfer creator revenue to address(this) - market contract, and make the fund freeze in contract forever).

This case just happen when

• nftContract doesn't have any support for royalties info
• overrideContract which was fetched fromroyaltyRegistry.getRoyaltyLookupAddress(nftContract) implements both function getRoyalties and royaltyInfo but doesn't support royaltyInfo by returning (address(this), 0).

Tools Used

Manual review

Recommended Mitigation Steps

Add check if royaltyAmount > 0 or not in 3rd priority

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183

Vulnerability details

Impact

Users can have more nfts than expected

Proof of concept

Variable limitPerAccount is used to indicate the max number of NFTs an account may have while minting. https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L111

Unfortunately, user can easily bypass this feature by using multiple accounts. For example: A drop has limitPerAccount = 10.

• Alice call function mintFromFixedPriceSale() with parameter count = 10. => Alice has 10 nfts in balance
• Alice transfer 10 nfts to Bob => Alice has 0 nfts in balance
• Alice call function mintFromFixedPriceSale() with parameter count = 10 again. Since Alice transfers all of her nfts to Bob, so it pass the check

/// link: https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L183
if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) {

=> Alice get 10 nfts.

• Bob transfer 10 nfts to Alice => Alice has 20 nfts in balance

In Example above, Alice can get 20 nfts > limitPerAccount = 10 nfts which was declared by creator.

Tools Used

Manual review

Recommended Mitigation Steps

I think there is no good solution to mitigate this issue. Can delete this variable if not needed.

Lines of code

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

Vulnerability details

Impact

Creator revenue will be distributed unfairly between royalties

Proof of concept

Function getRoyalties() is not a standardized for NFT contracts to declare how the royalty should be distributed.

But in the MarketFees.sol, it always assume that the royalties always calculated in term of BASIS_POINT. https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/shared/MarketFees.sol#L485-L488

if (creatorShares[i] > BASIS_POINTS) {
  // If the numbers are >100% we ignore the fee recipients and pay just the first instead
  totalShares = 0;
  break;
}

So if there are some variants that return with higher precision, the distribution can be wrong (the first one in array will gain all of the revenue).

For example: Royalties:

• creatorShares[Alice] = 1e6 (10%)
• creatorShares[Bob] = 5e6 (50%)
• creatorShares[Chloe] = 4e6 (40%)

It check creatorShares of Alice and see that it is bigger than BASIS_POINT so mark totalShares = 0. Since totalShares = 0, all of the creator funds will be sent to the first one (Alice).

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

if (totalShares == 0) {
  // If no shares were defined or shares were out of bounds, pay only the first recipient
  creatorRecipients.capLength(1);
  creatorShares.capLength(1);
}

In the same time Alice should be the one who gain minimal eth cause she has the lowest share. ==> It seem unfair for Bob and Chloe.

---

If Foundation protcol just support nft contract with royalties calculated in terms of BASIS_POINT, the checking should compare the totalShares with BASIS_POINT instead of comparing each creatorShares of each user with BASIS_POINT. Because the variant can calculated in another precision but all the royalties is less than BASIS_POINT.

Example:

• creatorShares[Alice] = 5500 (50%)
• creatorShares[Bob] = 5500 (50%)

Total shares = 5500 + 5500 = 11000 > 10000 = BASIS_POINT --> not supported but still distribute (It's still true, but not compatible with assumption: Foundation just support royalties calculated in terms of BASIS_POINT)

Tools Used

Manual review

Recommended Mitigation Steps

remove the check with BASIS_POINT

/// REMOVE THIS 
if (creatorShares[i] > BASIS_POINTS) {
  // If the numbers are >100% we ignore the fee recipients and pay just the first instead
  totalShares = 0;
  break;
}

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L181-L186

Vulnerability details

Impact

Function mintCountTo() in contract NFTDropCollection.sol use function _mint() to mint the token for specific to address.

/// link: https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L181-L186

for (uint256 i = firstTokenId; i <= latestTokenId; ) {
  _mint(to, i);
  unchecked {
    ++i;
  }
}

However this _mint() function does not check if to function is aware of incoming NFTs or not (If the recipient is a contract). This will lead to the scenario: the NFT can be locked in the recipient contract forever

Proof of concept

In contract ERC721.sol of Openzeppelin, there is a comment that mention about the discourage in using _mint() function when wanna mint a nft. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2dc086563f2e51620ebc43d2237fc10ef201c4e6/contracts/token/ERC721/ERC721.sol#L270

* WARNING: Usage of this method is discouraged, use {_safeMint} whenever possible

Tools Used

Manual review

Recommended Mitigation Steps

use safeMint function instead https://github.com/OpenZeppelin/openzeppelin-contracts/blob/2dc086563f2e51620ebc43d2237fc10ef201c4e6/contracts/token/ERC721/ERC721.sol#L247-L249