AI Arena - visualbits's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291-L303 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/141c947921cc5d23ee1d247c691a8b85cabbbd5d/contracts/token/ERC1155/ERC1155.sol#L120-L132

Vulnerability details

Description

In AiArena protocol, items will be created through the GameItems contract.

function createGameItem(
    string memory name_,
    string memory tokenURI,
    bool finiteSupply,
    bool transferable,
    uint256 itemsRemaining,
    uint256 itemPrice,
    uint16 dailyAllowance
) 
    public 
{
    require(isAdmin[msg.sender]);
    allGameItemAttributes.push(
        GameItemAttributes(
            name_,
            finiteSupply,
            transferable,
            itemsRemaining,
            itemPrice,
            dailyAllowance
        )
    );
    if (!transferable) {
        emit Locked(_itemCount);
    }
    setTokenURI(_itemCount, tokenURI);
    _itemCount += 1;
}

Due to the createGameItem function, each item can be either transferable or non-transferable, and this logic is enforced by restricting the ERC1155 safeTransferFrom method.

function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    uint256 amount,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    require(allGameItemAttributes[tokenId].transferable);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

However, the ERC1155 has another function, safeBatchTransferFrom, which handles multiple transfers, and GameItems contract does not override this function, allowing the transfer of tokens designated as non-transferable.

Impact

• The token owner or an approved account can transfer tokens despite their designation as non-transferable.

Proof of Concept

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

import {Test, console, stdError} from "forge-std/Test.sol";
import {Vm} from "forge-std/Vm.sol";
import {GameItems} from "@contracts/GameItems.sol";
import {Neuron} from "@contracts/Neuron.sol";
import "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";

contract AiArenaPOC is Test{
    address internal OWNER;
    address internal TREASURY = makeAddr("TREASURY");
    address internal CONTRIBUTORS = makeAddr("CONTRIBUTORS");
    address internal ALICE = makeAddr("ALICE");

    GameItems internal gameItems;
    Neuron internal neuron;

    constructor(){
        OWNER = address(this);

        gameItems = new GameItems(OWNER, TREASURY);
        neuron = new Neuron(OWNER, TREASURY, CONTRIBUTORS);

        neuron.addSpender(address(gameItems));

        gameItems.instantiateNeuronContract(address(neuron));
        gameItems.createGameItem("Battery", "https://ipfs.io/ipfs/", true, true, 10_000, 1e18, 10);
    }

    function test_CantTransferWhenItemIsNotTransferable() public {
        // Fund OWNER
        vm.prank(TREASURY);
        neuron.transfer(OWNER, 1000e18);
        assertEq(1000e18 == neuron.balanceOf(OWNER), true);

        // Create new game item (not transferable) and mint for address(this)
        gameItems.createGameItem(
            "Gloves", "https://ipfs.io/ipfs/gloves", false, false, 10_000, 1e18, 10
        );
        gameItems.mint(1, 1);

        // Transferring not transferable token to ALICE
        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        ids[0] = 1;
        amounts[0] = 1;
        gameItems.safeBatchTransferFrom(OWNER, ALICE, ids, amounts, ""); // It should be reverted but it hasn't been reverted.
        assertEq(gameItems.balanceOf(OWNER, 1), 0);
        assertEq(gameItems.balanceOf(ALICE, 1), 1);
    }

    function onERC1155Received(address, address, uint256, uint256, bytes memory) public pure returns (bytes4) {
        return this.onERC1155Received.selector;
    }
}

POC setup

foundry.toml:

[profile.default]
src = "src"
out = "out"
libs = ["lib"]
solc_version = "0.8.13"
optimizer = true
remappings = [
    "@openzeppelin/=lib/openzeppelin-contracts/",
    "forge-std/=lib/forge-std/src/",
    "ds-test/=lib/forge-std/lib/ds-test/src/",
    "@contracts/=src/"
]

forge test --mt test_CantTransferWhenItemIsNotTransferable

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Override safeBatchTransferFrom:

function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory amounts,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    require(allGameItemAttributes[tokenId].transferable);
    super.safeBatchTransferFrom(from, to, tokenId, amount, data);
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470

Vulnerability details

Description

Due to the design of the FigterFarm contract, the numElements for each generation is only set in the constructor. The contract does not provide any functionality to change numElements for subsequent generations; it only establishes numElements for the initial generation.

constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
    ERC721("AI Arena Fighter", "FTR")
{
    _ownerAddress = ownerAddress;
    _delegatedAddress = delegatedAddress;
    treasuryAddress = treasuryAddress_;
    numElements[0] = 3;
} 

When a player mints a fighter, the _createFighterBase function is executed. During this process, the element attribute of the fighter is calculated based on the fighter's DNA and the numElements of the fighter's generation.

function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

If the admin decides to increment the generation of a fighter type, and numElements remains unchanged, the previously mentioned function will fail due to a 'modulo zero' error when attempting to use numElements[nextGeneration] in the calculation.

Impact

• Players being unable to mint new fighters.

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

Create a function to modify the numElements in the next generation.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L502-L506 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L322-L330

Vulnerability details

Description

Due to the _createFighterBase function, the element and weight attributes must be within specific ranges:

• element should be in the range [0, 2].
• weight should be in the range [65, 95].

function _createFighterBase(
    uint256 dna, 
    uint8 fighterType
) 
    private 
    view 
    returns (uint256, uint256, uint256) 
{
    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
}

In the mintFromMergingPool function, there's a possibility for players to mint a fighter with attributes outside the designated range. This is because mintFromMergingPool utilizes a custom attribute parameter, unlike claimFighters and redeemMintPass, which rely on a 100 flag.

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to, 
        uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

