AI Arena - btk's results

Lines of code

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

Vulnerability details

Overview

The FighterFarm contract, implement a hook called _ableToTransfer. This function ensures that before every transfer, certain conditions are met: ownership verification, checking if the receiver has reached the maximum limit of fighters allowed, and confirming that the tokenId isn't currently staked. It's used for determining if a transfer is valid. However, while _ableToTransfer is invoked in ERC721::transferFrom() and ERC721::safeTransferFrom(), it's missed in ERC721::safeTransferFrom() with data.

Impact

Users can evade the necessary checks enforced by _ableToTransfer() by utilizing safeTransferFrom with data.

Proof of Concept

Vulnerability Details

The _ableToTransfer function is implemented as follows:

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

Although it's called in transferFrom & safeTransferFrom, there's an additional function in the OpenZeppelin ERC721 implementation that developers overlooked to override:

    function safeTransferFrom(
        address from,
        address to,
        uint256 tokenId,
        bytes memory data
    ) public virtual override {
        require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
        _safeTransfer(from, to, tokenId, data);
    }

Thus, users can use it to transfer NFTs that are staked or to surpass the MAX_FIGHTERS_ALLOWED limit.

Simple PoC to prove tha validity of the finding:

    function testBypassAbleToTransfer() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        // check that i'm unable to transfer since i staked
        vm.expectRevert();
        _fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        
        // check that i'm unable to transfer since i staked
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);

        // i'm able to transfer even do i staked
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Override safeTransferFrom with data and add the following check:

require(_ableToTransfer(tokenId, to));

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Overview

The GameItems contract manages items in AI Arena, each with a value that can be transferred. Initially, some items, like batteries, are set as non-transferable as seen in the deployment script. However, users can bypass this restriction using safeBatchTransferFrom.

Impact

Developers aimed to limit transferability for specific GameItems, like soulbound weapons or temporary restrictions on items like batteries. However, this issue allows users to bypass transfer restrictions entirely using safeBatchTransferFrom.

Proof of Concept

Vulnerability Details

The check is implemented as follows:

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

However, there's an additional function in the OpenZeppelin ERC1155 implementation that developers overlooked to override:

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Thus, users can use it to transfer items that are meant to be non-transferable.

Simple PoC to prove tha validity of the finding:

    function testBypassTransferable() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

        address receiver = makeAddr("Receiver");

        vm.prank(address(this));
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(address(this), receiver, 0, 1, "");
        assertEq(_gameItemsContract.balanceOf(receiver, 0), 0);

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

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

        vm.prank(address(this));
        _gameItemsContract.safeBatchTransferFrom(address(this), receiver, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(receiver, 0), 1);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Override safeBatchTransferFrom and add the following check:

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Overview

The redeemMintPass function allows users to exchange multiple mint passes for fighter NFTs. It verifies ownership of mint passes, burns them, and creates new fighter NFTs accordingly. However, it overlooks validating user-selected icons, potentially impacting the rarity of NFTs.

Impact

Users to mint any icon NFT, devaluing the rarity of early supporters NFTs and making a profit when selling them.

Proof of Concept

Vulnerability Details

Here is the redeemMintPass function implementation:

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        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)]
            );
        }
    }

• Allows users to exchange multiple mint passes for fighter NFTs.
• Checks that the sender owns each mint pass before burning it.
• Burns each mint pass, removing it from circulation.
• Creates a new fighter NFT for each burned mint pass with specified attributes.
• Iterates through each mint pass ID to handle the exchange for the corresponding fighter NFT.

Internally, redeemMintPass calls _createNewFighter, which further invokes AiArenaHelper.createPhysicalAttributes() to generate fighter attributes.

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

The redeemMintPass implementation allows users to pass any arbitrary icons. For instance, by passing a specific parameter (e.g., "3"), a user can mint an NFT with both a red diamond and a bowling ball, as indicated by a sponsor's comment suggesting these are special fighters for early supporters. However, this issue diminishes the rarity of these fighters, rendering them common because any user with a mint-pass can mint an icon.

