Losing bids can be refunded twice

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

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

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(
            block.timestamp <= minter.getAuctionEndTime(_tokenid),
            "Auction ended"
        );

At block.timestamp == minter.getAuctionEndTime(_tokenid), it is possible to call both of these functions. This oversight creates an opportunity for a severe theft of funds.

Proof of Concept

1. Adversary's contract makes few bids at the start of the auction
2. Adversary's contract eventually gets outbid
3. Adversary's contract makes a sufficiently high bid
4. t = minter.getAuctionEndTime(_tokenid). Adversary's contract calls claimAuction. claimAuction refunds his first bid, which triggers Adversary's fallback, which calls cancelBid, which successfully refunds Adversary's bid again. The same happens for each of adversary's bids, except the winning one.

Example of Adversary's fallback:

    uint public index;
    bool cancel = true;

    fallback() external payable {
        if (index < 9) {
            if (cancel) {
                cancel = false;
                auctionDemo(msg.sender).cancelBid(tokenId, index++);
            } else {
                cancel = true;
            }
        } else {
            owner.call{value: address(this).balance}("");
        }
    }

Full Foundry PoC (works with and without require(success))

Depending on whether the mitigation for the M-01 finding from the bot race is implemented, there are two ways of how the Step 4 will play out:

require(success) is not implemented:

1. Every losing bid is refunded to the bidders of the auction.
2. Each refund of adversary's bids triggers the fallback, which calls cancelBid for the respective bid.
3. The owner of the auctioned NFT will not get their payment as the contract will run out of ether. Depending on the size of Adversary's bids, some of the highest bids may not be refunded for the same reason.

require(success) is implemented:

require(success) after each call will make sure that every ether transfer has to be successful, or the whole transaction reverts. This means that the owner and all other bidders must get their ether back. If there is only one auction, and the Adversary tries to execute the original attack, the txn will revert, as at least one of the refunds will fail (due to insufficient balance of AuctionDemo).

Thus, the attack will not work unless there are other funds in the contract, but there usually will be, because the AuctionDemo contract may host multiple auctions at once.

To sum up:

if require(success) is used, the Adversary will need at least one other auction going in parallel for the attack to be successful;

if require(success) is not used, the attack won't have such requirement and will work out without other auctions.

Option to cancel the winning bid

The Adversary can also decide if he wants to pay for the NFT, or just get his winning bid back. Similarly to the previous strategy, he may add a call to his fallback, that would cancel his winning bid. This will essentially make the owner keep the NFT, and refund every bidder of the auction.

            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);

If the winning bid is cancelled, the condition will always be false, as the highest bid will have status == false

Draining of all other auctions

The contract also allows to make bids at t == minter.getAuctionEndTime(_tokenid). So the doubling strategy can be upgraded to draining all other auctions' funds.

Scenario:

1. Hacker's contract calls claimAuction
2. claimAuction refunds his bid
3. Hacker's fallback calls participateToAuction
4. claimAuction refunds his new bid
5. Hacker's fallback calls cancelBid
6. cancelBid calls Hacker's fallback

Steps 3-6 are repeated, each time with a bigger bid.

However, this particular scenario becomes impossible if the array length is cached (G-20 from the bot report).

Tools Used

Foundry

Recommended Mitigation Steps

    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);

Assessed type

Timing

[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