NextGen - smiling_heretic's results

Lines of code

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

Vulnerability details

Impact

In Attack Ideas there is:

"Consider ways in which an allowlist address can mint more tokens than what it is allowed to mint."

"Consider ways in which an address during the public phase can mint more tokens compared to what its allowed to mint (maxCollectionPurchases)"

Both limits are easily bypassed by exploiting re-entrancy in minter.mint() function.

Exploit description

1. Attacker calls minter.mint() with _numberOfTokens == maxAllowance.
2. The maxAllowance check (1, 2 or 3) passes as expected.
3. Then call to gencore.mint is reached here.
4. In this function, _mintProcessing is called before tokensMintedAllowlistAddress[_collectionID][_mintingAddress] or tokensMintedPerAddress[_collectionID][_mintingAddress] gets incremented.
5. In _mintProcessing, _safeMint is reached. This is an ERC721 function that triggers onERC721Received callback.
6. Now attacker can do something inside onERC721Received. They call minter.mint with _numberOfTokens == maxAllowance again.
7. Because tokensMintedAllowlistAddress[_collectionID][_mintingAddress] and tokensMintedPerAddress[_collectionID][_mintingAddress] haven't been incremented yet, the same checks as before (1, 2 or 3) pass without problem.
8. The minting loop from the reentrant call to minter.mint finishes. Now maxAllowance + 1 tokens have been minted.
9. Execution returns to the minting loop from the original call to minter.mint. It finishes minting without problem. Now 2 * maxAllowance of tokens have been minted.

Proof of Concept

Full code and exact steps to reproduce the exploit are in this secret gist.

Note, that only the case of bypassing maxCollectionPurchases in the public phase is included in this PoC.

The attack for the allowlist phase is the same (but more time-consuming to write coded PoC for due to use of Merkle Tree).

It’s worth noting that limits for the public phase can also be easily bypassed by a sybil attack (simply mint maxCollectionPurchases of tokens from many addresses and send them all to one, main attacker address).

Tools Used

Foundry, manual analysis

Recommended Mitigation Steps

Add ReentrancyGuard to the minter.mint() function.

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#L116-L117

Vulnerability details

Description

It’s possible to drain all ETH from the AuctionDemo contract. There are two bugs that make it possible when they're exploited together.

First bug:

In AuctionDemo there are two phases of an auction:

1. before minter.getAuctionEndTime(_tokenid), when users can call participateToAuction and cancelBid functions
2. after minter.getAuctionEndTime(_tokenid), when the winner can call claimAuction

However, =< and >= inequalities are used in all checks (1, 2, 3). Therefore, if block.timestamp == minter.getAuctionEndTime(_tokenid) then both phases are active at the same time and the winner of an auction can call all of these functions in the same transaction.

On Ethereum blockchain, one block is produced every 12 seconds. Therefore, for each auction there’s 1/12 probability that minter.getAuctionEndTime(_tokenid) will be exactly equal to block.timestamp for some block (assuming that minter.getAuctionEndTime(_tokenid) % 12 is uniformly random).

Second bug:

In cancelBid function, auctionInfoData[_tokenid][index].status is set to false when a user gets a refund. This is to prevent getting refund on the same bid multiple times.

However, there is no such protection in claimAuction when refunding all non-winning bids.

Exploit

These two bugs exploited together allow an attacker to quickly become the winner for an auction when minter.getAuctionEndTime(_tokenid) == block.timestamp, then place additional bid, and then get refunds on these bids by cancelling them.

Refund on the winning bid causes the winner to get the auctioned NFT for free.

Refund on the additional bid, allows the attacker to drain the contract because they get back twice the deposited amount of ETH (one refund from claimAuction and one from cancelBid, both for the same bid).

Impact

Attacker can “win“ an NFT from an auction and steal all ETH deposited for other auctions that are running at the time of the attack.

Condition that enables the attack is that minter.getAuctionEndTime(_tokenid) == block.timestamp for some block and for some _tokenid.

Let’s assume that:

1. minter.getAuctionEndTime(_tokenid) % 12 is a uniformly distributed random variable (all 12 possible values have the same probability 1/12).
2. One block is produced every 12 seconds
3. There were 100 auctions running in AuctionDemo (only some of them have to overlap in time).

Under these assumptions, probability of minter.getAuctionEndTime(_tokenid) == block.timestamp occurring is 1 - (11/12)^100 ~= 0.99983. So it’s nearly guaranteed to happen.

Proof of Concept

Exact steps and full code to reproduce the exploit are in this secret gist.