Here is a coded PoC to demonstrate the issue:

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

        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";
        _fighterTypes[0] = 0;
        _neuralNetHashes[0] = "neuralnethash";
        _modelTypes[0] = "original";
        _iconsTypes[0] = 3;

        _mintPassContract.approve(address(_fighterFarmContract), 1);

        _fighterFarmContract.redeemMintPass(
            _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
        );

        (, uint256 eyes, , , uint256 hands,) = _fighterFarmContract.getAttr(0);

        console.log("Fighter have red diamond eyes   :", eyes);
        console.log("Fighter have bowling ball hands :", hands);
    }

    function getAttr(uint256 tokenId) public view returns (uint256,uint256,uint256,uint256,uint256,uint256) {
        FighterOps.FighterPhysicalAttributes memory attrs = fighters[tokenId].physicalAttributes;
        return (attrs.head, attrs.eyes, attrs.mouth, attrs.body, attrs.hands, attrs.feet);
    }

Logs result:

  Fighter have red diamond eyes   : 50
  Fighter have bowling ball hands : 50

Test Setup:

• Incorporate the tests in FighterFarmTest.
• Paste getAttr function in FighterFarm.
• Execute: forge test --mt testBreakRedeemMintPass -vvv

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding some validation on the icons types input.

Assessed type

Other

Lines of code

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

Vulnerability details

Overview

The redeemMintPass function allows users to exchange multiple mint passes for fighter NFTs. It verifies ownership of mint passes, burns them, and creates new fighter NFTs accordingly. However, it overlooks validating user-selected fighterTypes. Thus, any user with a mint pass can mint a dendroid NFT.

Impact

Every user possessing a mint pass NFT can mint a dendroid fighter, undermining the rarity of this type.

Proof of Concept

In Ai Arena, fighters are categorized as either 0 (champion) or 1 (dendroid), with dendroids intended to be rarer than champions. Users can call redeemMintPass to redeem their mint pass NFTs:

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

As evident, there's no validation on the fighter types passed by the user. Thus, users can freely choose any fighter type, including dendroids, contrary to their intended rarity.

[!NOTE]
Sponsor confirmed: "This is not the intent - we need some validation onchain to prevent this"

Tools Used

Manual review

Recommended Mitigation Steps

Consider implementing validations within the redeemMintPass function

Assessed type

Other

Lines of code

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

Vulnerability details

Overview

The reRoll function is designed to allow token owners to update their fighters' attributes by paying a reroll cost in NRN tokens. While the tokenId is validated, the fighterType is not, which can lead to bypassing certain checks and potentially making malicious changes.

Impact

Users can exploit this vulnerability by passing a different fighterType to the reRoll function, enabling them to:

• Bypass the maximum allowed rerolls check.
• Change the DNA of their non-dendroid NFTs to dendroid DNA.
• Utilize attributes from another generation, altering the rarity of their NFTs.

This issue will allow users to manipulate attribute probabilities or DNA to create more valuable NFTs for profit.

Proof of Concept

The variable maxRerollsAllowed determines the maximum number of rerolls allowed for each fighter type:

uint8[2] public maxRerollsAllowed = [3, 3];

Initially, both champion (0) and dendroid (1) fighter types are limited to 3 rerolls. However, through the incrementGeneration function, called by the owner, this limit can be increased:

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

Suppose at some point, maxRerollsAllowed becomes:

maxRerollsAllowed = [3, 5];

If Bob's fighterType is 0 (champion) and he's reached his maximum rerolls, he can bypass the bellow check by calling reRoll with fighterType as 1, allowing him two extra rerolls beyond his limit.

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

Bob can further exploit this vulnerability to change his champion's DNA to dendroid DNA:

uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);

By passing 1 as fighterType in reRoll, Bob can change his champion's DNA to dendroid (Sponsor confirmed this as an issue).

Additionally, Bob can manipulate the attribute probabilities by calling reRoll with different fighterTypes, causing the fighter's attributes to regenerate based on an invalid generation[fighterType].

            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );

Tools Used

Manual review

Recommended Mitigation Steps

Consider storing the fighterType in the Fighter struct and retrieve it when reRoll is called:

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

Assessed type

Other

Lines of code

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

Vulnerability details

Overview

A Reward Pool of NRN will be distributed at the end of each Round of the Ranked Battle competition. The NRN reward received by an individual NFT is based on three drivers:

• Your NFT’s performance in the current Round of competition.
• The amount of NRN staked on your NFT throughout this Round of competition.
• Your NFT’s current Elo.

Each NFT accumulates Points, which are claims on the Reward Pool, during a Round. The Points are settled for NRN from the Reward Pool at the end of each Round. The Reward Pool will be divided proportionally amongst all NFTs with positive Point totals at the end of each Round. The issue is that users staking just 1 wei NRN earn the same rewards as those staking 3e18 NRN.

