AI Arena - radin100's results

Lines of code

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

Vulnerability details

Impact

Main invariant broke due to bad access validation

Proof of Concept

As you can see on https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262 the only restrictions to mint fighter NFT from 'FighterFarm::redeemMintPass' are to be the owner of mintPassToken and equal array lengths, which mean that any user with mintPassToken ownership could choose the fighter and icon type, which makes all unique NFTs not unique anymore

Tools Used

Recommended Mitigation Steps

Import requires right into 'FighterFarm::redeemMintPass' that paste limits on the users choices for their future fighter NFT properties

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Break invariant due to bad validation or missed logic

Proof of Concept

Details: The main part of the problem is that in 'FighterFarm.sol::reRoll' function users are allowed select any fighter type they want and there could be mismatch with the actual fighter type that comes from the given id. This could lead to extra reRolls for the fighter type with less maxReRollsAllowed

Proof of Code: !!!Note that before running this code you should change the numElements mapping from 'FighterFarm.sol::incrementGeneration' to avoid any zero division reverts from 'FighterFarm.sol::_createFighterBase'!!!

function testExtraRerolls() public {
    address user = makeAddr("USER");

    // Minting fighter to user with fighterType 0
    _mintFromMergingPool(user);
    _fundUserWith4kNeuronByTreasury(user);

    // Incrementing generation for fighterType 1
    vm.prank(_ownerAddress);
    _fighterFarmContract.incrementGeneration(1);
    uint8 tokenId = 0;
    uint8 fighterType = 1;

    _neuronContract.addSpender(address(_fighterFarmContract));

    vm.startPrank(user);
    _fighterFarmContract.reRoll(tokenId, fighterType);
    _fighterFarmContract.reRoll(tokenId, fighterType);
    _fighterFarmContract.reRoll(tokenId, fighterType);
    _fighterFarmContract.reRoll(tokenId, fighterType);
    vm.stopPrank();

    assert(
        _fighterFarmContract.numRerolls(0) >
            _fighterFarmContract.maxRerollsAllowed(0)
    );
}

Tools Used

Recommended Mitigation Steps

There two options to solve this problem depends on the application logic:

1. Change the figtherType to be equal to given one

   function reRoll(uint8 tokenId, uint8 fighterType) public {
        ...
        ...
        ...
        + fighters[tokenId].dendroidBool = bool(fighterType);
        fighters[tokenId].element = element;
        fighters[tokenId].weight = weight;
        fighters[tokenId].physicalAttributes = _aiArenaHelperInstance
            .createPhysicalAttributes( // view function!!!
            newDna,
            generation[fighterType],
            fighters[tokenId].iconsType,
            fighters[tokenId].dendroidBool
        );
        _tokenURIs[tokenId] = "";
    }
   

   }

2. Revert if fighters[tokenId]'s fighterType != fighterType

   function reRoll(uint8 tokenId, uint8 fighterType) public {
       +require(fighters[tokenId].dendroidBool == bool(fighterType))
    ...
    ...
    ...
    }
   

   }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

DoS of main function due to dividing by 0

Proof of Concept

Details: There many different scenarios about this issue, but the main part is that after single FighterType's generation increment, all functions that use to execute 'FighterFarm.sol::_createFighterBase' for this FighterType is going to revert no matter the input values are, because 'numElements[generation[fighterType]]' from uint256 element implementation(on L470) will always give zero. Here one example scenario.

Scenario:

1. Owner decides to increment generation by calling 'FighterFarm.sol::incrementGeneration'
2. By calling this function only the generation and the maxRolling of given fighterType are changed
3. User decides to mint new fighter NFT by calling 'FighterFarm.sol::claimFighters' or 'FighterFarm.sol::redeemMintPass'
4. After calling any of these 2 functions they are about to execute 'FighterFarm.sol::_createNewFighter' with last argument equal to [uint256(100), uint256(100)]
5. Therefore first if check in 'FighterFarm.sol::_createNewFighter' will pass which leads to 'FighterFarm.sol::_createFighterBase' execution(second argument given must have the same value as the fighterType incremented at the beginning)
6. 'FighterFarm.sol::_createFighterBase' is going to revert because to get first variable(element) the function is about to divide by 0

Proof of Code: Import the code below in FighterFarm.t.sol and don't forget to add import {stdError} from "forge-std/StdError.sol"; at the beginning:

function testClaimFightersRevertsAfterIncrementGeneration() public {
    // Owner(address(this)) increment generation
    _fighterFarmContract.incrementGeneration(0);

    // Pepearing variables for 'FighterFarm.sol::claimFighters'
    uint8[2] memory numToMint = [1, 0];
    bytes memory claimSignature = abi.encodePacked(
        hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
    );
    string[] memory claimModelHashes = new string[](1);
    claimModelHashes[
        0
    ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    string[] memory claimModelTypes = new string[](1);
    claimModelTypes[0] = "original";

    vm.expectRevert(stdError.divisionError);
    _fighterFarmContract.claimFighters(
        numToMint,
        claimSignature,
        claimModelHashes,
        claimModelTypes
    );
}

Tools Used

Recommended Mitigation Steps

1. To prevent this from happening consider implementing some logic into 'FighterFarm.sol::incrementGeneration' that will update the numElements mapping like shown below

   function incrementGeneration(uint8 fighterType, uint8 newNumElements) external returns (uint16) { require(msg.sender == _ownerAddress); generation[fighterType] += 1; maxRerollsAllowed[fighterType] += 1; +numElements[generation[fighterType]] = newNumElements; return generation[fighterType]; }

Note that both fighterTypes is going to have the same number of elements

2. Consider adding new mapping that points to numElements with figtherType and generation given

3. Think of using numElements as constant value

4. Add updateElement function to provide only admin/owner changes to numElement

Assessed type

DoS