NextGen - Jiamin's results

Lines of code

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

Vulnerability details

Impact

Should not enable claiming at auction end time, or the winner can claim token without pay the ETH.

Proof of Concept

There are 2 phases in an entire auction, the bidding phase and claiming phase.

In bidding phase, bidders place bids if they want to participate, or cancel bids if they want to leave; In claiming phase, winner claiming the token and the ETH will be returned to other bidders.

Unfortunately, if we look carefully at cancelBid and claimAuction, we can find that there is time overlap between the 2 phases:

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

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

Both claiming and canceling can be done at auctionEndTime, this means the winner may first claim the token and then cancel the winning bid to get ETH back at the time point.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, please remove the time overlap and only allow claiming after auctionEndTime.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Should not allow bidders to cancel bids before auction is ended, or the bidders will be grief attacked.

Proof of Concept

By calling cancelBid or cancelAllBids, bidder in an auction can cancel the bids before the auction is ended. This encourages a bidder to bid by calling participateToAuction. However, this can also be leveraged by an attacker who can bid with an unreasonably high price, honest bidders who tries to bid with a reasonable price will be grief attacked because only the highest bid is accepted.

    function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }

The highest bid is the attacker's bid and others cannot participate, right before auction is ended, the attacker cancel the bid and leave the auction with no bids.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, please do not allow bidders to cancel bids before auction is ended.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Should not minting new collection token before burning the old collection token, or the old collection may be leveraged by the owner before burning.

Proof of Concept

mintAndAuction allows collection token owner to mint a new collection token by burning the old one. The minting and burning is done through burnToMint in NextGenCore contract.

    function burnToMint(uint256 mintIndex, uint256 _burnCollectionID, uint256 _tokenId, uint256 _mintCollectionID, uint256 _saltfun_o, address burner) external {
        require(msg.sender == minterContract, "Caller is not the Minter Contract");
        require(_isApprovedOrOwner(burner, _tokenId), "ERC721: caller is not token owner or approved");
        collectionAdditionalData[_mintCollectionID].collectionCirculationSupply = collectionAdditionalData[_mintCollectionID].collectionCirculationSupply + 1;
        if (collectionAdditionalData[_mintCollectionID].collectionTotalSupply >= collectionAdditionalData[_mintCollectionID].collectionCirculationSupply) {
            _mintProcessing(mintIndex, ownerOf(_tokenId), tokenData[_tokenId], _mintCollectionID, _saltfun_o);
            // burn token
            _burn(_tokenId);
            burnAmount[_burnCollectionID] = burnAmount[_burnCollectionID] + 1;
        }
    }

The new collection token will be minted to the receiver before the old collection token is burned.

Given that the old collection token can be used as a collateral in a lending platform as it has value of mintpass. Consider a case where:

1. Token owner calls mintAndAuction from a contract to mint a new collection token;
2. New collection token is minted and transferred to the contract;
3. onERC721Received is called on the contract, contract uses the old collection token as collateral and borrows money from the lending platform;
4. Lending platform owns the old collection token but soon the token is burned by NextGenCore;
5. User gets new collection token and money.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, please burn the old collection token before minting the new one.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Should not pay back ETH to bidders while transferring token to winner, or bidders may not be successfully paid.

Proof of Concept

By calling claimAuction, the winner of an auction will get the token.

            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}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } 

And other bidders are getting ETH back.

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

The problem is that the transaction may fail when transferring the token to the winner, and this is possible if the receiver is a contract which does not implement the ERC721Receiver interface, especially for the onERC721Received function.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, please implement another method for the bidders to get ETH back.

Assessed type

ERC721