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

The mint() function is vulnerable to reentrancy, allowing malicious users to exceed the maximum allowed mint amount and inflate the collectionCirculationSupply. #648

Closed
c4-submissions opened this issue Nov 9, 2023 · 9 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-1517 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%)

Comments

@c4-submissions
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L189
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L195
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L197

Vulnerability details

Impact

The NextGenCore.mint() function does not follow the check-effect-interaction pattern.
A malious user can mint more than the maximum allowed mint amount and inflate the collectionCirculationSupply.

Proof of Concept

ERC721._safeMint() transfer control to the recipient when a token is minted.
When NextGenCore.mint() is called, it mints an NFT and transfers control to the _mintingAddress. It then updates either the tokensMintedAllowlistAddress or tokensMintedPerAddress mapping. If the _mintingAddress is a contract, then it can reenter and call NextGenMinterContract.mint() to mint more tokens than allowed.

A malious user can also infalte the collectionCirculationSupply by reentrancy but he is not able to mint more NFTs than collectionTotalSupply.
For example:

Given:
collectionCirculationSupply = 990
collectionTotalSupply = 1000

The attacker can inflate the collectionCirculationSupply to 1045 (990 + 10 + 9 + 8 + 7 + 6 + 5 + 4 + 3 + 2 + 1) by reentering the NextGenMinterContract.mint() function.

Tools Used

VS Code

Recommended Mitigation Steps

  1. Apply a reentrancy guard and follow the check-effect-interaction pattern in the NextGenCore.mint() function.
  2. Revert if collectionAdditionalData[_collectionID].collectionTotalSupply < collectionAdditionalData[_collectionID].collectionCirculationSupply

Assessed type

Reentrancy

@c4-submissions c4-submissions added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 9, 2023
c4-submissions added a commit that referenced this issue Nov 9, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #51

@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #1742

@alex-ppg
Copy link

Combination of #1742 and #1201.

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as satisfactory

@c4-judge c4-judge added satisfactory satisfies C4 submission criteria; eligible for awards partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) and removed satisfactory satisfies C4 submission criteria; eligible for awards labels Dec 8, 2023
@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as partial-25

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as unsatisfactory:
Insufficient quality

@c4-judge c4-judge added unsatisfactory does not satisfy C4 submission criteria; not eligible for awards and removed partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) labels Dec 8, 2023
@RunSoul22
Copy link

For re-entrancy attacks, the root cause of the vulnerability is considered to be the state which is not updated per the CEI pattern rather than the re-entrancy origin itself.

First, I referenced two places where states are not updated according to the CEI pattern in the Lines of code section and mentioned that the NextGenCore.mint() function does not follow the check-effect-interaction pattern in the Impact section.

  1. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L195
 tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
  1. https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L197
tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;

Secondly, I provided a clear explanation of the bug's impact.

Thirdly, I explained how it can be exploited to harm the protocol.

I need more context to understand why this finding is considered completely unsatisfactory and worthless.

@alex-ppg
Copy link

alex-ppg commented Dec 9, 2023

Hey @RunSoul22, thanks for requesting follow-up clarifications! A finding marked as unsatisfactory does not mean it is worthless; this is a comparative rating. In comparison to the other duplicated reports, this submission was one of the "unlucky" few that were lackluster in quality and thus invalidated.

If your submission was the only present in the contest, it would have been awarded in full. However, in the interest of competitiveness, I am inclined to grade submissions on a "curve" rather than impose the bare minimum. Keep in mind that the bare minimum is acceptable for QA submissions, but not submissions in the HM pool.

I will revisit this for a partial-25 rating after evaluating it in relation to other unsatisfactory submissions in the duplicate pool after the PJQA period.

@c4-judge c4-judge added partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%) and removed unsatisfactory does not satisfy C4 submission criteria; not eligible for awards labels Dec 12, 2023
@c4-judge
Copy link

alex-ppg marked the issue as partial-25

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-1517 partial-25 Incomplete articulation of vulnerability; eligible for partial credit only (25%)
Projects
None yet
Development

No branches or pull requests

5 participants