NextGen - ustas's results

Advanced smart contracts for launching generative art projects on Ethereum.

General Information

Platform: Code4rena

Start Date: 30/10/2023

Pot Size: $49,250 USDC

Total HM: 14

Participants: 243

Period: 14 days

Judge: 0xsomeone

Id: 302

League: ETH

NextGen

Findings Distribution

Researcher Performance

Rank: 67/243

Findings: 2

Award: $95.88

🌟 Selected for report: 0

🚀 Solo Findings: 0

External Links

Code4rena Contest Page Github Contest Repository Github Findings Repository NextGen

Findings Information

🌟 Selected for report: btk

Also found by: 00xSEV, 0x175, 0x180db, 0x3b, 0xAlix2, 0xJuda, 0xpiken, 0xraion, 3th, 836541, Al-Qa-qa, AvantGard, Aymen0909, Beosin, ChrisTina, DarkTower, DeFiHackLabs, EricWWFCP, Kose, Kow, KupiaSec, MrPotatoMagic, Neo_Granicen, PENGUN, PetarTolev, Ruhum, Soul22, SovaSlava, SpicyMeatball, Talfao, The_Kakers, Toshii, Tricko, VAD37, Viktor_Cortess, ZdravkoHr, _eperezok, alexxander, audityourcontracts, ayden, bird-flu, bronze_pickaxe, codynhat, critical-or-high, danielles0xG, degensec, droptpackets, evmboi32, fibonacci, flacko, gumgumzum, ilchovski, immeas, innertia, jacopod, joesan, ke1caM, kk_krish, mojito_auditor, nuthan2x, phoenixV110, pontifex, r0ck3tz, sces60107, seeques, sl1, smiling_heretic, stackachu, t0x1c, trachev, turvy_fuzz, ubl4nk, ustas, xAriextz, xuwinnie, y4y

Awards

0.152 USDC - $0.15

Labels

bug

3 (High Risk)

satisfactory

duplicate-1517

External Links

Link to GitHub issue

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/2467db02cc446374ab9154dde98f7e931d71584d/smart-contracts/MinterContract.sol#L213 https://github.com/code-423n4/2023-10-nextgen/blob/2467db02cc446374ab9154dde98f7e931d71584d/smart-contracts/MinterContract.sol#L217 https://github.com/code-423n4/2023-10-nextgen/blob/2467db02cc446374ab9154dde98f7e931d71584d/smart-contracts/MinterContract.sol#L224

Vulnerability details

Impact

Bypassing _maxAllowance in NextGenMinterContract.mint(): Enables minting more NFTs than permitted.
Exploiting reentrancy in NextGenMinterContract.burnToMint(): Allows acquiring both burnable and mintable NFTs at the same time.

Proof of Concept

The 1st case

The typical flow of a user mint is as follows: User tx -> NextGenMinterContract.mint() external call -> NextGenCore.mint() internal call -> NextGenCore._mintProcessing()

The root of the problem lies in the NextGenCore.mint() and NextGenCore._mintProcessing() functions.

function mint(
    uint256 mintIndex,
    address _mintingAddress,
    address _mintTo,
    string memory _tokenData,
    uint256 _saltfun_o,
    uint256 _collectionID,
    uint256 phase
) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    collectionAdditionalData[_collectionID].collectionCirculationSupply =
        collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (
        collectionAdditionalData[_collectionID].collectionTotalSupply
            >= collectionAdditionalData[_collectionID].collectionCirculationSupply
    ) {
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] =
                tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

function _mintProcessing(
    uint256 _mintIndex,
    address _recipient,
    string memory _tokenData,
    uint256 _collectionID,
    uint256 _saltfun_o
) internal {
    tokenData[_mintIndex] = _tokenData;
    collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
    tokenIdsToCollectionIds[_mintIndex] = _collectionID;
    _safeMint(_recipient, _mintIndex);
}

