AI Arena - ZanyBonzy's results

Lines of code

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

Vulnerability details

Impact

Game items upon creation have the tranferable status enabled/disabled. If the status is disabled, such an item should ideally not be transferrable.

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 
        public 
    {
 ...
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

This status can also be adjusted.

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

So when a game item is to be transferred, the safeTransferFrom function overrides the ERC1155 safeTransferFrom function and checks for the game item's transferrability status.

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

The issue is that this can be bypassed by calling the ERC1155 safeBatchTransferFrom function instead, passing in the tokens and the amount they'd like to transfer. Important to note that the safeBatchTransferFrom operates independently of the safeTransferFrom function.

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

Proof of Concept

• User wants to transfer game items that should be untransferable.
• He transfers them through the safeBatchTransferFrom function.
• Transfer executes, breaking the non-transferability invariant.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider introducing an check for transferability the safeBatchTransferFrom function also, or disabling it altogether.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When a fighter loses battles to a certain point, amount staked on it begins to get reduced. At a certain point however, stake is no longer deducted so as to prevent reversions due to underflow protection. A user can take advantage of this to rescue stakeAtRisk at only the voltage and gas cost.

Proof of Concept

When a user loses a battle, they lose points and subsequently stake. However, a cap is set on the amount that a user can lose, ideally, not more than the amount staked.

else if (battleResult == 2) {
            /// If the user lost the match

            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }
...
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }
        }
    }

Knowing this, a user can call the unstakeNRN function unstaking all the amountStaked on the fighter.

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        if (amount > amountStaked[tokenId]) {
            amount = amountStaked[tokenId];
        }
        amountStaked[tokenId] -= amount;
...
        }
    }

Since amountStaked on the fighter is now 0, the penalizations for losing games is now zero, as curStakeAtRisk is now 0.

            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }

Now, when games are won, a portion of the stakeAtRisk is claimed and added to amountStaked.

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {
...
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }
...
            }

The user can the call the unstakeNRN function again to rescue the newly rescued amountStaked.

In summary,

• User has stakeAtRisk
• He unstakes all of the amountStaked
• He keeps playing
• When he loses, he's not penalized since he has no amountStaked
• When he wins, a part of the stakeAtRisk added to the 0 amountStaked
• He quickly unstakes this amount, turning his amountStaked to be 0.
• He continues playing again repeating the unstake process after every win till his tokens are fully rescued.
• He can do this until he can start earning points on the last rescued and unstaked stakeAtRisk
• The only cost will be gas and voltage.

Tools Used

Manual code review

Recommended Mitigation Steps

Protocol has to consider if this is an acceptable risk. A potential fix is to prevent unstaking if a user has stakeAtRisk, although that can be a little heavy handed.

Assessed type

Other

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

6. Users can mint more fighters and potentially bypass max fighters allowed limit through _safemint reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L285 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L244 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L270 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L322-L349 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L416-L471 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/StakeAtRisk.sol#L104

Vulnerability details

Impact

This issue is based on the assumption that fighters can be sold/transferred on marketplace and events take place before a new round is set. A malicious user can honeypot unsuspecting users by selling them fighters with large amount of stake at risk. The users do this by accumulating a large amount of stakeAtRisk, unstaking the little amount they have left and then selling the fighter to unsuspecting users. The victims will not be able to rescue the stakeAtRisk in certain cases, and might not be able to have their game records updated.

Proof of Concept

When a user has stakes on a fighter for the first time, the fighter staking status is updated and locked, thereby preventing transfers.

    function stakeNRN(uint256 amount, uint256 tokenId) external {
...
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, true); //@note
            }
            amountStaked[tokenId] += amount;
            globalStakedAmount += amount;
            stakingFactor[tokenId] = _getStakingFactor(
                tokenId, 
                _stakeAtRiskInstance.getStakeAtRisk(tokenId)
            );
            _calculatedStakingFactor[tokenId][roundId] = true;
            emit Staked(msg.sender, amount);
        }
    }

