Foundation contest - 0xliumin's results

This is a great find!

The impact of this bug is:

• Bidder's funds are stuck in escrow in an unrecoverable way without an upgrade, and even with an upgrade it would have been non-trivial to offer a migration path to recover the funds (but it would have been possible to recover correctly).
• It allows sellers to stop the clock and/or back out of an auction. Normally once a bid is received we do not allow the seller to cancel the auction. With this bug, they could have created a new auction and then cancel that in order to back out of the deal entirely. This violates trust with collectors.

We have fixed this problem by adding the following code to createReserveAuction:

    // This check must be after _transferToEscrow in case auto-settle was required
    if (nftContractToTokenIdToAuctionId[nftContract][tokenId] != 0) {
      revert NFTMarketReserveAuction_Already_Listed(nftContractToTokenIdToAuctionId[nftContract][tokenId]);
    }

Thanks for the feedback!

• Architecture: Another user suggested something very similar. It is very tempting to go that route, it does seem like it would clean up the code and offer a clear separation of responsibilities. However it may prevent us for using efficient slot packing and would be a very significant change to make shortly before our launch.
• Use safeTransferFrom: This is a common recommendation but it does increase exposure to potential reentrancy issues. We do use nonReentrant for many calls, but we'd still like to limit external calls where possible just in case the modifier was missed in one of our flows. I'm not currently aware of a good use case which would require this -- but it is certainly something we are considering adding in the future.
• Large unchecked blocks: We have comments around the usage of unchecked throughout the code. If there are any which are unsafe or not documented, we should look into those closer. In general, the reason for the large unchecked blocks is usually due to limitations with Solidity. For instance, in order to uncheck the ++i portion of a for loop, we need to wrap the entire loop as unchecked since Solidity does not yet have a way of scoping the unchecked section in-line. Once that capability is added, we will clean up the code to make unchecked blocks more targeted and easier to reason about.
• Incorrect comments: This file is listed as out of scope -- however you are correct! This was a copy paste error in the comments and we have corrected them.

Unadjusted score: 55 (inclidung 30 points for the extensive architecture recommendations).

Extra 5 points from #24

Extra 5 points from #22.