Impact

Stakers can stake as little as 1 wei NRN and receive rewards equivalent to staking 3e18 NRN. Consequently, they claim the same portion of the reward pool as users staking 3e18 NRN.

Proof of Concept

In the RankedBattle contract, points are calculated based on the Elo rating and the staking factor:

points = stakingFactor[tokenId] * eloFactor;

Here's the formula to calculate the staking factor:

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    }   

The staking factor is calculated by taking the square root of the sum of the current staked amount and the stake at risk, divided by 10e18. If the result is 0, the _getStakingFactor function returns 1 instead of 0, which is unfair for stakers whose result is 1. Consequently, users staking 1 wei earn as many points (NRN) as users staking 3e18.

Here's the formula to calculate how much a user can claim:

claimableNRN = accumulatedPointsPerAddress * nrnDistribution / totalAccumulatedPoints;

Consider a simple example to illustrate the issue: in Round 1, there are only 4 users, but only 2 winners, Alice and Bob.

1. Alice has staked 3e18.
2. Bob has staked 1 wei.
3. Both users have an Elo rating of 1500.
4. Both users won 1 game.

Alice and Bob's staking factor is 1. Thus, they both earned 1500 points:

points = 1 * 1500;

When the round ends and the users attempt to claim their rewards (assuming nrnDistribution = 5000e18):

Bob's claim:

claimableNRN = accumulatedPointsPerAddress[Bob] * nrnDistribution / totalAccumulatedPoints;
2500e18 = 1500 * 5000e18 / 3000

Alice's claim:

claimableNRN = accumulatedPointsPerAddress[Alice] * nrnDistribution / totalAccumulatedPoints;
2500e18 = 1500 * 5000e18 / 3000

Despite the significant disparity in staked amounts, both Alice and Bob receive identical rewards due to the incorrect staking factor calculation.

Tools Used

Manual review

Recommended Mitigation Steps

Consider updating the _getStakingFactor as follow:

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      return stakingFactor_;
    }  

Assessed type

Math

Lines of code

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

Vulnerability details

Overview

When a Fighter NFT loses, and has 0 Points in the current Round, a small portion of their stake (not Points) is placed “at risk”. At the end of each Round, the at risk stake is lost permanently.

Furthermore, a Fighter NFT cannot earn NRN rewards if they have any stake at risk. Fighter NFTs can regain their at risk stake by winning matches before the end of the Round.

However, if the amount staked is less than 1000 Wei NRN, Fighters will lose nothing when they lose but will still earn points if they win.

Impact

Fighters staking less than 1000 Wei NRN won't incur any losses but will still earn points.

Proof of Concept

The calculation for curStakeAtRisk when fighters lose or win:

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

To illustrate the issue, let's use a simple example. Assuming bpsLostPerLoss = 10:

• Bob staked 1 wei NRN in the RankedBattle contract.
• When Bob loses a battle, here is his curStakeAtRisk:

// Bob will lose nothing
(10 * (1 + 0)) / 10**4 = 0

Conversely, when Bob wins a battle, as there is no stake at risk, he will earn points. Here is his points calculation, assuming his elo is 1500:

// Bob will earn points
1500 * 1 = 1500

Thus, when users stake less than 1000 Wei and win, they earn points. However, if they lose, the curStakeAtRisk will round down to zero, resulting in no loss for them.

Tools Used

Manual review

Recommended Mitigation Steps

Consider enforcing a minimum stake amount, for example, 5e18 NRN.

Assessed type

Math

Lines of code

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

Vulnerability details

Overview

The numElements mapping keeps track on the number of elements per generation. However, there's no setter function to modify or update this mapping.

Impact

This oversight leads to all new generations defaulting to zero elements, effectively halting the progression of the game.

Proof of Concept

New generations can be introduced using the incrementGeneration function:

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

Currently, there are only two fighter types: 0 (champion) or 1 (dendroid). Both types start with generation 0:

uint8[2] public generation = [0, 0];

The numElements value is hardcoded in the constructor:

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

While generation 0 begins with 3 elements, future generations cannot be added due to the absence of a setter function for updating numElements. This limitation impedes FighterFarm's functionalities, barring any chance of introducing new generations unless a new implementation of FighterFarm is deployed.

Furthermore, when incrementGeneration is invoked for fighterType 0 (champion), for instance, the generation of that fighter type becomes 1, and numElements[1] defaults to 0. Consequently, when _createFighterBase is called (either through reRoll or new minting):

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

