Minting limits are not consistently applied

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L213 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L217 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L224 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L236 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L189 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L194-L197 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L326 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L363 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L270 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L213

Vulnerability details

Summary

Minting can be done through MinterContract, via mint(), burnToMint(), burnOrSwapExternalToMint(), and airDropTokens().

Some functions don't check for minting limits, and some don't update the limits.

Inconsistencies on those functions allow users to mint NFTs besides their expected minting limit.

Impact

Users can mint more NFTs than expected. This in detrimented of expected minting patterns, which can impact on collection prices.

This is also featured on the Attack Ideas that concern the protocol:

Attack ideas (Where to look for bugs)

• Consider ways in which an allowlist address can mint more tokens than what it is allowed to mint.
• Consider ways in which an address during the public phase can mint more tokens compared to what its allowed to mint (maxCollectionPurchases)

Proof of Concept

MinterContract::mint() checks the limits an address can mint via retrieveTokensMintedALPerAddress() or retrieveTokensMintedPublicPerAddress() depending on the phase. And it prevents more minting if they exceed the expected value.

• MinterContract.sol#L213
• MinterContract.sol#L217
• MinterContract.sol#L224

Those limits are updated upon calling internally NextGenCore::mint():

• MinterContract.sol#L236
• NextGenCore.sol#L189
• NextGenCore.sol#L194-L197

Then there is the case of burnOrSwapExternalToMint(), which doesn't check the minting limits, bypassing any limit protection. It also calls internally NextGenCore::mint(), so the limit is updated anyway, which is inconsistent.

• MinterContract.sol#L326 (no limits check on the whole function)
• MinterContract.sol#L363 (calls gencore.mint() just like minter.mint())

In the case of burnToMint(), it doesn't check the minting limits either, bypassing any limit protection. And it uses internally gencore.burnToMint(), which doesn't update the minting limits. This is inconsistent with the previous functions, not counting this for the mint limit.

• MinterContract.sol#L270
• NextGenCore.sol#L213

Finally, in the case of airDropTokens(), due to its trusted recipient, it could be subject to debate. So, this is just informational. The function doesn't check the minting limits, nor does it update them.

• MinterContract.sol#L181
• MinterContract.sol#L189
• NextGenCore.sol#L178

Tools Used

Manual Review

Recommended Mitigation Steps

Apply minting limits consistently, considering if they also update the limits as well.

For example, in cases like burnToMint() and burnOrSwapExternalToMint(). None check minting limits, but burnToMint() updates the limits.

Check the consistency with the other functions as well.

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #409

[c4-pre-sort]

141345 marked the issue as duplicate of #1763

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The Warden specifies a clear inconsistency in the code concerning the enforcement of minting limitations.

As the Sponsor confirmed, this discrepancy is valid and needs to be rectified to ensure that the minting limitations cannot be bypassed via secondary "sale types" that involve f.e. burning an NFT.

The Warden's submission was selected as the best given that it specifies which functions were affected concisely, cites the attack ideas of the C4 contest to give it merit, and articulates that the proper way to alleviate this exhibit is to consistently apply limits albeit in a potentially unique way per function depending on the intents and purposes of the NextGen protocol.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

During burnToMint or burnOrSwap the normal minting is disabled, so noone can mint using the normal mint function on the NextGen collection (the maxCollectionPurchases will be set to 0). Only users who hold a specific token of the specific collection can mint by burning!

Now during burnToMint or burnOrSwap users can mint based on the tokens that they hold on the burnt collections. This is the normal process for us and the intended design.

If an address holds 3 tokens it can burnToMint or burnOrSwap 3 tokens. Also while the burnToMint or burnOrSwap is still active the same address can buy a token of the burnt collection from a secondary market and burn or swap that also to get a new token. The main reason for doing this is that we didn't want to take a snapshot for burnToMint or burnToSwap.

The retrieveTokensMintedALPerAddress() or retrieveTokensMintedPublicPerAddress() functions are not only used to limit minting but also to keep track of how many tokens an address has minted. When its deemed necessary as during the normal minting we check that limits but on the burnToMint or burnOrSwap we do not check the limits as the limit for each address depends on the tokens hold during the minting phases.

[alex-ppg]

