Foundation contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketReserveAuction.sol#L527-L565

Vulnerability details

PoC

Case A:

Given:

• A ReserveAuction is waiting to be finalized:
  • the creator createReserveAuction
  • the bidder bid with 1 ETH
  • wait until auction.endTime

When:

• buyer makeOffer with: 1 ETH
• bidder acceptOffer

Expected Results:

1. creator to receive bidder's payment of 1 ETH - fee;
2. bidder's balance remains unchanged （exclude gas and fee), because 1 ETH is paid for the auction, and 1 ETH received from the buyer;
3. buyer paid 1 ETH;
4. NFT transferred to the buyer

Actual Results:

• ETH payments are made as expected in 1, 2, 3;
• NFT was not transferred to the buyer, but to the bidder instead

Case B:

Given:

• A ReserveAuction is waiting to be finalized:
  • creator createReserveAuction
  • bidder bid with 1 ETH
  • wait until auction.endTime

When:

• buyer makeOffer with amount: 1 ETH
• bidder ignored the offer above, called setBuyPrice(nft.address, tokenId, 1 ETH)

Expected Results:

• setBuyPrice() L156 _autoAcceptOffer() to process the transactions same as the expectations of Case A;

Actual Results:

• The same actual results as Case A.

Root Cause

bidder acceptOffer will goes to L270 of NFTMarketOffer and calls _transferFromEscrow(nftContract, tokenId, offer.buyer, msg.sender):

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketOffer.sol#L246-L274

  function _acceptOffer(address nftContract, uint256 tokenId) private {
    Offer memory offer = nftContractToIdToOffer[nftContract][tokenId];

    // Remove offer
    delete nftContractToIdToOffer[nftContract][tokenId];
    // Withdraw ETH from the buyer's account in the FETH token contract.
    feth.marketWithdrawLocked(offer.buyer, offer.expiration, offer.amount);

    // Distribute revenue for this sale leveraging the ETH received from the FETH contract in the line above.
    (uint256 f8nFee, uint256 creatorFee, uint256 ownerRev) = _distributeFunds(
      nftContract,
      tokenId,
      payable(msg.sender),
      offer.amount
    );

    // Transfer the NFT to the buyer.
    try
      IERC721(nftContract).transferFrom(msg.sender, offer.buyer, tokenId) // solhint-disable-next-line no-empty-blocks
    {
      // NFT was in the seller's wallet so the transfer is complete.
    } catch {
      // If the transfer fails then attempt to transfer from escrow instead.
      // This should revert if the NFT is not in escrow of the `msg.sender` is not the owner of this NFT.
      _transferFromEscrow(nftContract, tokenId, offer.buyer, msg.sender);
    }

    emit OfferAccepted(nftContract, tokenId, offer.buyer, msg.sender, f8nFee, creatorFee, ownerRev);
  }

_transferFromEscrow(nftContract, tokenId, offer.buyer, msg.sender) will goes to L557 of NFTMarketReserveAuction and calls _finalizeReserveAuction(auctionId, false):

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketReserveAuction.sol#L527-L565

  function _transferFromEscrow(
    address nftContract,
    uint256 tokenId,
    address recipient,
    address seller
  ) internal virtual override {
    uint256 auctionId = nftContractToTokenIdToAuctionId[nftContract][tokenId];
    if (auctionId != 0) {
      ReserveAuction storage auction = auctionIdToAuction[auctionId];
      if (auction.endTime == 0) {
        // The auction has not received any bids yet so it may be invalided.

        if (auction.seller != seller) {
          // The account trying to transfer the NFT is not the current owner.
          revert NFTMarketReserveAuction_Not_Matching_Seller(auction.seller);
        }

        // Remove the auction.
        delete nftContractToTokenIdToAuctionId[nftContract][tokenId];
        delete auctionIdToAuction[auctionId];

        emit ReserveAuctionInvalidated(auctionId);
      } else {
        // If the auction has started, the highest bidder will be the new owner.

        if (auction.bidder != seller) {
          revert NFTMarketReserveAuction_Not_Matching_Seller(auction.bidder);
        }

        // Finalization will revert if the auction has not yet ended.
        _finalizeReserveAuction(auctionId, false);

        // Finalize includes the transfer, so we are done here.
        return;
      }
    }

    super._transferFromEscrow(nftContract, tokenId, recipient, seller);
  }

_finalizeReserveAuction(auctionId, false) will trasfer the NFT to the bidder only at L506 of NFTMarketReserveAuction.sol.

After _finalizeReserveAuction(auctionId, false), it goes back to L560 of NFTMarketReserveAuction.sol, which is a return, and then goes back to L273 of NFTMarketOffer.sol and that's the end of acceptOffer()。

As a result, the NFT will not be transferred from the bidder to the buyer, in spite of the buyer's ETH payment being paid at L255 of NFTMarketOffer.sol (_distributeFunds()).

