AI Arena - PetarTolev's results

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

[L-07] The user can choose fighters’ attributes in their favor

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

[L-01] Staker Role Cannot Be Revoked

Low Risk Findings Overview

Non-critical Findings Overview

---

Low Risk Findings

[L-01] Staker Role Cannot Be Revoked

In FighterFarm, the staker role has permission to lock/unlock fighters, determining their transferability.

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

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

However, if a staker behaves maliciously, the role cannot be revoked.

Recommendation

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

[L-02] getFighterPoints only returns the points for figter with id 1

MergingPool.getFighterPoints function should return the points for the fighters from 0 to maxId. However, the points array returned from the function is initialized with length of 1, thus the function always reverts for maxId > 1.

According to the NatSpec, it should return an aggregated points array.

/// @notice Retrieves the points for multiple fighters up to the specified maximum token ID.

Recomendation

function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) {
-   uint256[] memory points = new uint256[](1);
+   uint256[] memory points = new uint256[](maxId);
    ...
}

[L-03] The Neuron Contract Uses a Deprecated AccessControl Function

In Neuron, the functions addMinter, addStaker, addSpender use the deprecated _setupRole function.

OpenZeppelin DOCS

Recommendation

Instead of _setupRole, use _grantRole.

[L-04] Unstaked transferred fighter is unusable in the same roundId

When a user unstakes their NRNs and then decides to sell/transfer their fighter to another user, they must wait until the next round to restake and enter the ranked battle.

POC:

1. Alice has fighter 1 and stakes 100 NRNs.
2. Alice unstakes 100 NRNs.
3. Alice sells fighter 1 to Bob on OpenSea.
4. Bob cannot stake NRNs, thus cannot enter the ranked battle in the same round.

unstakeNRN sets hasUnstaked to true.

stakeNRN check only if tokenId and roundId have been unstaked.

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

Recommendation

Consider revising the logic to allow newly transferred fighters to participate in the same round.

[L-05] Loss of NRNs on token transfer failure in RankedBattle.unstakeNRN

If the NRN transfer fails, the user will lose their NRNs.

Recommendation

function unstakeNRN(uint256 amount, uint256 tokenId) external {
   ...
    bool success = _neuronInstance.transfer(msg.sender, amount);
+   require(success, "Transfer failed.");
-   if (success) {
        if (amountStaked[tokenId] == 0) {
            _fighterFarmInstance.updateFighterStaking(tokenId, false);
        }
        emit Unstaked(msg.sender, amount);
-   }
}

[L-06] Possible Cross-Chain Replay Attack in FighterFarm.claimFighters

The FighterFarm.claimFighters function does not use any nonce or chainId in the msgHash calculation.

Recommendation

Utilize EIP-155 to include chainId in the msgHash calculation.

[L-07] The user can choose fighters' attributes in their favor

The user can pre-calculate their fighter's attributes in advance and even select the desired ones.

The fighter's base is calculated based on the dna, which is provided as input by the user in redeemMintPass and then passed to the _createFighterBase method. This allows every user to control which fighter they will receive.

Non-critical Findings

[NC-01] Implement _ableToTransfer in the ERC20._beforeTokenTransfer

Move the _ableToTransfer check to the _beforeTokenTransfer hook in FighterFarm.

[NC-02] Use OpenZeppelin's AccessControl for Role Management

Replace the current ownership logic in the contracts with OpenZeppelin's AccessControl. This will help reduce the contracts' complexity and size. Remove transferOwnership, adjustAdminAccess, _ownerAddress, and isAdmin.

Instances: FighterFarm, GameItems, MergingPool, RankedBattle, VoltageManager.

[NC-03] Use OpenZeppelin's Ownable for Ownership Management

Replace the current ownership logic in the AiArenaHelper with OpenZeppelin's Ownable. This will help reduce the contract's complexity and size.

[NC-04] In Neuron Contract Utilize AccessControl Functionality

1. Replace the hasRole checks with onlyRole modifiers.
2. Set the _ownerAddress as DEFAULT_ADMIN_ROLE.
3. Remove the isAdmin mapping, instead grant admins the ADMIN_ROLE.

This will decrease the contract's size and complexity.

[NC-05] In Neuron Contract, Inherit OpenZeppelin's ERC20Burnable

To decrease the Neuron contract's size and complexity, inherit from ERC20Burnable and remove the burn and burnFrom functions.

[NC-06] Duplicated Validations Should Be Refactored to a Function

Consider the refactoring of duplicated replenish time validations in GameItems.