In the _createNewFighter function, the weight and element attributes are not constrained to specific ranges.

function _createNewFighter(
    address to, 
    uint256 dna, 
    string memory modelHash,
    string memory modelType, 
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
) 
    private 
{  
    // Some code ...
    else {
        element = customAttributes[0];
        weight = customAttributes[1];
        newDna = dna;
    }
    // Some code ...
}

Impact

• Player can mint fighter with attrbutes out of standard range.

Tools Used

Manual Review

Recommended Mitigation Steps

Bound weight and element attributes within a specific range, similar to the _createFighterBase function.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L212-L220 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L252-L260 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L322-L330

Vulnerability details

Description

In Ai Arena, players can create new fighters through redeemMintPass, claimFighters, and mintFromMergingPool functions, each of which requires providing a dna to _createNewFighter. However, the calculation of dna in these functions is not entirely random and can lead to unfair outcomes.

In both the mintFromMergingPool and claimFighters functions, dna is calculated based on msg.sender and the length of the fighters array. While msg.sender is predictable because it reflects the address of the entity that initiates the transaction also fighters length is predictable. Thus, attackers can potentially manipulate their msg.sender to influence the resulting dna.

function claimFighters(
    uint8[2] calldata numToMint,
    bytes calldata signature,
    string[] calldata modelHashes,
    string[] calldata modelTypes
) 
    external 
{
    // ...
    for (uint16 i = 0; i < totalToMint; i++) {
        _createNewFighter(
            msg.sender, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))),
            modelHashes[i], 
            modelTypes[i],
            i < numToMint[0] ? 0 : 1,
            0,
            [uint256(100), uint256(100)]
        );
    }
}

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to, 
        uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

Moreover, in the redeemMintPass function, dna calculation lacks randomness as the player provides a string to calculate dna, making the result entirely not random.

function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
    //Some code...
    _createNewFighter(
        msg.sender, 
        uint256(keccak256(abi.encode(mintPassDnas[i]))), 
        modelHashes[i], 
        modelTypes[i],
        fighterTypes[i],
        iconsTypes[i],
        [uint256(100), uint256(100)]
    );
    // Some code...
}

The restricted randomness stemming from the use of the modulo operation on dna for attributes such as weight, element, and physical attributes also contributes to predictability. This makes it simpler for players to exploit these predictable patterns when minting rare attributes.

function _createFighterBase(
    uint256 dna, 
    uint8 fighterType
) 
    private 
    view 
    returns (uint256, uint256, uint256) 
{
    uint256 element = dna % numElements[generation[fighterType]];
    uint256 weight = dna % 31 + 65;
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
}

function createPhysicalAttributes(
    uint256 dna, 
    uint8 generation, 
    uint8 iconsType, 
    bool dendroidBool
) 
    external 
    view 
    returns (FighterOps.FighterPhysicalAttributes memory) 
{
    // Some code...
    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
    finalAttributeProbabilityIndexes[i] = attributeIndex;
    // Some code...
}

Players can mint a rare fighter each time they use redeemMintPass. However, for mintFromMergingPool and claimFighters, they need to wait until the length of the fighters array matches their prediction before minting a token.

Impact

• Players can unfairly mint rare fighters, undermining the fairness and integrity of the game.

Proof of Concept

In this POC, I focused on demonstrating the exploitation of the redeemMintPass function for simplicity and only exploit weight and element for simplicity (same for physical attributes) I assume rare attribute are this:

• Weight = 70
• element = 2

To attain these values, a player needs to pass a string that meets the following conditions:

1. dna % numElemnts == 2
2. dna % 31 == 5

To obtain this specific string, we can utilize foundry fuzzing.

function test_findString(string calldata str) public {
    uint256 num = uint256(keccak256(abi.encode(str)));
    // numElemnts -> 3
    if(num % 3 == 2 && num % 31 == 5){
        console.log(str);
        assert(false);
    }else{
        assert(true);
    }
}

By utilizing the output string, player can mint fighters with rare attributes.

Tools Used

Manual Review Chisel Foundry

Recommended Mitigation Steps

Get random numbers using Chainlink's VRF.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L147-L176 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L291-L303

Vulnerability details

Description

In the GameItems contract, players have the ability to mint game items using the mint function, with the constraint that they can only mint up to their daily allowance amount.

function mint(uint256 tokenId, uint256 quantity) external {
    // Some code ...
    if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
        _replenishDailyAllowance(tokenId);
    }
    allowanceRemaining[msg.sender][tokenId] -= quantity;
    if (allGameItemAttributes[tokenId].finiteSupply) {
        allGameItemAttributes[tokenId].itemsRemaining -= quantity;
    }
    _mint(msg.sender, tokenId, quantity, bytes("random"));
    emit BoughtItem(msg.sender, tokenId, quantity);
}

But players can bypass the daily allowance limit by minting tokens from another account and transferring them to their own account, enabling them to claim items beyond their daily allowance.

Impact

• Players can bypass daily allowance restrictions.

Tools Used

Manual Review

Recommended Mitigation Steps

Check daily allowance in transfer functions.

function safeTransferFrom(
    address from, 
    address to, 
    uint256 tokenId,
    uint256 amount,
    bytes memory data
) 
    public 
    override(ERC1155)
{
    require(allGameItemAttributes[tokenId].transferable);
    require(getAllowanceRemaining(to, tokenId) > 0);
    super.safeTransferFrom(from, to, tokenId, amount, data);
}

Assessed type

Other