NextGen - 0x_6a70's results

Lines of code

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

Vulnerability details

Impact

Basically a user that wins the contest and calls claimAuction function can also reenter on the safeTransferFrom call and use cancelBid function to cancel his bid

Proof of Concept

Both functions share the same check :

 function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

And the cancelBid function:

function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

They both have the equal '=' in the check , this means that in precisely that moment when

block.timestamp == minter.getAuctionEndTime(_tokenid) 

, the malicious user can use:

IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); 

to reenter the cancelBid and withdraw his bid, which will leave him with the bid he entered to win the auction and the NFT!

Tools Used

Manual review

Recommended Mitigation Steps

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

This vulnerability may cause the contract to run out of funds and cause other auctions to be unable to be claimed and users will loose funds.

Proof of Concept

claimAuction and cancelBid has the same check for block.timestamp:

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

They both share the case where block.timestamp == minter.getAuctionEndTime(_tokenid) The other bidders in the auction with smaller than highestBid, bids can use the call function used to receive back their bids in claimAuction functionL

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

to reenter the cancelBid function and cancel their bid to get back twice their bid. This will leave the auctionDemo contract with less funds that intended and will cause other claimAuction, cancelBid and cancelAllBids function to revert because of no funds in the contract. Furthermore one malicious bidder can bid many times on one auction and than claim the auction and use those call function to drain all the funds in the contract.

Tools Used

Manual review

Recommended Mitigation Steps

CEI must be implemented and user status must be changed to false before sending tokens.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

A malicious receiver can forcibly revert transactions by reverting inside onERC721Received() or by using a loop to consume all the gas. Protocol should not assume that safeTransferFrom to an arbitrary address will succeed.

Proof of Concept

A user can forcibly revert the call in safeTransferFrom function:

 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 {}

This may lead to the user not being able to get the NFT that he won in the auction , but this will lead to DOS -attack and none of the remaining bidders won't be able to withdraw the funds stuck in the contract because there is no other function for them to use to get back their bids AFTER getAuctionEndTime:

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

Tools Used

Manual review

Recommended Mitigation Steps

Implement pull over push.

Assessed type

DoS