AI Arena - SAQ's results

Summary

Gas Optimization

Gas Optimizations

[G-01] Internal functions only called once can be inlined to save gas

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

file: /src/FighterFarm.sol

447    function _beforeTokenTransfer(address from, address to, uint256 tokenId)
        internal
        override(ERC721, ERC721Enumerable)
    {
        super._beforeTokenTransfer(from, to, tokenId);
    }

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

[G-02] Using fixed bytes is cheaper than using string

As a rule of thumb, use bytes for arbitrary-length raw byte data and string for arbitrary-length string (UTF-8) data. If you can limit the length to a certain number of bytes, always use one of bytes1 to bytes32 because they are much cheaper.

file: /src/GameItems.sol

49    string public name = "AI Arena Game Items";

52    string public symbol = "AGI";

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

file: /src/AiArenaHelper.sol

17    string[] public attributes = ["head", "eyes", "mouth", "body", "hands", "feet"];

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

file: /src/FighterOps.sol

41        string modelHash;
42        string modelType;

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterOps.sol#L41C1-L41C26

[G-03] Should use arguments instead of state variable

state variables should not used in emit , This will save near 97 gas

file: /src/VoltageManager.sol

///@audit ' ownerVoltage ' 

98        emit VoltageRemaining(msg.sender, ownerVoltage[msg.sender]);

111        emit VoltageRemaining(spender, ownerVoltage[spender]);

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

[G-04] Before transfer of some functions, we should check some variables for possible gas save

Before transfer, we should check for amount being 0 so the function doesn't run when its not gonna do anything

file: /src/StakeAtRisk.sol

143        return _neuronInstance.transfer(treasuryAddress, totalStakeAtRisk[roundId]);

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

[G-05] Do not calculate constant

When you define a constant in Solidity, the compiler can calculate its value at compile-time and replace all references to the constant with its computed value. This can be helpful for readability and reducing the size of the compiled code, but it can also increase gas usage at runtime.

file: /src/Neuron.sol

37    uint256 public constant INITIAL_TREASURY_MINT = 10**18 * 10**8 * 2;

    /// @notice Initial supply of NRN tokens to be minted and distributed to contributors.
40    uint256 public constant INITIAL_CONTRIBUTOR_MINT = 10**18 * 10**8 * 5;

    /// @notice Maximum supply of NRN tokens.
43    uint256 public constant MAX_SUPPLY = 10**18 * 10**9;

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L37C1-L45C41

[G-06] Use hardcode address instead address(this)

Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address. Foundry’s script.sol and solmate’s LibRlp.sol contracts can help achieve this. References: https://book.getfoundry.sh/reference/forge-std/compute-create-address

file: /src/RankedBattle.sol

250        _neuronInstance.approveStaker(msg.sender, address(this), amount);
251        bool success = _neuronInstance.transferFrom(msg.sender, address(this), amount);

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

[G-07] Pre-increment and pre-decrement are cheaper than +1 ,-1

file: /src/VoltageManager.sol

119        ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days);

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

file: /src/RankedBattle.sol

238        rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];

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

Summary

Analysis Issue Report

[File-1] AiArenaHelper

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

Ownership Transfer: The contract allows the owner to transfer ownership, which could be a potential risk if not handled carefully. If ownership is transferred to a malicious address, it might lead to unauthorized changes in the contract state or functionality.

Attribute Divisor Modification: The owner can modify attribute divisors, which may impact the randomness and distribution of generated attributes. Care should be taken to ensure fair and secure attribute generation.

Systemic Risks:

Ownership Dependency: Several critical functions are accessible only to the owner. Any compromise or loss of access to the owner's private key may result in the loss of control over the contract.

Attribute Probabilities: The contract heavily relies on attribute probabilities for generating attributes. Any miscalculation or manipulation of these probabilities may impact the fairness and randomness of attribute generation.

Technical Risks:

Gas Limit : The contract might face gas limit issues, especially during attribute generation, as it involves loops and calculations. Considerations should be made to optimize gas usage.

External Contract Dependency: The contract relies on external contracts, such as FighterOps.sol, for data structures and functions. Ensure that these external dependencies are secure and well-audited.

Integration Risks:

External Dependencies: The contract interacts with external contracts and relies on their correct behavior. Ensure that these external dependencies are trustworthy and well-tested to prevent unexpected issues.

Attribute Generation: Integration with other systems or contracts relying on the generated attributes may face risks if the attribute generation logic is not well-documented or understood.

Non-Standard Token Risks:

No Token Interaction: The provided code does not involve the creation or management of standard ERC-20 or ERC-721 tokens. However, if these contracts are part of a larger ecosystem, ensure that the interaction with tokens in other contracts is secure.