In the mint() function, the _mintProcessing() is called first and then the variables tokensMintedAllowlistAddress or tokensMintedPerAddress are updated. However, this creates a reentrancy issue because the _safeMint() at the end of _mintProcessing() calls _recipient.onERC721Received().

_recipient can be either the user or his chosen delegate, so passing a smart contract is not a problem.

To take advantage of the reentrancy, a user can pass a smart contract that will call the NextGenMinterContract.mint() function again as a delegate of the user. The function's checks will hold as the tokensMintedAllowlistAddress or tokensMintedPerAddress variables are still not updated. This method allows the user to mint as many tokens as the gas limit allows.

If the user is a smart contract, the execution is simplified as delegation is unnecessary.

The 2nd case

A malicious user can call NextGenMinterContract.burnToMint() from a smart contract. The flow is similar to the first case, where _mintProcessing() is called before burning the NFT, enabling the user to utilize both burnable and mintable NFTs together for potential profit extraction.

function burnToMint(
    uint256 mintIndex,
    uint256 _burnCollectionID,
    uint256 _tokenId,
    uint256 _mintCollectionID,
    uint256 _saltfun_o,
    address burner
) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
    collectionAdditionalData[_mintCollectionID].collectionCirculationSupply =
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
    if (
        collectionAdditionalData[_mintCollectionID].collectionTotalSupply
            >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply
    ) {
        _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        // burn token
        _burn(_tokenId);
        burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
    }
}

As long as the user has the burnable NFT in the end of his actions, the transaction will succeed.

Tools Used

Manual review

Recommended Mitigation Steps

Add reentrancy checks to the NextGenMinterContract.mint() and NextGenMinterContract.burnToMint() functions such as ReentrancyGuard by OZ or change the function to mint only after all the storage changes had been made.

MinterContract.sol

+ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

...

    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
+   ) public payable nonReentrant {

...

    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o)
        public
        payable
+       nonReentrant
    {

NextGenCore.sol

    function mint(
        uint256 mintIndex,
        address _mintingAddress,
        address _mintTo,
        string memory _tokenData,
        uint256 _saltfun_o,
        uint256 _collectionID,
        uint256 phase
    ) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply =
            collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (
            collectionAdditionalData[_collectionID].collectionTotalSupply
                >= collectionAdditionalData[_collectionID].collectionCirculationSupply
        ) {
-           _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
                    tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] =
                    tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
+           _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        }
    }

...

    function burnToMint(
        uint256 mintIndex,
        uint256 _burnCollectionID,
        uint256 _tokenId,
        uint256 _mintCollectionID,
        uint256 _saltfun_o,
        address burner
    ) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply =
            collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (
            collectionAdditionalData[_mintCollectionID].collectionTotalSupply
                >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply
        ) {
-           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
+           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        }
    }

Assessed type

Reentrancy

#0 - c4-pre-sort
2023-11-16T12:05:14Z

141345 marked the issue as duplicate of #51

#1 - c4-pre-sort
2023-11-26T14:03:01Z

141345 marked the issue as duplicate of #1742

#2 - alex-ppg
2023-12-08T16:30:12Z

Duplicate of #1597 as well.

#3 - c4-judge
2023-12-08T16:30:26Z

alex-ppg marked the issue as satisfactory

#4 - c4-judge
2023-12-08T16:30:32Z

alex-ppg marked the issue as partial-50

#5 - c4-judge
2023-12-08T19:17:12Z

alex-ppg marked the issue as satisfactory

#6 - ustas-eth
2023-12-09T12:52:40Z

Please consider splitting the issue into two:

Duplicate of https://github.com/code-423n4/2023-10-nextgen-findings/issues/1517
Duplicate of https://github.com/code-423n4/2023-10-nextgen-findings/issues/1597

Partial payment for one of them will be justified at the judge's discretion, but I want to emphasize that I've provided sufficient explanations for both of them and possible mitigation options.

