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()
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()