The new fighter element will be generated based on an invalid numElements.

Tools Used

Manual review

Recommended Mitigation Steps

Consider impelemting a function to adjust the numElements mapping for new generations.

Assessed type

Other

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

[L-07] Inability to remove roles in the Neuron contract

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

[L-06] claimRewards may result in OOG error

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

[L-12] Inability to remove staker role in the FighterFarm contract

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

Users can bypass GameItems allowances using mutiple addresses

Ai Arena Overview

AI Arena is a PvP platform where human-trained AI fighters are tokenized via FighterFarm.sol. Fighters have attributes like appearance, generation, weight, element, type, and model data. Players earn $NRN by entering fighters in ranked battles, facilitated by RankedBattle.sol. Staking tokens and winning battles yields rewards, with potential stake loss for poor performance. Voltage in wallets replenishes to 100 every 24 hours; battles consume 10 voltage units each. Batteries from GameItems.sol restore voltage to 100 or players wait for natural replenishment.

Findings Summary

[L-01] Missing setter function for rerollCost

Impact

The absence of a setter function for rerollCost may lead to various issues when the price fluctuates.

Description

The current implementation hardcodes the rerollCost, which is the price paid by fighters to reroll a new fighter with random traits:

uint256 public rerollCost = 1000 * 10**18;    

Hardcoding rerollCost can introduce challenges in the future, potentially limiting protocol functionalities. For instance, if the price of the NRN token increases, rerolling may become prohibitively expensive for fighters. Conversely, if the price decreases, rerolling may become too affordable, resulting in an influx of users changing their attributes and DNA simultaneously, potentially congesting the system.

Recommended Mitigation Steps

Consider adding a setter function to dynamically adjust the rerollCost in response to price changes.

[L-02] Missing setter function for treasuryAddress

Impact

Hardcoding the treasuryAddress in the constructor may lead to a loss of funds for the protocol.

Description

In the current implementation, treasuryAddress is initialized in the constructor. This address is pivotal as it dictates where funds will be transferred when users reRoll a fighter:

bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);

However, the absence of a setter function for treasuryAddress poses a significant risk. If, at any point in the future, this address is compromised, lost, or controlled by a malicious entity, there is no mechanism in place for the owner or protocol to rectify the situation. As a result, funds will perpetually be directed to the hardcoded address, potentially resulting in irreversible losses.

Recommended Mitigation Steps

Consider adding a simple setter function to change the treasuryAddress to make the system more robust.

[L-03] Insecure Elliptic Curve Recovery Mechanism

Impact

The elliptic curve recovery mechanism utilized in the FighterFarm contract is insecure, posing risks of Malleable Signatures and the potential return of a Null Address.

[!NOTE]
Acknowledgement: The Verification library is OOS. This vulnerability pertains to the in-scope (FighterFarm) contract employing a function from the out-of-scope (Verification) contract.

Description

FighterFarm.claimFighters function uses Verification.verify to check the validity of the fighter signature, Verification.verify is as follow:

    function verify(
        bytes32 msgHash, 
        bytes memory signature,
        address signer
    ) 
        public 
        pure 
        returns(bool) 
    {
        require(signature.length == 65, "invalid signature length");

        bytes32 r;
        bytes32 s;
        uint8 v;

        assembly {
            /*
            First 32 bytes stores the length of the signature

            add(sig, 32) = pointer of sig + 32
            effectively, skips first 32 bytes of signature

            mload(p) loads next 32 bytes starting at the memory address p into memory
            */

            // first 32 bytes, after the length prefix
            r := mload(add(signature, 32))
            // second 32 bytes
            s := mload(add(signature, 64))
            // final byte (first byte of the next 32 bytes)
            v := byte(0, mload(add(signature, 96)))
        }
        bytes memory prefix = "\x19Ethereum Signed Message:\n32";
        bytes32 prefixedHash = keccak256(abi.encodePacked(prefix, msgHash));
        return ecrecover(prefixedHash, v, r, s) == signer;
    }

The ecrecover() function is a low-level cryptographic function that should be utilized after appropriate sanitizations have been enforced on its arguments, namely on the s and v values. This is due to the inherent trait of the curve to be symmetrical on the x-axis and thus permitting signatures to be replayed with the same x-value (r) but a different y-value (s).

The ecrecover() function in Solidity has a few potential vulnerabilities that can make it insecure if not handled properly:

