AI Arena - PoeAudits's results

Lines of code

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

Vulnerability details

Impact

Users could exceed their number or rerolls allowed. https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L372

Users can roll attributes from a different generation https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L383-388 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L108 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L174 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L162

Affects element type and dna of rerolled fighters https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L472

Proof of Concept

The majority of the bug relies on differences between the generations of fighterType=0 and fighterType=1. Initially, the maxRerollsAllowed and generation variables set both fighterTypes to have the same starting values: https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L36 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L42

As shown by the FighterFarm.sol function incrementGeneration, generations are incremented for each fighterType, so there is no expectation for these to remain the same during the lifetime of the contract. https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L129-L134

When these become out of sync, fighters with fighterType=0 can call the reroll with an input of fighterType=1 to use extra rerolls if maxRerollsAllowed[0] < maxRerollsAllowed[1]. Additionally, if generations[0] != generations[1] the fighters can be rerolled with attributeProbabilities from a different generation. This results in rerolled fighters with odd stat profiles such as all zeros.

Note: The following function must be added to FighterFarm.sol in order to fix a bug which prevents this issue from executing. See "Reroll Breaks on Incrementing Generation Because of numElements Having no Setter Function"

    function setNumElements(uint8 index, uint8 input) public {
        require(msg.sender == _ownerAddress); // Following existing code
        numElements[index] = input; 
    }

To demonstrate the additonal rerolls, the below code is test/FighterFarm.t.sol function testRerollRevertWhenLimitExceeded with the additional code added as indicated.

    /// @notice Test that the reroll fails if maxRerolls is exceeded.
    function testRerollRevertWhenLimitExceeded() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model
        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);
            uint8 exceededLimit = maxRerolls + 1;
            uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
            uint8 tokenId = 0;
            uint8 fighterType = 0;

            _neuronContract.addSpender(address(_fighterFarmContract));
            for (uint8 i = 0; i < exceededLimit; i++) {
                if (i == (maxRerolls)) {
                    vm.expectRevert();
                    _fighterFarmContract.reRoll(tokenId, fighterType);
                    assertEq(_neuronContract.balanceOf(_ownerAddress) < neuronBalanceBeforeReRoll, true);
                } else {
                    _fighterFarmContract.reRoll(tokenId, fighterType);
                }
            }

            /////////////////////////////////////////////////////////////
            ////////////////// Added Code //////////////////////////////

            // Bug occurs after incrementing the generation
           _fighterFarmContract.incrementGeneration(1);

            // This reverts due to another bug in the contract 
            vm.expectRevert();
            _fighterFarmContract.reRoll(tokenId, 1);

            // This fixes the numElements bug - see other report for more info 
            _fighterFarmContract.setNumElements(1, 3);

            // Allowed to reroll again, albiet with poor stats
            _fighterFarmContract.reRoll(tokenId, 1);

        }
    }

    │   ├─ [31250] AiArenaHelper::createPhysicalAttributes(1, 1, 0, false) [staticcall]
    │   │   └─ ← FighterPhysicalAttributes({ head: 0, eyes: 0, mouth: 0, body: 0, hands: 0, feet: 0 })
    │   └─ ← ()
    └─ ← ()
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.47ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

The above PoC demonstrates both the issue of exceeding the number of allowed rerolls, which is admittedly somewhat of a minor issue. It also demonstrates that the user can reroll using the attributeProbabilities of the alternate fighterType's generation. This example shows the user rolling for a generation that does not exist for fighterType=0, which results in poor fighter stats. This is only potentially beneficial for the opposite case, where generation[fighterType] is the larger of the two values. In this case, the fighter can select the opposite fighterType to reroll using that generation's attribute profile.

The additional bug regarding affecting element and dna of fighters is fairly self-explanatory.

All three of these issues arise from the same bug which is that the reRoll function accepts a fighterType parameter and does not check the validity of this parameter.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

FighterType should not be an input to the FighterFarm.sol function reRoll. The fighterType data should be accessed from the token itself through the dendroidBool variable.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

User's fighters with tokenId > 255 cannot reroll.

Proof of Concept

There is a typo in the reRoll function on line 370. The input tokenId is set as a uint8 instead of a uint256 as it is in the rest of the codebase.