Additional Notes:

The contract includes functions for adding and deleting attribute probabilities, allowing flexibility in adjusting attributes for different generations.

The createPhysicalAttributes function is view-only and provides information about the physical attributes based on the provided DNA. Carefully validate the correctness of the generated attributes.

Recommendations:

Implement access control mechanisms carefully, considering potential risks associated with ownership. Ensure that external dependencies are well-vetted and secure. Thoroughly test the contract's attribute generation logic to ensure fairness and randomness. Consider adding events for critical state changes to facilitate monitoring and auditing.

[File-2] FighterFarm

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The _ownerAddress is a variable that designates the owner of the contract. This owner has significant privileges, such as transferring ownership, incrementing fighter generations, adding stakers, and instantiating other contracts. If this owner account gets compromised or if there are insufficient checks, it may lead to admin abuse risks.

Systemic Risks:

The contract has a maximum limit for the number of fighters allowed per address (MAX_FIGHTERS_ALLOWED), which is 10. This limit helps prevent abuse and ensures fair usage of the contract.

Technical Risks:

The contract relies on external contracts, such as FighterOps, Verification, AAMintPass, AiArenaHelper, and Neuron. The proper functioning and security of these contracts are crucial for the correct operation of FighterFarm. Any vulnerabilities in these dependencies may pose technical risks.

Integration Risks:

The contract interacts with external contracts, and the correctness of these interactions depends on the proper implementation of the external contracts. Integration risks may arise if there are changes or issues with the external contracts.

Non-Standard Token Risks:

The contract is an ERC-721 compliant token (FighterFarm), and it inherits from ERC721 and ERC721Enumerable. Compliance with standard interfaces generally reduces risks associated with non-standard tokens. However, careful consideration should be given to potential vulnerabilities in the specific implementation.

[File-3]

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The fighterCreatedEmitter function allows emitting a FighterCreated event. Emitting events itself doesn't pose admin abuse risks, but the event might be used for monitoring or triggering certain actions. Depending on the context in which this library is used, event emission may need to be carefully controlled.

Systemic Risks:

This library is focused on managing Fighter NFTs and emitting events. It seems to be part of a larger system, and the systemic risks would depend on the implementation and integration with other components.

Technical Risks:

The Fighter struct contains various attributes, and there are functions like getFighterAttributes and viewFighterInfo for accessing and viewing these attributes. The correctness of these functions and the underlying data structures is crucial for the proper functioning of any contract or system using this library.

Integration Risks:

Integration risks depend on how this library is integrated into other contracts or systems. It is crucial to ensure that the functions are used appropriately and that the events are handled correctly in the broader context of the application.

Non-Standard Token Risks:

This library focuses on managing Fighter NFTs, but it doesn't implement a full ERC-721 token. The actual token-related logic, such as minting, transferring, and ownership tracking, would be implemented in another contract using this library. Non-standard token risks would depend on the implementation of the full ERC-721 contract and its interaction with this library.

[File-4] GameItems

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The _ownerAddress variable designates the owner of the contract, who has significant privileges, such as transferring ownership, adjusting admin access, adjusting transferability, and creating game items. If this owner account gets compromised, there is a risk of admin abuse.

Systemic Risks:

The contract has a system for game items that includes attributes such as name, finite supply, transferability, remaining items, item price, and daily allowance. The systemic risks are related to the proper functioning of these attributes and their impact on the overall game item system.

Technical Risks:

The contract relies on external contracts, such as Neuron and the OpenZeppelin ERC1155 implementation. The proper functioning and security of these contracts are crucial for the correct operation of the GameItems contract. Any vulnerabilities in these dependencies may pose technical risks.

Integration Risks:

The contract interacts with external contracts, and the correctness of these interactions depends on the proper implementation of the external contracts, especially the Neuron contract. Integration risks may arise if there are changes or issues with the external contracts.

Non-Standard Token Risks:

The contract implements the ERC1155 standard for multi-token contracts. While this is a recognized standard, careful consideration should be given to potential vulnerabilities in the specific implementation, especially when handling the minting, burning, and transfer of game items.

Additional Notes:

The contract has events (BoughtItem, Locked, Unlocked) that provide transparency for crucial actions within the contract.

The GameItemAttributes struct encapsulates essential attributes for each game item, allowing for customizable and extensible game items.

The use of the OpenZeppelin ERC1155 contract provides a standardized and gas-efficient implementation for multi-token contracts.

Recommendations:

Conduct thorough testing, including unit tests and integration tests, to ensure the correctness of the contract logic.

Consider a comprehensive security audit to identify and address potential vulnerabilities.

