A cunning user can always win the auction and claim the token being auctioned for free

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L58
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L105
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#L135
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#L116

Vulnerability details

Impact

As described in the Proof of Concept section below, a cunning user is able to always win the auction and claim the auctioned token for free.

Proof of Concept

The file AuctionDemo.sol contains the functions: participateToAuction, claimAuction, cancelBid and cancelAllBids:

• participateToAuction function is intended to be used when the auction is OPEN
• claimAuction function is intended to be used when the auction is CLOSED
• cancelBid function is intended to be used when the auction is OPEN
• cancelAllBids function is intended to be used when the auction is OPEN

However, based on the implementations of these functions, exactly at the moment block.timestamp == minter.getAuctionEndTime(_tokenid), the auction is both OPEN and CLOSED at the same time. The snippets from these functions allowing an auction to be OPEN and CLOSED at the same time are given below:

• participateToAuction function: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L58C59-L58C112
• claimAuction function: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L105C17-L105C70
• cancelBid function: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L125C17-L125C70
• cancelAllBids function: https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L135C17-L135C70

The root of the issue is that non-strict inequalities are used: >= and <= instead of strict inequalities: > and < when block.timestamp and minter.getAuctionEndTime(_tokenid) are compared.

The fact that at block.timestamp == minter.getAuctionEndTime(_tokenid) the auction is both OPEN and CLOSED can be exploited and the following scenario is possible:

1. Alice calls the participateToAuction function and makes the highest bid up to the moment for the token being auctioned. The transaction becomes part of a block for which block.timestamp == minter.getAuctionEndTime(_tokenid) is true.

2. Alice calls the claimAuction function in a transaction that is part of a block for which block.timestamp == minter.getAuctionEndTime(_tokenid) is true. By calling the claimAuction function, Alice wins the auction (she has the highest bid) and gets the token. Because the return value of the low-level call function is not checked in the following 2 places:

• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L113C17-L113C81
• https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L116C17-L116C131

there is a possibility that the call wasn't successful, but there is no revert. In case these call function invocations fail, bidders and the original owner of the token being auctioned will not receive ETH and some ETH amount will remain inside the auctionDemo contract.

3. Alice calls the cancelBid function in a transaction that is part of a block for which block.timestamp == minter.getAuctionEndTime(_tokenid) is true. As a result of the cancelBid function invocation, Alice can claim the ETH amount from her winning bid back (this might happen if the auctionDemo contract has enough ETH, which can be possible if there are failing call function invocations in the claimAuction function beforehand, which is possible, since the return values of the call function invocations in claimAuction are not checked).

To recap, following this strategy:

• Alice always wins the token auction, because she makes the winning bid at the last possible moment (block.timestamp == minter.getAuctionEndTime(_tokenid)). Since the auction is both OPEN and CLOSED at that moment, she is able to claim the token being auctioned right after making her winning bid.

• Alice takes the ETH amount from her winning bid back by cancelling her winning bid at the moment block.timestamp == minter.getAuctionEndTime(_tokenid), right after claiming the auctioned token.

So, Alice managed to win the auction and get the auctioned token for free.

Tools Used

Manual review.

Recommended Mitigation Steps

To mitigate the possibility of a cunning user always winning the auction, we should make sure that in no moment in time is it possible the auction to be both OPEN and CLOSED. This can be achieved by modifying the claimAuction function in the way shown below:

    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}("");
>>>>>           require(success, "ETH transfer failed");           <<<<<
                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}("");
>>>>>           require(success, "ETH transfer failed");           <<<<<
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

The essential change is on the line starting with >>> and ending with <<< and the change consists of replacing block.timestamp >= minter.getAuctionEndTime(_tokenid) with block.timestamp > minter.getAuctionEndTime(_tokenid). This way a cunning user cannot make the winning bid and win the auction in the same block. This fix also ensures that the auction winner cannot take his winning bid back, since the cancelBid function cannot be successfully called after the claimAuction function, because cancelBid requires the auction to be OPEN and claimAuction requires the auction to be CLOSED and the fix ensures there is no moment in time when the auction is both OPEN and CLOSED. So, the cancelBid function can be called successfully only before the claimAuction function.

For completeness, I've also added checks for the success of the ETH transfer in the mitigated code snippet above, although that's something already known from the bot report. The lines of the checks added start with >>>>> and end with <<<<<.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #1904

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as duplicate of #1370

[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