function reRoll(uint8 tokenId, uint8 fighterType) public {

Any call to this function with a tokenId > type(uint8).max will revert effectively blocking access to this feature for most tokens.

Tools Used

Foundry, Manual Review

Recommended Mitigation Steps

Replace line 370 with the updated type:

function reRoll(uint256 tokenId, uint8 fighterType) public {

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Reroll function will no longer work after generation is incremented.

Proof of Concept

The root cause of this bug is that the numElements mapping has no way of being set for future generations. It is initially set in the constructor, and no function is available to update it. When calling the reRoll function, _createFighterBase is called, which modulates the dna variable by numElements[generation[fighterType]]. Since this cannot be anything other than zero for generations > 0, solidity throws a panic error when trying to modulate by zero.

Increment the generation prior to rerolling in FighterFarm.t.sol function testReroll:

function testReroll() public {
    _fighterFarmContract.incrementGeneration(0); // Add this line
    _mintFromMergingPool(_ownerAddress);
    // get 4k neuron from treasury
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    // after successfully minting a fighter, update the model
    if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
        uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(_ownerAddress);
        uint8 tokenId = 0;
        uint8 fighterType = 0;

        _neuronContract.addSpender(address(_fighterFarmContract));
        _fighterFarmContract.reRoll(tokenId, fighterType);
        assertEq(_fighterFarmContract.numRerolls(0), 1);
        assertEq(neuronBalanceBeforeReRoll > _neuronContract.balanceOf(_ownerAddress), true);
    }
}

$ forge test --mc FighterFarmTest --mt testReroll --no-match-test LimitExceeded -vvvv
[⠒] Compiling...
[⠔] Compiling 1 files with 0.8.13
[⠒] Solc 0.8.13 finished in 5.29s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[FAIL. Reason: panic: division or modulo by zero (0x12)] testReroll() (gas: 482363)
    ├─ [66055] FighterFarm::reRoll(0, 0)
    │   ├─ [586] Neuron::balanceOf(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1]) [staticcall]
    │   │   └─ ← 4000000000000000000000 [4e21]
    │   ├─ [24933] Neuron::approveSpender(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 1000000000000000000000 [1e21])
    │   │   ├─ emit Approval(owner: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], spender: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], value: 1000000000000000000000 [1e21])
    │   │   └─ ← ()
    │   ├─ [5948] Neuron::transferFrom(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, 1000000000000000000000 [1e21])
    │   │   ├─ emit Approval(owner: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], spender: FighterFarm: [0xd04404bcf6d969FC0Ec22021b4736510CAcec492], value: 0)
    │   │   ├─ emit Transfer(from: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: 0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf, value: 1000000000000000000000 [1e21])
    │   │   └─ ← 0x0000000000000000000000000000000000000000000000000000000000000001
    │   └─ ← panic: division or modulo by zero (0x12)
    └─ ← panic: division or modulo by zero (0x12)

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.76ms

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/FighterFarm.t.sol:FighterFarmTest
[FAIL. Reason: panic: division or modulo by zero (0x12)] testReroll() (gas: 482363)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Add a setter function for numElements either as a stand-alone function, or incorporate the setting logic into the increment generation function.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Users can determine the stats of their fighters at will. Devalues "rare" fighters.

Proof of Concept

This exploit relies on the fighter being transferable.

The core issue is how the contract generates the fighter's dna. The dna generation on line https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L379

        uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

Including msg.sender with the hashing function makes the function non-deterministic. This allows the user to transfer their fighter to another address they control for a different result when calling the reRoll function. This can be further abused using tools like foundry to simulate the result of the reRoll function from different addresses.

The following code can be added to the test/FighterFarm.t.sol to demonstrate the vulnerability.

Note: The test should alter the following import before running the test:

import {FighterFarm, FighterOps} from "../src/FighterFarm.sol";