Upon unstaking all amountStaked, the fighter is unlocked and can now be transferred.

   function unstakeNRN(uint256 amount, uint256 tokenId) external {
...
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

Now, when a fighter loses, the fighter loses points, and upon losing all points, begin to lose part of their stake.

else if (battleResult == 2) {
...
            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }

At a certain point, a malicious user who has racked up lots of stake at risk can decide to unstake all the amount they have left. Because of this, the amountStaked on the fighter becomes 0, which updates the fighter staking status to false, thereby unlocking the fighter for transfer.

This fighter can then be listed on a marketplace for unsuspecting users to purchase. Upon purchase/transfer, the fighter owner is now updated to the victim as the new owner.

The new owner cannot gain points on the fighter, because there's stake at risk.

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

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

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

            /// If the user has no NRNs at risk, then they can earn points
            if (stakeAtRisk == 0) { //@note there's stake at risk, so user gets no points
                points = stakingFactor[tokenId] * eloFactor;
            }

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

            /// If the user has stake-at-risk for their fighter, reclaim a portion
            /// Reclaiming stake-at-risk puts the NRN back into their staking pool
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner); //@note new owner cannot reclaim. 
                amountStaked[tokenId] += curStakeAtRisk;
            }
            ...

and might not be able reclaim the stake at risk because the reclaimNRN function checks against amount the new owner has lost.

    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        require(
            stakeAtRisk[roundId][fighterId] >= nrnToReclaim, 
            "Fighter does not have enough stake at risk"
        );

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

In this, case, user's battle record will not be updateable.

A brief summary would be;

• User A stakes on his fighter, locking it from transfer.

• He loses battles, loses points and begins to lose stake. His amountLost is also updated.

• Upon racking up lots of stake at risk, he calls the unstakeNRN function to unstake all of his remaining amount.

• Due to the check below, his staking status is updated, he can now transfer his fighter.

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

• He honypots an unsuspecting user B (a new user) by selling them the tokens.

• User B is unable to get points upon winning because there's stake at risk, due to this check.

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

• User B can also not reclaim stake at risk upon winning if he initially had no amountLost because of the functions below.

    ```
     if (curStakeAtRisk > 0) {
            _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
            amountStaked[tokenId] += curStakeAtRisk;
        }
    ```
    and the overflow
  
    ```
    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
    ...
    bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
    if (success) {
        stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
        totalStakeAtRisk[roundId] -= nrnToReclaim;
        amountLost[fighterOwner] -= nrnToReclaim; 
        emit ReclaimedStake(fighterId, nrnToReclaim);
    }
  

  } ```

• This causes the entire function chain to revert and the updateBattleRecord function fails.

Tools Used

Manual code review

Recommended Mitigation Steps

Consider preventing fighter transfer or unstaking if figher has stake at risk. Refactor the unstaking check.

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When a tokens are newly staked on a fighter, the staking status is updated, which locks the figter and prevents its transfer.

    function stakeNRN(uint256 amount, uint256 tokenId) external {
        ...
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, true);
            }
        ...
        }
    }

A user with stake at risk can unstake all of the tokens that is not at risk, which updates the staking status to false, opening the fighter up for transfer.

    function unstakeNRN(uint256 amount, uint256 tokenId) external {
        ...
        amountStaked[tokenId] -= amount;
        ...
        if (success) {
            if (amountStaked[tokenId] == 0) {
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

Now, even though, the user has no amount staked and technically should only have records updated, without calling the internal _addResultPoints function, because there's still stake at risk, the addResultPoints function will be called.

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        ...
        _updateRecord(tokenId, battleResult);
        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        if (amountStaked[tokenId] + stakeAtRisk > 0) { //@note call will go through because user only has stake at risk
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
        ...
    }

Upon winning games, they get to reclaim a portion of the stake at risk, which is added to the amount staked on the fighter. This breaks the invariant that fighters with stakes should not be transferrable.

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
...
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
...
        if (battleResult == 0) {
...
            /// If the user has stake-at-risk for their fighter, reclaim a portion
            /// Reclaiming stake-at-risk puts the NRN back into their staking pool
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk; //@note
            }
...
            }
        }

Upon starting a new round, the user can finally stake more on the fighter, and the fighting status will still not be updated because the user had an amountStaked from the previous round.

    function stakeNRN(uint256 amount, uint256 tokenId) external {
        ...
        require(hasUnstaked[tokenId][roundId] == false, "Cannot add stake after unstaking this round"); //@note will go through because new round

        _neuronInstance.approveStaker(msg.sender, address(this), amount);
        bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);
        if (success) {
            if (amountStaked[tokenId] == 0) { //@note amount staked is no longer 0 from previous rounds
                _fighterFarmInstance.updateFighterStaking(tokenId, true);
            }
            ...
        }
    }

