NextGen - 0xJuda's results

Lines of code

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

Vulnerability details

Impact

The identified vulnerability allows a user to exceed the prescribed token minting limit during the allowlist phase. If this user is the first to initiate minting during this phase and possesses sufficient capital, they can exploit this flaw to mint the entire available token supply from the collection.

Proof of Concept

A user gains allowlist access for minting a limited number of tokens from a new collection, utilizing a smart contract address for the allowlist. He invokes the MinterContract#mint method to start minting.

Here we jump into the first condition block because we are in the first phase. One important require block is here to check whether _maxAllowance is higher or equal to retrieveTokensMintedALPerAddress (Number of tokens minted by address during the first phase) + _numberOfTokens (Number of tokens user wants to mint). The user is allowed to mint limited number of tokens so this condition should fail when he tries to mint more. The retrieveTokensMintedALPerAddress method returns value from NextGenCore tokensMintedAllowlistAddress state variable that keeps track of how many tokens have been minted to an address.

require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit");

When this condition is passed, it gets to a for loop that calls NextGenCore#mint to process a token mint.

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

The root cause of this vulnerability lies here in the NextGenCore mint method. As you can see, it mints the token before it updates the count of tokens minted by address. This violates Checks-Effects-Interactions pattern and opens paths for reentrancy attacks.

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");
    // @note Update circulating supply
    collectionAdditionalData[_collectionID].collectionCirculationSupply = collectionAdditionalData[_collectionID].collectionCirculationSupply + 1;
    if (collectionAdditionalData[_collectionID].collectionTotalSupply >= collectionAdditionalData[_collectionID].collectionCirculationSupply) {
        // @note Mint token to user using safeMint.
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
        // @audit-issue Checks-Effects-Interactions pattern broken. The count of minted tokens should be updated before the token is minted.
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

When we check the _mintProcessing function we find out that it uses _safeMint method. This opens the attack path because the user wants the token to be minted to a smart contract address. Safe mint calls IERC721Receiver.onERC721Received in the recipient 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);
}

The user can use this callback to reenter the MinterContract#mint method and start the process again. There is the require(_maxAllowance >= gencore.retrieveTokensMintedALPerAddress(col, msg.sender) + _numberOfTokens, "AL limit"); condition that should revert the transaction as we describe at the beginning. But he reentered the method before the number of minted tokens was updated. Because of it, the user is able to mint as many tokens as he wants to up to the maximum supply if he has enough ether. He just need to reenter the mint method from the onERC721Received callback.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend fixing the NextGenCore#mint method by applying the C-E-I pattern or you may consider adding a reentrancy guard.

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

Vulnerability details

Impact

When the block.timestamp equals the auction end time, both the claimAuction and cancelBid functions can be invoked. This creates a scenario where the winning bidder can abuse the situation to profit by claiming an NFT and reentering the cancelBid function to retrieve their Ether, essentially obtaining the NFT for free. Additionally, other active bidders may also reenter the cancelBid function from their smart contracts to exploit the reentrancy vulnerability and gain Ether equal to the value of their bids. The situation becomes more severe when multiple auctions occur in the same contract, potentially draining Ether from other token auctions.

Proof of Concept

The vulnerability arises due to overlapping conditions in the claimAuction, cancelBid, and cancelAllBids functions. All of these functions can be invoked when block.timestamp == minter.getActionEndTime(_tokenId).

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
    // ...
}

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

function cancelAllBids(uint256 _tokenid) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

The claimAuction method is responsible for transferring the ERC721 token to the winner and refunding bids to other participants. It provides a reentrancy point through the safeTransferFrom call and low-level calls using payable(address).call{value:...}(""). These mechanisms enable users to easily reenter the contract.

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;
    // ...
    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); // @audit-issue reentrancy
            (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}(""); // @audit-issue reentrancy
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

Both the cancelBid and cancelAllBids functions lack reentrancy protection and do not check for the condition auctionClaim[_tokenid] == false as is done in the claimAuction method.

