AuctionDemo.sol: Bidding on auctions, cancelling bids, and ending/claiming of auctions can occur in the same block, resulting in a myriad of consequences including locking of user funds, refunding users too much ETH, and reentrancy drainage of the contract

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L57-L61
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104-L143

Vulnerability details

Impact

If a call to participateToAuction() (bidding) is included in the same block after the call to claimAuction() for that auction, then the user's bid will be stuck in the contract with no easy way to get it out. If a call to cancelBid() for a specific auction is included in the same block after the call to claimAuction() for that auction, then the user will be refunded double the amount of that bid. Draining of the contract via reentrancy is also possible.

The only way to retrieve the stuck bidding funds is to exploit the contract.

Proof of Concept

The three functions below implement bidding, claiming, and cancelling a bid:

    function participateToAuction(uint256 _tokenid) public payable { // Bid
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true); // MinterContract never sets auction status back to false, only from false to true
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }
    ...
    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){ // Refund non-highest bids and send the NFT to the highest bidder
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true; // claimAuction can only be called once for any tokenid; see the middle check in the line above
        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 {}
        }
    }
    
    function cancelBid(uint256 _tokenid, uint256 index) public { // Cancel a bid and refund the user
        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);
    }
    // Below function included for reference, does the same thing as cancelBid for all a user's bids
    function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
            if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true) {
                auctionInfoData[_tokenid][i].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
            } else {}
        }
    }

claimAuction should not be called until the auction is over, whereas bidding and cancelling bids should not be possible unless the auction is still active. However, notice that while all three functions check block.timestamp against minter.getAuctionEndTime(), the checks are all <= or >=. This is a problem, because cancelBid() and participateToAuction() can be called after claimAuction() if block.timestamp is equal to minter.getAuctionEndTime() and all the transactions are included in the same block. Note that there are no other checks that prevent this from happening.

Note that cancelBid() sets auctionInfoData[_tokenid][index].status to false in order to prevent one bid from being cancelled multiple times (and refunding the user multiple times). However, claimAuction() doesn't set auctionInfoData[_tokenid][index].status to false. Therefore, if a user calls cancelBid() and the transaction is executed after the relevant claimAuction() transaction, the user will be refunded for double their bid. (If the user happens to be the highest bidder, then the user will be refunded and receive the NFT for free.)

Another issue can occur if participateToAuction() is called after claimAuction(). In this case, the bid ETH will be stuck in the contract since claimAuction() can only be executed once for an auction, and the bidder cannot cancel the bid for a refund after the auction has ended.

Finally, the most severe consequence is an effectively complete drainage of the funds in AuctionDemo. See the example below:

1. Attacker makes one of the first bids in an auction using an attacker-controlled contract, and repeatedly bids in that auction using that contract to pack the front of the bids array so that the attacker's bids are refunded in the call to claimAuction() before many bids are refunded back to other users. The total bid amount by the attacker should be approximately half of the total ETH balance of the AuctionDemo contract. Note that the attacker can bid a greater total amount if anticipating that other users will bid more after the attacker.
2. If block.timestamp does not coincide with the auction ending time, the attacker's bids are refunded to the attacker. If block.timestamp does coincide with the ending time, then the following steps occur:
3. claimAuction() is called, and when the last bid from the attacker is refunded to the attacker's contract, the attacker's payable fallback function reenters the contract.
4. The attacker's contract reenters AuctionDemo, calling cancelAllBids(). (See above code block. This function does the same thing as cancelBid(), but for all of msg.sender's bids. The attacker's contract can also reenter cancelBid() multiple times to achieve the same effect, or to achieve maximum drainage if the total of the attacker's bids is greater than half the balance of AuctionDemo.)
5. The total bid amount of the attacker is sent to the attacker's contract a second time, draining AuctionDemo.
6. claimAuction() finishes executing, but the low-level ETH transfers to other users fail since the contract's balance is too low.

Since none of the success bools returned by the low-level transfers are checked, it's easy for the attacker to completely drain the contract; claimAuction() won't error if it attempts to transfer bids back to other users when the ETH balance of the contract is too low.

Note that the attacker does not want to call cancelAllBids() before all of the transfers to the attacker in claimAuction() occur, since claimAuction() checks that auctionInfoData[_tokenid][i].status == true, and cancelAllBids() will set these values for the attacker's bids to false.

Recommended Mitigation

The fix for all these issues is simple; eliminate the possibility of claiming occurring in the same block as bidding and cancelling bids:

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

Additional Recommendation

A reentrancy lock should be added to the relevant functions to eliminate the risk of reentrancy vulnerabilities, although it's not strictly necessary to fix the issues here.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #962

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1788

[c4-judge]

alex-ppg marked the issue as satisfactory