In summary:

• User has stakes on a fighter, with certain stakeAtRisk.
• He unstakes all of his fighter's amountStaked so his staking status gets deactivated.
• But, because he still has stakeAtRisk, updating his battle record still calls the _addResultPoints function.
• User wins, he has a portion of the fighter's stakeAtRisk reclaimed which is then added to the amountStaked on the fighter.
• At this stage, the staking status is not updated, so the fighter can be transferred, even though it has been staked on.
• When a new round is started, the user can call the stakeNRN function to stake more tokens. Since amountStaked still exists from the previous round, the fighter staking status is not updated.
• So fighter has stakes on it and can still be transferreed.

Tools Used

Manual code review

Recommended Mitigation Steps

The staking status should be updated after reclaiming stakeAtRisk.

if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk; 
                _fighterFarmInstance.updateFighterStaking(tokenId, true);
            }

Assessed type

Other

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

Daily Game Item allowance limit can be bypassed through transfers.

1. useVoltageBattery function should check for user replenish time.

Lines of code*

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L93

Impact

Users have the choice to replenish their voltage either by waiting for the replenish time to end or but using the voltage battery. The useVoltageBattery function doesn't check if the replenish time has passed before replenishing the user's voltage. Hence, users unaware can call the function, burning one of their batteries, only to have the voltage replenished again when the spendVoltage function is called.

    function useVoltageBattery() public { 
        require(ownerVoltage[msg.sender] < 100); 
        require(_gameItemsContractInstance.balanceOf(msg.sender, 0) > 0); 
        _gameItemsContractInstance.burn(msg.sender, 0, 1); 
        ownerVoltage[msg.sender] = 100; 
        emit VoltageRemaining(msg.sender, ownerVoltage[msg.sender]);
    } 

Recommended Mitigation Steps

Consider including a check for replenish time in an if else format, so as to save users batteries. Also, consider reducing the required voltage percentage to something lesser, e.g 10 so as to help users save cost on batteries.

    function useVoltageBattery() public { 
        require(ownerVoltage[msg.sender] < 100); 
        if (ownerVoltageReplenishTime[spender] <= block.timestamp) { 
            _replenishVoltage(spender);
        } 
        else{
        require(_gameItemsContractInstance.balanceOf(msg.sender, 0) > 0); 
        _gameItemsContractInstance.burn(msg.sender, 0, 1); 
        ownerVoltage[msg.sender] = 100; 
        }
        emit VoltageRemaining(msg.sender, ownerVoltage[msg.sender]);
    } 

---

2. Transferring ownership doesn't disable admin status for old owner or enable admin status for new owner

Lines of code* https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L146-L158 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L167 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L176 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L95-L99 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L108 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L117 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L71-L79 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L89 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L98 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L68-L76 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L85 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L118 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L51-L55 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L64 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/VoltageManager.sol#L73

Impact