Active bidders can exploit this vulnerability during the auction claim when block.timestamp equals the auction end time. This is facilitated by the absence of checks for the success of low-level function calls in the auction contract, which prevents transactions from reverting due to a lack of Ether in the contract.

Example

1. Several bidders participate in a token auction using smart contracts.
2. The block.timestamp equals the auction end time.
3. The claimAuction method is triggered.
4. The function iterates through active bids, refunding them to the bidders.
5. Bidders, having their receive functions prepared, reenter the cancelBid method. They not only receive their original bid back but also gain the same amount from the canceled bid.
6. After processing the bidders, it's the winner's turn. The safeTransferFrom method is invoked. Since the winner also used a smart contract for bidding, the IERC721Receiver#onERC721Received function is called.
7. The winner reenters the cancelBid method to retrieve their original bid.
8. The function continues, transferring the token to the winner, who now possesses a new ERC721 token and their original bid.

Tools Used

Manual review

Recommended Mitigation Steps

I strongly recommend eliminating the existing overlaps between conditions.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
-   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/smart-contracts/AuctionDemo.sol#L116

Vulnerability details

Impact

The vulnerability allows any participant, especially a malicious actor, to engage in a Denial of Service (DoS) attack on the auction claim functionality. By backrunning the auction creation and strategically participating in the bidding process, the attacker can exhaust the gas of the claim initiator, resulting in a transaction revert and locking the funds of other bidders in the auction contract.

Proof of Concept

The vulnerability arises in the claimAuction function, particularly in the bidding process where there is no gas limit set for the return of bids to participants. The lack of a gas limit enables a malicious participant to spend all the gas of the claim initiator, leading to a DoS attack.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    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) {
          // ...
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}(""); // @audit-issue no gas limit
            // ...
        } else {}
    }
}

The sequence of events leading to the DoS attack involves the creation of the auction, strategic bidding by the attacker, and the subsequent invocation of the claimAuction function:

1. Auction is created.
2. Hacker backruns auction creation to be the first to bid.
3. In his transaction he sends a few bids with a dust amount of wei from a malicious contract.
4. Other participants send their bids to the auction.
5. Winner calls the claimAuction function.
6. When bids are sent back to the hacker, the receive method in his contract spends all of it.
7. The transaction initiator is gas griefed and the transaction reverts because of an out-of-gas error.
8. Funds of all bidders are locked in the contract forever.

Hacker has to use multiple bids because of EIP150. Since this proposal, the gas sent to external calls is 63/64 of gas left.

Also, even with the gas limit in place, there is still a way to do the gas grieving. Hacker may use the receive method in his contract to return enormous data payload which is then saved to the memory. Writing (bool success, )is the same as (bool success, bytes memory data). Memory allocation becomes very costly and a huge amount of gas will be spent for allocating data to memory.

Remix PoC

A proof of concept has been developed using Remix to demonstrate how gas depletion occurs. The attacker exploits the absence of a gas limit, causing the transaction to fail due to an out-of-gas error.

1. Copy the snippet to https://remix.ethereum.org
2. Deploy Auction contract.
3. Copy the Auction contract address.
4. Deploy the GasGriefer contract. Use the Auction address as an input and send 10 wei with the transaction.
5. Call Auction#claim method with maximum Ethereum block gas limit which is 30 000 000.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

import "hardhat/console.sol";

contract Auction {
    struct Bid {
        address bidder;
        uint bid;
    }

    Bid[] public bidInfo;

    function claim() public {
        for (uint i; i < bidInfo.length; i++) {
            console.log("Return bid number:", i + 1);
            console.log("|  Gas before external call:", gasleft());
            (bool success,) = payable(bidInfo[i].bidder).call{value: bidInfo[i].bid}("");
            console.log("|  Gas after external call:", gasleft());
        }
    }

    function participate() public payable {
        Bid memory newBid = Bid(msg.sender, msg.value);
        bidInfo.push(newBid);
    }
}