I made a mistake in identifying the root cause because I thought that the reason was in the _mintProcessing() itself and the usage of _safeMint(), but these two functions are not malleable. The actual reason is their incorrect usage in the context of mint() and burnToMint().

Gist that contains the split reports: https://gist.github.com/ustas-eth/5b8a1e5392cdbdb6d2811957bab81057

Thank you

#7 - alex-ppg
2023-12-09T15:52:29Z

Hey @ustas-eth, thanks for marking this! I did make a note of this as you can observe above in the comments. The problem between them, as you correctly note, is that the root cause is not the re-entrancy. The usage of the NFT-to-be-burned cannot be resolved with re-entrancy guards, and there was insufficient emphasis put on the ramifications of it. I will revisit this alongside other submissions I have marked but will maintain this as a single submission for now.

To note, even if this is split, the payment will be partial under #1597 due to insufficient focus being put on it and a slightly incorrect root cause.

Findings Information

🌟 Selected for report: ast3ros

Also found by: 00xSEV, Al-Qa-qa, CaeraDenoir, Jiamin, Juntao, Ruhum, bart1e, circlelooper, crunch, gumgumzum, rishabh, smiling_heretic, ustas

Labels

bug

2 (Med Risk)

downgraded by judge

partial-50

duplicate-1597

Awards

95.7343 USDC - $95.73

External Links

Link to GitHub issue

Lines of code

Vulnerability details

Impact

Bypassing _maxAllowance in NextGenMinterContract.mint(): Enables minting more NFTs than permitted.
Exploiting reentrancy in NextGenMinterContract.burnToMint(): Allows acquiring both burnable and mintable NFTs at the same time.

Proof of Concept

The 1st case

The typical flow of a user mint is as follows: User tx -> NextGenMinterContract.mint() external call -> NextGenCore.mint() internal call -> NextGenCore._mintProcessing()

The root of the problem lies in the NextGenCore.mint() and NextGenCore._mintProcessing() functions.

function mint(
    uint256 mintIndex,
    address _mintingAddress,
    address _mintTo,
    string memory _tokenData,
    uint256 _saltfun_o,
    uint256 _collectionID,
    uint256 phase
) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    collectionAdditionalData[_collectionID].collectionCirculationSupply =
        collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (
        collectionAdditionalData[_collectionID].collectionTotalSupply
            >= collectionAdditionalData[_collectionID].collectionCirculationSupply
    ) {
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] =
                tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

function _mintProcessing(
    uint256 _mintIndex,
    address _recipient,
    string memory _tokenData,
    uint256 _collectionID,
    uint256 _saltfun_o
) internal {
    tokenData[_mintIndex] = _tokenData;
    collectionAdditionalData[_collectionID].randomizer.calculateTokenHash(_collectionID, _mintIndex, _saltfun_o);
    tokenIdsToCollectionIds[_mintIndex] = _collectionID;
    _safeMint(_recipient, _mintIndex);
}

_recipient can be either the user or his chosen delegate, so passing a smart contract is not a problem.

If the user is a smart contract, the execution is simplified as delegation is unnecessary.

The 2nd case

function burnToMint(
    uint256 mintIndex,
    uint256 _burnCollectionID,
    uint256 _tokenId,
    uint256 _mintCollectionID,
    uint256 _saltfun_o,
    address burner
) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
    collectionAdditionalData[_mintCollectionID].collectionCirculationSupply =
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
    if (
        collectionAdditionalData[_mintCollectionID].collectionTotalSupply
            >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply
    ) {
        _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        // burn token
        _burn(_tokenId);
        burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
    }
}

As long as the user has the burnable NFT in the end of his actions, the transaction will succeed.

Tools Used

Manual review

Recommended Mitigation Steps

MinterContract.sol

+ import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