// Helper functions for simulating dna generation:

    // Private function from FighterFarm.sol
    // If using private functions bothers you, 
    // there is an easier method of just forking the chain and rolling at different addresses post deployment.
    function _createFighterBase(uint256 dna, uint8 fighterType) 
        internal 
        view 
        returns (uint256, uint256, uint256) 
    {
        uint256 element = dna % _fighterFarmContract.numElements(_fighterFarmContract.generation(fighterType));
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

    // We add 1 to the number of rerolls since the code increments before hashing line FighterFarm.sol:L378 
    function _getDna(address sender, uint8 tokenId) public view returns(uint256) {
        return uint256(keccak256(abi.encode(sender, tokenId, _fighterFarmContract.numRerolls(tokenId) + 1)));
    }

// Start PoC
   function test_DeterministicReroll() public {
        _neuronContract.addSpender(address(_fighterFarmContract));

        // Mints token to staker
        address staker = vm.addr(3);
        _mintFromMergingPool(staker);
        _fundUserWith4kNeuronByTreasury(staker);        

        // We can assume we know the tokenId of the token we are rerolling. In this case its Id zero. 
        // Retrieving the relevant information needed to simulate the roll
        uint8 tokenId = 0;
        uint8 fighterType = 0; 
        uint8 generation = _fighterFarmContract.generation(fighterType);
        (,,,,,,,uint8 icon, bool isDendriod) = _fighterFarmContract.fighters(tokenId);

        // Snapshot the attributes before
        (, uint256[6] memory beforeRoll,,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);

        // Reduce runtime by setting pkey near actual value of pkey = 203987. 
        // Can just loop until you find what you are looking for.
        for (uint256 pkey = 203985; pkey < 400000; pkey++){
            // Create a puppet address using an integer private key
            address puppet = vm.addr(pkey); 
            // Simulate what the dna would be when sent from the puppet address.
            uint256 dna = _getDna(puppet, 0);
            (, , uint256 newDna) = _createFighterBase(dna, fighterType); 
            FighterOps.FighterPhysicalAttributes memory attributes = _helperContract.createPhysicalAttributes(newDna, generation, icon, isDendriod);
                // Check if all attributes meet the criteria
                // Can use whatever stats you want to find
            if (
                attributes.head >= 5 &&
                attributes.eyes >= 5 &&
                attributes.mouth >= 5 &&
                attributes.body >= 5 &&
                attributes.hands >= 5 &&
                attributes.feet >= 5
            ) {
                console.log("Attributes Found");
                console.log("Private Key: ", pkey);
                console.log("Address: ", puppet);
                // The staker sends the nft corresponding to tokenId to the puppet address.
                vm.startPrank(staker);
                _fighterFarmContract.approve(puppet, 0);
                _fighterFarmContract.transferFrom(staker, puppet, 0);
                vm.stopPrank();

                // The puppet address gets funded, normally from the staking address but this was simpler. 
                _fundUserWith4kNeuronByTreasury(puppet);
                // Puppet address calls the reRoll function to make a deterministic roll
                // The puppet address then sends back the token to staker.
                // Why can the puppet address call the function, because the staker knows the private key to this wallet
                // When we are looping over pkey we are looping over private key values, which are effectively just uint256 values
                vm.startPrank(puppet);
                    _fighterFarmContract.approve(address(_fighterFarmContract), 0);
                    _fighterFarmContract.reRoll(0, fighterType);
                    _fighterFarmContract.approve(staker, 0);
                    _fighterFarmContract.transferFrom(puppet, staker, 0);
                vm.stopPrank();

                break;

            }

        }
        // Snapshot the attributes after rolling
        (, uint256[6] memory afterRoll,,,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);
        
        // Print out the results
        console.logString("Fighter Attributes BEFORE Reroll: ");
        for (uint256 i; i < beforeRoll.length; ++i) {
            console.logUint(beforeRoll[i]);
        }
        console.logString("");
        console.logString("Fighter Attributes AFTER Reroll: ");
        for (uint256 i; i < beforeRoll.length; ++i) {
            console.logUint(afterRoll[i]);
        }
    }

This results in the following logs:

$ forge test --mc FighterFarmTest --mt DeterministicReroll -vvv
[⠒] Compiling...
[⠃] Compiling 1 files with 0.8.13
[⠊] Solc 0.8.13 finished in 5.73s
Compiler run successful!

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] test_DeterministicReroll() (gas: 825709)
Logs:
  Attributes Found
  Private Key:  203987
  Address:  0xDE2EE9A9f5460b92d3B8eA212D558798D903DAA3
  Fighter Attributes BEFORE Reroll:
  1
  4
  1
  1
  0
  1

  Fighter Attributes AFTER Reroll:
  5
  5
  6
  6
  5
  6

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.88ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