In the constructor of the Neuron, GameItems, VoltageManager, MergingPool, RankedBattle, contracts, the owner address is declared and immediately granted admin acess. However, upon transfer of ownership to another address, the new owner isn't immediately granted admin access, neither is the old owner's admin access revoked. The owner has to manually revoke the admin access for the previous owner and grant himself owner. For example, in the VoltageManager contract

    constructor(address ownerAddress, address gameItemsContractAddress) {
        _ownerAddress = ownerAddress;
        _gameItemsContractInstance = GameItems(gameItemsContractAddress);
        isAdmin[_ownerAddress] = true;
    } 
    ...
    function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
        _ownerAddress = newOwnerAddress;
    }
    ...
    function adjustAdminAccess(address adminAddress, bool access) external {
        require(msg.sender == _ownerAddress);
        isAdmin[adminAddress] = access;
    }  

Recommended Mitigation Steps

Consider rechecking if this is intended behaviour or fixing to ensure a more seamless transition.

    function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
        isAdmin[_ownerAddress] = false;
        _ownerAddress = newOwnerAddress;
        isAdmin[newOwnerAddress] = true;
    }

---

---

3. Consider checking for max allowance in the burnFrom function before decreasing allowance

Lines of code*

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L201

Impact

The burnFrom function deducts the amount to burn from the spender's allowance before actually burning. This doesn't take into consideration that spenders that might have max approval. Such users, ideally shouldn't have their allowances decreased whenever they make perform transactions on behalf of the owner.

    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;
        _burn(account, amount);
        _approve(account, msg.sender, decreasedAllowance);
    }

Recommended Mitigation Steps

Consider making a check for type(uint256).max approval before decreasing the allowance. Or making a call to the OZ ERC20 _spendallowance function instead.

---

4. mint function should use <= to account for last token when minting.

Lines of code*

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L156

Impact

The check against mint more than max supply uses the < operator, which ensures that the sum of totalSupply and amount to mint is always less than the max supply. Doing this will cause that the max supply will never be reached, leaving one last token unmintable. Consider using the <= operator to more appropriately cap the max supply.

require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply")

---

5. Introduce protections from approval race conditions

Lines of code* https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L171 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L184 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L163 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L375 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L250

Impact

As a recommendation from the EIP20 standard, to prevent attack vectors like the one described hereand discussed here, consider refactoring the approvals in such a way that they set the allowance first to 0 before setting it to another value for the same spender.

Recommended Mitigation Steps

Consider approving to zero first to before granting new allowances.

---

6. Users can mint more fighters and potentially bypass max fighters allowed limit through _safemint reentrancy

Lines of code* https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L529

Impact

When calling functions to mint a fighter in the FighterFarm, the internal _createNewFighter function is called. A check is made to ensure that user doesn't have more than the maximum allowed fighters. The issue here is that the _safeMint function that is called contains a callback function to.onERC721Received. A user can reenter the function through the hook.

   function _createNewFighter(
        address to, 
        uint256 dna, 
        string memory modelHash,
        string memory modelType, 
        uint8 fighterType,
        uint8 iconsType,
        uint256[2] memory customAttributes
    ) 
        private 
    {  
        require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
...
        _safeMint(to, newId);
        FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);
    }

Using the mintFromMergingPool function as an example,

• A winner(a contract) calls the claimRewards function.
• The function calls the mintFromMergingPool function, which calls the createNewFunction function.
• The _createNewFighter, which uses _safeMint internally.
• _safeMint calls onERC721Received() on the contract, the contract re-enters the function again, leading to multiple mints.

Recommended Mitigation Steps

Consider using the reentrancy guard.

7. Daily Game Item allowance limit can be bypassed through transfers.

Lines of code*

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

Impact

When creating a game item, a daily allowance limit is set for each user. When a user mints a game item, a check is made for the user's allowance upon which the quantity of game items minted is subtracted.

    function mint(uint256 tokenId, uint256 quantity) external {
...
            if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {
                _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);
        }
    }

However, if a game item is transferrable, users can bypass the allowance limit by transferring the items between themselves. as it makes no check for user's allowances.

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

Recommended Mitigation Steps

Consider including a check for user's allowances upon transfer.

Advanced Analysis Report for <img width= 150px src="https://gist.github.com/assets/112232336/d060bb61-2839-4d5c-a0b3-382d8ec0079d" alt="Logo">

Audit approach

Brief Overview

Roles

