c4-bot-7 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

The fundamental premise of AiArena is that players stake NRN and put that NRN at risk during battles in the hopes of winning points & prizes. A balance between prizes distributed and NRN lost by players is essential to the long-term longevity of the protocol.

However, if a player on a losing streak unstakes everything, he can still continue to play to reclaim his lost stake.

If he wins a battle: a small part of the lost stake gets reclaimed and he immediately unstakes that part.
If he loses a battle: nothing happens since there is no stake to lose.

This represents a direct financial loss to the protocol since the _sweepLostStake will be much smaller due to losing players reducing their lost stake without ever risking any loss.

Proof of Concept

Bob stakes 1M NRN on Fighter A.

amountStaked[A] = 1_000_000

Bob plays 20 battles over 2 days and he loses every battle.
amountStaked[A] = 980_000
stakAtRisk[A] = 20_000

He then decides to unstake everything and continues to play.
amountStaked[A] = 0
stakAtRisk[A] = 20_000

Since amountStaked[A] + stakeAtRisk[A] remains >0, _addResultsPoints will be called.

If Bob wins a battle, he reclaims a part of the stakeAtRisk which he unstakes immediately.

In _addResultsPoints:

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

If Bob loses a battle, nothing happens.

In _addResultsPoints:

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

   /// Do not allow users to lose more NRNs than they have in their staking pool
   if (curStakeAtRisk > amountStaked[tokenId]) {
      curStakeAtRisk = amountStaked[tokenId];
      // 20 > 0  => curStakeAtRisk = 0 and a 0 transfer will executed, which changes nothing 
   }

If the round still last for 12 more days (bi-weekly), Bob can potentially reclaim 12 10 20 = 2400 NRN while risking 0 NRN. Or he could buy 2 batteries every time he wins and play up to 12 5 10 = 600 extra battles in the round, thereby reclaiming up to 12000 extra NRN

Note: It is practically impossible for a normal player to lose all their stake. You lose 0.1% of your stake with every loss, so for Alice (deposit 1000 NRN) to lose everything, she would have to lose 1000 battles in a row. Since a normal round only has 140 battles, this would require buying 86 batteries with a 100% loss rate (nearly impossible with an elo-matchmaking system). So this situation can only occur as a deliberate misuse of the system.

Tools Used

Manual Review

Recommended Mitigation Steps

The migitation is straightforward. If a player wins, require that his staked NRN be at least the same as his stakeAtRisk.

In _addResultsPoints


        if (battleResult == 0) {
            /// If the user won the match
+           require(amountStaked[tokenId] >= stakeAtRisk,"You cannot reclaim NRN if you have no stake");

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

Assessed type

Other

c4-pre-sort commented 9 months ago

raymondfam marked the issue as sufficient quality report

c4-pre-sort commented 9 months ago

raymondfam marked the issue as duplicate of #136