Based on the Sponsor's follow-up clarification, I will downgrade this submission to a QA (L). As the documentation does not directly contradict this, and the Sponsor specifies that the current implementation is desirable, I cannot grade this as a medium-risk vulnerability.

I believe that the minting limits are inconsistent and this is a valid criticism of the project's layout and documentation. Additionally, a minting limitation imposed during burnOrSwapExternalToMint does make sense as the user has no control over those supplies, whereas for burnToMint relations we can know whether a collection will increase in its supply based on whether a collection is frozen, its supply, etc.

[c4-judge]

alex-ppg changed the severity to QA (Quality Assurance)

[c4-judge]

alex-ppg marked the issue as grade-b

[c4-judge]

alex-ppg marked the issue as not selected for report

[mcgrathcoutinho]

Hi @alex-ppg and @a2rocket, need some clarification on this one:

1. In the primary issue here, the sponsor mentioned that the retrieveTokensMintedALPerAddress() or retrieveTokensMintedPublicPerAddress() functions are not only used to limit minting but also to keep track of how many tokens an address has minted. What I believe the sponsor was against is the addition of maxAllowance checks. But the accounting of the tokens being minted here is not done as expected by the highlighted comment cited.
2. The fact that burnOrSwapExternalToMint() increments it here is clearly inconsistent, which would mean that all issues mentioning missing tokensMintedPerAdrress increment are valid while those mentioning that about the maxAllowance checks are invalid. This is why they were submitted as separate issues.
3. I believe this is why the sponsor confirmed this issue #1763.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for flagging this! There is a lot of confusion around this aspect of the codebase and I agree there are inconsistencies, however, I am reluctant to label them as medium-risk for the following reasons:

• The documentation is not clear, meaning that there is no contradiction
• The Sponsor maintains that the sale model types are distinct, and this is the case per the documentation

The fact that burnOrSwapExternalToMint increments the tokens minted in an allowlist / public phase is indeed invalid as it would affect allowances. However, I do not think any Warden has submitted this and will revisit it to make sure during the PJQA process. The Sponsor should revisit this submission regardless, so I am tagging them to make sure they see it: @a2rocket

[mcgrathcoutinho]

Hi @alex-ppg, thanks for the response.

Wardens have submitted the issue tokensMintedPerAddress not incremented in burnToMint() because of it's inconsistency with how mint() increments it in the gencore contract, which is used by both mint() and burnOrSwapExternalToMint() in the MinterContract.sol contract. The natural train of thought for wardens arises from that comparison and needing them to explicitly mention that burnOrSwapExternalToMint() should not be incremented is way too strict on words written in the report, considering that even the docs do not mention anything about these functions and where increments should be applied in specific.

At the end, I definitely agree on the sponsor @a2rocket taking another look on the tokensMintedPerAddress missing increment issues and deeming if it is indeed an issue or not.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for providing criticism concerning this issue's judgment. The discrepancy in burnOrSwapExternalToMint increasing the limits has been properly reported in a single submission, #1096.

All other Warden submissions that have been duplicated under this group specify that mint limits are inconsistently applied and that they should be enforced in burnToMint / burnOrSwapExternalToMint. The maximum impact of this is that minting limitations are not enforced by burnToMint / burnOrSwapExternalToMint; the documentation does not indicate this needs to happen and as such cannot be considered a vulnerability.

The principle of reasonable assumptions dictates that if a feature is missing from the documentation and the Sponsor confirms this omission is deliberate, then the feature should not exist. In an allowlist sale, the Merkle root consisting of specific token IDs is sufficient in enforcing an "allowlist limit".

In both a public and allowlist sale, the burnOrSwapExternalToMint configuration is accompanied by a burnOrSwapIds range indicating which token IDs the collection accepts. As such, a limitation is already enforced indirectly in the public variant as well.

A submission that would specify burnOrSwapExternalToMint increases the limit but does not enforce it would highlight an interference of the Burn-to-Mint model with the allowlist of other models which is not described in the documentation and thus constitutes a valid submission. #745 talks about this but as a very minor side-note and as such, I cannot consider this submission to be describing this vulnerability.

1096 is a submission that makes note of this properly and I have notified the Sponsor to revisit it. Once the Sponsor provides their feedback, I will consult with fellow judges on how to best handle duplicates of that submission.

