NextGen - pipidu83's results

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