This particular example is only searching for attribute values >= 5. This can replicated to search for specific values of the fighter's element, weight and attributes. This gives players with the capability of exploiting the reRoll logic a significant and unfair advantage.

Once the system is deployed, it is even easier to write a script to abuse the reRoll function by forking the chain and calling the function directly.

Tools Used

Foundry, manual review

Recommended Mitigation Steps

There are two solutions to this problem.

The most robust solution is to receive random numbers using a service like Chainlink's VRF. This however, would require significant changes to the codebase and system design. The lesser solution in the same vein is for the project to compute random numbers off chain themselves and supply them to the contract, although implementing such a system is non-trivial.

The more reasonable solution would be to only use variables independent of user influence in the hash function. Removing msg.sender in line 379:

uint256 dna = uint256(keccak256(abi.encode(tokenId, numRerolls[tokenId])));

This leaves the hash function with variables outside the user's control, and the numRerolls variable acts like a nonce value, incrementing on each reroll ensuring the hash is different than before. This does, however, mean that the reRoll function is still deterministic such that users can pre-determine what the rerolls for each token will be. They will be able to pick from attribute profiles for as many rerolls they have remaining. This solution will no longer allow the user to select exactly the attributes they want.

Assessed type

Other

Contract Overview

AiArenaHelper:

The AiArenaHelper contract is a helper contract that generates and manages Ai Arena fighter's physical attributes. The contract includes functionality to handle ownership, attribute probabilities, and the creation of physical attributes based on a fighter's DNA. It is used in other contracts to facilitate the generation of Ai fighters.

FighterOps:

This library manages the creation and attributes of Fighter NFTs within the game. The library is designed to be used by other contracts to retrieve the relevant data for Ai fighters. The library includes the data structures that define each fighter and includes relevant functions to expose this data publicly.

GameItems:

The GameItems smart contract inherits from OpenZeppelin's ERC-1155 standard allowing for the creation of fungible and non-fungible tokens within a single contract. The contract is designed to manage game items for Ai Arena. The GameItems smart contract allows for trusted admins to create unique game items, and allows anyone to mint these items as long as pre-set requirements are fulfilled. When creating an item, the admin can set the transferability, price, daily allowance, and supply of the game items. The supply can be infinite if desired. The contract also defines the logic for transferring and burning game items, while complying with uri requirements set by ERC-1155. Payment for game items is done through the Neuron ERC-20 token.

MergingPool:

The MergingPool contract includes functionality for managing points associated with fighter tokens, selecting winners, and claiming rewards. Users earn points by winning rounds through the RankedBattle contract, which calls the addPoints function of the MergingPool contract. This contract also allows admin accounts to select the winners of a round, and allows the winners to claim rewards, which are additional Ai fighters.

Neuron:

The Neuron contract is the payment system for Ai Arena, and is an ERC-20 token with a maximum supply of 10e27 and 18 decimals.

VoltageManager:

The VoltageManager contract is designed to manage a "voltage" system for AI Arena. The voltage system is effectively a stamina system seen in other online games. Users are given a set amount of voltage to use per day, and actions like battling consume voltage. Voltage can also be replenished through voltage batteries, and are sold through the GameItems contract. The voltage system can be extended to be used in other aspects of the Ai Arena ecosystem.

StakeAtRisk:

The StakeAtRisk contract is designed to manage the stakes that are at risk of being lost during a round of competition in Ai Arena. The contract keeps track of the stakes that fighters have at risk in a given round, allows for the reclamation of tokens if a fighter wins, and updates the records of at-risk stakes when a fighter loses. At the end of a round, it sweeps all lost stakes to a treasury address.

RankedBattle:

The RankedBattle contract defines the logic for the ranked system of Ai Arena. The contract allows users to stake their Neuron tokens. Users can stake Neuron tokens which is a condition to earn specific rewards when winning a round. Winning a round without staking increases the fighter's points, while winning a round reclaims some of the stake at risk for the user. Losing while having a positive points balance deducts points from the user, while having a zero point balance will move some of the staked Neuron to the StakeAtRisk contract.

FighterFarm:

The FighterFarm contract is an ERC-721 token used to represent the fighters themselves. The contract manages the creation, ownership, and trading of Ai Arena fighter NFTs. It allows users to mint new NFTs, claim pre-determined fighters, reroll existing fighters, and burn mint passes in exchange for fighters.

