AI Arena - rekxor's results

QA Report : AI Arena Contest | 9th Feb 2024 - 21st Feb 2024

Executive summary

AI Arena is a PvP platform fighting game where the fighters are AIs that were trained by humans.

Overview

---

Findings Description

---

---

[L-01] Unspecified Compiler version pragma

Description:

Avoid floating pragmas for non-library contracts.

While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.

A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.

It is recommended to pin to a concrete compiler version.

Code Snippets:

Instance-1, Instance-2, Instance-3, Instance-4, Instance-5, Instance-6, Instance-7, Instance-8

Recommendation:

Use a fixed pragma solidity version across all contracts for compatibility, and since v0.8.20 onwards Solidity introduced PUSH0 opcode, which is still not supported by other chains. Thus, v0.8.19 has been recommended here. If we are not planning for multiple chains for deployment, we can go ahead with v0.8.20

- pragma solidity >=0.8.0 <0.9.0;
+ pragma solidity 0.8.19;  

[L-02] Redundant operation of initializing attributeProbabilities mapping in constructor

Description:

In the constructor while initializing state variables, we make addAttributeProbabilities(0, probabilities) function call which has the following function body:

Code: AiArenaHelper.sol::addAttributeProbabilities()

function addAttributeProbabilities(uint256 generation, uint8[][] memory probabilities) public {
    require(msg.sender == _ownerAddress);
    require(probabilities.length == 6, "Invalid number of attribute arrays");
// here we have generation as 0
    uint256 attributesLength = attributes.length;
    for (uint8 i = 0; i < attributesLength; i++) {
        attributeProbabilities[generation][attributes[i]] = probabilities[i];
    }
}

But our constructor does this same operation after calling the above function, following is the code from the constructor: AiArenaHelper.sol::constructor

uint256 attributesLength = attributes.length;
for (uint8 i = 0; i < attributesLength; i++) {
    attributeProbabilities[0][attributes[i]] = probabilities[i];
    attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
}

Recommendation:

Based on the implementation of the addAttributeProbabilities() it appears that, it just initializes the attributeProbabilities mapping, but in the constructor for loop we also initialize the attributeToDnaDivisor mapping. So addAttributeProbabilities() and addAttributeDivisor() should be called in constructor or either keep the for loop in the constructor which does the initialization for both mappings.

[L-03] Use OpenZeppelin's Ownable contract for ownership related actions

Description:

There are missing checks for owner address, though owner is trusted entity, but following best practices is always appreciated.

Code Snippets:

Neuron.sol::transferOwnership, AiArenaHelper.sol::transferOwnership , GameItems.sol::transferOwnership, MergingPool.sol::transferOwnership,
RankedBattle.sol::transferOwnership , FighterFarm.sol::transferOwnership, Neuron.sol::transferOwnership

Code:

 function transferOwnership(address newOwnerAddress) external {
     require(msg.sender == _ownerAddress);
     _ownerAddress = newOwnerAddress;
}

Recommendation:

Inherit OZ's Ownable contract and override the transferOwnership() function.

[L-04] setupAirdrop() function arg: recipients[] can have duplicates

Description:

If a recipient address appears more than once in the recipients array, the allowances for that recipient will be overwritten in each iteration of the loop. This means that the final allowance for a duplicate recipient will be set to the value specified in the last iteration of the loop.

Neuron.sol::setupAirdrop() :

Code:

function setupAirdrop(address[] calldata recipients, uint256[] calldata amounts) external {
        require(isAdmin[msg.sender]);
        require(recipients.length == amounts.length);
        uint256 recipientsLength = recipients.length;
        for (uint32 i = 0; i < recipientsLength; i++) {
            _approve(treasuryAddress, recipients[i], amounts[i]);
        }
}

Recommendation:

You might consider updating the logic to handle duplicate recipients more appropriately. One approach could be to check whether an allowance already exists for a recipient before setting a new one, and perhaps summing up the amounts for duplicate recipients instead of overwriting the allowance.

[L-05] Unsafe ERC20 Operation(s), use SafeERC20 library

Description:

ERC20 operations can be unsafe due to different implementations and vulnerabilities in the standard.

It is therefore recommended to always either use OpenZeppelin's SafeERC20 library or at least to wrap each operation in a require statement.

Neuron.sol#L171-L177 Neuron.sol#L184-L190

Code:

function approveSpender(address account, uint256 amount) public {
        require(
            hasRole(SPENDER_ROLE, msg.sender), 
            "ERC20: must have spender role to approve spending"
        );
// To circumvent ERC20's approve functions front running
// vulnerability use OpenZeppelin's SafeERC20
        _approve(account, msg.sender, amount);
    }

Recommendation:

Import SafeERC20 to use it's call on IERC20 interface: whose functions are implemented in Neuron.sol (ERC20 Token) contract. SafeERC20 library's safe{Increase|Decrease}Allowance functions can be used .

+ import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";
+ using SafeERC20 for IERC20;

// ...

+ IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

[L-06] transferFrom() returns a bool value in claim(), which is not checked

Description:

The implementation of tranferFrom() in ERC20 contract has a bool value to be returned, which is not checked while using it in our claim(). Neuron.sol::claim()

    function claim(uint256 amount) external {
        require(
            allowance(treasuryAddress, msg.sender) >= amount, 
            "ERC20: claim amount exceeds allowance"
        );
        transferFrom(treasuryAddress, msg.sender, amount);
        emit TokensClaimed(msg.sender, amount);
    }

Recommendation:

Introduce a bool variable to check whether if transferFrom() returns true. Add require() to check and revert in case of failure.

- transferFrom(treasuryAddress, msg.sender, amount);
+ bool success = transferFrom(treasuryAddress, msg.sender, amount); 
+ require(success, "ERC20 transfer failed");

Alternatively, we can mitigate this issue by using SafeERC20 library of OpenZeppelin.

import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";

// ...

IERC20(token).safeTransferFrom(msg.sender, address(this), amount);

[L-07] Consideration for Timestamp overflow in _replenishVoltage()

Description:

In the provided function _replenishVoltage(), the ownerVoltageReplenishTime is set using a uint32 data type for storing which can have maximum timestamp till August 2106. Using uint32 is no better than uint256, on the contrary, the later is more gas efficient.

<u>Timestamp Overflow:</u> The Unix timestamp (block.timestamp) represents the number of seconds that have passed since January 1, 1970 (UTC). If your application is designed to last for a long time, a uint32 may overflow sooner than a uint64. A uint32 can represent timestamps until around the year 2106, while a uint64 can cover an astronomically longer time.

VoltageManager.sol: Code:

function _replenishVoltage(address owner) private {
    ownerVoltage[owner] = 100;
//q for uint32 max can be aug -12 2106, better for uint64?
    ownerVoltageReplenishTime[owner] = uint32(block.timestamp + 1 days);
}  

Recommendation:

Update mapping(address => uint32) public ownerVoltageReplenishTime; to have value of type uint64.

- mapping(address => uint32) public ownerVoltageReplenishTime;
+ mapping(address => uint64) public ownerVoltageReplenishTime;

---

---

[NC-01] Contracts doesn't follow the layout ordering

Description :

The order in which you layout contract elements in Solidity can contribute to code readability, maintainability, and overall organization. Helps the fellow engineers/ auditors understand the code with less pain.

Recommendation:

Inside each contract, library or interface, use the following order :

1. Type declarations
2. State variables
3. Events
4. Errors
5. Modifiers
6. Functions

[NC-02] Functions mutating storage, can better emit events for off-chain tracking

Description:

Transfers ownership to new EOA. This update of storage can better be acknowledged by the protocol team, to reduce unaware centralization risk. Similarly, there are many other instances present in the contracts, linked here:

AiArenaHelper.sol::transferOwnership , GameItems.sol::transferOwnership, MergingPool.sol::transferOwnership,
RankedBattle.sol::transferOwnership , FighterFarm.sol::transferOwnership, Neuron.sol::transferOwnership , StakeAtRisk.sol::setNewRound

Code:

 function transferOwnership(address newOwnerAddress) external {
     require(msg.sender == _ownerAddress);
     _ownerAddress = newOwnerAddress;
}

Recommendation:

Declare an event in the contract and emit it in desired functions.

Another Instance:

AiArenaHelper.sol::deleteAttributeProbabilities

function deleteAttributeProbabilities(uint8 generation) public {
        require(msg.sender == _ownerAddress);

        uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
            attributeProbabilities[generation][attributes[i]] = new uint8[](0);
        }
    }

[NC-03] Better to emit events before making an external call

Description:

It is considered best practice to follow pattern of event emission before making any external call. In the below code snippet: Neuron.sol::claim()

    function claim(uint256 amount) external {
        require(
            allowance(treasuryAddress, msg.sender) >= amount, 
            "ERC20: claim amount exceeds allowance"
        );
        transferFrom(treasuryAddress, msg.sender, amount);
        emit TokensClaimed(msg.sender, amount);
    }

Recommendation:

Emit the event before making transferFrom() external call given in the code snippet below.

    function claim(uint256 amount) external { 
        require(
            allowance(treasuryAddress, msg.sender) >= amount, 
            "ERC20: claim amount exceeds allowance"
        ); 
        emit TokensClaimed(msg.sender, amount); 
        transferFrom(treasuryAddress, msg.sender, amount);
    }

