AI Arena - KmanOfficial's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L333-L365 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L161-L184

Vulnerability details

Impact

The FighterFarm contract implements the OpenZeppelin version of the ERC721 and ERC721 Enumerable contracts. In all versions of the OpenZeppelin documentation it states for ERC721 the "Required interface of an ERC721 compliant contract." involves the following safeTransferFrom signatures:

safeTransferFrom(from, to, tokenId) and safeTransferFrom(from, to, tokenId, data)

The FighterFarm contract only overrides the safeTransferFrom(from, to, tokenId) signature function which has a call to _ableToTransfer() which makes sure that the fighter NFT is not staked or being sent to an recipient who owns 10 fighter NFTs already.

As safeTransferFrom(from, to, tokenId, data) will be apart of the contract in order to be a fully compliant ERC721 contract, without an override a user could transfer NFTs using this method and circumvent these restrictions.

Proof of Concept

With no safeTransferFrom(from, to, tokenId, data) override present in FighterFarm.sol, the user will be able to use the safeTransferFrom(from, to, tokenId, data) defined in the OpenZeppelin ERC721 contract when calling it in a transaction to the FighterFarm contract bypassing any game specific restrictions.

Tools Used

Manual Code Review

Recommended Mitigation Steps

Implement the following override function in FighterFarm.sol

    /// @notice Safely transfers an NFT from one address to another.
    /// @dev Add a custom check for an ability to transfer the fighter.
    /// @param from Address of the current owner.
    /// @param to Address of the new owner.
    /// @param tokenId ID of the fighter being transferred.
    /// @param data Arbitrary data that is passed.
    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        bytes memory data
    ) 
        public 
        override(ERC721, IERC721)
    {
        require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, data);
    }

Assessed type

ERC721

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L131-L146 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L289-L303

Vulnerability details

Impact

GameItems.sol implements/inherits the OpenZeppelin implementation of the ERC1155 standard. The required compliant interface for ERC1155 involves the function signature safeBatchTransferFrom(address from, address to, uint256[] ids, uint256[] values, bytes data)

The game/protocol allows for items in the game to be created which has an transferable attribute which denotes whether the item can be traded or not. The safeTransferFrom function, which is a part of the ERC1155 compliant implementation, has been overridden in order to ensure a check if in place to only allow items which allowed to be traded, to be transferred to another address. - (require(allGameItemAttributes[tokenId].transferable);).

As the safeBatchTransferFrom function has not been overridden, it allows for an address to send non-transferrable items to another address using this function in the standard ERC1155 form.

Proof of Concept

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

There is no overridden form of safeBatchTransferFrom in the contract, meaning if the function is called, it will use the parent implementation from OpenZeppelin ERC1155 contract.

Tools Used

Manual Code Review - VS Code

Recommended Mitigation Steps

Implement the following overridden safeBatchTransferFrom in GameItems.sol

    /// @notice Safely transfers multiple NFTs from one address to another.
    /// @dev Added a check to see if the game item is transferable.
    //@audit - Ok
    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        for (uint256 i = 0; i < ids.length; ++i) {
            require(allGameItemAttributes[ids[i]].transferable, "At least one item is not transferable");
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

If the user has not claimed NRN rewards for several rounds. When the user eventually goes to claim the rewards, the transaction could revert due to consuming too much gas making the rewards stuck in the contract for that specific user.

Proof of Concept

User decides to play the game after large amount of rounds has taken place and then eventually accumulate points to claim or accumulated points in an early round and not until many rounds later do they decide to claim NRN.

• User calls claimNRN()
• Due to so many iterations to go through in the for loop, the transaction runs out of gas and reverts.
• User funds, rewards are stuck in the contract.

Tools Used

Manual Code Review, VS Code

Recommended Mitigation Steps

I suggest adding a maximum traversal limit parameter on the function to allow the for loop to go from the lower bound to lower bound + limit, in order to ensure the function can never experience DoS from the loop being unbounded.

It may be beneficial to have a mapping that records the first round that a user became eligible for rewards when recording the amounts of points a user is eligible for. This can then be referenced when claimNRN() is called in order to determine what is the initial lower bound that the for loop within claimNRN() should start from for a specific msg.sender and the numRoundsClaimed[msg.sender] would be added to this value in order to determine lowerBound.

Another suggestion, could be making rewards from after a certain amount of rounds unclaimable - e.g. unclaimed rewards after 10 rounds is no longer considered (which is 10 weeks - if 1 round lasts about a week). This allows the protocol to ensure that the for loop is always limited to a certain number of iterations eliminating the risk of DoS. The require statement for the function would need to be modified to ensure whether all NRN distributions for a user up to the latest round has been claimed.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L132-L134 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L333-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L322-L349

Vulnerability details

Impact

A fighter NFT may have stake at risk whilst a round is in progress, if the owner has fights where it loses and they are staking NRN against the fighter and they have no points. The more losses, the more NRN is staked at risk for the NFT. If the user doesn't win matches in an attempt to recover all of that stake at risk before the round ends, that NRN staked at risk is lost forever.

If the user at some point during the current round unstakes their whole amount of NRN that is no longer at risk, the NFT will also be unstaked. The user will be free to transfer this NFT to another address given that the NFT is no longer staked and the recipient owns less than 10 Fighter NFTs. The user who receives this NFT will now have an NFT that has stake at risk. The impact of this is that if a user now stakes NRN and loses a fight, their staked NRN is automatically subject to a higher penalty than expected (as the loss is a percentage of your staked + staked at risk on the asset) leading to potential loss for the user.

Proof of Concept

When the battle record is updated for the user (RankedBattle.updateBattleRecord()), it gets the stake at risk for the asset. _stakeAtRiskInstance.stakeAtRisk() returns the data that is held in a mapping that references the Fighter NFT Token Id and the current roundId. This mapping does not take in account the current msg.sender at all, so stake at risk is transferred to another user.

Tools Used

Manual Code Review - VS Code

Recommended Mitigation Steps

I suggest blocking transfers for NFTs that has stake at risk for a current round.

If this is something the game/protocol doesn't want to do, an alternative suggestion is modifying the stakeAtRisk mapping to include the referencing of msg.sender within the data structure. This allows the new address to not have a stakeAtRisk associated with using the NFT. If the original owner chooses to transfer the NFT to another address, they accept that the stakeAtRisk is potentially permanently lost, if they do not become the owner of the NFT again before the round ends are able to win a portion or all of the StakeAtRisk back.

Assessed type

Other

Low Risk / QA Issues

[01] Unnecessary emit of PointsAdded event for the Merging Pool when a fight has been won but the stakeAtRisk is more than 0.

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

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) {
                points = stakingFactor[tokenId] * eloFactor;
            }

            /// Divert a portion of the points to the merging pool
            uint256 mergingPoints = (points * mergingPortion) / 100;
            points -= mergingPoints;
            _mergingPoolInstance.addPoints(tokenId, mergingPoints);