contract GasGriefer {
    uint public counter;

    constructor(address auction) payable {
        for (uint i; i < 4; i++) {
            Auction(auction).participate{value: i + 1}();
        }
    }

    receive() external payable {
        counter = 0;
        while (true) {
            counter++;
        }
    }
}

The result is that the transaction fails because of an out-of-gas error. The attacker paid only 10 wei to spend all the maximum gas in the block and brick the auction.

console.log:
Return bid number: 1
| Gas before external call: 29972904
| Gas after external call: 467942
Return bid number: 2
| Gas before external call: 465525
| Gas after external call: 6928
Return bid number: 3
| Gas before external call: 4510
transact to Auction.claim errored: Error occured: out of gas.

Why is this different from the bot race DoS issue?

In case you think that this is a duplicate of the issue reported in bot race by Hound, Permanent DoS due to non-shrinking array usage in an unbounded loop, let me explain why this issue is different.

Yes, the impact is similar - gas grieving and DoS of auction claim. But the root causes and solutions aren't the same and solving one won't solve the other. Hound's issue is based on growing the size of the array indefinitely. The transaction will revert because of iterating through the "infinitely" long array. Vulnerability I found doesn't depend on the growing array. All I need are three bids to DoS the claim and it's because of the missing gas limit.

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, the following mitigation steps are highly recommended:

1. Add gas limit to the low-level call when returning bids.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    // ...
    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) {
            // ...
        } else if (auctionInfoData[_tokenid][i].status == true) {
-           (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
+           (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid, gas: GAS_LIMIT}("");
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

2. To address the problem of spending gas on copying huge payload to the memory, think about implementing a low-level assembly call.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    // ...
    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) {
            // ...
        } else if (auctionInfoData[_tokenid][i].status == true) {
-           (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
+           bool success;
+           address bidder = auctionInfoData[_tokenid][i].bidder;
+           uint bid = auctionInfoData[_tokenid][i].bid;
+           assembly {
+               success := call(GAS_LIMIT, bidder, bid, 0, 0, 0, 0)
+           }
            emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
        } else {}
    }
}

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

If the winning bidder in the auction contract does not implement the IERC721Receiver#onERC721Received method, the execution of the claimAuction() function will revert because of ERC721#_checkOnERC721Received. As a result, bidders will be unable to retrieve their funds after the auction period ends.

Proof of Concept

Bidders may call 3 methods to participate in the auction or cancel their bid.

1. participateToAuction
2. cancelBid
3. cancelAllBids

When you investigate those functions they share a require block. They can be called only when the block.timestamp is lower or equal to the auction end time. Because of that the bidders can't add bids or withdraw their funds after the auction end.

function participateToAuction(uint256 _tokenid) public payable {
    require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
    // ...
}

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

function cancelAllBids(uint256 _tokenid) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

Logic for returning ether to non-winners is in the claimAuction method. This function goes through all the bids and returns them. For the winning bid it tries to transfer the token to winner using safeTransferFrom function. Here lies the vulnerability.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    // ...
    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}("");
            // ...
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            // ...
        } else {}
    }
}

In the event that the winning bidder is another smart contract, it must implement the onERC721Received method. Failure to do so results in a transaction revert, leaving the ether used for bidding on a specific token trapped in the contract because participants can't cancel their bids nor send new higher bid to change the winner.

This vulnerability poses a significant risk, as it opens the door for malicious users to launch a Denial of Service (DoS) attack on the contract, effectively locking up a substantial amount of ether. Consider a scenario in which a popular token is being auctioned, and approximately 100 participants place bids. If bids range from 10,000 to 10,099, the total value locked in the contract could reach 1,004,950. An attacker would only need to spend 10,100 to render this amount inaccessible, resulting in a potential loss of approximately 1% of the total value.

Moreover, there is the risk of unintentional contract bricking if the winning bidder forgets to implement the required onERC721Received method.

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, remove the require statements in the cancelBid and cancelAllBids functions to allow bidders to cancel their bids after auction end.

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

function cancelAllBids(uint256 _tokenid) public {
-   require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

Assessed type

DoS