NextGen - TuringConsulting's results

Lines of code

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

Vulnerability details

Impact

Reentrancy in claimAuction() allows a user to obtain an auctioned NFT for free by reentering cancelBid() while claiming the auction, essentially allowing an attacker to obtain the auctioned NFT, and then immediately obtaining its their amount refunded.

Proof of Concept

When a user is the highest bidder, the claimAuction() must be triggered so that the NFT can be claimed, and the bid amount can be transferred to the contract owner:

 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) {
             ...
            } else {}
        }
    }

We can see how the previous block of code allows block.timestamp to be higher or equal to the minter.getAuctionEndTime(_tokenid). If now we analyze the cancelBid() method, we'll see that block.timestamp is required to be smaller or equal to minter.getAuctionEndTime(_tokenid). This essentially means that cancelBid() and claimAuction() can be called in the same block.

An attacker can leverage the fact that such timestamp checks are poorly performed in order to craft a reentrancy attack, where the bid can be cancelled after obtaining the NFT for free, but before actually transferring the bid funds to the contract owner. The following steps illustrate how such attack would take place:

1. The highest bidder (a contract with the onERC721Received() transfer hook) executes the claimAuction() function so that it can obtain the NFT, and such execution is triggered exactly at timestamp minter.getAuctionEndTime(_tokenid)
2. When the loop reaches the point where the NFT needs to be transferred to the highest bidder, the safeTransferFrom() transfer is performed, sending the NFT to the malicious highest bidder contract.
3. The malicious NFT contract with the onERC721Received() transfer hook will then execute and reenter the cancelBid() function. Because block.timestamp is equal to minter.getAuctionEndTime(_tokenid), the function will execute gracefully, effectively refunding the highest bidder with its bid ETH amount.
4. When cancelBid() ends, execution will move back to the initial claimAuction() flow. Execution jumped when transferring the NFT from claiming the auction to cancelling the bid, so now that execution has been returned to claiming the auction, next step is to refund the owner. Because the contract holds ETH from other users and auctions, the owner will correctly obtain their corresponding highestBid amount, and execution will finish gracefully, effectively claiming an auctioned NFT while also obtaining the highest bid funds back

Tools Used

Manual review

Recommended Mitigation Steps

Add a reentrancy guard for claimAuction(). An effective way to add such checks is by using OpenZeppelin's Reentrancy guard.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

It is possible to cause a denial of service in the claimAuction() function by bidding with a contract that fully consumes the gas allocated for the transaction. Because claimAuction() loops over all bidders to refund bidders who lost or transfer the NFT to the auction winner, this function can be completely DoS'ed:

function claimAuction(
        uint256 _tokenid
    ) public WinnerOrAdminRequired(_tokenid, this.claimAuction.selector) {
        ...
        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 {}
        }
    }

Proof of Concept

Suppose a malicious contract bids an amount in an auction. With this amountonly being of 1 wei the attack can be performed. The auction takes place, with several bidders bidding, and eventually it reaches an end. claimAuction() should now be executed so that the highest bidder can obtain the auctioned NFT, and all other bidders can get their bid amounts back.

Because claimAuction() will try to refund all bidders (our malicious contract included), a malicious contract can be crafted with a fallback() function that tries to consume all gas allocated for the call when receiving the refunded ETH. If this occurs, the claim transaction will ALWAYS run out of gas due to the fact that our malicious contract will be successfully consuming all the allocated gas.

This will prevent the auction from ever being claimed, effectively preventing loser bidders from such auction from ever obtaining their bidded ETH back, while also making the highest bidder not capable of getting their corresponding NFT for winning the bid.

Tools Used

Manual review

Recommended Mitigation Steps

It is recommended to incorporate the "Pull over push" pattern. Essentially, the idea is to make users execute a transaction to obtain their own ETH or NFT back, instead of looping through all bidders to "push" the amounts to them

Assessed type

DoS