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

User may get a bad price when minting in a Descending Sale #438

Closed
c4-submissions opened this issue Nov 7, 2023 · 2 comments
Closed

User may get a bad price when minting in a Descending Sale #438

c4-submissions opened this issue Nov 7, 2023 · 2 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 duplicate-1275 edited-by-warden partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Nov 7, 2023

Lines of code

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

Vulnerability details

Impact

At each time period of a Descending Sale, the minting cost decreases exponentially or linearly until it reaches its resting price (ending minting cost) and stays there until the end of the minting phase. However, user may get a price at collectionMintCost (starting price) when minting in a Descending Sale if the token is minted at the end of the sale period.

Proof of Concept

User can mint collection item during the public sale period:

        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {

When getting the minting price of collection, protocol will first check salesOption, if salesOption is 2 (Descending Sale), then protocol compares block.timestamp with publicEndTime.

Only if block.timestamp is less than publicEndTime, protocol will calculate the minting price based on Descending Sale Model:

        } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){

If block.timestamp is equal to publicEndTime, the minting price will be collectionMintCost, which would be much higher than collectionEndMintCost:

        } else {
            // fixed price
            return collectionPhases[_collectionId].collectionMintCost;
        }

Please see below test case and run it in nextGen.test.js:

it("#getPriceCol4AtPulicEndTime", async function() {
      await time.increaseTo(1796931278)
      const price = await contracts.hhMinter.getPrice(
        4, // _collectionID
      )

      expect(parseInt(price)).equal(1000000000000000000);
    })

It may not seems like a high chance that the token is minted at publicEndTime, however, it's still likely to happen if the collection is very popular and many user want to buy it.

Tools Used

Manual Review

Recommended Mitigation Steps

Protocol should calculate the minting price based on Descending Sale Model, when block.timestamp is less than or equal to publicEndTime:

-       } 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){

Assessed type

Context

@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 7, 2023
c4-submissions added a commit that referenced this issue Nov 7, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #1391

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as partial-50

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Dec 8, 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 duplicate-1275 edited-by-warden partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%)
Projects
None yet
Development

No branches or pull requests

4 participants