Ensure that the external contracts (Neuron and ERC1155) are secure and meet the requirements of the GameItems contract.

Implement access controls carefully and review the admin-related functions to prevent unauthorized access.

Continuously monitor and follow best practices for smart contract development and security.

[File-5] MergingPool

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The _ownerAddress variable designates the owner of the contract, who has significant privileges, such as transferring ownership, adjusting admin access, and updating the number of winners per period. If this owner account gets compromised, there is a risk of admin abuse.

Systemic Risks:

The contract manages a merging pool where users can earn new fighter NFTs based on points earned in a ranked battle contract. Systemic risks are related to the proper functioning of the merging pool, including adding points, picking winners, and claiming rewards.

Technical Risks:

The contract relies on external contracts, such as FighterFarm, and interactions with the ranked battle contract. The proper functioning and security of these contracts are crucial for the correct operation of the MergingPool contract. Any vulnerabilities in these dependencies may pose technical risks.

Integration Risks:

The contract interacts with the FighterFarm contract, and the correctness of these interactions depends on the proper implementation of the FighterFarm contract. Integration risks may arise if there are changes or issues with the external contracts.

Non-Standard Token Risks:

The contract interacts with fighter NFTs, and the specific implementation details related to the minting and handling of these NFTs should be carefully reviewed. Ensure that the FighterFarm contract adheres to standards and best practices.

Additional Notes:

The contract uses events (PointsAdded, Claimed) to provide transparency for crucial actions within the contract.

Admins can update the number of winners per period, which can influence the distribution of rewards.

The contract tracks the number of rounds claimed by each user, preventing multiple claims for the same round.

Recommendations:

Conduct thorough testing, including unit tests and integration tests, to ensure the correctness of the contract logic.

Consider a comprehensive security audit to identify and address potential vulnerabilities.

Ensure that the external contracts (FighterFarm and the ranked battle contract) are secure and meet the requirements of the MergingPool contract.

Implement access controls carefully and review the admin-related functions to prevent unauthorized access.

Continuously monitor and follow best practices for smart contract development and security.

[File-6] Neuron

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The contract uses the OpenZeppelin AccessControl library to manage roles, and it defines roles such as MINTER_ROLE, SPENDER_ROLE, and STAKER_ROLE. Admin abuse risks arise if the owner or other entities with admin roles misuse their privileges to mint tokens, adjust admin access, or interfere with the contract's intended operation.

Systemic Risks:

The contract implements various roles (MINTER_ROLE, SPENDER_ROLE, STAKER_ROLE) to control access to critical functions. Risks may arise if these roles are misconfigured or if there are vulnerabilities in the implementation of these role-based access controls.

Technical Risks:

The contract relies on external dependencies such as the OpenZeppelin ERC20 and AccessControl libraries. Any vulnerabilities or issues in these dependencies may pose technical risks to the contract. It's crucial to ensure that these dependencies are secure and up-to-date.

Integration Risks:

The contract interacts with other addresses, including treasury, contributor, and recipients. Integration risks may arise if these external interactions are not handled correctly, leading to unintended behavior or vulnerabilities.

Non-Standard Token Risks:

The contract is an ERC-20 token, and the implementation appears standard. However, it is essential to review the specific requirements and features relevant to the use case to ensure that the token meets the project's needs.

Additional Notes:

The contract initializes with an initial supply of tokens minted to the treasury and contributors.

Roles such as minter, spender, and staker are defined, and certain functions can only be called by entities with the corresponding roles.

There are events (TokensClaimed and TokensMinted) for transparency on token claims and minting activities.

The contract provides functions for minting, burning, and approving allowances for different roles.

Recommendations:

Conduct thorough testing, including unit tests and integration tests, to ensure the correctness and security of the contract.

Consider a comprehensive security audit to identify and address potential vulnerabilities, especially in the role-based access control.

Keep external dependencies, such as the OpenZeppelin libraries, up-to-date to benefit from any security patches or improvements.

Clearly document the roles and responsibilities of each role, especially in the context of admin and sensitive functions.

Ensure that external interactions, such as with treasury and contributors, are well-handled and secure.

[File-7] RankedBattl

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The isAdmin mapping is used to designate certain addresses as administrators.

Admin privileges include adjusting access, setting the game server address, and updating the round.

The owner address has elevated privileges and can transfer ownership.

Admin access control is not modular, as it's handled directly in the contract code.

Systemic Risks:

The contract relies on external contracts such as FighterFarm, VoltageManager, MergingPool, Neuron, and StakeAtRisk.

Dependency on external contracts introduces potential risks if these contracts are compromised or behave unexpectedly.

The contract uses the Neuron contract for token-related functionalities, and the StakeAtRisk contract for managing stakes.

