Malicious Creator can steal from collectors upon minting with a custom NFT contract

[code423n4]

Lines of code

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

Vulnerability details

Malicious Creator can steal from collectors upon minting with a custom NFT contract

In the case of a fixed price sale where nftContract is a custom NFT contract that adheres to INFTDropCollectionMint, a malicious creator can set a malicious implementation of INFTDropCollectionMint.mintCountTo() that would result in collectors calling this function losing funds without receiving the expected amount of NFTs.

Impact

Medium

Proof Of Concept

Here is a foundry test that shows a fixed price sale with a malicious NFT contract, where a collector pays for 10 NFTs while only receiving one. It can be described as follow:

• A creator creates a malicious nftContract with mintCountTo minting only one NFT per call, regardless of the value of count

• The creator calls NFTDropMarketFixedPriceSale.createFixedPriceSale() to create a sale for nftContract, with limit set to 15.

• Bob is monitoring the CreateFixedPriceSale event. Upon noticing CreateFixedPriceSale(customERC721, Alice, price, limit), he calls NFTDropMarketFixedPriceSale.mintFromFixedPriceSale(customERC721, count == 10,). He pays the price of count = 10 NFTs, but because of the logic in mintCountTo, only receives one NFT.

Note that mintCountTo can be implemented in many malicious ways, this is only one example. Another implementation could simply return firstTokenId without performing any minting.

Tools Used

Manual Analysis, Foundry

Mitigation

The problem here lies in the implementation of INFTDropCollectionMint(nftContract).mintCountTo(). You could add an additional check in NFTDropMarketFixedPriceSale.mintCountTo() using ERC721(nftContract).balanceOf().

+ uint256 balanceBefore = IERC721(nftContract).balanceOf(msg.sender);
207:     firstTokenId = INFTDropCollectionMint(nftContract).mintCountTo(count, msg.sender);
+ uint256 balanceAfter = IERC721(nftContract).balanceOf(msg.sender);
+ require(balanceAfter == balanceBefore + count, "minting failed")

[ghost]

This assumes a custom NFT contract with a bad implementation of mintCountTo which may be a stretch but I agree that your mitigation steps should be added as a sanity check.

[HardlyDifficult]

We will be making the recommended change.

There's not really anything we can do to completely stop malicious contracts - this is an inherit risk with NFT marketplaces. Even the recommended solution here is something a malicious contract could fake in order to bypass that requirement.

What sold us on making a change here was not malicious creators / contracts but instead potential errors in the implementation or misunderstanding of the interface requirements our marketplace expects. To prevent these errors, we are introducing the recommended change (and it only added 1,300 gas to the mint costs!)