Here is the test with main logic of the exploit:

    function testExploit() public {
        // get id of of 1st auctioned token
        uint256 tokenId = gencore.viewTokensIndexMin(collectionId);

        // check initial state
        assertEq(gencore.ownerOf(tokenId), nftOwner);
        assertEq(attacker.balance, 5 ether);
        assertEq(honestUser.balance, 5 ether);
        assertEq(address(auction).balance, 0);
        assertEq(auctionOwner.balance, 0);

        // required approval so that claimAuction won't revert
        vm.prank(nftOwner);
        gencore.approve(address(auction), tokenId);

        // honest user participates in two auctions
        vm.startPrank(honestUser);
        auction.participateToAuction{value: 1 ether}(tokenId);
        auction.participateToAuction{value: 3.06 ether}(tokenId + 1);
        vm.stopPrank();

        // attacker waits until block.timestamp == auctionEndTime
        vm.warp(minter.getAuctionEndTime(tokenId));

        // exploit
        vm.startPrank(attacker);
        // attacker places three bids and becomes the winner of the auction
        auction.participateToAuction{value: 1.01 ether}(tokenId);
        auction.participateToAuction{value: 1.02 ether}(tokenId);
        auction.participateToAuction{value: 1.03 ether}(tokenId);

        // attacker claims the auctioned NFT
        // and recieves refunds on their two non-winning bids
        auction.claimAuction(tokenId);

        // attacker gets refunds on all three bids
        auction.cancelAllBids(tokenId);
        vm.stopPrank();

        // check final state
        assertEq(gencore.ownerOf(tokenId), attacker);
        assertEq(attacker.balance, 7.03 ether);
        assertEq(honestUser.balance, 5 ether - 3.06 ether);
        assertEq(address(auction).balance, 0);
        assertEq(auctionOwner.balance, 1.03 ether);
    }

Tools Used

Foundry, manual analysis

Recommended Mitigation Steps

Use strict inequality (> instead of >=) in claimAuction to exclude the exact minter.getAuctionEndTime(_tokenid) from 2nd phase of the auction.

Set auctionInfoData[_tokenid][index].status = false in claimAuction before sending refunds (just like in cancelBid) to protect against double refunds.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The purpose of burnToMint() function is to allow users to burn an NFT from one collection and receive freshly minted NFT from another collection. Pairs of (_burnCollectionID, _mintCollectionID,) for which this operation is allowed have to be approved by admin.

In this function, _mintProcessing is called before _burn which allows the attacker to do something inside the onERC721Received callback right before token gets burned.

Moreover, there is no check that would verify the owner of the burned _tokenId before it gets burned.

It follows that, if attacker transfers the NFT in the callback, then burnToMint function won't revert. NFT will just get burned after changing its owner.

There are protocols out there that allow users to borrow tokens against NFTs as collateral (e.g. JPEG'd) and AMMs for NFTs (e.g. Caviar). This allows the attacker to profit from this last transfer right before _burn.

This exploit:

1. is profitable to the attacker because they get both freshly minted NFT from _mintCollectionID and tokens from selling (or borrowing against) an NFT from _burnCollectionID.
2. It harms the external protocols that support NextGen NFTs, because the NFT gets burned right after they buy it (or lend money against it).
3. Long-term it harms reputation and composability of NextGen, because it will likely get blacklisted by protocols like these.

Proof of Concept

Full code and exact steps how to reproduce the exploit are in this secret gist.

Tools Used

Foundry, manual analysis

Recommended Mitigation Steps

Add a check for gencore.ownerOf(tokenId) before calling _burn in gencore.burnToMint.

Call _burn before _mintProcessing, not after.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

In Attack Ideas there is:

"Consider ways in which the owner of the token will not receive the funds of the highest bid after an Auction is claimed."

This makes it clear that expected behavior of the AuctionDemo.claimAuction function is to send highest bid to the owner of the auctioned token.

However, highest bid is sent to the owner of the AuctionDemo contract instead (AuctionDemo inherits from Ownable).

So if the owner of the contract is different from the owner of the token, then highest bid is sent to wrong address.

Proof of Concept

Full code to and exact steps to reproduce the issue are in this secret gist.

Here is only test that demonstrates the issue (without setup):

    function testWrongOwner() public {
        // get id of of the auctioned token
        uint256 tokenId = gencore.viewTokensIndexMin(collectionId);

        // check initial state
        assertEq(gencore.ownerOf(tokenId), nftOwner);
        assertEq(auction.owner(), auctionOwner);

        // Ownabele owner of the contract
        // and owner of the auction tokenId
        // are two different addresses
        assertTrue((auctionOwner != nftOwner));
        assertEq(auctionOwner.balance, 0);
        assertEq(nftOwner.balance, 0);

        // required approval so that claimAuction won't revert
        vm.prank(nftOwner);
        gencore.approve(address(auction), tokenId);

        // user1 participates in the acution
        vm.prank(user1);
        auction.participateToAuction{value: 1 ether}(tokenId);

        // user2 wins the auction
        vm.prank(user2);
        auction.participateToAuction{value: 2 ether}(tokenId);

        // wait until claimAuction available
        vm.warp(minter.getAuctionEndTime(tokenId) + 12);

        // call claimAuction
        vm.prank(user2);
        auction.claimAuction(tokenId);

        // check final state
        assertEq(gencore.ownerOf(tokenId), user2);
        assertEq(auction.owner(), auctionOwner);

        // Verify that ETH was sent to auctionOwner, not nftOwner
        assertTrue((auctionOwner != nftOwner));
        assertEq(auctionOwner.balance, 2 ether);
        assertEq(nftOwner.balance, 0);
    }

Tools Used

Foundry, manual analysis

Recommended Mitigation Steps

Change owner() to ownerOfToken in AuctionDemo.claimAuction function.

Assessed type

Other