Scope and Architecture Overview

Codebase Overview

Centralization Risks

Systemic Risks

Recommendations

Conclusions

Resources

---

1. Audit approach

• Documentation Dive: A comprehensive analysis of provided docs was conducted to understand protocol functionality, key points were noted, ambiguities were discussed with the devs, and possible risk areas were mapped.

• Static Analysis & Linter Sweep: Initial vulnerability and error detection was performed using static analyzers and linters and known issues and false positives were excluded.

• Code Inspection: Manual review of each contract within defined sections was conducted, testing function behavior against expectations, and working out potential attack vectors. Vulnerabilities related to dependencies and inheritances, were assessed. Comparisons with similar protocols (including older commits) was also performed to identify recurring issues and evaluate fix effectiveness.

• Report Compilation: Identified issues were generated into a comprehensive audit report.

---

2. Brief Overview

• AI Arena is a fighting game where players train and battle AI fighters. AI Arena lets users collect and train AI-powered fighters as NFTs, hone their skills, then send them into battle against other players' champions in a platform fighting game. These fighters are represented as NFTs with unique traits like appearance, abilities, and type.
• Players can enter their fighters into ranked battles to win rewards in the game's own token called $NRN and points, but stand a risk of losing part of their stake upon losing. To keep the game fair, a StakingFactor, based on the square root of the staked amount to is used for points calculation, each wallet has voltage (energy) replenished daily to 100 and needs to be refilled either by waiting for natural refill time or buying a battery from the store.

---

3. Roles

• Owner - The contract deployer and initial admin of the contracts. He's responsible for setting most of the important protocol parameters and addresses.

• Admins - The admins can be or be set by the contract owner. His responsibilities include starting new game rounds, setting token uri, pick raffle winners, setting up airdrop recipients and so on.

• Minters - These are addresses responsible for minting the NRN tokens. Ideally, this role is granted to the RankedBattle contract, but can also be granted to other addresses as well.

• Burners - These are addresses granted the permission to burn game items or mint passes. The VoltageManager and the FighterFarm contracts are granted this role to burn game items and mint passes respectively.

• Delegate - The delegated address is the address in charge of signing in messages for claiming Fighter NFTs.

• Stakers - These adresses have the permissions to stake Fighter NFTs and NRN on behalf of the user. This role ideally is granted to the RankedBattle contract.

• Spenders - These are the roles approved to spend user's NRNs and voltage on their behalf. The FighterFarm and GameItems contract are granted the spender role for the NRN tokens, while the RankedBattle contract is allowed to spend voltage on behalf of the user.

---

4. Scope and Architecture Overview

<p align="center"> Contract Architecture </p> <p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/bfe6f0fa-3157-46f1-b7ba-4925972ed36a" alt="Contract Architecture"> </p>

---

<h3 align="center"> <b>Core Game Mechanics</b> </h3>

RankedBattle.sol

• The RankedBattle contract is probably the most important contract in the protcol, handling the process of 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. Due to its important nature, it also holds a number of important roles in the protocol, which are all granted by the owner including minter and staker roles in the Neuron contract, burner role in the GameItem contract and so on.

Round Setup - The admin, to start a new round calls the setNewRound function which clears the stakes at risk in the previous round and sets the new neruon tokens to distribute to winners.

Token staking and unstaking - Users can stake NRN tokens on their fighter every round by calling the stakeNRN, theres also the option to keep staking more, icreasing their stake, hence, their point output upon winning battles. They can also call the unstakeNRN function to remove a portion or all of their stake if they so wish. The functions, in theory have been setup to prevent fighter transfer while staking.

Battle record update - Since a large portion of the game mechanics is stored offchain, it becomes important to keep the figher's records up to date so as to properly calculate the user's ranking on the leaderboard and how much rewards/punishment they're entitled to. The gameServerAddress is takes care of this, by calling the updateBattleRecord function which updates the fighter's wins, losses and ties, updates the points a user is entitled to after each battle.

