`mintFromFixedPriceSale` for a custom contract can lead to users losing funds

[code423n4]

Lines of code

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

Vulnerability details

mintFromFixedPriceSale for a custom contract can lead to users losing funds

NFTDropMarketFixedPriceSale.createFixedPriceSale allows creators to create a sale drop. A creator can create a drop sale for their custom NFT Contract that adheres to INFTDropCollectionMint.

INFTDropCollectionMint.mintCountTo must return the firstTokenId being minted, but it is not clear as to what should be returned upon all tokens being minted. A valid implementation could for instance return 0 if called after the last token has been minted.

But the drop market expects the call to mintCountTo to revert upon the last token being minted, meaning a user calling it afterwards would lose the ETH they sent.

Impact

Medium

Proof Of Concept

• Alice creates a customERC721 contract adhering to INFTDropCollectionMint. She writes mintCountTo() so that it returns 0 if called when all the tokens have been minted.
• The sale happens and collectors call mintFromFixedPriceSale until all the tokens have been minted.
• Bob now calls mintFromFixedPriceSale. Because all the tokens have been minted, the call to mintCountTo does not revert but returns 0.
• The function call then proceeds to distribute the funds.
• Bob have lost mintCost ETH, while not receiving any NFT.

You can find this foundry test reproducing the issue.

Note that this is not an issue of a malicious creator rugging collectors with a malicious implementation: they have implemented their contract to adhere to INFTDropCollectionMint, and the sale went as expected.

It is not unrealistic to imagine collectors monitoring CreateFixedPriceSale and calling mintFromFixedPriceSale based on it. In this case, all the mintFromFixedPriceSale processed after the last token being minted would lead to loss of funds.

Tools Used

Manual Analysis, Foundry

Mitigation

You can 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")

You can also specify in INFTDropCollectionMint that mintCountTo must revert if called after all tokens have been minted.

[ghost]

~You shouldn't make your PoC public as some of these contracts may already be live on mainnet.~

[joestakey]

The gist is secret so only people that can access this repo - which is private - can see the link. Is it not the correct way to link PoCs?

[ghost]

Ah whoops, must have saw wrongly 😅 You're all good!

[HardlyDifficult]

Although this submission uses a different POC, we believe it's the same issue & root cause as https://github.com/code-423n4/2022-08-foundation-findings/issues/211

Dupe of https://github.com/code-423n4/2022-08-foundation-findings/issues/211

[joestakey]

I will argue this issue is actually different than #211, although they both come from the same function call:

• in #211, the issue lies in the logic performed in INFTDropCollectionMint.mintCountTo(), more precisely the fact that a malicious implementation can perform incorrect state logic, which results in any collector calling NFTDropMarketFixedPriceSale.mintFromFixedPriceSale losing funds without receiving the expected amount of NFTs.
• here, the issue lies in the return value of INFTDropCollectionMint.mintCountTo() in an edge case - when all the tokens have been minted. There is no malicious implementation or wrong state logic: users calling NFTDropMarketFixedPriceSale.mintFromFixedPriceSale will receive the expected amount of NFTs. The problem is when the minting is done: the DropMarket expect subsequent calls to mintCountTo() to fail. While you can argue not reverting after the final token has been minted is breaking a semantic requirement, it still complies with the interface INFTDropCollectionMint. Not reverting on failure is a behavior that exists in other standards, such as some ERC20 tokens for instance, like ZRX.

To illustrate the difference between the two issues, take the NFT contract used in the PoC for this issue: users calling NFTDropMarketFixedPriceSale.mintFromFixedPriceSale will receive the expected amount of NFTs - ie it is not affected by the issue #211. The problem arises only upon the final token being minted.

In summary, #211 is about malicious implementations that users should be made aware of (docs or UI warnings) while this issue has to do with the fact INFTDropCollectionMint.mintCountTo() should define a stricter behavior when the last token has been minted, perhaps by adding a comment such as:

/// if the minting fails it MUST result in the transaction being reverted.

[joestakey]

@HickupHH3 thoughts?

I will argue this issue is actually different than #211, although they both come from the same function call:

• in #211, the issue lies in the logic performed in INFTDropCollectionMint.mintCountTo(), more precisely the fact that a malicious implementation can perform incorrect state logic, which results in any collector calling NFTDropMarketFixedPriceSale.mintFromFixedPriceSale losing funds without receiving the expected amount of NFTs.
• here, the issue lies in the return value of INFTDropCollectionMint.mintCountTo() in an edge case - when all the tokens have been minted. There is no malicious implementation or wrong state logic: users calling NFTDropMarketFixedPriceSale.mintFromFixedPriceSale will receive the expected amount of NFTs. The problem is when the minting is done: the DropMarket expect subsequent calls to mintCountTo() to fail. While you can argue not reverting after the final token has been minted is breaking a semantic requirement, it still complies with the interface INFTDropCollectionMint. Not reverting on failure is a behavior that exists in other standards, such as some ERC20 tokens for instance, like ZRX.

To illustrate the difference between the two issues, take the NFT contract used in the PoC for this issue: users calling NFTDropMarketFixedPriceSale.mintFromFixedPriceSale will receive the expected amount of NFTs - ie it is not affected by the issue #211. The problem arises only upon the final token being minted.

In summary, #211 is about malicious implementations that users should be made aware of (docs or UI warnings) while this issue has to do with the fact INFTDropCollectionMint.mintCountTo() should define a stricter behavior when the last token has been minted, perhaps by adding a comment such as:

/// if the minting fails it MUST result in the transaction being reverted.

[HickupHH3]

Apologies @joestakey, didn't receive a notification about the comment till now.

The root cause for both issues is about the "potential errors in the implementation or misunderstanding of the interface requirements". Simply put, the ambiguity regarding the specification of mintCountTo() allows for it to be exploited. As you pointed out, #211 is exploited by malicious implementations while this issue happens even if the implementation is seemingly compliant to the interface because of the ambiguity.

It's a tough decision because while the methods are different, the root cause and consequence (users losing funds) are the same.

I'll side with you on this one, because the attack vectors are quite distinct. It's similar to how I separated the strategist rug vectors for the Rubicon contest.