Auction manipulation and DoS by canceling high bids

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Any auction could be manipulated and disrupted by placing a very low initial bid. Subsequently, the attacker places a really high bid and/or consistently front-run bids, dissuading other bidders from participating. Just before the auction concludes, all but the first attacker's bids are canceled and refunded, enabling the attacker to win the auction with the initially low bid, incurring minimal costs, primarily limited to gas expenses.

The current implementation of cancelAllBids() and cancelBid() allows for the cancellation of any bid, even if it happens to be the winning bid:

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        ...

    function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
            if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true) {
          ...

Proof of Concept

Modify your fixtures hardhat/scripts/fixturesDeployment.js to add a 4th addr:

const { ethers } = require("hardhat");

// Setup test environment:
const fixturesDeployment = async () => {
  const signersList = await ethers.getSigners();
  const owner = signersList[0];
  const addr1 = signersList[1];
  const addr2 = signersList[2];
  const addr3 = signersList[3];
  const addr4 = signersList[4];

  const signers = {
    owner: owner,
    addr1: addr1,
    addr2: addr2,
    addr3: addr3,
    addr4: addr4,
  };
   ...

Use this modified nextGen.test.js that:

1. Sets the collection
2. Mints and Auctions an NFT to addr2
3. Bids with a low bid as addr3 intended to win
4. Places a blocking high bid with addr4, attacker will keep placing high bids to block other users
5. Cancels and gets refunded for all addr4 bids before auction ends
6. Checks thats addr3 wins the auction with the low bid

Tools Used

vs code, hardhat

Recommended Mitigation Steps

Modify the cancelAllBids() and cancelBid() to check that the highest bid is not canceled:

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bid < returnHighestBid(_tokenid), "Can not cancel HighestBid");

    function cancelAllBids(uint256 _tokenid) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        uint256 highestBid = returnHighestBid(_tokenid);
        for (uint256 i=0; i<auctionInfoData[_tokenid].length; i++) {
            if (auctionInfoData[_tokenid][i].bidder == msg.sender && auctionInfoData[_tokenid][i].status == true && auctionInfoData[_tokenid][index].bid < highestBid) {
                auctionInfoData[_tokenid][i].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit CancelBid(msg.sender, _tokenid, i, success, auctionInfoData[_tokenid][i].bid);
            } else {}
        }
    }

Additionally, I would like to suggest incorporating a minBidIncreasePercentage check to ensure that a new bid is a certain percentage larger than the last high bid. This measure is intended to discourage sniping by 1 wei or front-runners.

Assessed type

DoS

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

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)