Recommendations:

    function mint(uint256 tokenId, uint256 quantity) external {
        require(tokenId < _itemCount);
        uint256 price = allGameItemAttributes[tokenId].itemPrice * quantity;
        require(_neuronInstance.balanceOf(msg.sender) >= price, "Not enough NRN for purchase");
        require(
            allGameItemAttributes[tokenId].finiteSupply == false ||
            (
                allGameItemAttributes[tokenId].finiteSupply == true &&
                quantity <= allGameItemAttributes[tokenId].itemsRemaining
            )
        );
        require(
-           dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp ||
+           isReplenishTimePassed(msg.sender, tokenId) ||
            quantity <= allowanceRemaining[msg.sender][tokenId]
        );

        _neuronInstance.approveSpender(msg.sender, price);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, price);
        if (success) {
-           if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
+           if (isReplenishTimePassed(msg.sender, tokenId)) {
                _replenishDailyAllowance(tokenId);
            }
            allowanceRemaining[msg.sender][tokenId] -= quantity;
            if (allGameItemAttributes[tokenId].finiteSupply) {
                allGameItemAttributes[tokenId].itemsRemaining -= quantity;
            }
            _mint(msg.sender, tokenId, quantity, bytes("random"));
            emit BoughtItem(msg.sender, tokenId, quantity);
        }
    }

    function getAllowanceRemaining(address owner, uint256 tokenId) public view returns (uint256) {
        uint256 remaining = allowanceRemaining

[owner][tokenId];
-       if (dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp) {
+       if (isReplenishTimePassed(owner, tokenId)) {
            remaining = allGameItemAttributes[tokenId].dailyAllowance;
        }
        return remaining;
    }

+   function isReplenishTimePassed(address owner, uint256 tokenId) internal view returns(bool) {
+       return dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp;
+   }

Gas Optimization Issues

[G-01] Redundant attributeProbabilities Initialization in the AiArenaHelper Constructor

GitHub: Link, Link

The deployment cost is unnecessarily increased due to the probabilities being set twice in the attributeProbabilities mapping within the constructor: initially through the addAttributeProbabilities function and then within a for loop. Therefore, it's advised to eliminate the addAttributeProbabilities call from the constructor.

Additionally, incorporate the require(probabilities.length == 6, "Invalid number of attribute arrays"); statement in the constructor for validation.

[G-02] Utilize Cached Variable in Require Statement Checks

In the AiArenaHelper.addAttributeDivisor, utilize the cached attributesLength for the require statement check to decrease gas costs.

In the MergingPool.pickWinner, utilize the cached winnersLength for the require statement check to decrease gas costs.

[G-03] Use _spendAllowance in Neuron.burnFrom

Utilizing ERC20._spendAllowance can reduce gas costs due to its efficiency.

Recommendation

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

[G-04] Omit Redundant Allowance Check in Neuron.claim

The allowance check is already performed within ERC20.transferFrom -> ERC20._spendAllowance, making it unnecessary.

ERC20._spendAllowance

function _spendAllowance(
    address owner,
    address spender,
    uint256 amount
) internal virtual {
    uint256 currentAllowance = allowance(owner, spender);
    if (currentAllowance != type(uint256).max) {
        require(currentAllowance >= amount, "ERC20: insufficient allowance");
        unchecked {
            _approve(owner, spender, currentAllowance - amount);
        }
    }
}

[G-05] Cache Array Length in Loops

In FighterFarm.redeemMintPass, caching mintpassIdsToBurn.length will decrease gas costs.

Recommendation

function redeemMintPass(
    uint256[] calldata mintpassIdsToBurn,
    uint8[] calldata fighterTypes,
    uint8[] calldata iconsTypes,
    string[] calldata mintPassDnas,
    string[] calldata modelHashes,
    string[] calldata modelTypes
)  external {
+   uint256 mintpassIdsToBurnLength = mintpassIdsToBurn.length;

    require(
-       mintpassIdsToBurn.length == mintPassDnas.length &&
+       mintpassIdsToBurnLength == mintPassDnas.length &&
        mintPassDnas.length == fighterTypes.length &&
        fighterTypes.length == modelHashes.length &&
        modelHashes.length == modelTypes.length
    );
-   for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
+   for (uint16 i = 0; i < mintpassIdsToBurnLength; 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)]
        );
    }
}

[G-06] Reuse Cached Variables

In RankedBattle.updateBattleRecord, a variable stakeAtRisk for the specified tokenId is already cached. However, _addResultPoints also makes an external call to stakeAtRiskInstance. To decrease gas costs, pass the stakeAtRisk as a parameter to _addResultPoints.

function updateBattleRecord(
    uint256 tokenId,
    uint256 mergingPortion,
    uint8 battleResult,
    uint256 eloFactor,
    bool initiatorBool
) external {
    ...
    uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
    if (amountStaked[tokenId] + stakeAtRisk > 0) {
-       _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
+       _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner, stakeAtRisk);
    }
    ...
}

[G-07] Avoid Modifying Storage Variables with Zero

Not modifying storage variables with a zero value saves gas.

Recommendation:

Update points after points > 0 check.

function _addResultPoints(
    uint8 battleResult,
    uint256 tokenId,
    uint256 eloFactor,
    uint256 mergingPortion,
    address fighterOwner
) private {
    ...
    if (battleResult == 0) {
       ...

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