Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

On a Linear or Exponential Descending Sale Model, a user that mint on the last block.timestamp mint at an unexpected price. #1275

Open
c4-submissions opened this issue Nov 12, 2023 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-04 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report

Comments

@c4-submissions
Copy link
Contributor

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:
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-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 12, 2023
c4-submissions added a commit that referenced this issue Nov 12, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #1391

@c4-judge
Copy link

c4-judge commented Dec 5, 2023

alex-ppg marked the issue as selected for report

@c4-judge c4-judge reopened this Dec 5, 2023
@c4-judge c4-judge added primary issue Highest quality submission among a set of duplicates selected for report This submission will be included/highlighted in the audit report labels Dec 5, 2023
@alex-ppg
Copy link

alex-ppg commented Dec 5, 2023

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.

@c4-judge
Copy link

c4-judge commented Dec 5, 2023

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 5, 2023
@C4-Staff C4-Staff added the M-04 label Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-04 primary issue Highest quality submission among a set of duplicates satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report
Projects
None yet
Development

No branches or pull requests

5 participants