AI Arena - lsaudit's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L390

Vulnerability details

Impact

Function reRoll() is used to rolls a new fighter with random traits. However, the dna, which is used to calculate traits is depended on msg.sender address, thus it's deterministic. Addresses in blockchain are deterministic. Before starting the interaction with AI Arena, user may pre-determine multiple of different smart contract addresses before deployment or generate multiple of EOAs - and them use those addresses to pre-calculate reRoll() outcome.

This will allow the attacker to choose the best address (msg.sender) which he should use to interact with AI Arena. After calling reRoll() by using that address - he will receive the desired new fighter (with previously pre-calculated rarity and attributeIndex).

This implies, that re-rolling a new fighter is not random at all and can be pre-calculated.

Proof of Concept

File: FighterFarm.sol

function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
[...]
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
[...]
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }    

Whenever we call reRoll() - the dna is calculated as: uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));. As we can see, every parameter of keccak256 is known. We know tokenId, we know numRerolls[tokenId] and we know msg.sender.

Then on that dna, we call _aiArenaHelperInstance.createPhysicalAttributes. Let's check how it's implemented

File: AiArenaHelper.sol

function createPhysicalAttributes(
       [...]
                } else {
                    uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;
                    uint256 attributeIndex = dnaToIndex(generation, rarityRank, attributes[i]);
[...]
            );
        }
    }

The rarity is calculated as (dna / attributeToDnaDivisor[attributes[i]]) % 100. The attributeIndex is calculated as dnaToIndex(generation, rarityRank, attributes[i]). As we can see, when we can control the value of dna - we can manipulate the result of rarity and attributeIndex.

There are two possible attack scenarios, which we will describe in the below section.

• Using EOAs

1. Attacker generates multiple of EOAs.
2. Attacker uses each of EOA to calculate the outcome of reRoll(). Since every parameter of dna calculation is known, he is able to pre-calculate which EOA will give him desired rarity and attributeIndex after calling reRoll().
3. When attacker decides which EOA is the best for him - he finally uses that EOA to interact with AI Arena.
4. Attacker calls reRoll() - he received the fighter with desired (previously calculated) rarity and attributeIndex

• Using smart contracts

1. Attacker creates a smart contract which will interact with AI Arena.
2. CREATE2 allows to pre-determine contract addresses before deployment.
3. Attacker slightly modifies the contract's code, to pre-determine multiple of different addresses before deployment.
4. He uses those addresses to calculate dna.
5. He decides which dna gives him the best fighter (with the best rarity and attributeIndex).
6. He chooses that smart contract, deploys it, and starts interact with AI Arena.
7. Deployed smart contract calls reRoll() - and receives the fighter with desired (previously calculated) rarity and attributeIndex.

To sum up these attack scenarios - attacker can pre-calculate multiple of addresses and simulate the result of reRoll() for each of these addresses. Then he chooses the address which gave him the best fighter and uses that address to interact with Arena AI. He then calls reRoll() and receives the desired fighter.

Tools Used

Manual code review

Recommended Mitigation Steps

Re-rolls should guarantee full randomness. It should not be possible to pre-calculate fighter rarity and attributeIndex based on the caller's address. It's recommended to add some truly random parameter which will take part in dna calculation.

Assessed type

Other

[1] getFighterPoints() may lead to DoS

File: MergingPool.sol

File: src/MergingPool.sol

205:     function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) {
206:         uint256[] memory points = new uint256[](1);
207:         for (uint256 i = 0; i < maxId; i++) {
208:             points[i] = fighterPoints[i];
209:         }
210:         return points;
211:     }

Function getFighterPoints() iterates from 0 to maxId while maxId is provided by user. Whenver maxId is too large, the loop would revert with out of gas error - thus it won't be possible to retrieve points for multiple of fighters. Much better idea would be to re-implement this function and allow to provide both minId and maxId. Instead of iterating from 0 to maxId, it would be more effective to loop from minId to maxId. That way, when we want to retrieve the points of fighters whose ids are very big, we can specify the starting range (minId) which won't revert with out of gas error. E.g., let's say we want to iterate to maxId = 100 000. This will be 100 000 iterations which will cost a lot of gas. Instead of iterating from 0, minId would allow us to iterate from, e.g., 90 000 to 100 000 - that would be only 10 000 iterations, instead of 100 000.

[2] Randoms bytes in _mint() function are not random at all

File: GameItems.sol

File: src/GameItems.sol

173:             _mint(msg.sender, tokenId, quantity, bytes("random"));
174:             emit BoughtItem(msg.sender, tokenId, quantity);

While bytes("random") suggests that parameter should be random - minting new game items will always pass the same bytes: random as the data parameter. Having this might be very misleading. Even though this data does not need to be random at all - it'd be good practice to have this data different for every new token. It would be better to derive data parameter from tokenId, instead of having hard-coded string random.

[3] mint() in GameItems.sol might revert

File: GameItems.sol

File: src/GameItems.sol