Centralization Risks:

Ai Arena is a highly centralized project in which the owners have nearly unlimited control over the execution and logic of the game. The owners control the game servers, select the winners and losers, and can replace most of the contracts with a new implementation if desired. Ai Arena is designed as a centralized system that uses blockchain for revenue generation and general distribution. User ownership of the underlying tokens is not absolute.

Due to the centralization of the project, should a trusted role become compromised, the project could suffer significant consequences. Additional information can be found in the section on Access Control - Risk Mitigation.

System Design

Modularity

AiArena uses a modular design, where individual aspects of the system are mostly contained in their own separate contracts. There is some overlap in contracts like FighterFarm.sol, but for the most part, it remains modular by design. Some contracts include logic to replace contract instances with new ones at the discretion of the _ownerAddress which allows for effectively upgrading these contracts. A word of caution to remember when switching contract instances is that all smart contracts have their own storage which will not be replicated to a new instance. This can be remediated by setting the variables in the constructor, but this can become prohibitively gas expensive. This could become an issue due to how these contracts handle state variables, which is discussed more in its own section. A more advanced implementation might include beacon proxies, which could be considered if the system grows in size and complexity. While not an ideal method for allowing upgrades, the implementation is reasonable for this project.

Testing

AiArena mostly uses unit testing with magic numbers in the testing of their system. This can lead to false confidence in the robustness of the testing. The test names are verbose and explain the purpose of the tests, although as a whole, they mainly check the surface level of the functions themselves. I would encourage extending the test suite by including tests that call the existing unit tests in different orders, as I managed to discover interesting errors without writing any additional code. I would also recommend adding more fuzz and integration tests to better discern issues in the codebase.

State

One of the most important parts of writing smart contracts, and programming in general is the management of state. Where data is stored, and how it gets updated is the majority of smart contract development. State fragmentation commonly results in bugs if the state is not synced properly between contracts. Ideally, there is a single state that is referenced by everything in the system to prevent such issues, although for some systems, this becomes impractical or infeasible. The current implementation of AiArena has excessive state fragmentation particularly in its access control, which is described in more detail in its own section. Some state variables are set in multiple contracts like roundId which then requires being updated in multiple places through cross-contract calls. As it seems some of these contracts are supposed to be updated in the future through function calls like setStakeAtRiskAddress, these state variables may need to be reinitialized during deployment becoming gas expensive or effectively impossible altogether. This is different compared to the more common proxy pattern, as the storage is kept in the proxy rather than the implementation. Overall, how AiArena handles state has considerable room for improvement.

Access Control

The project uses an ill-advised access control mechanism, which is a weakness of the project. The root of the issue is a lack of proper state management, which is evident throughout the smart contracts, and discussed more in its own section.

The project uses several access roles with control over different aspects of the system. Systems with the need for multiple levels of access control traditionally use Openzeppelin's AccessControl contract, or one of its derivatives. With the release of Openzeppelin package version 5.0, they released the AccessManager contract, a contract tailored to managing multiple contracts from a single manager contract. Either option is viable for implementation.

The Neuron contract does import Openzeppelin's AccessControl contract, although its implementation is poor. The Neuron contract does not leverage the onlyRole modifier and instead opts to use require statements to restrict access. Additionally, the implementation uses depreciated functions (_setupRole Neuron:#L95,103,111).

The ecosystem has several roles:

_ownerAddress: A psuedo role with super-admin permissions set to the initial deployer of the contract. This user is set as an admin Neuron:#L73 and can set and remove admins Neuron:#L118-121. Hopefully this is transferred to a multi-sig wallet, as its permissions are absolute.

admin: Not an admin in the traditional sense, as its permissions are somewhat limited. The admin role is not restricted to a single address, and a state with no admins can be achieved.

MINTER_ROLE: The MINTER_ROLE is in charge of minting Neuron tokens and is expected to be given to the RankedBattle contract as shown in RankedBattle.t.sol.

SPENDER_ROLE: The SPENDER_ROLE is permissioned to call the _approve function of any account for any amount of Neuron tokens. This role is expected to be given to the GameItems contract, as shown in GameItems.t.sol.

STAKER_ROLE: The STAKER_ROLE is permissioned to call the _approve function of any account for any amount of Neuron tokens. This role is expected to be given to the RankedBattle contract, as shown in RankedBattle.t.sol.

