NextGen - bronze_pickaxe's results

Lines of code

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

Vulnerability details

Proof of Concept

A collection will be either in the allowlist phase or public phase. In both these phases, the creator of the collection can specify the max amount of NFT's a participant can mint by using the _maxCollectionPurchases in NextGenCore.setSollectionData(): NextGenCore.sol#L151

    function setCollectionData(..) {
            collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
    }

However, due to a reentrancy, a person can mint more than the allowance. I have written a PoC for the public mint and for the allowlist mint, which are provided of the end of each section.

Public Mint

The problem lays in the usage of _safeMint before updating the variable tokensMintedPerAddress first. _safeMint gets called in _mintProcessing: NextGenCore.sol#L231

    function _mintProcessing(..){
        //.. ommitted code
        _safeMint(_recipient, _mintIndex);
    }

During the public mint phase, _mintProcessing gets called before updating the tokensMintedPerAddress variable: NextGenCore.sol#L231

    function mint(..)
            //.. ommitted code
            _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
            //.. ommitted code
            } else {
                tokensMintedPerAddress[_collectionID][_mintingAddress] = tokensMintedPerAddress[_collectionID][_mintingAddress] + 1;
            }
        }
    }

This results in the ERC721 token being minted before tokensMintedPerAddress gets updated. This means that a malicious user can mint with a contract that implements the onERC721Received() function. When the contract receives the ERC721 token, the onERC721Received function will be triggered, which will call MinterContract.mint() again and again.

This check, which is meant to prevent people from minting more than allowed, will always pass until the tokensMintedPerAddress is updated: MinterContract.sol#L224

require(gencore.retrieveTokensMintedPublicPerAddress(col, msg.sender) + _numberOfTokens <= gencore.viewMaxAllowance(col), "Max");

Here is a gist of the Public Mint PoC, it is written using Foundry. Instructions are in the gist. Click here

Allowlist Mint

The same logic explained above applies to the Allowlist period. _safeMint, which gets called in NextGenCore._mintProcessing, is called before the variable tokensMintedAllowlistAddress is updated:

  function mint(..){
      _mintProcessing(mintIndex, _mintTo, _tokenData, _collectionID, _saltfun_o);
      if (phase == 1) {
          tokensMintedAllowlistAddress[_collectionID][_mintingAddress] = tokensMintedAllowlistAddress[_collectionID][_mintingAddress] + 1;
          }
      }

The same logic as during the public mint applies here as well. A person can loop their mint because this check will not fail until the tokensMintedAllowlistAddress is updated:

MinterContract.sol#L213 MinterContract.sol#L217

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

Here is a gist of the Allowlist PoC, it is written using Foundry. Instructions are in the gist. Click here

Impact

The max amount minted per address is one of the most important settings for NFT collections. You want your loyal collector to be able to collect your NFT pieces by allowlisting them a set amount. Having a single person or a few persons exploiting this to hoard more supply than the rest is not what you want. With the current setup of the minting process, everyone is able to mint as much as he wants, breaking the core functionality of this project.

Tools used

Foundry, manual review

Recommended mitigation steps

Follow the CEI-pattern, update the state variables before calling _safeMint().

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L58 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L122-L143 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L102-L120

Vulnerability details

Background

This report will show two scenarios that will happen due to wrong validation of block.timestamp + not correctly validating the return value. This two issues combined will result in a bidder stealing the funds of others.

Impact

In Auctiondemo.sol, a person can bid an amount on an auctioned NFT using participateToAuction, as long as you fulfil the following requirements:

AuctionDemo.sol#L58

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

The person placing a bid has to bid higher than the current highest bid AND the block.timestamp has to be less or equal to the ending time of the auction AND the auction status of the tokenId has to be true.

A person can cancel their bid using either cancelBid or cancelAllBids, both requiring:

AuctionDemo.sol#L122-L143

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

The block.timestamp has to be less or equal to the ending time of the auction, just like the participateToAuction function.

The winner of an auction(or the admin) can call claimAuction to claim the auctioned NFT. In this function the losing bids will all be refunded. This requires:

AuctionDemo.sol#L102-L120

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

