No sanitiy check on allowlist and public phases, potential fixed price for salesOption == 2

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L540

Vulnerability details

Impact

In the case where collectionPhasesDataStructure.allowlistEndTime is set to a value higher than collectionPhasesDataStructure.publicEndTime, the getPrice function will return collectionPhasesDataStructure.collectionMintCost instead of the adjusted decreasing price, resulting in an excessive cost for the NFT minter.

Proof of Concept

The documentation indicates that sales models can be combined with phased allowlists to execute highly complex drops. This includes phases with one or several optional allowlist sales, as well as optional public sales.
Given the flexibility of the contract, to avoid a public sale, setCollectionPhases should be fed with _publicStartTime == _publicEndTime.

If salesOption == 2 and, either by mistake or intentionally, publicEndTime is set to a past value or simply to a value of 0, the getPrice function will return a fixed price collectionPhases[_collectionId].collectionMintCost instead of the adjusted price for the descending sale.

The CollectionAdmin calls setCollectionPhases prior to minting but also for each update of the merkleRoot, increasing the probability of human error.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Implement two distinct functions to set up Public sales (setCollectionPhases) and Allowlist Sales (setAllowlistPhases) to avoid confusion, redundancy and reduce the risk of error.

2. Add sanity checks to ensure that collectionPhasesDataStructure.allowlistEndTime is less than or equal to collectionPhasesDataStructure.publicStartTime, and that collectionPhasesDataStructure.publicStartTime is less than or equal to collectionPhasesDataStructure.publicEndTime.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #1391

[c4-judge]

alex-ppg marked the issue as not a duplicate

[alex-ppg]

The Warden specifies that the price function will yield an incorrect value if the allowlistEndTime is specified and the publicEndTime is lower than the allowlistEndTime.

Based on the fact that the documentation does not warn against this (it only warns for the StartTime values) and that the function is callable by the collection administrator rather than the NextGen team, I consider this to be a valid QA (L) finding.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-c