AI Arena - EagleSecurity's results

Lines of code

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

Vulnerability details

Impact

The number of elements for a generation is hardcoded in the constructor of the FighterFarm contract for the initial (0th) generation. However, when the generation for either a champion or a dendroid is incremented, the number of elements for that generation defaults to 0. As a consequence, when a user attempts to reroll, the calculation of the element for the fighter, which includes modulo by the number of elements for a generation, reverts. This is because division or modulo by zero is not possible, effectively creating a Denial of Service (DoS) scenario for absolutely every reroll thereafter. From then on one of the most important features is gone and absolutely ruining the game for its users.

Proof of Concept

The following test with documentation is a solid proof of the massive impact this vulnerability causes. To simulate the following test you should add the test in the FighterFarm.t.sol file and execute the forge test --mt testRerollRevertWhenGenerationIsIncremented -vvvv Finally, you can also change the fighterType and incrementGeneration values to 1 and the result will be the same.

    /// @notice Test that the reroll fails if generation is incremented
    function testRerollRevertWhenGenerationIsIncremented() public {
        _mintFromMergingPool(_ownerAddress);
        // First we mint a fighter of type 0 ( Champion )

        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // The user is funded with 4000 Neuron tokens by the treasury

        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            // Checking if the owner of the fighter with ID 0 is the owner's address

            uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
            // Storing the neuron balance of the owner before the reroll

            uint8 tokenId = 0;
            // Token ID of the fighter to reroll

            uint8 fighterType = 0;
            // Type of fighter to reroll (Champion)

            _neuronContract.addSpender(address(_fighterFarmContract));
            // Adding the fighter farm contract as an approved spender for Neuron tokens

            _fighterFarmContract.reRoll(tokenId, fighterType);
            // Attempting to reroll the fighter

            assertEq(_neuronContract.balanceOf(_ownerAddress) < neuronBalanceBeforeReRoll, true);
            // Asserting that the neuron balance of the owner has decreased after the reroll, indicating a successful reroll

            _fighterFarmContract.incrementGeneration(0);
            // Incrementing the generation of champions

            vm.expectRevert();
            // Expecting the next operation to revert

            _fighterFarmContract.reRoll(tokenId, fighterType);
            // Attempting to reroll the fighter again, which should revert due to the incremented generation
        }
    }

The result of executing the forge test --mt testRerollRevertWhenGenerationIsIncremented -vv command can be seen in the screenshot below.

https://imgur.com/a/nqt4UHO

As can be seen from the test with the extensive documentation - the vulnerability immediately denies the rerolling function with either using fighterType 0 or 1.

Tools Used

Manual review

Recommended Mitigation Steps

The simplest and most effective solution to this massive vulnerability would be to set the number of elements per generation for a fighter type to 3 when incrementing the generation as shown below.

    /// @notice Increase the generation of the specified fighter type.
    /// @dev Only the owner address is authorized to call this function.
    /// @param fighterType Type of fighter either 0 or 1 (champion or dendroid).
    /// @return Generation count of the fighter type.
    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        numElements[generation[fighterType]] = 3;
        return generation[fighterType];
    }

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/VoltageManager.sol#L105-L112

Vulnerability details

Impact

In the VoltageManager.sol file, the spendVoltage function lacks a check to verify whether the user's current available voltage is sufficient for their intended spending. As a consequence, if a user has an available replenishment, it is used up before the voltage spending. In essence, this results in the user's resources being forcibly wasted, in contrast to the intended behavior as confirmed by the sponsors.

Proof of Concept

The following test with documentation is a solid proof of the unwanted impact this issue has. To simulate the following test you should add the test in the VoltageManager.t.sol file and execute the forge test --mt testSpendVoltageTriggerReplenishVoltageEvenWhenNotNeeded command.

    /// @notice Test a player trying to spend voltage and triggering the replenishing voltage function
    /// even though the player has enough voltage to not have to waste his replenishing

    function testSpendVoltageTriggerReplenishVoltageEvenWhenNotNeeded() public {
        address player = _ownerAddress;
        // Assigning the _ownerAddress to the player

        _mintGameItemForReceiver(player);
        // Minting battery for the player

        vm.prank(player);
        // Pranking to be the player

        _voltageManagerContract.useVoltageBattery();
        // Using voltage battery
        
        uint256 ownerVoltageReplenishTime = _voltageManagerContract.ownerVoltageReplenishTime(player);
        // Getting the time when the player's voltage will replenish
        emit log_uint(ownerVoltageReplenishTime);
        
        uint8 voltageSpendAmount = 1;
        // Setting the amount of voltage to spend
        
        uint256 currentVoltage = _voltageManagerContract.ownerVoltage(player);
        // Getting the current voltage of the player
        
        if(currentVoltage >= voltageSpendAmount &&
        ownerVoltageReplenishTime <= block.timestamp){
            // Checking if the player has enough voltage to spend and if replenishing is available
            
            _voltageManagerContract.spendVoltage(player, voltageSpendAmount);
            // Spending the specified amount of voltage by the player
            
            assertEq(_voltageManagerContract.ownerVoltage(player), 100 - voltageSpendAmount);
            // Asserting that the player's voltage has decreased by the spent amount
            
            emit log_uint(_voltageManagerContract.ownerVoltageReplenishTime(player));
            assertEq(_voltageManagerContract.ownerVoltageReplenishTime(player) > 0, true);
            // Asserting that the player's voltage replenish time has been updated
            // This assertion effectively demonstrates that the player's replenishment was unnecessary and thus wasted.
        }
    }

As can be seen by the test, the user's resources are being used up despite already having an excess of 100 times the required amount.

Tools Used

Manual review

Recommended Mitigation Steps

The simplest and most effective solution to this issue would be to add an additional check if the current voltage available to the user is enough to cover the intended voltage to be spent as shown below.

    function spendVoltage(address spender, uint8 voltageSpent) public {
        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
        if (ownerVoltageReplenishTime[spender] <= block.timestamp &&
            ownerVoltage[spender] < voltageSpent) {
            _replenishVoltage(spender);
        }
        ownerVoltage[spender] -= voltageSpent;
        emit VoltageRemaining(spender, ownerVoltage[spender]);
    }

Assessed type

Other