Technical Risks:

The use of external libraries (FixedPointMathLib) introduces potential vulnerabilities if the library itself or its implementation is flawed.

The contract uses complex calculations, such as ELO factor, staking factors, and point distributions, which might introduce computational costs and potential risks if not implemented correctly.

The contract updates battle records, accumulates points, and distributes rewards, which are complex processes and require careful auditing.

Integration Risks:

The contract interacts with multiple external contracts. Any changes to these contracts may impact the functionality of the RankedBattle contract.

The integration of various external contracts makes the system interdependent, and upgrades or changes in one contract may require adjustments in others.

Non-Standard Token Risks:

The contract uses the ERC-20 token standard (Neuron), which is a widely accepted standard.

However, the contract introduces additional functionalities, such as staking, claiming, and battle records, which are specific to this use case.

Overall:

The smart contract seems to be well-structured and modular, but thorough testing and auditing are crucial due to the complexity of the interactions and dependencies on external contracts. The use of admin roles should be carefully managed to mitigate potential abuse risks. Additionally, external libraries and mathematical calculations should be reviewed for security and correctness.

[File-8] StakeAtRisk

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

There are no explicit admin roles or privileged functions in the StakeAtRisk contract.

The contract relies on the RankedBattle contract to trigger certain actions, such as setting a new round and reclaiming NRN tokens, which helps minimize the potential for admin abuse.

Systemic Risks:

The contract depends on external contracts, specifically the Neuron and RankedBattle contracts.

Any changes to these external contracts may impact the functionality of the StakeAtRisk contract.

The StakeAtRisk contract is part of a larger system involving ranked battles and stake management.

Technical Risks:

The contract involves token transfers using the Neuron contract, which should be carefully audited for potential vulnerabilities.

The use of transfer functions could lead to reentrancy vulnerabilities if not handled properly.

The contract relies on the correct execution of functions in the RankedBattle contract, and any issues there might affect the operation of this contract.

Integration Risks:

The contract relies on the RankedBattle contract for critical operations, and any issues in the integration between these contracts could impact their functionality.

The contract has an event (ReclaimedStake) that is intended to be emitted by the RankedBattle contract.

Non-Standard Token Risks:

The contract interacts with the Neuron contract, which is assumed to follow the ERC-20 token standard.

It doesn't introduce any new functionalities related to token standards; rather, it focuses on managing NRNs (Neuron tokens) at risk during rounds of competition.

Overall:

The StakeAtRisk contract seems well-designed with a clear purpose of managing NRNs at risk during ranked battles. However, thorough testing and auditing are essential, especially considering the interaction with external contracts. Additionally, it's important to ensure that the integration between the StakeAtRisk and RankedBattle contracts is robust and secure.

</details>

[File-9] VoltageManage

The bellow issues related to the File ( Link )

GitHub File Link

Admin Abuse Risks:

The contract has an ownership mechanism that allows the current owner to transfer ownership to a new address.

The owner can adjust admin access for other addresses and also modify the list of allowed voltage spenders.

Admin-related functions (transferOwnership, adjustAdminAccess, adjustAllowedVoltageSpenders) are protected by the requirement that the caller must be the current owner or an admin.

There is a risk of admin abuse if the owner or admins misuse their privileges, potentially impacting the functionality of the contract.

Systemic Risks:

The contract relies on an external contract (GameItems) for certain operations, such as checking the balance of a user and burning items.

Any changes to the external contract might affect the functionality of the VoltageManager contract.

The ownership and admin mechanisms are critical to the contract's functioning, and any issues with these mechanisms could lead to systemic risks.

Technical Risks:

The contract uses a timestamp-based mechanism for voltage replenishment (ownerVoltageReplenishTime). While this is a common approach, it's important to consider potential issues related to timestamp manipulation or time-dependent vulnerabilities.

The contract depends on the correct execution of functions in the external GameItems contract, and any vulnerabilities in that contract might impact this contract.

Integration Risks:

The contract interacts with the GameItems contract, and any issues in the integration between these contracts could affect their functionality.

There's a dependency on external contracts for specific functionality, and any changes to these contracts may require updates and careful testing of the VoltageManager contract.

Non-Standard Token Risks:

The contract interacts with an external contract (GameItems) which is assumed to handle non-standard tokens. The contract uses the burn function to decrease the balance of a user for a specific item.

Overall:

The VoltageManager contract appears to be well-structured and focuses on managing voltage for users in an AI Arena. However, thorough testing and auditing, especially concerning the interactions with external contracts and the timestamp-based replenishment mechanism, are crucial to ensure the security and reliability of the system.

Time spent:

9 hours