Reentrancy in claimAuction()

[c4-submissions]

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

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as satisfactory