code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L209 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L230 https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/collections/SequentialMintCollection.sol#L73-L78

Vulnerability details

Impact

The NFTDropMarket via the NFTDropMarketFixedPriceSale mixin has a nftContractToFixedPriceSaleConfig mapping used to track NFT collections.

The NFT collection is allowed to self-destruct via a call to NFTCollection.selfDestruct or NFTDropCollection.selfDestruct.

When a collection is destroyed, it's critical that the NFTDropMarket is updated such that it clears the nftContractToFixedPriceSaleConfig mapping of the destructed contract. Otherwise, if a user decides to create a collection with the same address, the destructed contract info within nftContractToFixedPriceSaleConfig mapping references the destructed contract and not the new contract.

This can cause de-syncing issues as well as preventing an admin from not being able to create a fixed sale price for the NFT collection since the following check below will result in a revert if the destroyed contract already has a fixed price.

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

Proof of Concept

Self-destruction calls in NFTCollection and NFTDropCollections:

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTDropCollection.sol#L209

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/NFTCollection.sol#L230

Internal self-destruct function SequentialMintCollection._selfDestruct:

https://github.com/code-423n4/2022-08-foundation/blob/792e00df429b0df9ee5d909a0a5a6e72bd07cf79/contracts/mixins/collections/SequentialMintCollection.sol#L73-L78

NFTCollectionFactory can create NFT collections with same address assuming the previous contract is destroyed and the same nonce is used by the msg.sender:

https://github.com/code-423n4/2022-08-foundation/blob/main/contracts/NFTCollectionFactory.sol#L257

Tools Used

Text editor

Recommended Mitigation Steps

When destroying a NFT collection, the NFTDropMarket mapping nftContractToFixedPriceSaleConfig should delete the mapping of the contract address.

HardlyDifficult commented 2 years ago

ACK.

We believe this is low risk. This seems like something we need to ensure creators are aware of.

We have architected these contracts to be completely independent / stand alone. Someone could use our NFT factory to create a collection and then a custom contract or another platform to sell it. Or they could use a custom collection contract and then sell it via our NFTDropMarket. This separation of concerns creates standalone components that creators can choose to use separately, or together.

Given this separation, there is no clear way for the collection to know where it's been listed at. And for custom collections, it may be more challenging to ensure the market is made aware of actions like this.

We also think the current flow can be a feature for some. Say the creator has made a collection, defined the sale terms, started advertising the collection... but then they notice a typo in their symbol. They could quickly self destruct and recreate the collection with the same address to fix the mistake - and links to the sale already provided would still be correct.

HickupHH3 commented 2 years ago

Good point raised by the warden!

The problem is that the issue applies not only to the foundation NFT collections, but any collection with this selfdestruct + create2 = resurrection ability. Hence, adopting the recommendation won't entirely solve the problem. However, it's a reasonable assumption that majority of the NFT collections to be sold are created by the foundation templates.

A solution would be to perform the sale on other marketplaces if the current sale terms are to be modified as well, for the resurrected contract. Or create a new one.

Downgrading to QA because it's an issue with state handling.