---

---

Gas Report : AI Arena Contest | 9th Feb 2024 - 21st Feb 2024

Executive summary

AI Arena is a PvP platform fighting game where the fighters are AIs that were trained by humans. In AI Arena you train an AI character to battle in a platform fighting game. Imagine a cross between Pokémon and Super Smash Bros, but the characters are AIs, and you can train them to learn almost any skill in preparation for battle.

Overview

---

List of Findings

---

---

[G-01] Check for amount = 0 before making _burn() call

Description:

It is unnecessary to call _burn() on amount = 0, it leads to wastage of gas as _burn() itself doesn't have such check before writing to state variables of the contract, thereby calling SSTORE EVM opcode, which costs significant amount of gas.

"SSTORE, the opcode which saves data in storage, costs 20,000 gas when a storage value is set to a non-zero value from zero and costs 5000 gas when a storage variable’s value is set to zero or remains unchanged from zero."

Neuron.sol::burn()

Code:

function  burn(uint256 amount) public virtual {
	//gas- don't call for amt = 0, wastage of gas on add/sub 0 
	//to/from state variable.
	_burn(msg.sender, amount);
}

Recommendation:

Updating the above code snippet to the below given can avoid unnecessary usage of excessive gas caused by absurd operations.

function  burn(uint256 amount) public virtual {
	 require(amount!=0,"Zero Amount not allowed")
	_burn(msg.sender, amount);
}

[G-02] Cache attributes.length only once to avoid extra gas usage

Description:

The attributesLength caches to avoid computation of array length of state variable (costly) with each iteration is appreciated.

But, if we declare it before doing require(attributeDivisors.length == attributes.length); it can save some gas, though we might introduce mstore opcode use, but since the function is only callable by owner, it is less likely that above require check to fail.

AiArenaHelper::addAttributeDivisor

Code:

function addAttributeDivisor(uint8[] memory attributeDivisors) external {
       require(msg.sender == _ownerAddress);
       require(attributeDivisors.length == attributes.length);
   
       uint256 attributesLength = attributes.length;
       for (uint8 i = 0; i < attributesLength; i++) {
           attributeToDnaDivisor[attributes[i]] = attributeDivisors[i];
       }
   }    

Recommendation:

function addAttributeDivisor(uint8[] memory attributeDivisors) external {
+ 	uint256 attributesLength = attributes.length;
        require(msg.sender == _ownerAddress);
        require(attributeDivisors.length == attributesLength);

- 	uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
            attributeToDnaDivisor[attributes[i]] = attributeDivisors[i];
        }
}    

Another Instance:

AiArenaHelper.sol::createPhysicalAttributes

Code:

uint256[] memory finalAttributeProbabilityIndexes =  new  uint[](attributes.length);

uint256 attributesLength = attributes.length;

Recommendation:

+ uint256 attributesLength = attributes.length;
  uint256[] memory finalAttributeProbabilityIndexes =  new  uint[](attributes.length);
- uint256 attributesLength = attributes.length;

[G-03] Redundant operation of initializing attributeProbabilities mapping in constructor leads to unnecessary gas consumption

Description:

In the constructor while initializing state variables, we make addAttributeProbabilities(0, probabilities) function call which has the following function body:

Code: AiArenaHelper.sol::addAttributeProbabilities()

function addAttributeProbabilities(uint256 generation, uint8[][] memory probabilities) public {
    require(msg.sender == _ownerAddress);
    require(probabilities.length == 6, "Invalid number of attribute arrays");
// here we have generation as 0
    uint256 attributesLength = attributes.length;
    for (uint8 i = 0; i < attributesLength; i++) {
        attributeProbabilities[generation][attributes[i]] = probabilities[i];
    }
}

But our constructor does this same operation after calling the above function, following is the code from the constructor: AiArenaHelper.sol::constructor

uint256 attributesLength = attributes.length;
for (uint8 i = 0; i < attributesLength; i++) {
    attributeProbabilities[0][attributes[i]] = probabilities[i];
    attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
}

This causes additional gas usage for doing an operation that has been done again, the modification of the state variables by extra call will not be done. Just the contract deployer will be charged additional gas for doing redundant initialization of the mapping.

Recommendation:

Based on the implementation of the addAttributeProbabilities() it appears that, it just initializes the attributeProbabilities mapping, but in the constructor for loop we also initialize the attributeToDnaDivisor mapping. So addAttributeProbabilities() and addAttributeDivisor() should be called in constructor or either keep the for loop in the constructor which does the initialization for both mappings. Thus, include initialization only once, either via function call, or initializing in the constructor itself to avoid unnecessary usage of gas.

---

---