158:         require(
159:             dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp || 
160:             quantity <= allowanceRemaining[msg.sender][tokenId] 
161:         );
162: 
163:         _neuronInstance.approveSpender(msg.sender, price);
164:         bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, price);
165:         if (success) {
166:             if (dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp) {  
167:                 _replenishDailyAllowance(tokenId);
168:             }
169:             allowanceRemaining[msg.sender][tokenId] -= quantity;

When dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp, then even when quantity > allowanceRemaining[msg.sender][tokenId] - require statement at line 158 won't revert (because of the OR operation, only one condition needs to be fulfilled). This implies, that scenario when quantity > allowanceRemaining[msg.sender][tokenId] is possible. Since dailyAllowanceReplenishTime[msg.sender][tokenId] <= block.timestamp, then, at line 167, we will call function _replenishDailyAllowance(). This function sets allowanceRemaining[msg.sender][tokenId] = allGameItemAttributes[tokenId].dailyAllowance;. However, nothing guarantees, that allGameItemAttributes[tokenId].dailyAllowance needs to be greater than passed variable quantity. This - again - proves, that scenario when quantity > allowanceRemaining[msg.sender][tokenId] is possible. Since this scenario is possible - at line 169 - function can revert because of the underflow (we are subtracting bigger value from a smaller one).

Reverting without any reason (e.g., without clear information provided in the require) may be very misleading to the end user. Our recommendation is to add additional require, which will make sure that user will know why function might have reverted.

require(allowanceRemaining[msg.sender][tokenId] >= quantity, "Trying to mint more than allowed!");
allowanceRemaining[msg.sender][tokenId] -= quantity;

Even though this does not constitute any security risk (the revert will occur after all) - it's a good practice to inform a user why the revert occurred.

[4] Redudant assigment in getAllowanceRemaining()

File: GameItems.sol

File: src/GameItems.sol

268:     function getAllowanceRemaining(address owner, uint256 tokenId) public view returns (uint256) {
269:         uint256 remaining = allowanceRemaining[owner][tokenId];
270:         if (dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp) {  
271:             remaining = allGameItemAttributes[tokenId].dailyAllowance;
272:         }
273:         return remaining;
274:     }

When dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp, we should immediately return allGameItemAttributes[tokenId].dailyAllowance, instead of assigning it to remaining. This will shorten the code, remove redundant operations, thus increase the code readability:

    function getAllowanceRemaining(address owner, uint256 tokenId) public view returns (uint256) {
        uint256 remaining = allowanceRemaining[owner][tokenId];
        if (dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp) {  
            return allGameItemAttributes[tokenId].dailyAllowance;
        }
        return remaining;
    }

[5] Using _approve() may lead to race condition scenarios

File: Neuron.sol

According to Code4rena Severity Categorization, this issue should be classified as NC.

Will the ongoing discussing continues - if this issue is really a security threat - we've decide to report this potential scenario in the QA report - so that the developer will be able to assess the risk on their own. Nonetheless, we do believe that this issue does not constitute a huge threat - thus should be treated as informational only.

Let's consider a scenario when Alice wants to approve Bob to spend her tokens. She calls _approve(0xB0b, 1000) and then she notices, that she mistakenly approved too much. She tries to fix her mistake, by calling _approve(0xB0b, 100) again (this time with the proper amount). However, Bob notices that and front-runs the 2nd _approve() by transferring from Alice 1000 tokens - and - after her second _approve() - he transfers another 100 tokens. At the end, he managed to get 1100 from Alice (1000 + 100).

To avoid this issue, our recommendation is to use increaseAllowance, decreaseAllowance functions, which protects from above scenario.

File: src/Neuron.sol

184:     function approveStaker(address owner, address spender, uint256 amount) public {
185:         require(
186:             hasRole(STAKER_ROLE, msg.sender), 
187:             "ERC20: must have staker role to approve staking"
188:         );
189:         _approve(owner, spender, amount);
190:     }

In the above example, user with STAKER_ROLE, whenever approving incorrect amount for the first time - may be front-run by the user.

File: src/Neuron.sol

171:     function approveSpender(address account, uint256 amount) public {
172:         require(
173:             hasRole(SPENDER_ROLE, msg.sender), 
174:             "ERC20: must have spender role to approve spending"
175:         );
176:         _approve(account, msg.sender, amount);
177:     }

In the above example, user with SPENDER_ROLE, whenever approving incorrect amount for the first time - may be front-run by the user.

File: src/Neuron.sol

127:     function setupAirdrop(address[] calldata recipients, uint256[] calldata amounts) external { 
128:         require(isAdmin[msg.sender]);
129:         require(recipients.length == amounts.length);
130:         uint256 recipientsLength = recipients.length;
131:         for (uint32 i = 0; i < recipientsLength; i++) {
132:             _approve(treasuryAddress, recipients[i], amounts[i]);
133:         }
134:     }

In the above example, admin, whenever approving incorrect amount for the first time - may be front-run by multiple of users.

[6] spendVoltage() in VoltageManager.sol may revert

File: VoltageManager.sol

File: src/VoltageManager.sol

105:     function spendVoltage(address spender, uint8 voltageSpent) public {
106:         require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
107:         if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
108:             _replenishVoltage(spender);
109:         }
110:         ownerVoltage[spender] -= voltageSpent; 

In function spendVoltage(), there's no additional check which guarantees that ownerVoltage[spender] >= voltageSpent, this implies, that when voltageSpent passed by the user is too big - function will revert because of the underflow. While this does not lead to any security risk (function will revert after all) - it's a good practice to inform the end user about the reason of revert.

Reverting without any reason (e.g., without clear information provided in the require) may be very misleading to the end user. Our recommendation is to add additional require, which will make sure that user will know why function might have reverted:

require( ownerVoltage[spender] >= voltageSpent, "Trying to spend too much voltage");
ownerVoltage[spender] -= voltageSpent; 

[7] Incorrect comment in getUnclaimedRewards() in MerginPool.sol

File: MergingPool.sol

File: src/MergingPool.sol

169:     /// @notice Gets the unclaimed rewards for a specific address.
170:     /// @param claimer The address of the claimer.
171:     /// @return numRewards The amount of unclaimed fighters.
172:     function getUnclaimedRewards(address claimer) external view returns(uint256) {

/// @notice Gets the unclaimed rewards for a specific address. should be changed to /// @notice Gets the amount of unclaimed rewards for a specific address., since this function returns only how many unclaimed rewards the address passed as claimer parameter has.

[8] Use descriptive constants instead of hard-coding numbers

File: RankedBattle.sol

File: src/RankedBattle.sol

439:         curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

Above code requires explanation why it divides by 10**4. Our recommendation is to use constant with descriptive names, instead of hard-coding numbers into the source code.

[9] Incorrect function name in MerginPool.sol

File: MergingPool.sol

File: src/MergingPool.sol

111:     /// @notice Allows the admin to pick the winners for the current round.
[...]
118:     function pickWinner(uint256[] calldata winners) external { 

Function pickWinner() picks multiple of winners for the current round. This implies, that the function name should be changed to pickWinners(). The current function name suggests that only one winner can be chosen.

[10] Incorrect grammar

File: FighterFarm.sol

File: src/FighterFarm.sol

186:     /// @dev The function verifies the message signature is from the delegated address.

The function verifies the message signature is should be changed to The function verifies if the message signature is

[11] It's possible to call updateFighterStaking() on non-existing tokenId

File: FighterFarm.sol

File: src/FighterFarm.sol

268:     function updateFighterStaking(uint256 tokenId, bool stakingStatus) external {  
269:         require(hasStakerRole[msg.sender]);
270:         fighterStaked[tokenId] = stakingStatus;
271:         if (stakingStatus) {
272:             emit Locked(tokenId);
273:         } else {
274:             emit Unlocked(tokenId);
275:         }
276:     }

Function updateFighterStaking() does not verify if provided tokenId exists. This implies, that it would be possible to update fighterStaked mapping of future (not yet created) tokens. Our recommendation is to verify that tokenId exists, before updating fighterStaked[tokenId]

[12] It's possibe to set token URI of non-yet-existing tokens

Files: FighterFarm.sol, GameItems.sol

File: src/FighterFarm.sol

180:     function setTokenURI(uint256 tokenId, string calldata newTokenURI) external {  
181:         require(msg.sender == _delegatedAddress);
182:         _tokenURIs[tokenId] = newTokenURI;
183:     }

File: src/GameItems.sol

194:     function setTokenURI(uint256 tokenId, string memory _tokenURI) public {
195:         require(isAdmin[msg.sender]);
196:         _tokenURIs[tokenId] = _tokenURI;
197:     }

Functions setTokenURI does not verify if provided tokenId exists. This implies, that it would be possible to update _tokenURIs mapping of future (not yet created) tokens. Our recommendation is to verify that tokenId exists, before updating _tokenURIs[tokenId]

[13] Typos

Files: FighterFarm.sol, RankedBattle.sol, AAMintPass.sol, GameItems.sol

File: src/AAMintPass.sol

84:     /// @dev Set the delagated address (Founder-Only)

delagated should be changed to delegated.

File: src/FighterFarm.sol

77: 
78:     /// @notice Mapping to keep track of how many times an nft has been re-rolled.

nft should be changed to NFT.

File: src/RankedBattle.sol

415:     /// @param fighterOwner The address which owns the fighter whos points are being altered.

whos points should be changed to whose points.

File: src/GameItems.sol

57:     /// @notice The address that recieves funds of purchased game items.

recieves should be changed to receives.

[14] Using modifiers instead of hard-coding require will improve the code readability

Contract does not implement any modifier. Instead, it hard-codes require statements to verify the same information. E.g., let's consider function transferOwnership() from MergingPool.sol:

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

It calls require(msg.sender == _ownerAddress); to check if msg.sender is the owner. The same line is inside adjustAdminAccess() function:

   function adjustAdminAccess(address adminAddress, bool access) external {
        require(msg.sender == _ownerAddress);
        isAdmin[adminAddress] = access;
    }  

To improve the code quality, it would be much better idea to implement a single modifier, which can be used both for transferOwnership() and adjustAdminAccess() functions. This issue occurs across the whole code-base. E.g., in RankedBattle.sol, multiple of functions check: require(msg.sender == _ownerAddress);. To make the QA report clear and easy to read - we decided to report this as an overall issue, without listing all the affected instances. Our recommendation is to run grep require -ri . | grep "msg.sender ==" over the whole code-base (it returns 46 instances) and consider implement modifier for those results.

[15] Setters should prevent re-setting of the same value

Files: MergingPool.sol, RankedBattle.sol, Neuron.sol, VoltageManager.sol, GameItems.sol

Reported in this QA report instances were missed by the bot-report: N-63.

It's possible to call function which updates the state variable with the same value. It's a good practice to make sure, that whenever we update some value, it's not being set to the same value which it was before the update. E.g., let's consider a scenario where isAdmin[0xaaa] = true. Calling adjustAdminAccess(0xaaa, true) is possible, even though it won't change the value of the variable: isAdmin[0xaaa] will remain true. Our recommendation is to implement additional check which verifies if value is really changed: require(isAdmin[adminAddress] != access, "Nothing to change").

File: src/MergingPool.sol

098:     function adjustAdminAccess(address adminAddress, bool access) external {
099:         require(msg.sender == _ownerAddress);
100:         isAdmin[adminAddress] = access;
101:     }   

File: src/RankedBattle.sol

176:     function adjustAdminAccess(address adminAddress, bool access) external {
177:         require(msg.sender == _ownerAddress);
178:         isAdmin[adminAddress] = access;
179:     }  

File: src/GameItems.sol

117:     function adjustAdminAccess(address adminAddress, bool access) external {
118:         require(msg.sender == _ownerAddress);
119:         isAdmin[adminAddress] = access;
120:     }  

File: src/Neuron.sol

118:     function adjustAdminAccess(address adminAddress, bool access) external {
119:         require(msg.sender == _ownerAddress);
120:         isAdmin[adminAddress] = access;
121:     }  

File: src/VoltageManager.sol

73:     function adjustAdminAccess(address adminAddress, bool access) external { 
74:         require(msg.sender == _ownerAddress);
75:         isAdmin[adminAddress] = access;
76:     }  

File: src/VoltageManager.sol

82:     function adjustAllowedVoltageSpenders(address allowedVoltageSpender, bool allowed) external {
83:         require(isAdmin[msg.sender]);
84:         allowedVoltageSpenders[allowedVoltageSpender] = allowed;
85:     }

[16] Important, state-changing functions do not emit events

Files: FighterFarm.sol, MergingPool.sol, AiArenaHelper.sol, RankedBattle.sol, Neuron.sol, AAMintPass.sol, VoltageManager.sol, GameItems.sol

Code-base implements multiple of functions which change the state of the contract (e.g. transfer ownership, adds/removes new admins, changing the addresses, etc.). None of these functions emit events. Whenever contract changes the state of the blockchain and perform important/critical operation (e.g. transferring ownership is critical operation) - it should emit an event.

The QA report contains the list of functions which do not emit events

File: src/FighterFarm.sol

120:     function transferOwnership(address newOwnerAddress) external {
129:     function incrementGeneration(uint8 fighterType) external returns (uint8) {
139:     function addStaker(address newStaker) external {
147:     function instantiateAIArenaHelperContract(address aiArenaHelperAddress) external {
155:     function instantiateMintpassContract(address mintpassAddress) external {
163:     function instantiateNeuronContract(address neuronAddress) external {
171:     function setMergingPoolAddress(address mergingPoolAddress) external {
180:     function setTokenURI(uint256 tokenId, string calldata newTokenURI) external {  
283:     function updateModel(

File: src/MergingPool.sol

89:     function transferOwnership(address newOwnerAddress) external {
98:     function adjustAdminAccess(address adminAddress, bool access) external {
106:     function updateWinnersPerPeriod(uint256 newWinnersPerPeriodAmount) external {

File: src/AAMintPass.sol

55:     function transferOwnership(address _newFounderAddress) external {
65:     function addAdmin(address _newAdmin) external {
72:     function removeAdmin(address adminAddress) external {
79:     function setFighterFarmAddress(address _fighterFarmAddress) external {
86:     function setDelegatedAddress(address _delegatedAddress) external {
93:     function setPaused(bool _state) external {

File: src/RankedBattle.sol

167:     function transferOwnership(address newOwnerAddress) external {
176:     function adjustAdminAccess(address adminAddress, bool access) external {
184:     function setGameServerAddress(address gameServerAddress) external {
192:     function setStakeAtRiskAddress(address stakeAtRiskAddress) external {
218:     function setRankedNrnDistribution(uint256 newDistribution) external {
226:     function setBpsLostPerLoss(uint256 bpsLostPerLoss_) external {
233:     function setNewRound() external {

File: src/GameItems.sol

108:     function transferOwnership(address newOwnerAddress) external {
117:     function adjustAdminAccess(address adminAddress, bool access) external {
185:     function setAllowedBurningAddresses(address newBurningAddress) public {
194:     function setTokenURI(uint256 tokenId, string memory _tokenURI) public {

File: src/Neuron.sol

118:     function adjustAdminAccess(address adminAddress, bool access) external {
127:     function setupAirdrop(address[] calldata recipients, uint256[] calldata amounts) external {

File: src/AiArenaHelper.sol

61:     function transferOwnership(address newOwnerAddress) external {

File: src/VoltageManager.sol

64:     function transferOwnership(address newOwnerAddress) external {
73:     function adjustAdminAccess(address adminAddress, bool access) external { 
82:     function adjustAllowedVoltageSpenders(address allowedVoltageSpender, bool allowed) external {

[17] Use _grantRole() instead of deprecated _setupRole()

File: Neuron.sol

According to OZ's AccessControl implementation, function _setupRole() is deprecated:

* NOTE: This function is deprecated in favor of {_grantRole}.

However, Neuron.sol uses it in multiple of functions.

Our recommendation is to use _grantRole() in the below instances:

File: src/Neuron.sol

093:     function addMinter(address newMinterAddress) external {
094:         require(msg.sender == _ownerAddress);
095:         _setupRole(MINTER_ROLE, newMinterAddress);
096:     }
[...]
101:     function addStaker(address newStakerAddress) external {
102:         require(msg.sender == _ownerAddress);
103:         _setupRole(STAKER_ROLE, newStakerAddress);
104:     }
[...]
109:     function addSpender(address newSpenderAddress) external {
110:         require(msg.sender == _ownerAddress);
111:         _setupRole(SPENDER_ROLE, newSpenderAddress);
112:     }

[18] Use uint256 instead of uint

File: RankedBattle.sol

Even though uint is a shorter form of uint256 - using uint256 increases the code readability. We have detected that uint256 is used across the whole code-base. The only exception was noticed in the below instance:

File: src/RankedBattle.sol

19:     using FixedPointMathLib for uint;

AiArenaHelper.sol

attributes and defaultAttributeDivisor can be packed into single variable

The code-base does not provide any ability to extend attributes array. The list of attribute seems to be non-modifiable:

File: src/AiArenaHelper.sol

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

This suggests, that instead accessing every element of attributes - it will be more gas-efficient to pack this array into single uint256 variable.

This would require multiple of refactoring steps, which we will describe here. Firstly, we need to decide how attributes will be packed into array. Let's define, that each byte will represent different attribute, e.g:

1 - 000001 -> feet
2 - 000010  -> hands
4 - 000100 -> body
8 - 001000 -> mouth
16 - 010000 -> eyes
32 - 100000 -> head

This basically means, that attributes will be of type uint256: uint256 public attributes. Updating the attributes type would require us to update other mappings, e.g., attributeToDnaDivisor will be changed from mapping(string => uint8) to mapping(uint256 => uint8).

Now, the most interesting part - almost every function loops over attributes.length. We need to re-implement these loops. E.g., let's take a look at loop in addAttributeDivisor() function:

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

we can easily rewrite that loop into a version when we'll be unpacking attributes.

        for (uint8 i = 0; i < 6; i++) {
            attributeToDnaDivisor[1 << i] = attributeDivisors[i];
        }

Similarly, loops in other functions needs to be rewritten. That way, we will save a lot of gas - instead of using string[] public attributes, our attributes will be packed into single integer!

The same issue occurs with defaultAttributeDivisor. Since the length of this array is constant, we can pack it into a single uint256. Instead of using below array:

File: src/AiArenaHelper.sol

20:     uint8[] public defaultAttributeDivisor = [2, 3, 5, 7, 11, 13];

every value can be packed into a single uint256.

Every occurrence of attributes.length can be pre-calculated

The code-base does not provide any ability to extend attributes array. The list of attribute seems to be non-modifiable:

File: src/AiArenaHelper.sol

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

This suggests, that the length of this array is constant - 6. Instead of using attributes.length in the AIArenaHelper.sol - every occurrence of attributes.length can be changed to a constant value: 6.

E.g.:

File: src/AiArenaHelper.sol

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

can be changed to:

        for (uint8 i = 0; i < 6; i++) {
            attributeProbabilities[generation][attributes[i]] = new uint8[](0);

dnaToIndex() can be optimized to return earlier

File: src/AiArenaHelper.sol

178:         for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
179:             cumProb += attrProbabilities[i];
180:             if (cumProb >= rarityRank) {
181:                 attributeProbabilityIndex = i + 1;
182:                 break;
183:             }
184:         }
185:         return attributeProbabilityIndex;

As soon as we break from the loop, function returns with attributeProbabilityIndex. This implies, that we can immediately return inside the loop, instead of wasting gas on break operation and redundant attributeProbabilityIndex = i + 1 assignment:

for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
                return i + 1;
            }

attributes.length should be cached earlier

The current implementation caches the attributes.length before the loop, so that it won't be calculated during every loop iteration. However, the length of attributes can be calculated earlier, since it's used in require statement.

File: src/AiArenaHelper.sol

70:         require(attributeDivisors.length == attributes.length);  // @audit: cache attributes.length here, so that cached variable will be used both in require and in a loop
71: 
72:         uint256 attributesLength = attributes.length;
73:         for (uint8 i = 0; i < attributesLength; i++) {

Our recommendation is to rewrite above code to:

        uint256 attributesLength = attributes.length;
        require(attributeDivisors.length == attributesLength);
        for (uint8 i = 0; i < attributesLength; i++) {

The same issue occurs in createPhysicalAttributes()

File: src/AiArenaHelper.sol

96:             uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length); // @audit: cache attributes.length here, so that cached variable will be used both in require and in a loop
97: 
98:             uint256 attributesLength = attributes.length;

require statement which uses less gas should be used first

File: src/AiArenaHelper.sol

132:         require(msg.sender == _ownerAddress);
133:         require(probabilities.length == 6, "Invalid number of attribute arrays");

Reading state variable _ownerAddress uses more gas than reading function's parameter probabilities, thus the order of require statements should be changed.

FighterFarm.sol

Do not calculate the length of arrays multiple of times

Calculating the length of arrays costs gas. It's better to calculate it once and cache the result. Moreover, since we want to make sure that every parameters length is the same, it will be much better idea to calculate the length of the first parameter, cache it and compare it with the length of the others. That way we won't be calculating the length of other parameter twice.

In other words:

if (A.length == B.length &&
    B.length == C.length &&
    C.length == D.length &&
    D.length == E.length)

is the same as:

a = A.length
if (a == B.length &&
    a == C.length &&
    a == D.length &&
    a == E.length)

Below code:

File: src/FighterFarm.sol

243:         require(
244:             mintpassIdsToBurn.length == mintPassDnas.length && 
245:             mintPassDnas.length == fighterTypes.length && 
246:             fighterTypes.length == modelHashes.length &&
247:             modelHashes.length == modelTypes.length
248:         );
249:         for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {

can be changed to:

        uint256 cachedMintpassIdsToBurn = mintpassIdsToBurn.length;
        require(
            cachedMintpassIdsToBurn == mintPassDnas.length && 
            cachedMintpassIdsToBurn == fighterTypes.length && 
            cachedMintpassIdsToBurn == modelHashes.length &&
            cachedMintpassIdsToBurn == modelTypes.length
        );
        for (uint16 i = 0; i < cachedMintpassIdsToBurn; i++) {

Redundant variables in _createFighterBase()

There's no need to declare element, weight, newDna, since they are used only once, the code can be simplified to:

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
        return (dna % numElements[generation[fighterType]], dna % 31 + 65, (fighterType == 0 ? dna : uint256(fighterType)));
    }

reRoll() can be optimized

State variable used multiple of times should be cached. Local variables used only once do not need to be declared at all. Below code demonstrates above optimizations:

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        uint256 cachedNumRerolls = numRerolls[tokenId]; // @audit: cache numRerolls[tokenId]
        require(cachedNumRerolls < maxRerollsAllowed[fighterType]);  // @audit: use cached numRerolls
        uint256 cachedRerollCost = rerollCost; // @audit: cache rerollCost
        require(_neuronInstance.balanceOf(msg.sender) >= cachedRerollCost, "Not enough NRN for reroll"); // @audit: use cached rerollCost 

        _neuronInstance.approveSpender(msg.sender, cachedRerollCost);  // @audit: use cached rerollCost 
        if (_neuronInstance.transferFrom(msg.sender, treasuryAddress, cachedRerollCost)) { // @audit: use cached rerollCost, do not declare local var used only once
            numRerolls[tokenId] = cachedNumRerolls + 1
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(uint256(keccak256(abi.encode(msg.sender, tokenId, cachedNumRerolls + 1)));, fighterType); // @audit: cached numRerolls, line above numRellors were increased by one; redundant variable dna was removed
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }    

State variables in _createNewFighter() should be cached

Function _createNewFighter() reads state variable twice. Reading state variable costs more gas than caching that variable into local variable and read the local variable.

File: src/FighterFarm.sol

499:         if (customAttributes[0] == 100) {  // @audit: cache customAttributes[0]
[...]
503:             element = customAttributes[0];  // @audit: cache customAttributes[0]
[...]
512:             generation[fighterType],  // @audit: cache generation[fighterType]
[...]
524:                 generation[fighterType],  // @audit: cache generation[fighterType]
[...]
530:         FighterOps.fighterCreatedEmitter(newId, weight, element, generation[fighterType]);  // @audit: cache generation[fighterType]
[...]

FighterOps.sol

Struct FighterCreated can be more tightly packed

When we take a look at function _createFighterBase() from FighterFarm.sol, we can see, that the initial max. weight is 30 + 65:

File: src/FighterFarm.sol

471:         uint256 weight = dna % 31 + 65;

This suggests, that there's no need to declare weight as uint256, since it's very unlikely that this value ever reach uint256. The same reasoning goes with the element. This implies, that below struct can be repacked from:

File: src/FighterOps.sol

36:     struct Fighter {
37:         uint256 weight;
38:         uint256 element;
39:         FighterPhysicalAttributes physicalAttributes;
40:         uint256 id;
41:         string modelHash;
42:         string modelType;
43:         uint8 generation;
44:         uint8 iconsType;
45:         bool dendroidBool;
46:     }

to:

    struct Fighter {
        uint128 weight;
        uint128 element;
        FighterPhysicalAttributes physicalAttributes;
        uint256 id;
        string modelHash;
        string modelType;
        uint8 generation;
        uint8 iconsType;
        bool dendroidBool;
    }

GameItems.sol

Consider packing GameItemAttributes struct more tightly

It's very unlikely that itemPrice will ever reach the max uint256 value. Consider changing its type to something smaller, e.g., uint128, so that it can be packed with bool fields, thus use one less storage slot:

    struct GameItemAttributes {
        string name;
        bool finiteSupply;
        bool transferable;
        uint128 itemPrice;
        uint256 itemsRemaining;
        uint256 dailyAllowance;
    }  

Revert when quantity is 0 in mint()

Whenever quantity parameter is passed as 0 - the price will be calculated as 0 too, because: price = allGameItemAttributes[tokenId].itemPrice * quantity, thus price = allGameItemAttributes[tokenId].itemPrice * 0. This implies, that we won't mint anything, but just use gas on useless operations (nothing will be minted). Consider adding additional require statement which will protect us from that scenario:

require(quantity > 0, "Nothing to mint").

Use post-increment to limit the number of state variable readings.

File: src/GameItems.sol

230:         if (!transferable) {
231:           emit Locked(_itemCount);
232:         }
233:         setTokenURI(_itemCount, tokenURI);
234:         _itemCount += 1;
235:     }

Variable _itemCount is used 3 times. Since it's a state variable we can limit a number of its readings by using post-incrementing. Above code can be rewritten to:

        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount++, tokenURI);

Remove redundant assignment and variable declaration in getAllowanceRemaining()

File: src/GameItems.sol

268:     function getAllowanceRemaining(address owner, uint256 tokenId) public view returns (uint256) {
269:         uint256 remaining = allowanceRemaining[owner][tokenId];
270:         if (dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp) {  
271:             remaining = allGameItemAttributes[tokenId].dailyAllowance;
272:         }
273:         return remaining;
274:     }

Above code can be rewritten to get rid of remaining declaration and redundant assignment: remaining = allGameItemAttributes[tokenId].dailyAllowance:

    function getAllowanceRemaining(address owner, uint256 tokenId) public view returns (uint256) {
        if (dailyAllowanceReplenishTime[owner][tokenId] <= block.timestamp) {  
            return allGameItemAttributes[tokenId].dailyAllowance;
        }
        return allowanceRemaining[owner][tokenId];
    }

MerginPool.sol

Do not loop over the whole winnersLength in claimRewards() and getUnclaimedRewards

According to the documentation - each round may have multiple of winners (chosen by pickWinner() function), however, the same address cannot be set as the winner multiple of times during the same round. This implies, that each rounds have multiple of unique winners.

File: src/MergingPool.sol

152:             for (uint32 j = 0; j < winnersLength; j++) {
153:                 if (msg.sender == winnerAddresses[currentRound][j]) {
154:                     _fighterFarmInstance.mintFromMergingPool(
155:                         msg.sender,
156:                         modelURIs[claimIndex],
157:                         modelTypes[claimIndex],
158:                         customAttributes[claimIndex]
159:                     );
160:                     claimIndex += 1;
161:                 }

This implies, that in the second loop at line 152, when we already find that msg.sender == winnerAddresses[currentRound][j], we don't need to continue looping over winnersLength - since we have already found that msg.sender == winnerAddresses[currentRound][j], thus we can break that loop. Our recommendation is to add break after claimIndex += 1; at line 160. Since we've already found that msg.sender is the winner in the current loop, there's no need to continue looping over winnersLength. We can break for that loop and continue to look for a winner in another round (external loop at line 149).

The same issue occurs in getUnclaimedRewards()

File: src/MergingPool.sol

176:         for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
177:             winnersLength = winnerAddresses[currentRound].length;
178:             for (uint32 j = 0; j < winnersLength; j++) {
179:                 if (claimer == winnerAddresses[currentRound][j]) {
180:                     numRewards += 1;
181:                 }
182:             }
183:         }

When we found that claimer == winnerAddresses[currentRound][j] in loop at line 178, we can increase numRewards (numRewards += 1), break the loop and continue looping in loop at line 176:

        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (claimer == winnerAddresses[currentRound][j]) {
                    numRewards += 1;
                    break;
                }
            }
        }

Neuron.sol

Use _grantRole() instead of _setupRole().

Since, according to OZ's AccessControl implementation, function _setupRole() is deprecated - Neuron.sol should use _grantRole() instead. The _setupRole() implementation in OZ's AcccessControl implementation, directly calls _grantRole():

    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

This implies, that by calling _setupRole(), we waste gas on additional call. It's more gas efficient to call _grantRole() directly. Below instances should use _grantRole() instead of _setupRole().

File: src/Neuron.sol

093:     function addMinter(address newMinterAddress) external {
094:         require(msg.sender == _ownerAddress);
095:         _setupRole(MINTER_ROLE, newMinterAddress);
096:     }
[...]
101:     function addStaker(address newStakerAddress) external {
102:         require(msg.sender == _ownerAddress);
103:         _setupRole(STAKER_ROLE, newStakerAddress);
104:     }
[...]
109:     function addSpender(address newSpenderAddress) external {
110:         require(msg.sender == _ownerAddress);
111:         _setupRole(SPENDER_ROLE, newSpenderAddress);
112:     }

claim() should check for 0-amount

Function claim() performs transferFrom(). However, it does not check if amount is non-zero. Whenever amount is 0, transferring 0 does basically nothing - since transferring 0-amount does not change the state of the blockchain, it's basically useless. To save gas, our recommendation is to verify if amount > 0, before performing tranferFrom().

Add additional require statement: require(amount > 0, "Nothing to claim");.

No need to check for allowance() in claim()

File: src/Neuron.sol

138:     function claim(uint256 amount) external {
139:         require(
140:             allowance(treasuryAddress, msg.sender) >= amount, 
141:             "ERC20: claim amount exceeds allowance"
142:         );
143:         transferFrom(treasuryAddress, msg.sender, amount);
144:         emit TokensClaimed(msg.sender, amount);
145:     }

Function transferFrom() already checks the allowance. When we look at OZ's implementation of transferFrom(), it already calls _spendAllowance(). This implies, that there's no need to call additional allowance() in claim(). If claim amount exceeds allowance - function will revert on transferFrom(), thus lines 139-142 are redundant and can be removed.

Cache allowance() result in burnFrom()

In function burnFrom(), function allowance() is called twice. It's much better idea to call it once and cache its result in local variable:

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

Do not declare redundant variables

In function burnFrom(), variable decreasedAllowance is used only once, thus there's no need to declare it at all:

 _approve(account, msg.sender, allowance(account, msg.sender) - amount);

StakeAtRisk.sol

reclaimNRN() could be optimized

roundId is a state variable, thus reading it costs more gas. Function reclaimNRN() reads this variable 3 times, while function updateAtRiskRecords() reads it twice. Our recommendation is to cache roundId into local variable to avoid reading a state variable multiple of times:

    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
       [...]
       uint256 cachedRoundId = roundId;
        require(
            stakeAtRisk[cachedRoundId][fighterId] >= nrnToReclaim, 
[...]
            stakeAtRisk[cachedRoundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[cachedRoundId] -= nrnToReclaim;

    function updateAtRiskRecords(
[...]
        uint256 cachedRoundId = roundId;
        stakeAtRisk[cachedRoundId][fighterId] += nrnToPlaceAtRisk;
        totalStakeAtRisk[cachedRoundId] += nrnToPlaceAtRisk; 

Redundant success variables

There's no need to initialize bool success variable. Code like this:

bool success = someFunction();
if (success)

can be rewritten to if(someFunction()). That way, we can avoid declaration of additional variable.

File: src/StakeAtRisk.sol

80:         bool success = _sweepLostStake();
81:         if (success) {

can be changed to: if (_sweepLostStake())

File: src/StakeAtRisk.sol

100:         bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
101:         if (success) {

can be changed to: if (_neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim))

RankedBattle.sol

setNewRound() could be optimized

File: src/RankedBattle.sol

233:     function setNewRound() external {
234:         require(isAdmin[msg.sender]);
235:         require(totalAccumulatedPoints[roundId] > 0);
236:         roundId += 1;
237:         _stakeAtRiskInstance.setNewRound(roundId);
238:         rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];
239:     }

Since roundId is a state variable, accessing it multiple of times cost a lot of gas. Our recommendation is to cache this variable. Moreover, since at line 236 roundId is increased by one - we will use post-incrementing our cached variable (at line 235) - to avoid performing addition twice. If we didn't do that, we would be needed to perform additional addition operation at line 237. We can. however, avoid that as it's presented in the example below:

    function setNewRound() external {
        uint256 cachedRoundId = roundId;
        require(isAdmin[msg.sender]);
        require(totalAccumulatedPoints[cachedRoundId++] > 0);  
        roundId  = cachedRoundId;
        _stakeAtRiskInstance.setNewRound(cachedRoundId);
        rankedNrnDistribution[cachedRoundId] = rankedNrnDistribution[cachedRoundId - 1];
    }

unstakeNRN() can be optimized

Multiple of optimizations can be implemented to reduce the gas-intake of unstakeNRN(). In our report, we present all of them, including the final implementation. In unstakeNRN(), amountStaked[tokenId] is a state variable which is read multiple of times. We can reduce number of readings by caching this variable. The same goes with roundId - which can also be cached. Moreover, we do not need to declare variable used only once (e.g., success). Since if condition guarantees that amount > amountStaked[tokenId], amountStaked[tokenId] -= amount can be unchecked. The same with globalStakedAmount -= amount, since globalStakedAmount >= amountStaked[tokenId] Our recommendation is to re-write unstakeNRN() as below:

   function unstakeNRN(uint256 amount, uint256 tokenId) external {  
        require(_fighterFarmInstance.ownerOf(tokenId) == msg.sender, "Caller does not own fighter");
        uint256 cachedAmountStaked = amountStaked[tokenId];  // @audit: caching amountStaked[tokenId]
        uint256 cachedRoundId = roundId;  // @audit: caching roundId
        if (amount > cachedAmountStaked) {  // @audit: using cachedAmountStaked
            amount = cachedAmountStaked; // @audit: using cachedAmountStaked
        }
        unchecked {  // @audit, if condition guarantees this won't revert
        amountStaked[tokenId] -= amount;
        globalStakedAmount -= amount;
        }
        stakingFactor[tokenId] = _getStakingFactor(
            tokenId, 
            _stakeAtRiskInstance.getStakeAtRisk(tokenId)
        );
        _calculatedStakingFactor[tokenId][cachedRoundId] = true;  // @audit: using cached cachedRoundId
        hasUnstaked[tokenId][cachedRoundId] = true;  // @audit: using cached cachedRoundId
        if (_neuronInstance.transfer(msg.sender, amount)) {  // @ audit: get rid of redundant variable success
            if (cachedAmountStaked - amount == 0) {  // @audit: using cachedAmountStaked, since we have updated `amountStaked[tokenId] -= amount` (line 275) and cachedAmountStaked contains old value, we need to sub. amount.
                _fighterFarmInstance.updateFighterStaking(tokenId, false);
            }
            emit Unstaked(msg.sender, amount);
        }
    }

VoltageManager.sol

Use _replenishVoltage() directly in spendVoltage() - to decrease a number of state variable readings

Using _replenishVoltage() code directly in spendVoltage(), would allow to cache state variables. In the current implementation, we read state variable ownerVoltage 3 times (twice in spendVoltage() and once in _replenishVoltage()). Using code from _replenishVoltage() directly in spendVoltage() would allow us to get rid of one state variable reading, whenever the replenish needs to be done.

Whenever replenish needs to be done, we know that the voltage remaining after the replenish is always 100 - voltageSpent. That way, we don't need to read ownerVoltage[spender] state variable again in the VoltageRemaining event.

Below code demonstrates the solution which saves us from additional state variable reading when replenish is needed.

File: src/VoltageManager.sol

   function spendVoltage(address spender, uint8 voltageSpent) public {
        require(spender == msg.sender || allowedVoltageSpenders[msg.sender]);
        if (ownerVoltageReplenishTime[spender] <= block.timestamp) {
            ownerVoltage[spender] = 100 - voltageSpent;
            ownerVoltageReplenishTime[spender] = uint32(block.timestamp + 1 days);
            emit VoltageRemaining(spender, 100 - voltageSpent);  // @audit: saving gas here, using `100 - voltageSpent` instead of reading `ownerVoltage[spender]` again
        }
        else {
            ownerVoltage[spender] -= voltageSpent; 
            emit VoltageRemaining(spender, ownerVoltage[spender]);
        }
        
    }

useVoltageBattery() is reading state variable, while its value is known

File: src/VoltageManager.sol

97:         ownerVoltage[msg.sender] = 100;
98:         emit VoltageRemaining(msg.sender, ownerVoltage[msg.sender]);

ownerVoltage[msg.sender] is assigned to 100, thus ownerVoltage[msg.sender] in VoltageRemaining() event will always return 100. We can get rid of reading state variable and hard-code 100 into that event:

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