block.timestamp has to be greater or equal to the end time of the auction. The problem, here lays in the fact that a person can execute all these above-mentioned functions when block.timestamp == auctionEndtime.

Scenario 1: Winner takes NFT + full refund

In this scenario, the winner of an auction takes the NFT + gets a full refund of the amount he should have paid, breaking the main invariant that should never be broken according to the Sponsor:

README.md#L184

- The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded.

Let:

• Auction ends at 2000
• Address(1) bids 1 ether
• Address(2) bids 2 ether
• Malicious user bids 3 ether using an exploit contract
• block.timestamp is 2000

Malicious user calls claimAuction to claim the auction. In this function, address(1) and address(2) will be successfully refunded, however, the problem lays here:

AuctionDemo.sol#L112-L113

IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
(bool success, ) = payable(owner()).call{value: highestBid}("");

A malicious user can just integrate the onERC721Received function in his exploit contract wherein a call will be made to cancelBid. This call to cancelBid will not fail because the check that checks if an auction has ended will pass because current block.timestamp == 2000 == auctionEndTime.

This results in the winner being able to receive the ERC721 token + cancelling his bid and getting a successful refund. In the next line, the wallet that was temporary holding the auctioned NFT, is supposed to get paid:

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

However, this will silently fail because there are no funds left in Auctiondemo.sol to pay him out.

Here is a PoC I wrote, instructions are provided in the gist. Click here

Scenario 2: Bidder steals funds from other bidders

In this scenario, the bidder is able to steal the funds of other bidders.

Let:

• Malicious user bids the following amount using an exploit contract:
  • 1 ether, 2 ether, 3 ether, 4 ether, 5 ether
• Address(1) bids 7.5 ether
• Address(2) bids 7.5 ether + 1 wei to win the auction.

Address(2) wins the auction and calls claimAuction to claim the ERC721 token. However, the exploit contract implements a fallback function that calls cancelBid every time it receives ETH, which will happen in the loop during the claimAuction function:

AuctionDemo.sol#L116-L117

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);

Since the malicious user has placed the bids before the bids of address(1) and address(2), he will get refunded first because the loop goes through the auctionInfoData starting from index 0. This means that every time the exploit contract receives a refund, they can double-dip by cancelling their own bid using cancelBid. This call will succeed because block.timestamp == 2000 == auctionEndTime and every failed call will silently revert because the return value of the low level .call() is not being checked.

This scenario also breaks the main invariant:

README.md#L184

- The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded.

I wrote a PoC for this scenario as well - instructions are provided in the gist. Click here

Tools used

Foundry, Manual Review

Recommended mitigation steps

Usage < and > instead of =< and >= will solve the current scenarios above, however, it is also recommended to handle the return value of calls.

Assessed type

Other

Lines of code

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

Vulnerability details

Background

The Sponsor stated in the README.md of this contest:

Consider if any functionality does not work with a SAFE (formerly Gnosis) wallet.

However, any function that ends up using .safeTransferFrom to send an ERC721 token will not always work with Safe wallets because not all Safe wallets will have a fallbackManager implemented due to various (security) reasons. The fallbackManager ensures that a Safe wallet is able to receive ERC721 tokens by implementing onERC721Received.

Since in this project, safeTransferFrom is used to transfer the ERC721, this transfer will revert with the following error code:

file: smart-contracts/ERC721.sol
require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner or approved");

What for impact does this have? Consider the following:

• Alice put's her 1/1 NFT on auction.
• BobDAO bids on the 1/1 NFT with the intention to win. They use their Safe multisig wallet.
• BobDAO ends up winning the auction out of 100 participants.
• The admin calls AuctionDemo.claimAuction to distribute the NFT.
• However, this call will revert because the Safe does not implement the ERC721TokenReceiver.
• This means that, all 100 other participants, will not be able to be refunded their money. The participants can not call cancelBid because the auction has ended.

Tools used

Manual review.

Recommended mitigation

For the exact scenario described above, split the refund mechanism into a different function that is not at mercy of the ERC721.safeTransferFrom() function succeeding, which people can call after the auction has ended.

Assessed type

Other