As a basic breakdown, users gain points for winning, lose points for losing. Upon which after losing all their accumulated point, they begin to lose a portion of their stake (the stakeAtRisk). By winning again, users can reclaim a portion of the stakeAtRisk. No changes are made on ties.If users wish, they can divert a portion of their points into a raffle in the MergingPool contract.

NRN reward claim - After a round is ended, winners can call the claimNRN function which calculates the amount they're entitled to as a function of their accumulated points. The function mints the NRN tokens to the deserving winners.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/f3fbe3ff-b55c-43b4-b588-3d6ee7236c21" alt="RankedBattle.sol"> </p> <p align="center"> sLOC - 300 </p>

StakeAtRisk.sol

• The StakeAtRisk contract is an extension of the RankedBattle contract that handles the accounting for the user stakes at risk. Virtually all calls to this contract are expected to originate from the RankedBattle contract.

Stake sweeping - Upon a call from the RankedBattle's setNewRound function, a call to the setNewRound function which sweeps the stakes at risk for the previous round before updating the current round.

StakeAtRisk reclaim and record update - When users who have stake at risk finally win games, their updated record in the RankedBattle contract makes a call to the reclaimNRN function to return the tokens back to the user. It also calls the updateAtRiskRecords function which makes an update to the stake at risk records.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/71c752b3-2b68-44aa-b5aa-d210375a06eb" alt="StakeAtRisk.sol"> </p> <p align="center"> sLOC - 63 </p>

MergingPool.sol

• The MergingPool contract is the raffle contract through which lucky users can be selected by the contract admin. Selected users can then claim their free fighter NFT.

Winner selection - The admin can select certain addresses as the winners of the raffle for each round. They do this by calling the pickWinner function. Rewards claiming - The winners can call the claimRewards to claim their rewards for multiple rounds, once for each round. THe function mints their NFT to them from the FighterFarm

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/0adafe53-f4ea-4dc6-a376-7eceeb32cef3" alt="AiArenaHelper.sol"> </p> <p align="center"> sLOC - 110 </p>

---

<h3 align="center"> <b>Fighter Management</b> </h3>

FighterFarm.sol

• The contract manages the creation and mint new fighters either by claiming, minting from a merging pool or redeeming of mint passes. Users also have the option to reroll if they don't like their fighter attributes. The fighters can also be transferred to other users if its not being staked on.

Fighter minting - Fighters can be minted in three major ways through signature, mintpass redemption and winning raffles from the MergingPool. Using a provided signature, users can claim a predetermined number of fighters by calling the claimFighters function, passing it a given signature, which allows the user mint the fighters. Users who have a mintpass can burn the mintpass to redeem a fighter by calling the redeemMintPass function. Raffle winners who decide to claim their prize in the MergingPool contract get their claims transfered to this contract through the mintFromMergingPool function. Users not pleased with their minted fighter have the option of calling the reRoll function to change the fighter's attributes.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/bbf3ea4c-7ce8-4551-82be-47e922cce456" alt="FighterFarm.sol "> </p> <p align="center"> sLOC - 327 </p>

FighterOps.sol

• This library is for managing fighters in the AI Arena game.It holds various information about a fighter like its weight, element, id, modelHash, type generation and so on. The getFighterAttributes and the viewFighterInfo functions can be called to view these information.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/63c81af3-ddc3-4b1c-9321-b9d44ed15ffe" alt="FighterOps.sol"> </p> <p align="center"> sLOC - 74 </p>

AiArenaHelper.sol

• This contracts holds and manages a fighter's attributes. Here, contract owner can create physical attributes for a user based on their (fighter) DNA, add and delete attributes and its probabilities and convert the DNA and rarity rank into an attribute probability index.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/bacb9637-3148-4c34-9fec-940cd97147de" alt="AiArenaHelper.sol"> </p> <p align="center"> sLOC - 95 </p>

---

<h3 align="center"> <b>Game Assets & Economy</b> </h3>

GameItems.sol

