Insufficient check are made in the setCollectionPhases function, which could lead to unusable collections

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

No check is being performed on the different parameters of a collection upon creation in the setCollectionPhases function.
This means collection with wrong parameters can be created and be potentially unusable, polluting the MinterContract.

For such collections, most of the core functions of the contract would be unusable. However this vulnerability is only marked as MEDIUM as the setCollectionPhases function can only be called by the collection admin, who can call it several time and fix the errors.

Proof of Concept

Let’s have a look at the setCollectionPhases function below

function setCollectionPhases(uint256 _collectionID, uint _allowlistStartTime, uint _allowlistEndTime, uint _publicStartTime, uint _publicEndTime, bytes32 _merkleRoot) public CollectionAdminRequired(_collectionID, this.setCollectionPhases.selector) {
        require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
        collectionPhases[_collectionID].allowlistStartTime = _allowlistStartTime;
        collectionPhases[_collectionID].allowlistEndTime = _allowlistEndTime;
        collectionPhases[_collectionID].merkleRoot = _merkleRoot;
        collectionPhases[_collectionID].publicStartTime = _publicStartTime;
        collectionPhases[_collectionID].publicEndTime = _publicEndTime;
    }

We can imagine a range of scenarios for a caller to create dysfunctional collections, among which:

• if _allowlistEndTime < _allowlistStartTime and _publicEndTime < _publicStartTime, the functions burnOrSwapExternalToMint and burnToMint will always revert.

• if _publicEndTime < _allowlistStartTime and collectionPhases[_collectionId].salesOption == 2), the getPrice function will return a fixed price even though the descending scale is set as exponential

• if _allowlistStartTime < collectionPhases[_collectionID].timePeriod, the mintAndAuction and the mint functions will revert if lastMintDate[_collectionID] == 0 as collectionPhases[col].allowlistStartTime - collectionPhases[col].timePeriod would underflow.

Tools Used

Manual Review / Visual Studio

Recommended Mitigation Steps

An easy fix would be to add a list of checks in the setCollectionPhases function using multiple require statements as per below.

function setCollectionPhases(uint256 _collectionID, uint _allowlistStartTime, uint _allowlistEndTime, uint _publicStartTime, uint _publicEndTime, bytes32 _merkleRoot) public CollectionAdminRequired(_collectionID, this.setCollectionPhases.selector) {
        require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
        require(_allowlistStartTime < _allowlistEndTime, “start after end”);
        require(_publicStartTime < _publicEndTime, “start after end”);
        require(_allowlistEndTime <= _publicStartTime, “allow list should happen prior to public”);
        require(_allowlistStartTime < collectionPhases[_collectionID].timePeriod, “allow list should be less than period”);
        collectionPhases[_collectionID].allowlistStartTime = _allowlistStartTime;
        collectionPhases[_collectionID].allowlistEndTime = _allowlistEndTime;
        collectionPhases[_collectionID].merkleRoot = _merkleRoot;
        collectionPhases[_collectionID].publicStartTime = _publicStartTime;
        collectionPhases[_collectionID].publicEndTime = _publicEndTime;
}

This would prevent all the scenarios listed above and ensure the contract does not get polluted with odd collections.

We should also ensure that the setCollectionCosts function cannot be called again after setting the phases, meaning we should add a require statement like so require(setMintingCosts[_collectionID] == true, "Set Minting Costs"); in the setCollectionCosts.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #1274

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #2033

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The Warden has described all potential scenarios that would fail if the time stamps have been misconfigured for a collection and has described it in great detail.

The main issue I observe is that the collection can be simply reconfigured to rectify the issue, meaning that a QA severity is better suited than a Medium severity.

I invite the wardens of this and all duplicate exhibits to elaborate on a potential attack path based on this misconfiguration (i.e. loss of funds, unauthorized access to sale type) to upgrade its severity.

[c4-judge]

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

[c4-judge]

alex-ppg marked the issue as grade-a

[c4-judge]

alex-ppg marked the issue as grade-b

[c4-judge]

alex-ppg marked the issue as not selected for report