search

code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

If an airdrop happens before a mint the price could skyrocket #381

Open c4-submissions opened 11 months ago

c4-submissions commented 11 months ago

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 commented 11 months ago

141345 marked the issue as duplicate of #643

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

c4-judge commented 10 months ago

alex-ppg marked the issue as duplicate of #881

c4-judge commented 10 months ago

alex-ppg marked the issue as not a duplicate

c4-judge commented 10 months ago

alex-ppg marked the issue as duplicate of #246

c4-judge commented 10 months ago

alex-ppg marked the issue as selected for report

c4-judge commented 10 months ago

alex-ppg marked the issue as satisfactory

alex-ppg commented 10 months ago

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

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 commented 10 months ago

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 #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 #380 due to the root cause being the same.

Thank you for taking the time to read this.

alex-ppg commented 10 months ago

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:

To fix this:

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.