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

Sales Model 3 Includes Airdrops tokens to get price instead of just Minted tokens #246

Closed
c4-submissions opened this issue Nov 3, 2023 · 6 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-381 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

c4-submissions commented Nov 3, 2023

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/ff8cfc5529ee4a567e1ce1533b4651d6626d1def/smart-contracts/MinterContract.sol#L536

Vulnerability details

Impact

Users will have to pay more than usual when minting from a collection that utilizes the Period Model and has a rate set.

Proof of Concept

For sales model 3, the protocol wants the price to increase as minting increases when rate is set.
https://seize-io.gitbook.io/nextgen/nextgen-smart-contracts/minter

If the rate is set, then the minting cost price increases based on the tokens minted. In the Period model the
rate refers to a percentage that is used to calculate the new price based on the starting price and current
minting supply, ex. If the starting cost is 1ETH and the rate is set to 10%, during each minting the price 
will be increased by 0.1 ETH. 

In the bid to achieve that, the code is implemented as seen below:
https://github.com/code-423n4/2023-10-nextgen/blob/ff8cfc5529ee4a567e1ce1533b4651d6626d1def/smart-contracts/MinterContract.sol#L535-L536

if (collectionPhases[_collectionId].rate > 0) {
return collectionPhases[_collectionId].collectionMintCost + ((collectionPhases[_collectionId].collectionMintCost / collectionPhases[_collectionId].rate) * gencore.viewCirSupply(_collectionId));

However, instead of considering only the actual minted tokens, the calculation utilizes the entire circulationSuplly which includes the airdropped tokens, this oversight will abnormally increase the price to be paid by users when minting than it's supposed to be.

A proof that circulationSupply includes airdrops
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L180

collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;

Add this PoC to nextGen.test.js file

    //////////////////MY POC TEST//////////////////////////////
    it("#AIRDROPNFTCol3", async function () {
      const circulatingSupplyBefore = await contracts.hhCore.viewCirSupply(3);
      const price = await contracts.hhMinter.getPrice(
        3 // _collectionID
      );
      //NOTE: Two tokens were previously minted and mintingPrice is 1ETH
      expect(parseInt(price)).equal(1200000000000000000); //
      console.log(
        "Supply After Two Mint and Zero Air-Drop:",
        circulatingSupplyBefore,
        "Price:",
        price
      );

      //AIRDROP
      await contracts.hhMinter.airDropTokens(
        [signers.addr1.address, signers.addr2.address], // _recipients
        ['{"tdh": "100"}', '{"tdh": "200"}'], // _numberOfTokens
        [1, 2], // _varg0
        3, // _collectionID
        [1, 2] // _numberOfTokens
      );

      //CHECK PRICE AFTER AIRDROP
      const Newprice = await contracts.hhMinter.getPrice(
        3 // _collectionID
      );

      //OBSERVE THE INCREASE AS A RESULT OF AIRDROPPED TOKENS
      expect(parseInt(Newprice)).equal(1500000000000000000); //
      const circulatingSupplyAfter = await contracts.hhCore.viewCirSupply(3);
      console.log(
        "Supply After Two Mint and Three Air-Drop:",
        circulatingSupplyAfter,
        "Price:",
        Newprice
      );
    });

Output

Supply After Two Mint and Zero Air-Drop: 2n Price: 1200000000000000000n
Supply After Two Mint and Three Air-Drop: 5n Price: 1500000000000000000n

Tools Used

Manual report and Hardhat.

Recommended Mitigation Steps

Exclude airdropped tokens from the calculation, as the documentation explicitly specifies that the increase should solely be determined by minted tokens.

To implement this, you can maintain a record of the total airdropped token quantity and deduct that amount from the circulation supply.

Assessed type

Math

@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 3, 2023
c4-submissions added a commit that referenced this issue Nov 3, 2023
@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Nov 16, 2023
@c4-pre-sort
Copy link

141345 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the primary issue Highest quality submission among a set of duplicates label Nov 16, 2023
@c4-pre-sort
Copy link

141345 marked the issue as primary issue

@c4-sponsor
Copy link

a2rocket (sponsor) confirmed

@alex-ppg
Copy link

alex-ppg commented Dec 5, 2023

I believe this to be a duplicate of #2012 albeit referring to airdropped tokens instead of tokens minted via other means. I will most likely merge, but will keep it separate for now.

@c4-judge
Copy link

c4-judge commented Dec 7, 2023

alex-ppg marked issue #381 as primary and marked this issue as a duplicate of 381

@c4-judge c4-judge closed this as completed Dec 7, 2023
@c4-judge c4-judge added duplicate-381 and removed primary issue Highest quality submission among a set of duplicates labels Dec 7, 2023
@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards 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-381 edited-by-warden satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

6 participants