• The GameItems contract holds the support items used while gaming in the AI Arena. Admin can create an item and set its various characteristics, including price and supply. Users can then purchase these items to use in their games while paying a fee. Currently, no game item has been created, but the devs plan on creating the battery which can be used to replenish voltage if default given voltage is exhaused.

Game item creation - The admin calls the createGameItem function to create a new game item of a specific token id. Here, admin can specify the item name, price, uri, its total supply and the user's daily allowance. Admin can also set if the item is transferrable and if the supply is finite.

Game item purchase, distribution and usage - To purchase a game item, the user calls the mint function passing in the required game item token id and the quantitiy. The function then mints them the item. By calling the burn function on behalf of a user, the specified burn addresses (in this case, the VoltageManager contract) can destroy the game item upon usage. If a game item is transferrable, users can distribute it among themselves by calling the safeTransferFrom function. Important to note that users have a specific allowance limit for items they can hold. This limit can however be bypassed by transfer.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/0630a47e-73c0-4682-ae54-6a127b58a7d0" alt="AiArenaHelper.sol"> </p> <p align="center"> sLOC - 163 </p>

VoltageManager.sol

• The VoltageManager contract is the adapter contract that controls the functions of the battery game item. Here, users can manage the use of their batteries or their voltage.

Battery Usage - When a user is low on voltage, they call the useVoltageBattery function, which burns a battery from them, in exchange for replenishing their voltage back to max.

Voltage usage - A user or the approved voltage spenders (including the RankedBattle contract) can call the spendVoltage function to use a certain quantity of the user's remaining voltage. The function also replenishes a user's default voltage if the replenish time has passed.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/305a10a4-123a-48a1-8028-d6880bf0aab6" alt="VoltageManager.sol"> </p> <p align="center"> sLOC - 47 </p>

Neuron.sol

• This is the NRN token contract. It's the ERC20 token native to the protcol used as the protocol's economic system, motivating players and aligning their goals. Players can acquire NRN by buying it on the market place or earning it through rewards.
• The maximum token supply is about 9 billion, with 200 million being minted directly to the treasury and another 500 to the contributors upon launch.

Role Distribution by owner - The contract owner by calling certain functions can distribute the various contract roles to addresses. The owner can do this by calling the addMinter, addStaker, addSpender and adjustAdminAccess functions.

Airdrop approval and claim - The admin by calling the setupAirdrop function can approve a number of recipients for aidrop. These recipients can now call the claim function to be transferred their airdrop due.

Token minting and burning - The address with the minter role, the RankedBattle address can call the mint function to mint a specified amount of tokens to a specified recipient. Users who so wish can call the burn and burnFrom function to permanently destroy their tokens and remove it from circulation.

Granting Approvals - The RankedBattle address and any other address with the staker role can call the approveStaker function to approve an address to stake on behalf of the user. The same goes for the FighterFarm and GameItem contracts and addresses with the spender role, upon calling the approveSpender function, to approve an address to spend the user's tokens.

<p align="center"> <img width= auto src="https://gist.github.com/assets/112232336/d5f68aa8-adc4-4185-9c10-1f838c2c0eb0" alt="Neuron.sol"> </p> <p align="center"> sLOC - 92 </p>

---

5. Codebase Overview

• Audit Information - For the purpose of the security review, the Ai-Arena codebase consists of eight smart contracts totaling 1271 SLoC. It holds five external imports and four structs. Its core design principle is composition, enabling efficient and flexible integration. It is scheduled to be deployed on the Arbitrum L2 chain, giving each user the freedom to transact in and across all platforms. It depends on the protocol's game server oracle which puts the battle results on chain.

• Documentation and NatSpec - The codebase provides important resources in form of the documentation and an overview of the website. While the documentation provides a good enough overview, it appears to be outdated, passing off old information. The contracts are well commented, not strictly to NatSpec but each function was well explained. It made the audit process easier.

• Error handling and Event Emission - Require errors with error strings is used in the codebase. Events are well handled, emitted for important parameter changes, although in some cases, they seemed to not follow the checks-effects-interaction patterns.

