If an airdrop happens before a mint the price could skyrocket

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L536

Vulnerability details

Impact

As explained by the docs, several steps can occur during the minting process. However, an airdrop before salesOption 3 can lead to price inflation.

Proof of Concept

Under salesOption 3, getPrice returns:

return collectionPhases[_collectionId].collectionMintCost
                    + ((collectionPhases[_collectionId].collectionMintCost / collectionPhases[_collectionId].rate) * gencore.viewCirSupply(_collectionId));

This is the increased rate based on the NFTs already in circulation. If an airdrop occurs before a mint with salesOption 3, the price will be much higher than intended.

Example:

Steps	Option	NFTs	Price	Rate
1 OG users	Airdropped	20 NFTs	free	-
2 Whitelisted	Sales 3	10 NFTs	1 ETH	10 (0.1 ETH increase)
3 Public	Sales 1	70 NFTs	2 ETH	-

With the current three steps, after the airdrop, salesOption 3 should start at 1 ETH and gradually increase to 2 ETH. Afterward, it should mint at a constant rate of 2 ETH.

However, when sales option 3 starts, getPrice will return 3 ETH instead of 1 ETH. This will cause the initial users to pay an inflated price, which was not intended by the owner and can harm their reputation. It's also unfair to the users, as these so-called special (whitelisted) users will pay increased prices.

$$ \begin{align*} \text{collectionMintCost} + \left(\frac{\text{collectionMintCost}}{\text{rate}}\right) \times \text{cirSupply} \\ &= 1 \text{eth} + 0.1 \text{eth} \times 20 \\ &= 1 \text{eth} + 2 \text{eth} \\ &= 3 \text{eth} \end{align*} $$

POC

Gist - https://gist.github.com/0x3b33/558b919a57101e7a0942e557a464078a
Add to remappings - contracts/=smart-contracts/
Run it with forge test --match-test test_airdrop --lib-paths ../smart-contracts

Tools Used

Manual review.

Recommended Mitigation Steps

You can implement a mapping with the airdropped NFTs and deduct this value from gencore.viewCirSupply(_collectionId) to avoid disrupting the minting process.

Assessed type

Error

[c4-pre-sort]

141345 marked the issue as duplicate of #643

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #881

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #246

[c4-judge]

alex-ppg marked the issue as selected for report

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

After further consideration, I have decided to merge several periodic mint-related findings into two separate categories:

• Incorrect Price Calculations (If an airdrop happens before a mint the price could skyrocket #381)
• Incorrect Enforcement of Periodic Mint Limitations (Multiple mints can brick any form of salesOption 3 mintings #380)

The reasoning behind this change is that both rely on different aspects of the code and have different root causes. The former highlights a flaw in the getPrice function and how it calculates prices, whilst the latter highlights a flaw in the mint function and how it calculates the lastMintDate. Fixing one does not infer that the other is fixed, reinforcing the idea that they are separate vulnerabilities.

I consider this submission to be of a lower severity than #380 correctly given that it can be rectified. Specifically, a lastMintDate update in the future per #2012 cannot be reversed, while an incorrect price as indicated in this and relevant submissions can be reversed by reconfiguring the collection and can be controlled by the user simply not willing to pay the inflated price until the price is corrected.

A problem when selecting the best report in this submission is that all Warden submissions make mention of "airdropped" tokens and fail to identify that the general circulating supply affects the price in their proposed remediation, such as #380. I have thus chosen this report as the best given that it cites the relevant documentation, contains a privately accessible valid PoC, and provides a basic representation of the pricing formula using mathematical notation to illustrate the problem.

[mcgrathcoutinho]

Hi @alex-ppg, here is why I believe this issue (and its duplicates) should be marked as a duplicate of #380 but with a partial grading.

1. The root cause is the same i.e. existing circulating supply causes sale model 3 to work incorrectly.
2. The impact mentioned here and that in Multiple mints can brick any form of salesOption 3 mintings #380 are two sides of the same coin.
3. The issue mentions that prices will skyrocket (which is true) but the issue also mentions that initial users to pay an inflated price, which is incorrect since the user cannot mint (pay) in the first place due to the lastMintDate being set to a date far ahead in the future.
4. I mention partial grading because although the root cause is correct, the impact demonstrated is invalid. Thus, according to the C4 final SC verdict in the docs, the issue should deserve a partial-grading, while being marked as a dup of Multiple mints can brick any form of salesOption 3 mintings #380 due to the root cause being the same.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for contributing! The root cause is not the presence of the circulating supply, it is the incorrect price calculation of a sale model 3. While it may make sense to carry over sale model 3 sales across different periods, it does not make sense to carry over mint restrictions. For example:

• Collection A runs a periodic sale, amassing 10 NFTs sold, and reaching a price of 10 ETH per NFT
• Collection A performs an airdrop as part of an incentive program
• Collection A opens up the periodic sale for a public run, wanting to maintain the 10 ETH per NFT price

To fix this:

• If an airdrop happens before a mint the price could skyrocket #381 would need to update its price calculation to use solely the circulating supply minted via the periodic sale.
• Multiple mints can brick any form of salesOption 3 mintings #380 would need to reset its lastMintDate entirely

A fix for #380 does not fix #381 and vice versa. Both concern different parts of the code and require different alleviations. Arguing that #381 is not possible due to #380 existing in the codebase is not valid reasoning; each submission is treated as a "black box" independently assuming that all other code works as intended.

Based on the above, I will maintain these submissions as separate.