allowedVoltageSpenders: The allowedVoltageSpenders is a mapping in the VoltageManager contract that allows addresses to spend the voltage of users.

_gameServerAddress The _gameServerAddress is a single address allowed to call the updateBattleRecord function in the RankedBattle contract.

_delegatedAddress The _delegatedAddress is a single address allowed to update the TokenURI in the FighterFarm contract. It is also used in the hash for signature verification in the claimFighters function.

One major issue the current implementation is regarding state and access control. Each deployed contract keeps track of its own admin mapping, _ownerAddress, and any other included roles. An admin added to the FighterFarm contract is not also added to the Neuron contract, and vice-versa. Keeping track of which addresses have which roles in each contract is difficult, and can easily lead to instances of improper access of crucial functions.

Instead, all contracts should reference the same access control state governed by a single AccessControl or AccessManager contract. In this manner, the code for adjusting access control need not be repeated in each contract. In the current implementation there are seven instances of the following function:

    /// @notice Transfers ownership from one address to another.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newOwnerAddress The address of the new owner
    function transferOwnership(address newOwnerAddress) external {
        require(msg.sender == _ownerAddress);
        _ownerAddress = newOwnerAddress;
    }

Other functions like adjustAdminAccess are repeated several times as well. These can all be reduced to a single instance in the AccessControl contract. Regardless if the intended functionality of the contract is that admins of one contract are not admins of another contract, this same effect is easily achieved using the role-based access control offered by Openzeppelin's implementations.

The current implementation of role based access is missing a crucial component. Role-based access achieves two major purposes: separation of responsibilities, and risk mitigation.

Separation of Responsibilities

Each role should control a specific action or group of similar actions, ideally without overlap. One example of this is the MINTER_ROLE, which is required for the minting of tokens and nothing else. If an entity requires the ability to mint tokens, it must be given the MINTER_ROLE through a higher-level role like an admin, or in this case _ownerAddress. In the current implementation, there is significant overlap between the _ownerAddress and the admin role, and permissions for each seem somewhat arbitrary. There is also significant overlap in the STAKER_ROLE and the SPENDER_ROLE. Both are able to approve an arbitrary amount of Neuron tokens for any address.

Risk Mitigation

Access control allows for roles to be distinguished based on their potential impact on the protocol. High-impact roles like admins can be separated from roles like allowedVoltageSpenders, where if the former is compromised, the protocol may be irrecoverable, whereas if the latter is compromised the lasting damage to the protocol is minor or nonexistant. Wallets with high-level access to the protocol should be cold-wallets with their private key never exposed to the internet or multi-sigs, whereas private-keys of wallets with lower-level access might be used in scripts running cron jobs for instance. Both should be as secure as possible while still achieving their necessary roles. In the current implementation, all roles besides allowedVoltageSpenders, if compromised, can cause significant or permanent damage to the protocol.

Additional Recommendations

I would consider adding a method for the _gameServerAddress to update multiple battle records at a time. This can be done through a multi-call process or a custom implementation where the _gameServerAddress supplies equal length arrays of the variables in updateBattleRecord which iteratively calls the aforementioned function repeatedly. The current design has the _gameServerAddress call the updateBattleRecord function seemingly twice for each match (winner and loser), which quickly becomes infeasible as the number of matches in any given time period increases. The base gas costs of transactions will quickly scale the gas consumed, and overall, it doesn't seem feasible long term.

The current method of adding new gameItems is interesting and works reasonably well for "cosmetic" items requiring the items to be owned by an account for their effects to be experienced. However, for consumable-type items additional logic must be present in the smart contracts making them less feasible. The current implementation only considers the battery item, with the item's logic implementation found in the VoltageManager contract. I would encourage the project to plan ahead for what items may be added to the game ahead of time and consider which aspects of the code the items may interact with.

While not exactly an error in the code, a highly viable strategy seems to be to play the game without staking Neuron until the player generates enough points to comfortably stake their Neuron without risking losing any until their points buffer is exhausted. There is no penalty for losing while having no points and no Neuron staked. As there is a condition for winning a match with no Neuron staked, it is feasible that players will lose while having no points and no Neuron staked. A user can exploit this by repeatedly playing the game until they accumulate a significant point buffer before staking their Neuron tokens.

Time spent:

15 hours