HardlyDifficult
commented
2 years ago Invalid. It's correct that reentrancy is possible, and malicious NFTs could be listed for sale... however no actual vulnerabilities were identified. Without a real POC, this does not seem to be something that warrants a change -- the recommendations increase gas costs and it's not clear there's any value add from that.
HickupHH3
Lines of code
https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L170-L219 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/nftDropMarket/NFTDropMarketFixedPriceSale.sol#L118-L157
Vulnerability details
Impact
NFTDropMarket is open for all NFT, at least those implementing INFTDropCollectionMint interface. Some of them can be malicious. NFTDropMarket.mintFromFixedPriceSale() is a key accounting function, but it does not check these risk directly. It leans on its internal function, that they would work perfectly to defend.
There no defense from malicious NFT contracts, there is no reentracy guards, but NFT contract receives many external call during te sale from the market. This is a strong attack vector with not so much defending mechanics written. Dangerous to play with it and leave everything as it is.
But simple steps can be taken to be confident that the distribution of ETH is correct (ETH taken from buyers == ETH distributed)
Proof of Concept
1) Add malicious NFT contract to NFTDropMarket.createFixedPriceSale() 2) Buy these NFTs 3) Try to attack using many instruments available (reenter, output maliciously when Market calls NFT contract) to break the balance [ETH taken from buyers == ETH distributed]
Tools Used
Hardhat
Recommended Mitigation Steps
1) add reentrancy guard to NFTDropMarket.mintFromFixedPriceSale() 2) add simple direct checks that (ETH taken from buyers == ETH distributed)