• Testability - Test coverage is about 90% which is very gooed. The implemented tests are mosty unit tests which tests the basic functionalities of the functions and helped with basic bugs. The more complex parts of the codebase are not fuzz tested, neither are there any invariant tests.

• Token Support - The protocol works with the Fighter ERC721 token, Game Item ERC1155 token and NRN ERC20 tokens.

• Attack Vectors - For the protocol, major points of attack include gaming reward system, losing battles while staking without losing points of tokens, bypassing various protocol and so on.

---

6. Centralization Risks

Like any protocol that incorporates owner and admin functions including a number of other roles, hence the protocol is fairly decentralized. However, the actions of a malicious central powers can negatively impact the protocol and its users. Some of them include:

• Griefing users by setting bpsLostPerLoss to max which causes users to lose all their staked amount upon losing a battle.
• Setting rankedNrnDistribution to 0 so that users earn no rewards upon winning battles.
• Setting various important protocol address to zero or wrong values.

---

7. Systemic Risks

• User activity is very important as the protcol will not really function if users are not very active.
• Malicious users looking for various ways to game the point system while not having much stake to lose.
• The protocol's game server oracle can fail and affect battle report update, hence reward distribution.
• Lack pause protocols in place during emergency market conditions.
• Issues with external dependencies, solidity version bugs and open zeppelin contracts.
• Issues from other smart contracts, which at best may be contained in the erring contracts, at worst affect the whole protocol.

---

8. Recommendations

• The codebase should be sanitized, and the comments should also be brought up to date to conform with NatSpec. The same goes for the provided documentation. A lot of the information in there are outdated. These need to be fixed.

• While the NRN token is a standard ERC20 token, and is the only ERC20 token interacting with the protocol, using basic transfer methods for is not advisable. The return values are not explicitly checked for failure (only for success), so the protocol still has the problem of transfers failing silently, which in certain situations doesn't restore updated state changes back to default; Consider using safer methods instead;

• Game items are made with an explicit maximum amount that users are allowed to hold daily, however, this amount doesn't take into consideration that certain items are transferable. Transferring these items can be used to bypass daily limit. Consider deciding if this is intended or taking maximum daily limit into consideration upon transfer.

• Information about the tokenUri should also include a chainid in the rarecase of a hardfork. This helps remove the confusion of which owner owns which token on which chain. Important to also note that newly rerolled fighters have an empty string as tokenUri, consider setting a default uri for them instead.

• Consider checking also, if a user is using their voltage battery, that a user's replenish time is passed. This saves the users from spending on voltage when they could easily just get their daily replenish voltage.

• In the constructor of most of the contracts, the owner is explicitely set as admin, however, upon ownership transfer, the previous owner is still left as admin, while the new owner doesn't have admin functions. This is not advisable. Consider fixing this, unless, it's by design.

• The protocol uses the game server as the oracle for updating user records. For this purpose, a fallback oracle will also be of great help in cases of server crash or downtime.

• A proper pause function can be implemented to protect users in cases of turbulent market situations and black swan events. It also helps prevent malicious activities or bugs from causing significant damage while the team investigates the potential issues without affecting users' funds.

• Two step variable and address updates should be implemented. Most of the setter functions implement the changes almost immediately which can be jarring for the contracts/users. Adding these fixes can help protect from mistakes and unexepected behaviour. At the same time, a timelock also helps and gives users/admins time to react quickly to these situtaions. Other sanity checks, max/min values check also protect the users from griefing, intentional or not.

• Testing should be improved, including invariant and fuzzing tests;

• Solidity and OpenZeppelin contract versions should be updated to the latest versions as they provide lots of benefits in security and optimization. It's best to use a specific compiler version;

---

9. Conclusions

• The AI-Arena codebase appears to be fairly well-designed,As is the reason for the audit, the identified risks need to be fixed. Recommended measures should be implemented to protect the protocol from potential attacks. Timely audits and sanitizations should be conducted to keep the codebase fresh and up to date with evolving security times.

---

10. Resources

• C4 ReadMe
• Documentation
• Website

Time spent:

36 hours