Decrease rate for Exponential Descending Sales is always zero

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

There is an error in the calculation that makes the decrease rate to always be zero when calculating the price of exponential descending mintings.

This report is aware about the comment on the Readme regarding rounding errors, but it is not related to the finding being presented here, as this is regardless

We are aware of the price rounding errors when the Exponential Descending Sales Model is used and the minting cost is low, thus any finding on this is not valid.

Impact

Exponential Descending Sales will always be priced at their initial price, which is the highest expected for this kind of sale.

This will affect all NFTs sales using this model, discouraging all sales, as the price will always be at its maximum.

Proof of Concept

If the decrease rate is zero for exponential descending sales, it will always return the maximum price, which is used for token minting/sales:

• MinterContract.sol#L551
• MinterContract.sol#L559-L560

The formula is defined as:

    tDiff = (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) / collectionPhases[_collectionId].timePeriod;

    decreaserate = ((price - (collectionPhases[_collectionId].collectionMintCost / (tDiff + 2))) / collectionPhases[_collectionId].timePeriod) * ((block.timestamp - (tDiff * collectionPhases[_collectionId].timePeriod) - collectionPhases[_collectionId].allowlistStartTime));

Let's replace the variables with easier to read ones:

    tDiff = (NOW - START_TIME) / TIME_PERIOD;

    decreaserate = ((price - (MINT_COST / (tDiff + 2))) / TIME_PERIOD) * ((NOW - (tDiff * TIME_PERIOD) - START_TIME));

We'll show how the right-most term is zero: ((NOW - (tDiff * TIME_PERIOD) - START_TIME)) == 0, meaning that regardless of the rest, decreaserate will be zero.

In other words:

    tDiff = (NOW - START_TIME) / TIME_PERIOD;

    decreaserate = X * (
        (NOW - (tDiff * TIME_PERIOD) - START_TIME) // @audit this is always 0
    );

Let's verify how NOW - (tDiff * TIME_PERIOD) - START_TIME is always zero, by replacing tDiff in the equation:

    NOW - (((NOW - START_TIME) / TIME_PERIOD) * TIME_PERIOD) - START_TIME

The TIME_PERIOD term can be canceled:

    NOW - (((NOW - START_TIME))) - START_TIME

Removing the parenthesis (and operating the substraction):

    NOW - NOW + START_TIME - START_TIME == 0;

We're mindful of Solidity precision errors, but as shown above, this is not the case here, but an error in the calculations making the decrease rate be always zero, or towards zero at most.

Tools Used

Manual Review

Recommended Mitigation Steps

Review the implemented formula to implement an exponential descending model correctly. Separate internal calculations to make clear what they actually do. Here's a debate on how ENS does it: https://discuss.ens.domains/t/exponential-price-decay-contract/10125.

Assessed type

Math

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

price changes based on this price = collectionPhases[_collectionId].collectionMintCost / (tDiff + 1); so if decreaserate = 0 it does not return the collectionmintingcost.

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[alex-ppg]

The Warden specifies that prices are incorrectly calculated, however, they apply a calculation elimination that is incompatible with Solidity due to mathematical truncation.

Specifically, the statement a / b * b == a while valid in mathematics is invalid here because the a / b calculation will truncate if a % b != 0, which is highly likely in the referenced calculation.

As such, I consider this exhibit to be invalid.

EDIT: After further analyzing the code, my verdict stands. Apart from the numerator and denominator nullification that was incorrectly carried out, the base price calculation is price, and the decreaserate refers to the in-period decrease. To illustrate:

• price: The price per exact equality of a period (so the price at the start of period 1, the price at the start of period 2, and so on)
• decreaserate: The amount by which the price should further decrease to indicate the progress of moving between periods

As such, a decreaserate of 0 is valid in the referenced timestamp scenario as the price calculation by itself is correct.

Sure, there may be a better way to represent the calculations but they are presently correct.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid