Bidder Funds Can Become Unrecoverable Due to 1 second Overlap in participateToAuction() and claimAuction()

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Bidder funds may become irretrievable if the participateToAuction() function is executed after claimAuction() during a 1-second overlap.

Proof of Concept

The protocol allows bidders to use the participateToAuction() function up to the auction's end time.

    function participateToAuction(uint256 _tokenid) public payable {
      ->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);
    }

However, the issue arises when an auction winner immediately calls claimAuction() right after the auction concludes, creating a 1-second window during which both claimAuction() and participateToAuction() can be executed.

    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) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

The issue arises when claimAuction() is executed before participateToAuction() within this 1-second overlap. In such a scenario, the bidder's funds will become trapped in AuctionDemo.sol without any mechanism to facilitate refunds. Both cancelBid() and cancelAllBids() functions will revert after the auction's conclusion, making it impossible for bidders to recover their funds.

Tools Used

Manual Review

Recommended Mitigation Steps

    function participateToAuction(uint256 _tokenid) public payable {
-       require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
+       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);
    }

Assessed type

Context

[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 #1926

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The Warden has clearly specified what the vulnerability is, has provided a recommended course of action that aligns with best practices, and has specified all aspects of the contract that would fail for the user if they tried to reclaim their lost funds.

The likelihood of this exhibit manifesting in practice is relatively low (requires a block.timestamp that exactly matches the auction). In the post-merge PoS Ethereum that the project intends to deploy, blocks are guaranteed to be multiples of 12 and can only be manipulated as multiples of it.

The impact is high as the funds of the user are irrecoverably lost even with administrative privileges as no rescue mechanism exists, rendering this exhibit a medium severity issue.

[c4-judge]

alex-ppg marked the issue as satisfactory

[ZdravkoHr]

This and 1323 should be duplicates. They both have found the same root cause, but are showing different impacts. From the docs:

All issues which identify the same functional vulnerability will be considered duplicates

[mcgrathcoutinho]

Hi @ZdravkoHr, I disagree that these both have the same root cause.

The issue here is that "<=" is used instead of "<" in participateToAuction() while #1323 mentions the same but in claimAuction(). Solving #1323 does not solve this issue here since funds can still be locked. Additionally, for #1323 there are two mitigations, either 1) using "<" instead of "<=" in claimAuction() or 2) setting auctionInfoData to false for each bid in the for loop of claimAuction. Since we do not know which mitigation the sponsor may apply, it is best to keep this separate.

[0xbtk]

Hey @alex-ppg, I think this should be partial of #1323. The warden identified the root cause but didn't cover the full impact. Otherwise you should consider splitting re-entrancies and back-running attacks into separate issues.

Regarding the warden's above comment, fixing #1323 will fix this as there aren't two mitigations; they're the same and the sponsor needs to apply them both to fix #1323.

[alex-ppg]

Hey @ZdravkoHr, @mcgrathcoutinho, and @0xbtk; thanks a lot for your feedback! I will attempt to address everyone's response in this reply.

Root Cause

The project contained multiple vulnerabilities that can be argued to have the same root cause. The root cause of this submission is that it is possible to participate in an auction beyond its end. As @mcgrathcoutinho specified earlier, this is a distinct case as #1323 does not rely on the participateInAuction function being executable at a specific timestamp.

If we know both issues, of course, we can say that the expected remediation is to simply code claimAuction in a way that does not permit the overlap for both simultaneously. However, each submission is meant to be treated in a black box and as such, adjusting bid cancellations to not be possible at the last second is an entirely acceptable remediation of #1323 that would not solve this submission.

Re-Entrancies

Re-entrancies are indeed split into separate attacks. You can observe this by evaluating #1597 and #1517 which use the same re-entrancy origin differently. In general, a re-entrancy attack's root cause is the absence of the CEI pattern; not the re-entrancy itself which is a trait of the EVM. If there are instances where you believe re-entrancy attacks have been merged incorrectly, please flag them and I will review them.

Back-Running

Back-running attacks and re-entrancy attacks are the same when it comes to this particular exhibit. A back-running attack is the prime vulnerability / root cause because the time overlap permits a cancellation to occur at the same timestamp as a claim. If we fix re-entrancy, we will not fix the back-running attack but if we fix the back-running attack the re-entrancy will no longer be possible.

As such, re-entrancy is simply a tool via which time inconsistency is exploited. Whether you choose to exploit it via a re-entrancy or a back-running attack; they are equivalent. If you mention re-entrancy guards as the correct fix, however, your submission will be penalized.

Please let me know if you have any further concerns.