Bidders whose bid settles at auction end timestamp might have their bidding funds permanently lost to them

[c4-submissions]

Lines of code

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#L58

Vulnerability details

Impact

In AuctionDemo.sol, the participateToAuction() and claimAuction() flows have vulnerabilities that will likely result in bidders permanently losing their bidding funds when they submit their bids close to the auction end timestamp.

And since bidding close to the auction end timestamp is expected bidding behavior that happens regularly in auctions where a bidding war is encouraged until the auction ends, I think this is a high-severity issue.

Proof of Concept

There are (2) vulnerabilities at work here.

(1) The boundary of the time range between the active auction period and the auction end is not cleanly handled. A time overlap is allowed at auction end timestamp;
(2) When an auction is claimed - the nft is minted and bidding funds are returned, the auction claim status is not checked in participateToAuction();

On top of the (2) above, claimAuction() allows the winner to call the function to claim their nft and distribute bidding funds, which is essentially making claimAuction() publicly available to whoever has the highest bid at the auction end timestamp.

// smart-contracts/AuctionDemo.sol
    function participateToAuction(uint256 _tokenid) public payable {
// @audit auction end time is allowed here
|>      require(
            msg.value > returnHighestBid(_tokenid) &&
                block.timestamp <= minter.getAuctionEndTime(_tokenid) &&
                minter.getAuctionStatus(_tokenid) == true
        );
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }
...

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

// smart-contracts/AuctionDemo.sol
    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
// @audit auction end time is allowed here
|>      require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
// @audit auction claimed status is reset, however, this is never checked in `participateToAuction()`
        auctionClaim[_tokenid] = true;
...

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

As seen above, when users trying to outbid each other close to the auction end, their transaction might settle at minter.getAuctionEndTime(_tokenid), in which case, their bidding funds will be paid and bidding will be successful.

However, to solidify their win, the existing highest bidder will claimAuction() at the earliest which is also the same timestamp as minter.getAuctionEndTime(_tokenid). In such cases, the bidders whose transaction is settled after the existing highest bidder will have their funds lost. In addition, calling cancelBid() after the fact to recover their bidding funds will be too late since the transaction will revert when the timestamp passes the auction end time.

We can see that in AuctionDemo.sol there is no emergency withdrawal or funds sweeps mechanism to recover any funds locked in the scenario above.

Tools Used

Manual Review

Recommended Mitigation Steps

In paricipateToAuction, either change the require statement into block.timestamp < minter.getAuctionEndTime(_tokenid), or add check to ensure auctionClaim[_tokenid] != true.

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-sponsor]

a2rocket (sponsor) confirmed

[c4-judge]

alex-ppg marked issue #1547 as primary and marked this issue as a duplicate of 1547

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1926

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)