• Malleable Signatures: One of the main vulnerabilities is that ecrecover is susceptible to malleable signatures. This means that a valid signature can be transformed into a different valid signature without needing access to the private key. This introduces challenges when relying on signatures for uniqueness or identification.

• Return of Null Address: Another vulnerability is that ecrecover can return a null address (address(0)). This can happen when the recovery ID (v) is set to any value other than 27 or 28. An unsecure contract may allow an attacker to spoof an authorized-only method into executing as though the authorized account is the signer.

Recommended Mitigation Steps

Consider adopting OpenZeppelin's battle-tested ECDSA which incorporates safeguards against both Malleable Signatures and the Return of Null Address vulnerabilities.

[L-04] Use safeTransferFrom instead of transferFrom for ERC721 transfers

Impact

The sent NFTs will be locked up in the contract forever.

Description

Use of transferFrom method for ERC721 transfer is discouraged and recommended to use safeTransferFrom whenever possible by OpenZeppelin. This is because transferFrom() cannot check whether the receiving address know how to handle ERC721 tokens.

For example, If ERC721 token is sent to "to" with the transferFrom method and this "to" is a contract and is not aware of incoming ERC721 tokens, the sent token could be locked up in the contract forever.

Reference: https://docs.openzeppelin.com/contracts/3.x/api/token/erc721

Recommended Mitigation Steps

Use _safeTransfer in transferFrom instead of _transfer:

    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }

[L-05] NFTs will have empty URI

Impact

NFT tokens lacking a tokenURI will disrupt off-chain tools reliant on this information, potentially leading to broken functionality or malformed data display.

Furthermore, providing asset metadata allows applications like OpenSea to pull in rich data for digital assets and easily display them in-app. Digital assets on a given smart contract are typically represented solely by a unique identifier (e.g., the tokenId in ERC721), so metadata allows these assets to have additional properties, such as a name, description, and image. Thus, NFTs with empty URIs will be unsellable in such platforms.

Description

The token URI is typically assigned manually by the _delegatedAddress:

    function setTokenURI(uint256 tokenId, string calldata newTokenURI) external {
        require(msg.sender == _delegatedAddress);
        _tokenURIs[tokenId] = newTokenURI;
    }

However, a function called reRoll is utilized by fighters to generate a new fighter with random traits. reRoll function sets the tokenURI to an empty string:

_tokenURIs[tokenId] = ""; 

As a result, all rerolled tokenId URI will be invalid.

[!CAUTION] This issue also breaks the EIP-721: The URI may point to a JSON file that conforms to the "ERC721 Metadata JSON Schema".

Recommended Mitigation Steps

Consider initializing the URI when users mint a new fighter and reset it when they reRoll.

[L-06] claimRewards may result in OOG error

Impact

The MergingPool.claimRewards function could generate an "Out Of Gas" error.

Description

In the MergingPool contract, the claimRewards function enables users to batch claim rewards for multiple rounds. It iterates through each round and then through the winnerAddresses array:

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

When a user is listed in the winnerAddresses, an NFT is minted to them using the mintFromMergingPool function. Internally, _createNewFighter is called, which involves multiple storage reads and invocation of createPhysicalAttributes.

This function iterates through attribute arrays to generate rarities and attributes, further invoking dnaToIndex, which loops through attribute probabilities.

While these iterations might not pose an issue with a single winner or round, OOG errors are likely with multiple winners and unclaimed rounds.

Recommended Mitigation Steps

Consider limiting users to claiming rewards for only one round at a time to mitigate the risk of OOG errors.

[L-07] Inability to remove roles in the Neuron contract

Impact

The absence of a role revocation mechanism in the Neuron contract poses a significant risk of access control issues and potential loss of funds.

Description

The Neuron contract utilizes AccessControl to enforce role-based access control. It employs the internal _setupRole() function to grant roles such as addMinter, addStaker, and addSpender. However, it lacks a corresponding function to revoke these roles.

    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

Consequently, if an address becomes compromised, lost, or controlled by a malicious entity in the future, the contract owner will be unable to revoke the role. This limitation arises from the fact that AccessControl.revokeRole can only be invoked by the role admin.

    function revokeRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
        _revokeRole(role, account);
    }

Notably, Neuron fails to initialize a role admin for the roles or establish a DEFAULT_ADMIN_ROLE, severely restricting the contract's functionalities.

Moreover, in the event of a compromised account holding the minter role, the attacker could mint the entire supply without intervention from the contract owner.