...
}

If stakeAtRisk is 0, it means the user is eligible to gain some points based on how much they have staked, but initially points is initialised to 0 within the function so there is no need to do calculate mergingPoints if points will be 0. It leads to unnecessary execution of _mergingPoolInstance.addPoints which under all circumstances emits an pointsAdded event which is state data that doesn't need to be indexed as no points were actually added to the mergingPool.

Recommendation: Wrap the merging points calculation points and addition code within an if statement - if(points > 0).

--

[02] Total Battles is incorrectly updated twice for the same battle.

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

The totalBattles value globally tracks the number of fights that has taken place. A fight between two users is one battle. RankedBattle.UpdateBattleRecord is called twice for one fight. Once for the initiator and once for the opponent. The code incorrectly increments total battles by 1 for each call for the respective users in the fight. This would give the indication that two separate battles took place, when it's the same battle.

Recommendation: Move totalBattles += 1; to within the if (initiatorBool) { statement and increment the number of battles done globally across the game when the battle record update is sent to the blockchain for the initiator.

--

[03] GameItems mint allows for quantity to be set to 0 allowing for unnecessary execution and emit of the boughtItem event.

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

The missing require statement on the quantity allows for unchecked execution of the mint function leading to 0 value transfers and mint and the emitting of the boughtItem event with a quantity - despite no harm to the protocol. It is against best practice for recording unnecessary state data.

Recommendation: add an require statement at the top of the function before main exeuction - require(amount > 0, "Mint at least one item");

--

[04] If the user has all their stake converted to stakeAtRisk and are still participating in battles, upon updating their battle record the emitting of IncreasedStakeAtRisk events takes place despite the user have 0 non risk NRN tokens to add.

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

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L115-L127

When the user in question has lost the fight, if they have no points it is intended that stake at risk is deducted instead. If all of their users initially proposed staked, is now at risk, there is no more NRN to add to the staked at risk balance for the fighter NFT for that current round. The function will perform a 0 transfer which will result in success and then call stakeAtRisk.updateAtRiskRecords which will emit an IncreasedStakeAtRisk event. Although it has no huge consequences, it is against best practice as the stake at risk has not increased and has in fact stayed the same.

Recommendation: Modify the if statement before calling stakeAtRisk.updateStakeAtRisk from if (success) to if (success && curStakeAtRisk > 0) At L494 in RankedBattle.sol

--

[05] User can spend their voltage in a direct transaction despite not fighting.

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/VoltageManager.sol#L105-L112

spendVoltage can be operated by the msg.sender themselves or via an authorised spender such as the Ranked Battle contract. It is too be spent for actions in the game such as the user who initiates a battle regardless of the outcome. The user is able to directly post a transaction to the blockchain with this function and spend any amount of voltage. Although there is no benefit to a user doing this, it is still unintended behaviour where a user could accidentally spend all voltage and have to wait until their replenish time before they use the game again.

Recommendation: Don't allow spend voltage to be called directly by the msg.sender as there is no need. Any interaction with the protocol can handle the voltage spending directly as authorised spenders on behalf of users of the game.

require(spender == msg.sender || allowedVoltageSpenders[msg.sender]); to require(allowedVoltageSpenders[msg.sender]); instead.