Foundation Drop contest - byndooa's results

Lines of code

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

Vulnerability details

Impact

It is designed to cap one buys per account by limitperAccount configured during when sale is created. It should revert with NFTDropMarketFixedPriceSale_Cannot_Buy_More_Than_Limit() if (IERC721(nftContract).balanceOf(msg.sender) + count > saleConfig.limitPerAccount) Thus to pass this check IERC721(nftContract).balanceOf(msg.sender) should be minimized. It is very easy to do in one transaction, if buyer is a smart-contract (buy->transfer->buy->transfer) As the result, anyone can buy as much NFT as he wish, even all the collection. But the seller expect that the limit works.

Proof of Concept

Steps - in one transaction from a smart-contract:

1. Buy one
2. Transfer this one somewhere
3. Buy new one
4. Transfer again ...and so one N) Transfer all to the one EOA Result - as many tokens per one account as desired.

Tools Used

Hardhat

Recommended Mitigation Steps

Options:

1. delete this input, accept that it not possible to limit buys per user
2. only EOA should buy
3. block sales if tx.origin does not change
4. block transfers when sale is on
5. something more smart, it is many option possible

Lines of code

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

Vulnerability details

Impact

In the https://os.foundation.app/docs/creator-tools/drop we can outline that:

1. any NFT can be added to sale, not only deployed by the NFTCollectionFactory (NFTDropMarket.createFixedPriceSale() is correct here, it accept any NFT, at least those implementing INFTDropCollectionMint interface)
2. The market itself is "A collection primitive for delegated minting" So it is a market's role to arrange minting for payments.

NFTDropMarket perfectly takes payments from buyers, but do not check if the mint is successful after the payment. Mints will be ok if NFT contracts are deployed through NFTCollectionFactory.sol, but for malicious NFT it is not a guarantee. Check for interface implemented are not enough.

Proof of Concept

NFTDropMarket function only checks that the NFT implements INFTDropCollectionMint interface. But for malicious NFT contracts it is still too many options to write something bad - like mining 0 NFTs, after taking payments on NFTDropMarket. So the steps are:

1. add malicious NFT contract to sale, through NFTDropMarket.createFixedPriceSale()
2. it is listed
3. when buyers trigger NFTDropMarket.mintFromFixedPriceSale(), ETH is taken perfectly from buyers, then distributed, but malicious NFT contract can react as it desires, like not minting NFTs.

Tools Used

Hardhat

Recommended Mitigation Steps

in NFTDropMarket.mintFromFixedPriceSale() check that the mint happened (like balanceOf check, or anything else) Or consider additional checks when adding NFTs to sales in NFTDropMarket.createFixedPriceSale()