...

    function mint(
        uint256 _collectionID,
        uint256 _numberOfTokens,
        uint256 _maxAllowance,
        string memory _tokenData,
        address _mintTo,
        bytes32[] calldata merkleProof,
        address _delegator,
        uint256 _saltfun_o
+   ) public payable nonReentrant {

...

    function burnToMint(uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o)
        public
        payable
+       nonReentrant
    {

NextGenCore.sol

    function mint(
        uint256 mintIndex,
        address _mintingAddress,
        address _mintTo,
        string memory _tokenData,
        uint256 _saltfun_o,
        uint256 _collectionID,
        uint256 phase
    ) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        collectionAdditionalData[_collectionID].collectionCirculationSupply =
            collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
        if (
            collectionAdditionalData[_collectionID].collectionTotalSupply
                >= collectionAdditionalData[_collectionID].collectionCirculationSupply
        ) {
-           _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            if (phase == 1) {
                tokensMintedAllowlistAddress[_collectionID][_mintingAddress] =
                    tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] =
                    tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
+           _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        }
    }

...

    function burnToMint(
        uint256 mintIndex,
        uint256 _burnCollectionID,
        uint256 _tokenId,
        uint256 _mintCollectionID,
        uint256 _saltfun_o,
        address burner
    ) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply =
            collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (
            collectionAdditionalData[_mintCollectionID].collectionTotalSupply
                >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply
        ) {
-           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
+           _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
        }
    }

Assessed type

Reentrancy

#0 - c4-judge
2023-12-13T19:57:05Z

alex-ppg marked the issue as duplicate of #1597

#1 - c4-judge
2023-12-13T19:57:14Z

alex-ppg marked the issue as partial-50

#2 - c4-judge
2023-12-13T19:58:01Z

alex-ppg changed the severity to 2 (Med Risk)

NextGen - ustas's results

Advanced smart contracts for launching generative art projects on Ethereum.

General Information

Findings Distribution

Researcher Performance

External Links

[H-01] Reentrancy in `NextGenMinterContract.mint()` allows exceeding max allowance and concurrent use of NFTs in `NextGenMinterContract.burnToMint()` - $0.15

Findings Information

Awards

Labels

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

The 1st case

The 2nd case

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-pre-sort2023-11-16T12:05:14Z

#1 - c4-pre-sort2023-11-26T14:03:01Z

#2 - alex-ppg2023-12-08T16:30:12Z

#3 - c4-judge2023-12-08T16:30:26Z

#4 - c4-judge2023-12-08T16:30:32Z

#5 - c4-judge2023-12-08T19:17:12Z

#6 - ustas-eth2023-12-09T12:52:40Z

#7 - alex-ppg2023-12-09T15:52:29Z

[M-03] Reentrancy in `NextGenMinterContract.mint()` allows exceeding max allowance and concurrent use of NFTs in `NextGenMinterContract.burnToMint()` - $95.73

Findings Information

Labels

Awards

External Links

Lines of code

Vulnerability details

Impact

Proof of Concept

The 1st case

The 2nd case

Tools Used

Recommended Mitigation Steps

Assessed type

#0 - c4-judge2023-12-13T19:57:05Z

#1 - c4-judge2023-12-13T19:57:14Z

#2 - c4-judge2023-12-13T19:58:01Z

#0 - c4-pre-sort
2023-11-16T12:05:14Z

#1 - c4-pre-sort
2023-11-26T14:03:01Z

#2 - alex-ppg
2023-12-08T16:30:12Z

#3 - c4-judge
2023-12-08T16:30:26Z

#4 - c4-judge
2023-12-08T16:30:32Z

#5 - c4-judge
2023-12-08T19:17:12Z

#6 - ustas-eth
2023-12-09T12:52:40Z

#7 - alex-ppg
2023-12-09T15:52:29Z

#0 - c4-judge
2023-12-13T19:57:05Z

#1 - c4-judge
2023-12-13T19:57:14Z

#2 - c4-judge
2023-12-13T19:58:01Z