https://github.com/code-423n4/2022-02-foundation/blob/4d8c8931baffae31c7506872bf1100e1598f2754/contracts/mixins/NFTMarketReserveAuction.sol#L490-L518

  function _finalizeReserveAuction(uint256 auctionId, bool keepInEscrow) private {
    ReserveAuction memory auction = auctionIdToAuction[auctionId];

    if (auction.endTime >= block.timestamp) {
      revert NFTMarketReserveAuction_Cannot_Finalize_Auction_In_Progress(auction.endTime);
    }

    // Remove the auction.
    delete nftContractToTokenIdToAuctionId[auction.nftContract][auction.tokenId];
    delete auctionIdToAuction[auctionId];

    if (!keepInEscrow) {
      /*
       * Save gas by calling core directly since it cannot have another escrow requirement
       * (buy price set or another auction listed) until this one has been finalized.
       */
      NFTMarketCore._transferFromEscrow(auction.nftContract, auction.tokenId, auction.bidder, address(0));
    }

    // Distribute revenue for this sale.
    (uint256 f8nFee, uint256 creatorFee, uint256 ownerRev) = _distributeFunds(
      auction.nftContract,
      auction.tokenId,
      auction.seller,
      auction.amount
    );

    emit ReserveAuctionFinalized(auctionId, auction.seller, auction.bidder, f8nFee, creatorFee, ownerRev);
  }

Recommendation

When there is an unfinalized ReserveAuction, consider preventing acceptOffer and makeOffer.

Lines of code

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

Vulnerability details

The protocol is charging a different fee rate when the NFT is sold by the creator for the first time on the platform, the fee rate is currently set to a constant value of 15%.

For other sales, the fee rate is only 5%.

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

if (isCreator && !_nftContractToTokenIdToFirstSaleCompleted[nftContract][tokenId]) {
    fee = PRIMARY_FOUNDATION_FEE_BASIS_POINTS;
} else {
    fee = SECONDARY_FOUNDATION_FEE_BASIS_POINTS;
}

However, since the smart contract is checking if the seller's address equals the creator's for, by:

1. using another address as the creator when minting the NFT;
2. transferring the NFT to another address;
3. listing through a very thin smart contract;

the 15% PRIMARY_FOUNDATION_FEE can effectively be bypassed.

Since the benefit of doing so is considerable (paying only 1/3 of the fees), while the cost is low. We would argue that this creator listing fee bypassing can and will eventually be implemented, for example, a smart contract listing agent with UI can be popular.

Recommendation

We don't see a way to prevent this higher fee for the creator mechanism from being bypassed.

Instead, we would recommend reversing the fee rate settings and making the creator pay a lower fee, and therefore, encourages creators to list their artwork on this platform rather than other platforms.

Lines of code

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

Vulnerability details

Based on our research, getRoyalties() is not a standardized API for NFT contracts to indicate how the royalties should be distributed among the recipients.

However, in the current implementation, it always assumes that getRoyalties() return in terms of BPS.

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

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

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

As a result, if a particular implementation is returning get Royalties() with higher precision (say 1e6 for 100% instead of 1e4/BPS), the distribution of royalties can be distorted.

PoC

Given:

• NFT-Token1 Royalties:

  • Address0 = 10,000 (1%)
  • Address1 = 10,000 (1%)
  • Address2 = 100,000 (10%)
  • Address3 = 880,000 (88%)

• Alice owns the NFT-Token1

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

2. Bob buy() with 10 ETH:

• foundationFee = 0.5 ETH
• creatorFee = 1 ETH
• ownerRev = 8.5 ETH

Since creatorShares[address2] > BASIS_POINTS (10,000), all the creatorFee will be sent to the first address: Address0, which is expected to receive only 1% of the royalties.

Recommendation

Consider removing this and change to:

// Determine the total shares defined so it can be leveraged to distribute below
uint256 totalShares;
unchecked {
  // The array length cannot overflow 256 bits.
  for (uint256 i = 0; i <= maxCreatorIndex; ++i) {
    // The check above ensures totalShares wont overflow.
    totalShares += creatorShares[i];
  }
}
if (totalShares == 0) {
  maxCreatorIndex = 0;
}

Lines of code

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

Vulnerability details

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

if (nftContract.supportsERC165Interface(type(IRoyaltyInfo).interfaceId)) {
  try IRoyaltyInfo(nftContract).royaltyInfo{ gas: READ_ONLY_GAS_LIMIT }(tokenId, BASIS_POINTS) returns (
    address receiver,
    uint256 /* royaltyAmount */
  ) {
    if (receiver != address(0)) {
      recipients = new address payable[](1);
      recipients[0] = payable(receiver);
      // splitPerRecipientInBasisPoints is not relevant when only 1 recipient is defined
      if (receiver == seller) {
        return (recipients, splitPerRecipientInBasisPoints, true);
      }
    }
  } catch // solhint-disable-next-line no-empty-blocks
  {
    // Fall through
  }
}

The current implementation of EIP-2981 support will always pass a constant BASIS_POINTS as the _salePrice.

As a result, the recipients that are supposed to receive less than 1 BPS of the salePrice may end up not receiving any royalties.

Furthermore, for the NFTs with the total royalties rate set less than 10% for some reason, the current implementation will scale it up to 10%.

Recommendation

1. Instead of passing a constant of 10,000 as the _salePrice, we suggest using the actual _salePrice, so there the royalties can be paid for recipients with less than 1 BPS of the royalties.
2. When the total royalties cut is lower than 10%, it should be honored. it's capped at 10% only when the total royalties cut is higher than 10%.

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;
}