Platform: Code4rena
Start Date: 11/08/2022
Pot Size: $40,000 USDC
Total HM: 8
Participants: 108
Period: 4 days
Judge: hickuphh3
Total Solo HM: 2
Id: 152
League: ETH
Rank: 65/108
Findings: 1
Award: $42.83
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: itsmeSTYJ
Also found by: 0x1f8b, 0x52, 0xDjango, Ch_301, Chom, KIntern_NA, PwnedNoMore, Treasure-Seeker, auditor0517, byndooa, cccz, csanuragjain, ladboy233, nine9, shenwilly, thank_you, yixxas, zkhorse
42.8343 USDC - $42.83
It is possible to bypass the saleConfig.limitPerAccount set by the creator by transferring the NFTs out. For highly sought after NFT drops, a single smart contract can buy out the entire drop simply by calling mintFromFixedPriceSale then transferring the NFTs out and repeating the process multiple times.
Modify the FixedPriceDrop.sol foundry test with the following changes.
diff --git a/FixedPriceDrop.sol.orig b/FixedPriceDrop.sol index 0a6d698..56808f8 100644 --- a/FixedPriceDrop.sol.orig +++ b/FixedPriceDrop.sol @@ -71,14 +71,26 @@ contract TestFixedPriceDrop is Test { /** List for sale **/ uint80 price = 0.5 ether; - uint16 limitPerAccount = 10; + uint16 limitPerAccount = 3; vm.prank(creator); nftDropMarket.createFixedPriceSale(address(nftDropCollection), price, limitPerAccount); /** Mint from sale **/ uint16 count = 3; vm.deal(collector, 999 ether); - vm.prank(collector); + vm.startPrank(collector); + nftDropMarket.mintFromFixedPriceSale{ value: price * count }(address(nftDropCollection), count, payable(0)); + + // Check that available count for collector is 0 + uint256 remaining = nftDropMarket.getAvailableCountFromFixedPriceSale(address(nftDropCollection), collector); + assertEq(remaining, 0); + + // Transfer all bought NFTs out + nftDropCollection.transferFrom(collector, address(5), 1); + nftDropCollection.transferFrom(collector, address(5), 2); + nftDropCollection.transferFrom(collector, address(5), 3); + + // Buy 3 more NFT nftDropMarket.mintFromFixedPriceSale{ value: price * count }(address(nftDropCollection), count, payable(0)); } }
Manual analysis, foundry
#0 - HardlyDifficult
2022-08-17T21:11:17Z
ACK.
This is accurate! We had several meetings about this concern while building the contract, ultimately deciding to move forward with this approach knowing that it has limitations. The trouble is every limit solution suggested and used in the wild today can be gamed, it's just varying levels of friction for an attacker to work around it. Once someone has coded up a workaround, it could easily be used on any of the collections being sold by our marketplace. So we decided to KISS.
But if it can be gamed, why include a limit at all? Creators want one. It sets expectations with the community and makes the sale feel more fair. Many users will respect the limit as communicated - we suspect more often than not, this simple limit check will be sufficient.
What if it's not sufficient? If someone were to clearly abuse the system it may degrade the value of the collection for all. There are options available to the creator at that point. For example, the creator could create a new collection to replace the original - airdropping NFTs to their legit holders, or allowing them to do an NFT swap (so the original collection can slowly be removed from circulation) -- this swap could also have a deny list so that the abused sales cannot be used to redeem from the new collection, and presumably the original collection will quickly lose value so long as the creator's community is on board with this process. Or the creator and their community could choose to simply accept that the sale went down this way and wait for things to balance out again on the secondary market.
I selected this instance as the primary submission for having a simple & clear coded POC.
We agree Medium risk is appropriate for this since it could "leak value with a hypothetical attack path with stated assumptions"