AI Arena - Honour's results

Lines of code

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

Vulnerability details

Impact

risk-free play, players can stake NRN and accumulate points on wins without putting their stakes at risk or loosing their stake on losses. Stake-at-risk on loss can be considered an invariant

Proof of Concept

The player is allowed to stake any amount of NRN > 0,from RankedBattle::getStakingFactor the staking factor will always be 1 for all stakes below 1NRN. Meaning players are always guaranteed points when they stake NRN regardless of the stake. However, the potential stake at risk per match calculated as curStakeAtRisk = (bpsLostPerLoss * totalStake) / 10^4 will always be 0 for stakes below 1000weiNRN(0.000000000000001NRN) at a bpsLostPerLoss of 10(the default). In a nutshell any stake below 1000weiNRN will always have a stake at risk of 0, but a staking factor of 1. The player won't risk their stake on losses but accumulate points on wins.<br> Add Test to RankedBattle.t.sol

function test_playerCanStakeWithoutRisk() public {
    address player1 = makeAddr("player1");
    address player2 = makeAddr("player2");

    //mint fighters for both players and fund them with NRN
    _mintFromMergingPool(player1);
    _mintFromMergingPool(player2);
    _fundUserWith4kNeuronByTreasury(player1);
    _fundUserWith4kNeuronByTreasury(player2);
    uint256 player1Stake = 999; //999weiNRN
    uint256 player2Stake = 3_000 * 10 ** 18; //3_000NRN

    //Both players stake
    vm.prank(player1);
    _rankedBattleContract.stakeNRN(player1Stake, 0);
    vm.prank(player2);
    _rankedBattleContract.stakeNRN(player2Stake, 1);
    assertEq(_rankedBattleContract.amountStaked(0), player1Stake);
    assertEq(_rankedBattleContract.amountStaked(1), player2Stake);

    //Both players play 5 matches with 3 losses and 2 wins each
    vm.startPrank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
    _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
    _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
    _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);
    _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);

    _rankedBattleContract.updateBattleRecord(1, 0, 2, 1500, true);
    _rankedBattleContract.updateBattleRecord(1, 0, 2, 1500, true);
    _rankedBattleContract.updateBattleRecord(1, 0, 2, 1500, true);
    _rankedBattleContract.updateBattleRecord(1, 0, 0, 1500, true);
    _rankedBattleContract.updateBattleRecord(1, 0, 0, 1500, true);
    vm.stopPrank();

    //player1 accumulates 3000 points
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), 3000);
    //player2 doesnt accumulate any points
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(1, 0), 0);

    //player1 has no stake at risk
    assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 0);
    //player2 has stake at risk 3NRN
    assertEq(_stakeAtRiskContract.getStakeAtRisk(1), (10 * player2Stake) / 10 ** 4);//3NRN
}

Tools Used

Manual Review

Recommended Mitigation Steps

There should be a minimum stake amount(say 1000weiNRN) that would always incur a stake at risk.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

new players/winners of the mergingPool rewards will be unable to claim their rewards after a significant number of merging pool rounds

Proof of Concept

MergingPool::claimRewards uses nested for-loops that iterates through all the rounds starting from the last round the user claimed and then through the winners list for each round , this can be highly gas-inefficient for first time winners/new players especially after a significantly large number of rounds have been played since the numRoundsClaimed will be 0 <br>

Add test to MergingPool.t.sol

<details> <summary> click to expand code </summary>

function test_claimRewardsForNewWinners() public {
    uint256 round = 8_000;
    uint256 roundId_slot = 1;
        vm.store(
        address(_mergingPoolContract),
        bytes32(roundId_slot),
        bytes32(round)
    ); //setting the round

    //mint fighters for new players
    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(_DELEGATED_ADDRESS);

    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1;
    // winners of round 8,000 are picked
    _mergingPoolContract.pickWinner(_winners);
    assertEq(_mergingPoolContract.isSelectionComplete(round), true);

    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] = uint256(1);
    _customAttributes[0][1] = uint256(80);

    uint256 startingGas = gasleft();
    // first time winner claims rewards for round 8,000
    _mergingPoolContract.claimRewards(
        _modelURIs,
        _modelTypes,
        _customAttributes
    );
    // other first time winner claims rewards for round 8,000
    vm.prank(_DELEGATED_ADDRESS);
    _mergingPoolContract.claimRewards(
        _modelURIs,
        _modelTypes,
        _customAttributes
    );

    //both claims collectively use more than the block limit
    assert(startingGas - gasleft() > 30_000_000);
}

</details> <br> After 8_000 rounds the gas cost of both winners claiming their reward already exceeds the block gas limit. <br><br>

Tools Used

Manual Review

Recommended Mitigation Steps

storage mapping mapping(address => uint256) numRoundsWon to track the number of rounds won by each address ,incremented in MergingPool::pickWinner then used in MergingPool::claimRewards for iterating through rounds.

<details> <summary> click to expand code </summary>

//MergingPool.sol

//mapping of address to total number of rounds won by address
mapping(address => uint256) numRoundsWon;

    function pickWinner(uint256[] calldata winners) external {
        require(isAdmin[msg.sender]);
        require(
            winners.length == winnersPerPeriod,
            "Incorrect number of winners"
        );
        require(!isSelectionComplete[roundId], "Winners are already selected");
        uint256 winnersLength = winners.length;
        address[] memory currentWinnerAddresses = new address[](winnersLength);
        for (uint256 i = 0; i < winnersLength; i++) {
            currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(
                winners[i]
            );
    @>          numRoundsWon[currentWinnerAddresses[i]]+=1;
            totalPoints -= fighterPoints[winners[i]];
            fighterPoints[winners[i]] = 0;
        }
        winnerAddresses[roundId] = currentWinnerAddresses;
        isSelectionComplete[roundId] = true;
        roundId += 1;
    }


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

    function getUnclaimedRewards(address claimer) external view returns(uint256) {
        return numRoundsWon[claimer] - numRoundsClaimed[claimer];
    }

</details>

Assessed type

DoS