c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Impact

A user mint a token at an incorrect price and losing funds if he mint on the last authorized block.timestamp during a Linear or Exponential Descending Sale Model.

Proof of Concept

On a Linear or Exponential Descending Sale Model, the admin set the collectionMintCost and the collectionEndMintCost. In context of these sale models, the collectionMintCost is the price for minting a token at the beginning and the collectionEndMintCost the price at the end of the sale.

The minting methods use the function MinterContract::getPrice() to compute the correct price at the actual timing, check the function with the branch for a Linear or Exponential Descending Sale Model:

    function getPrice(uint256 _collectionId) public view returns (uint256) {
        uint tDiff;
        //@audit If Periodic Sale
        if (collectionPhases[_collectionId].salesOption == 3) {
            ...
        } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){

            tDiff = (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) / collectionPhases[_collectionId].timePeriod;
            uint256 price;
            uint256 decreaserate;
            //@audit If Exponential Descending Sale
            if (collectionPhases[_collectionId].rate == 0) {
                price = collectionPhases[_collectionId].collectionMintCost / (tDiff + 1);
                decreaserate = ((price - (collectionPhases[_collectionId].collectionMintCost / (tDiff + 2))) / collectionPhases[_collectionId].timePeriod) * ((block.timestamp - (tDiff * collectionPhases[_collectionId].timePeriod) - collectionPhases[_collectionId].allowlistStartTime));
            //@audit If Linear Descending Sale
            } else {
                if (((collectionPhases[_collectionId].collectionMintCost - collectionPhases[_collectionId].collectionEndMintCost) / (collectionPhases[_collectionId].rate)) > tDiff) {
                    price = collectionPhases[_collectionId].collectionMintCost - (tDiff * collectionPhases[_collectionId].rate);
                } else {
                    price = collectionPhases[_collectionId].collectionEndMintCost;
                }
            }
            if (price - decreaserate > collectionPhases[_collectionId].collectionEndMintCost) {
                return price - decreaserate; 
            } else {
                return collectionPhases[_collectionId].collectionEndMintCost;
            }
        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
    }

We can see that if the collectionPhases[_collectionId].salesOption == 2 (it's the number for a descending sale model), and if the block.timestamp is > allowlistStartTime and < publicEndTime. The price is correctly computed. A little check on the mint() function:

    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
        ...
        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
              ...
        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
    }

But if the publicEndTime is strictly equal to publicEndTime, the returned price is the collectionMintCost instead of collectionEndMintCost because the logic go to the "else" branch. It's an incorrect price because it's the price at the beginning of the collection. And as you can see on mint(), a user can mint a token on the block.timestamp publicEndTime.

User that mint on the last block.timstamp mint at an unexpected price and for all the minting methods which use the getPrice() function.

Logs

You can see the logs of the test with this image:

And a link of a gist if you want to execute the PoC directly. https://gist.github.com/AxelAramburu/c10597b5ff60616b8a15d091f88de8da And you can execute the test with this command:

forge test --mt testgetWrongPriceAtEnd -vvv (or with -vvvvv to see all the transaction logs)

Tools Used

Manual Review

Recommended Mitigation Steps

Change the < & > to <= & => on the else if branch inside the getPrice() function.

    function getPrice(uint256 _collectionId) public view returns (uint256) {
        uint tDiff;
        //@audit-info If Periodic Sale
        if (collectionPhases[_collectionId].salesOption == 3) {
            ...
        -- } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && -- block.timestamp < collectionPhases[_collectionId].publicEndTime){
        ++ } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp => collectionPhases[_collectionId].allowlistStartTime && ++ block.timestamp <= collectionPhases[_collectionId].publicEndTime){

            tDiff = (block.timestamp - collectionPhases[_collectionId].allowlistStartTime) / collectionPhases[_collectionId].timePeriod;
            uint256 price;
            uint256 decreaserate;
            //@audit If Exponential Descending Sale
            if (collectionPhases[_collectionId].rate == 0) {
                price = collectionPhases[_collectionId].collectionMintCost / (tDiff + 1);
                decreaserate = ((price - (collectionPhases[_collectionId].collectionMintCost / (tDiff + 2))) / collectionPhases[_collectionId].timePeriod) * ((block.timestamp - (tDiff * collectionPhases[_collectionId].timePeriod) - collectionPhases[_collectionId].allowlistStartTime));
            //@audit If Linear Descending Sale
            } else {
                if (((collectionPhases[_collectionId].collectionMintCost - collectionPhases[_collectionId].collectionEndMintCost) / (collectionPhases[_collectionId].rate)) > tDiff) {
                    price = collectionPhases[_collectionId].collectionMintCost - (tDiff * collectionPhases[_collectionId].rate);
                } else {
                    price = collectionPhases[_collectionId].collectionEndMintCost;
                }
            }
            if (price - decreaserate > collectionPhases[_collectionId].collectionEndMintCost) {
                return price - decreaserate; 
            } else {
                return collectionPhases[_collectionId].collectionEndMintCost;
            }
        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }
    }

Assessed type

Invalid Validation

c4-pre-sort commented 12 months ago

141345 marked the issue as duplicate of #1391

c4-judge commented 11 months ago

alex-ppg marked the issue as selected for report

alex-ppg commented 11 months ago

The Warden specifies an inconsistency within the code whereby the price calculation function will misbehave when measured for a sale type of 2 combined with the block.timestamp being the publicEndTime of the collection's sale period.

The finding is correct given that the relevant mint functions in the MinterContract perform an inclusive evaluation for both the allowlistStartTime and the publicEndTime, permitting this vulnerability to manifest when the block.timestamp is exactly equal to the publicEndTime.

The Sponsor has confirmed this in #1391 and I accept the medium severity classification based on the submission's likelihood being low (due to the strict block.timestamp requirement) but the impact being high as an NFT would either be bought at a discount an arbitrary number of times, hurting the artist, or at a high markup, hurting the buyer.

The Warden's submission was selected as the best due to the presence of a PoC with logs that properly emphasize how the issue can be exacerbated depending on the configuration of a sale and its recommended mitigation being sufficient in rectifying the vulnerability.