NextGen - inzinko's results

Lines of code

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

Vulnerability details

Impact

In the claimAuction and cancelBid Function, their is a single opportunity that at the right timing a malicious winner that won the auction, can prepare both the two function transactions, before the auction end time, then send the transactions to the mempool, which will get exactly mined at the right time the windows open with this kind of vulnerability, but with the claimAuction executing their functions, first and then the cancelBid following. If all this process happens, then the user can both claim The NFT and also get paid in a refund for the same NFT they just claimed. A user could potentially execute claimAuction right after the auction ends (thus transferring the NFT to themselves and sending the highest bid to the owner) and then execute cancelBid (thus refunding their highest bid amount) if both transactions are mined in the same block

Proof of Concept

Let's Explore how this Vulnerability can happen and what it will take to actually make it happen

Analysis of claimAuction Function

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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 {}
        }
    }

Checks: It requires that the auction has ended, the auction hasn't been previously claimed, and the auction is still active require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true). Claim Process: If the conditions are met, it marks the auction as claimed, transfers the NFT to the highest bidder, and sends the highest bid amount to the owner of the contract.

Analysis of cancelBid Function

function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

Checks The function requires that the auction hasn't ended yet block.timestamp <= minter.getAuctionEndTime(_tokenid) and that the sender is the bidder of the specific bid they are trying to cancel, and the bid is active. Cancellation Process: If the conditions are met, it sets the bid status to false (canceled) and refunds the bid amount to the bidder.

Potential Exploit Scenario Timing: executing claimAuction just after the auction ends and then immediately executing cancelBid while still within the same block, assuming the timestamp hasn't updated yet.

Outcome: If both transactions are included in the same block and if cancelBid doesn't check whether the auction has already been claimed, the winner might be able to claim the NFT and then cancel their bid to get their money back. Executing two transactions in the same block with precise timing is challenging but not impossible. It would require a good understanding of Ethereum transaction mechanics and possibly some luck.

The main Cause of this kind of Vulnerability, is the window of opening of the required checks in which there is a chance that if both are called at the same time the user might get away with this exploit.

When a NFT Auction winner tries to call the Claim Auction function and gets their NFT Transferred to them, their bid struct status is not set to false as seen down below

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

This Makes this kind of attack a possibility as the cancelBid function checks to see if the auction bid struct status is still true, as shown below

require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);

This kind of issue creates a Window For the Malicious user to execute. All things considered, the fact that a huge profit can be gained by this kind of exploit mainly based on Expensive NFTs and the simple fix that it will take to mitigate the risk, makes it a high-priority fix for the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

The Mitigation for this is simple as i have pointed out earlier in my report, their is actually no reason for the check in the claimAuction function to have a = sign when checking if the time allotted to the auction has ended, it can simply have a simple > check, Then this issue is completely fixed.

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

There is another fix for this kind of vulnerability, if you add a check to the cancel bid to check if the auction has been claimed before, then it should revert. But I think that the first one is simply the best way to resolve this issue.

Assessed type

MEV