[mcgrathcoutinho]

Hi @alex-ppg, sponsor seems to have responded in #1096. Highlighting it here since sponsor didn't tag you or reference this issue.

[alex-ppg]

Hello everyone, I have debated this particular submission and #1096 significantly as it is a generally unprecedented situation concerning the application of "reasonable assumptions".

Specifically, we have the following situation:

• Some Wardens claim that the absence of limitations is incorrect
• Some Wardens claim that the increase of counters that the limitations use in other models is incorrect
• Sponsor claims that both are desirable as they pertain to different aspects of the code that have a secondary use case (i.e. counters double as limitations)

The main problem that led to this situation arising was relatively undefined documentation and a slightly non-conventional code approach. I believe criticism about the documentation being ambiguous is valid, especially when there are functions such as MinterContract::acceptPrimaryAddressesAndPercentages defined in the documentation but not existing in the contract itself. (Note: It also appears the documentation is undergoing live updates, further reflecting code that is not present such as MinterContract::excludeTokensOrResetLD).

After consulting with other judges, I believe siding with the Sponsor is the "best" outcome for this particular submission. I was initially inclined to award #1096 properly as it did point to a potential discrepancy in the code, however, the Sponsor's clarifications throughout all submissions do not contradict the documentation and can be accepted as the truth. This is further reinforced by the fact that the NextGenCore documentation does list the NextGenCore::retrieveTokensMintedPublicPerAddress and NextGenCore::retrieveTokensMintedALPerAddress functions as simply yielding counters rather than variables solely useful for imposing limitations.

In detail, the codebase will:

• Increment the allowlist & public counters when there is a MinterContract::mint or MinterContract::burnOrSwapExternalToMint operation and both properly support the definition of an allowlist / public phase
• Not increment the allowlist & public counters when there is a MinterContract::burnToMint, MinterContract::mintToAuction, or MinterContract::airDropTokens operation as these functions have no concept of an allowlist / public phase

Nothing in the documentation contradicts the above behavior, and the code can be argued that it behaves correctly. In this particular instance, we have the Wardens who claim the code should behave one way and the Sponsor who claims the code should behave the other. In order to not set a C4 precedence that might affect future judgments in a negative way, I will have to side with the Sponsor in this regard. The Sponsor is the penultimate author of the code and if the code is up to interpretation with no unequivocal vulnerabilities, the Sponsor's definition should be accepted as the truth.

The reason I am utilizing "unequivocal" here is that there are certain cases where, despite what the Sponsor suggests, we can safely interpret them as real vulnerabilities. For example, if it was possible to somehow extract funds from the contract via the function and the documentation did not say funds should be extracted, we can safely assume that regardless of what the Sponsor says this is a vulnerability.

In the scenario described by the submission, we are debating the absence of limitations as well as the increase of counters. As both are considered valid by the Sponsor and there is no way to "exploit" them in an unequivocal way, the QA (L) rating for this submission will remain. To note, the maximum theoretical impact here is either:

• A) Sale limitations are increased incorrectly
• B) Sale limitations are not enforced correctly

For scenario B, it is clear that the absence of limitations is not impactful and may indeed be undesirable as the Sponsor states. In scenario A, the lines become a little bit blurry and we can argue that the counters used for limitations are "incorrectly" increased. The problem is that the documentation clearly specifies the counters are simply the total allowlist / public sales and not necessarily limitations. Additionally, the total limitations between phases deliberately carry over which is a good argument as to why the counters should carry over between sale types that support them as well.

I understand that this may not be the outcome Wardens were hoping for, but it is the "fairest" interpretation of the code that we can establish due to the absence of documentation. I think the main output of this submission is that the C4 process in relation to the scouting phase should perhaps be made more strict, ensuring that the documentation indeed matches the code and that ambiguities are minimized to enhance the ultimate security value of the C4 audits and to minimize Warden time that is wasted on perceived discrepancy evaluation that may ultimately represent desirable behavior.

I thank everyone who contributed to the PJQA phase, and I will retain the QA (L) ruling that this and duplicate submissions have. To note, the PJQA period is over and I advise everyone as a friendly reminder to refrain from contributing to the discussion further to avoid any backstage rule-breaks.