Recommended Mitigation Steps

Consider implementing a role admin for each role using _setRoleAdmin or properly initialize the DEFAULT_ADMIN_ROLE:

    function _setRoleAdmin(bytes32 role, bytes32 adminRole) internal virtual {
        bytes32 previousAdminRole = getRoleAdmin(role);
        _roles[role].adminRole = adminRole;
        emit RoleAdminChanged(role, previousAdminRole, adminRole);
    }

[L-08] globalStakedAmount will not reflect the overall staked amount

Impact

The globalStakedAmount variable fails to accurately reflect the total staked amount within the RnakedBattle contract.

Description

During battles, when a fighter loses, the game server executes updateBattleRecord to reward the winner and penalize the loser. In case of a loss, a portion of the loser's staked NRN is slashed and transferred to the StakeAtRisk contract. If the user wins, the stake is reclaimed; otherwise, it is all transferred to treasuryAddress when the round end:

    function _sweepLostStake() private returns(bool) {
        return _neuronInstance.transfer(treasuryAddress, totalStakeAtRisk[roundId]);
    }

Consequently, even after setNewRound is called and totalStakeAtRisk is transferred to the treasury, globalStakedAmount remains unaltered, rendering it greater than the actual staked amount by fighters.

Recommended Mitigation Steps

Consider deducting the totalStakeAtRisk from globalStakedAmount when the setNewRound is called.

[L-09] GameItems breaks ERC1155 specifications

Impact

GameItems breaks ERC1155 specifications.

Description

GameItems implements the setTokenURI function to update the URI:

    function setTokenURI(uint256 tokenId, string memory _tokenURI) public {
        require(isAdmin[msg.sender]);
        _tokenURIs[tokenId] = _tokenURI;
    }

Whenever there is a URI update, the standard requires emitting an event:

MUST emit when the URI is updated for a token ID.

Thus, by not emitting an event, GameItems contract does not adhere to ERC1155.

Recommended Mitigation Steps

Consider emitting an event whenever there is a URI update.

[L-10] Users can bypass GameItems allowances using mutiple addresses

Impact

GameItems contract can be exploited by utilizing multiple addresses to mint unlimited items, thus bypassing the intended allowances system.

Description

The GameItems contract incorporates an allowances mechanism designed to restrict users from instantly minting all available items. Each user is allocated a daily allowance for each game item, which can be replenished daily. For instance, if the daily allowance for a specific game item is set to 5, each user is entitled to acquire up to 5 items daily. However, the issue arises when users exploit this system by utilizing multiple addresses, allowing them to circumvent the imposed limitations and mint any desired quantity of items.

Recommended Mitigation Steps

Consider implementing a whitelist of authorized addresses that are permitted to purchase items. By doing so, users will be required to utilize authenticated addresses, mitigating the ability to create random addresses for purchasing items.

[L-11] User nft may be stuck when all his amountStaked is slashed

Impact

When all of a fighter's staked amount is slashed, they are unable to transfer their NFT, even if they are no longer staking.

Description

In the RankedBattle contract, the function updateFighterStaking is called when fighters stake NRN tokens:

    function updateFighterStaking(uint256 tokenId, bool stakingStatus) external {
        require(hasStakerRole[msg.sender]);
        fighterStaked[tokenId] = stakingStatus;
        if (stakingStatus) {
            emit Locked(tokenId);
        } else {
            emit Unlocked(tokenId);
        }
    }

The fighter's staking status is set to true to prevent them from transferring their NFTs while staking. When they unstake and their amountStaked reaches 0, updateFighterStaking is called again to set the status to false.

However, the RankedBattle contract implements a mechanism to slash losers:

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

The issue arises when all of the fighter's amountStaked is slashed. In this case, updateFighterStaking is not called, leaving the fighter's NFT stuck and unable to be transferred.

Recommended Mitigation Steps

Consider checking if amountStaked is 0 and call updateFighterStaking accordingly:

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

[L-12] Inability to remove staker role in the FighterFarm contract

Impact

Inability to remove staker role in the FighterFarm contract.

Description

The hasStakerRole mapping keeps track of addresses allowed to stake fighters. New addresses get added by the contract owner using this function:

    function addStaker(address newStaker) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = true;
    }

Once added, they gain access to the updateFighterStaking function. However, the owner can't remove a staker.

Recommended Mitigation Steps

Consider updating the addStaker function like this:

    function addStaker(address newStaker, bool isStaker) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = isStaker;
    }