AI Arena - shaflow2's results

Lines of code

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

Vulnerability details

Impact

Due to the lack of overloading safeBatchTransferFrom, even if an administrator sets the 'transferable' attribute of a game item to false, players can still transfer this type of game item arbitrarily using safeBatchTransferFrom.

Proof of Concept

The reason for designing a game item structure with a "transferable" attribute corresponding to an ERC1155 token is to restrict whether this type of token can be transferred to others. This design anticipates future scenarios where certain game items, such as weapons bound to a character's soul, may need to be restricted from transfer.
The "GameItems" contract overrides the safeTransferFrom function, checking whether an item is transferable before calling the parent contract's safeTransferFrom.
Github:[291]

    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId,
        uint256 amount,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }

But the ERC1155 standard also provides a batch transfer function safeBatchTransferFrom, which has not been overridden to accommodate the transferable attribute limitation.Its implementation has no effect on the implementation of the safeTransferFrom function.
GitHub:[134]

 function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

So at any time, players can transfer game items using safeBatchTransferFrom.

POC

If the game item with token ID 0 is set to non-transferable, using safeTransferFrom fails, but using safeBatchTransferFrom succeeds.

    function testSafeTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.adjustTransferability(0,false);
        _gameItemsContract.mint(0, 1);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        console.log("SafeTransferFrom Failed!");

        uint256[] memory ids = new uint256[](1);
        uint256[] memory amount = new uint256[](1);
        amount[0] =1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS,ids, amount, "");
        console.log("SafeBatchTransferFrom Success!");

        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
        console.log("State Check Success!");
    }

result

[PASS] testSafeTransferFrom() (gas: 190411)
Logs:
  SafeTransferFrom Failed!
  SafeBatchTransferFrom Success!
  State Check Success!

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.65ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual audit,foundry

Recommended Mitigation Steps

Overloading the safeBatchTransferFrom function.

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override(ERC1155) {
        require(ids.length == amounts.length);
        uint256 length = ids.length;
        for (uint256 i = 0; i < length; i++){
            if(!allGameItemAttributes[ids[i]].transferable){
                revert("restricted transfer!");
            }
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Or to directly disable safeBatchTransferFrom.

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override {
        revert("safeBatchTransferFrom is disabled in this contract");
    }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/RankedBattle.sol#L527 https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/RankedBattle.sol#L482

Vulnerability details

Impact

In general, a high stake amount implies high risk and high reward.But due to some simplified approximations in calculations, there might be a strategy allowing players to earn points in each round without financial risk.

Proof of Concept

First, The function _getStakingFactor returns a Staking factor of 1 for NRN tokens staked less than 1 ether. This implies that for token quantities ranging from 1 to 1 ether, the staking rewards would be the same under similar conditions.

Github:[527]

    function _getStakingFactor(
        uint256 tokenId, 
        uint256 stakeAtRisk
    ) 
        private 
        view 
        returns (uint256) 
    {
      uint256 stakingFactor_ = FixedPointMathLib.sqrt(
          (amountStaked[tokenId] + stakeAtRisk) / 10**18
      );
      if (stakingFactor_ == 0) {
        stakingFactor_ = 1;
      }
      return stakingFactor_;
    }

Secondly,When calculating curStakeAtRisk, stakes below 1000 will be discarded, meaning stakes below 1000 will never have funds at risk.

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

Then, when a user loses a battle, the contract checks if the fighter has any points. If points exist, regardless of whether they are sufficient to cover stakingFactor[tokenId] * eloFactor, the worst-case scenario is merely resetting the points to zero, without threatening the assets.
Github:[481]

        points = stakingFactor[tokenId] * eloFactor;
        if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
            points = accumulatedPointsPerFighter[tokenId][roundId];
        }

Thirdly, without risking funds or stakes, any battle will not affect the points.This means that once a player achieves a satisfactory amount of points, they can withdraw their stake, ensuring that their score for that round remains unchanged.
Github:[342]

    if (amountStaked[tokenId] + stakeAtRisk > 0) {
        _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
    }

Combining the three calculations mentioned above, we can deduce that:

• Before staking, try to increase your Elo ranking as much as possible.
• A player initially stakes 1 NRN (not 1 ether) for battle.(or just under 1000)
• If he wins, his points will be greater than 0. If he loses, he continue battling or repeat the battle until his points exceed 0.(stake never at risk)
• Then, the player continues to stake a large amount of NRN and participates in a battle. If he wins the battle, the player earns a substantial number of points. However, if he loses the battle, the player's assets are not at risk. He can then immediately withdraw his stake.
• Since he has already withdrawn his token stake, if he is attacked and loses the battle afterward, his points will not decrease.

Conditions for this strategy to work effectively:

• The player holds a large amount of NRN tokens. (Ensure satisfactory points are obtained in a single battle, but the square root operation may weaken this)
• The player owns multiple fighters. (allowing them to employ multiple strategies within a single round)
• The total number of points in the pool is relatively small, so the points obtained are enough to achieve satisfactory returns at the end of the round.

Restrictions:

• the strategy can only be implemented once per fight in a round. (After withdrawing the stake, it will not be possible to continue adding to the stake)So, the duration of each round is quite important. The sponsor mentioned that each round will last for one week.I think it's not long.
• The total points in the pool, Elo ratings, and other factors all influence the profitability of the strategy.

I'm not sure if the sponsor allows this scenario to happen. Although there are many restrictions, I believe it's very suitable for investors seeking stable profits, as this strategy involves no asset risk.

POC:

1. Staking 1 NRN and 1 ether of NRN results in the same point under all other identical conditions.

    function testNoStakePlayerWinBattleAndGetPoint() public {
        address player = vm.addr(3);
        address player2 = vm.addr(4);
        _mintFromMergingPool(player);
        _mintFromMergingPool(player2);
        _fundUserWith4kNeuronByTreasury(player);
        _fundUserWith4kNeuronByTreasury(player2);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(1, 0);

        vm.prank(player2);
        _rankedBattleContract.stakeNRN(1 ether, 1);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);

        _rankedBattleContract.setNewRound();
        emit log_uint(_rankedBattleContract.accumulatedPointsPerAddress(player, 0));
        emit log_uint(_rankedBattleContract.accumulatedPointsPerAddress(player2, 0));
        vm.prank(player);
        _rankedBattleContract.claimNRN();
        vm.prank(player2);
        _rankedBattleContract.claimNRN();

    }

result

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testNoStakePlayerWinBattleAndGetPoint() (gas: 1772254)
Logs:
  750
  750

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.13ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

2. under 1000 stake, never lose stake

    function testUnder1000Stake() public{
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(999, 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        console.log("stake at risk:");
        emit log_uint(_stakeAtRiskContract.stakeAtRisk(0,0));

    }

result

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testUnder1000Stake() (gas: 800974)
Logs:
  stake at risk:
  0

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

3. After a failure without points, some assets enter risk.

    function testLose() public{
        address player = vm.addr(3);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(4_000 * 10 ** 18, 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        console.log("strategyPlayer stake at risk:");
        emit log_uint(_stakeAtRiskContract.stakeAtRisk(0,0));
    }

result

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testLose() (gas: 839095)
Logs:
  strategyPlayer stake at risk:
  4000000000000000000

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

4. strategy I simulated the first round where the strategy succeeded in earning tokens, and the second round where the strategy failed, but no funds were at risk.

    function testStrategy() public{
        address strategyPlayer = vm.addr(4);
        address player = vm.addr(3);
        _mintFromMergingPool(strategyPlayer);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        _fundUserWith4kNeuronByTreasury(strategyPlayer);

        vm.prank(player);
        _rankedBattleContract.stakeNRN(4_000 * 10 ** 18, 1);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);

        vm.prank(strategyPlayer);
        _rankedBattleContract.stakeNRN(1, 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        vm.prank(strategyPlayer);
        _rankedBattleContract.stakeNRN(4_000 * 10 ** 18 - 1 , 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);
        vm.prank(strategyPlayer);
        _rankedBattleContract.unstakeNRN(4_000 * 10 ** 18,0);

        _rankedBattleContract.setNewRound();
        vm.prank(player);
        _rankedBattleContract.claimNRN();
        vm.prank(strategyPlayer);
        _rankedBattleContract.claimNRN();
        emit log_uint(_rankedBattleContract.amountClaimed(player));
        emit log_uint(_rankedBattleContract.amountClaimed(strategyPlayer));

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);


        vm.prank(strategyPlayer);
        _rankedBattleContract.stakeNRN(1, 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        vm.prank(strategyPlayer);
        _rankedBattleContract.stakeNRN(4_000 * 10 ** 18 - 1 , 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        vm.prank(strategyPlayer);
        _rankedBattleContract.unstakeNRN(4_000 * 10 ** 18,0);
        console.log("strategyPlayer stake at risk:");
        emit log_uint(_stakeAtRiskContract.stakeAtRisk(1,1));
    }

result

Logs:
  2480314960629921259842
  2519685039370078740157

Points fix by stakingFactor[tokenId] * eloFactor
  94500
  strategyPlayer stake at risk:
  0

Tools Used

Manual audit,foundry

Recommended Mitigation Steps

• Consider setting a minimum stake amount.
• Consider setting some assets as "at risk" when points are insufficient for payment.Instead of simply clearing insufficient points, consider marking some assets as "at risk."

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/befe84a9ddccba0cd0251082046e57ecabffb0cf/src/Neuron.sol#L93 https://github.com/code-423n4/2024-02-ai-arena/blob/befe84a9ddccba0cd0251082046e57ecabffb0cf/src/Neuron.sol#L101 https://github.com/code-423n4/2024-02-ai-arena/blob/befe84a9ddccba0cd0251082046e57ecabffb0cf/src/Neuron.sol#L109

Vulnerability details

Impact

The contract lacks admin roles for MINTER_ROLE, STAKER_ROLE, and PENDER_ROLE, which means roles cannot be revoked by anyone other than themselves. Due to the absence of a function in the role contracts to relinquish privileges, the old privileged roles cannot be revoked.If these roles are compromised, further losses cannot be prevented by revoking their authorization.

Proof of Concept

First of all, I want to say that roles such as MINTER_ROLE and STAKER_ROLE in the NRN token contract have significant permissions. They can control any account to perform approve operations on themselves, thus transferring their tokens.

Github:[171]

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

    /// @notice Approves the specified amount of tokens for the staker address.
    /// @dev The caller must have the staker role.
    /// @param owner The owner of the tokens.
    /// @param spender The address for which to approve the allowance.
    /// @param amount The amount of tokens to be approved.
    function approveStaker(address owner, address spender, uint256 amount) public {
        require(
            hasRole(STAKER_ROLE, msg.sender), 
            "ERC20: must have staker role to approve staking"
        );
        _approve(owner, spender, amount);
    }

The NRN contract inherits the AccessControl contract, in which three functions are implemented to add these roles. They can only be called by the owner.

Github:[101]

    /// @notice Adds a new address to the minter role.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newMinterAddress The address to be added as a minter
    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

    /// @notice Adds a new address to the staker role.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newStakerAddress The address to be added as a staker
    function addStaker(address newStakerAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(STAKER_ROLE, newStakerAddress);
    }

    /// @notice Adds a new address to the spender role.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newSpenderAddress The address to be added as a spender
    function addSpender(address newSpenderAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(SPENDER_ROLE, newSpenderAddress);
    }

It is important to note that when the owner calls these functions again to add new roles, the existing roles will not be overwritten, and their permissions will not be revoked.Because it only sets the relevant mapping for the new address to true.

    function _grantRole(bytes32 role, address account) internal virtual {
        if (!hasRole(role, account)) {
            _roles[role].members[account] = true;
            emit RoleGranted(role, account, _msgSender());
        }
    }

How can authorization for the original role be revoked?

Upon observing the NRN contract, I noticed that it does not encapsulate the _setRoleAdmin(bytes32 role, bytes32 adminRole) function for administrator setting, resulting in the inability to add administrators, and the owner is not the default_admin_role either.
Therefore, others cannot revoke authorization for roles that have already been granted. The only way to revoke authorization is for these roles to call the renounceRole method themselves.
However, these contract roles such as GameItems.sol, FighterFarm.sol, and RankedBattle.sol do not implement the relevant functions, preventing authorized addresses from revoking authorization.

POC:

    function testRoleRevork() public {
        
        _neuronContract.addSpender(_DELEGATED_ADDRESS);
        assertEq(_neuronContract.hasRole(keccak256("SPENDER_ROLE"), _DELEGATED_ADDRESS), true);

        address newSpenderAddress = makeAddr("newSpenderAddress");
        _neuronContract.addSpender(newSpenderAddress);
        assertEq(_neuronContract.hasRole(keccak256("SPENDER_ROLE"), newSpenderAddress), true);
        assertEq(_neuronContract.hasRole(keccak256("SPENDER_ROLE"), _DELEGATED_ADDRESS), true);

        vm.expectRevert();
        _neuronContract.revokeRole(keccak256("SPENDER_ROLE"),_DELEGATED_ADDRESS);
        
    }

result

Running 1 test for test/Neuron.t.sol:NeuronTest
[PASS] testRoleRevork() (gas: 101536)
Traces:
  [101536] NeuronTest::testRoleRevork()
    ├─ [27180] Neuron::addSpender(0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0)
    │   ├─ emit RoleGranted(role: 0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, account: 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0, sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [721] Neuron::hasRole(0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0) [staticcall]
    │   └─ ← true
    ├─ [0] VM::addr(<pk>) [staticcall]
    │   └─ ← newSpenderAddress: [0xFB497e8C257f47A0623E72e5430753E4DFb0643B]
    ├─ [0] VM::label(newSpenderAddress: [0xFB497e8C257f47A0623E72e5430753E4DFb0643B], newSpenderAddress)
    │   └─ ← ()
    ├─ [25180] Neuron::addSpender(newSpenderAddress: [0xFB497e8C257f47A0623E72e5430753E4DFb0643B])
    │   ├─ emit RoleGranted(role: 0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, account: newSpenderAddress: [0xFB497e8C257f47A0623E72e5430753E4DFb0643B], sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [721] Neuron::hasRole(0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, newSpenderAddress: [0xFB497e8C257f47A0623E72e5430753E4DFb0643B]) [staticcall]
    │   └─ ← true
    ├─ [721] Neuron::hasRole(0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0) [staticcall]
    │   └─ ← true
    ├─ [0] VM::expectRevert()
    │   └─ ← ()
    ├─ [34479] Neuron::revokeRole(0x7434c6f201a551bfd17336985361933e0c4935b520dac8a49d937b325f7d5c0a, 0x22F4441ad6DbD602dFdE5Cd8A38F6CAdE68860b0)
    │   └─ ← "AccessControl: account 0x7fa9385be102ac3eac297483dd6233d62b3e1496 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000"
    └─ ← ()

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

Tools Used

Manual audit,foundry

Recommended Mitigation Steps

The most convenient method is to add the owner as the default_admin_role role. If you want to implement more complex management permissions, please read the relevant comments in the AccessControl contract.

    constructor(address ownerAddress, address treasuryAddress_, address contributorAddress)
        ERC20("Neuron", "NRN")
    {
        _ownerAddress = ownerAddress;
        treasuryAddress = treasuryAddress_;
        isAdmin[_ownerAddress] = true;
        _setupRole(DEFAULT_ADMIN_ROLE, ownerAddress);
        _mint(treasuryAddress, INITIAL_TREASURY_MINT);
        _mint(contributorAddress, INITIAL_CONTRIBUTOR_MINT);
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

Due to the assets in stake at risk not being considered, in certain cases, the GameServer calling the updateBattleRecord function may revert.

Proof of Concept

A fighter can only be transferred to another person when it has not been staked with NRN.
When we call the stakeNRN function of the RankedBattle contract for the first time in each round, it will change the FighterStaking status of the fighter to true.
Github:[253]

if (amountStaked[tokenId] == 0) {
    _fighterFarmInstance.updateFighterStaking(tokenId, true);
}

Github:[285] We call the unstakeNRN function to change the FighterStaking status of the fighter to false and enable the transfer to another person when amountStaked[tokenId] equals 0.

    if (amountStaked[tokenId] == 0) {
        _fighterFarmInstance.updateFighterStaking(tokenId, false);
    }

However, the fighter may have assets at risk.
In that case, if the fighter is transferred to another person, then in the same round, the fighter's battle will still proceed to settle in the _addResultPoints function.
Github:[342]

    if (amountStaked[tokenId] + stakeAtRisk > 0) {
        _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
    }

In this scenario, draws and losses in battle results can be updated normally, but victorious battle results will be revert.
Because the amountLost in the StakeAtRisk contract may leading to underflow and causing the transaction to revert.
Github:[102]

    stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
    totalStakeAtRisk[roundId] -= nrnToReclaim;
    amountLost[fighterOwner] -= nrnToReclaim;

This issue will disappear once the next round begins.

POC: When player1 stakes the token with token ID = 0, and some assets of fighter are at risk, then player1 revokes all stakes and transfers the fighter to player2 (assuming player2 purchases the fighter), all the combat victories updates of player2 in this turn would be reverted. However, losing and tie battles can be updated normally.

    function testRevertupdateBattleRecord() public{
        address player = vm.addr(3);
        address player2 = vm.addr(4);
        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        _fundUserWith4kNeuronByTreasury(player2);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
        vm.prank(player);
        _rankedBattleContract.unstakeNRN(3_000 * 10 ** 18,0);
        console.log("token id 0 some at risk player1 unstakeNRN transfer to player2:");
        vm.prank(player);
        _fighterFarmContract.safeTransferFrom(player, player2, 0);  

        console.log("If player2 win,updateBattleRecord will be revert");
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);  

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);

        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 1, 1500, true);

    }

result

Running 1 test for test/RankedBattle.t.sol:RankedBattleTest
[PASS] testRevertupdateBattleRecord() (gas: 1009702)
Logs:
  token id 0 some at risk player1 unstakeNRN transfer to player2:
  If player2 win,updateBattleRecord will be revert

Tools Used

Manual audit,foundry

Recommended Mitigation Steps

• Method one. Consider stake-at-risk assets when changing the fighter's staking status again.

    if (
        amountStaked[tokenId] == 0 &&
    +     _stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0
        ) {
            _fighterFarmInstance.updateFighterStaking(tokenId, false);
    }

The limitation of this modification is that the player wishing to transfer the fighter must call unstakeNRN again after the start of the new round if his fighter has NRN at risk in this round.And the transferred fighter cannot participate in staking during new round.They need to wait for one more round.

• Method two.

Adding condition restrictions to the transfer, but it will require significant changes.

Change in FighterFarm.sol

    + mapping(uint256 => bool) public stakeAtRisk;

    + function updateStakingAtRisk(uint256 tokenId, bool stakingStatus) external {
    +     require(hasStakerRole[msg.sender]);
    +     stakeAtRisk[tokenId] = stakingStatus;
    + }

    function _ableToTransfer(
        uint256 tokenId,
        address to
    ) private view returns (bool) {
        return (_isApprovedOrOwner(msg.sender, tokenId) &&
            balanceOf(to) < MAX_FIGHTERS_ALLOWED &&
            !fighterStaked[tokenId] 
    +        &&!stakeAtRisk[tikenId]);
    }

Change in RankedBattle.sol

    
    /// If the user has stake-at-risk for their fighter, reclaim a portion
    /// Reclaiming stake-at-risk puts the NRN back into their staking pool
    if (curStakeAtRisk > 0) {
        _stakeAtRiskInstance.reclaimNRN(
            curStakeAtRisk,
            tokenId,
            fighterOwner
        );
        amountStaked[tokenId] += curStakeAtRisk;
    }
    + if (_stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0) {
    +     _fighterFarmInstance.updateStakingAtRisk(tokenId, false);
    + }

    else {
        /// If the fighter does not have any points for this round, NRNs become at risk of being lost
            bool success = _neuronInstance.transfer(
            _stakeAtRiskAddress,
                curStakeAtRisk
            );
    +       if (_stakeAtRiskInstance.getStakeAtRisk(tokenId) == 0) {
    +           _fighterFarmInstance.updateStakingAtRisk(tokenId, true);
    +       }
            if (success) {
                 _stakeAtRiskInstance.updateAtRiskRecords(
                    curStakeAtRisk,
                    tokenId,
                    fighterOwner
                );
                amountStaked[tokenId] -= curStakeAtRisk;
    
            }
    }

Assessed type

Other

Low[1] - "totalBattle" does not accurately represent the number of battles.

Impact

"totalBattle" represents not the correct total number of battles, but twice the total number of battles.

Proof of Concept

When a battle is initiated, the gameServer will invoke the updateBattleRecord function twice: once to update the relevant information of the battle initiator, and once to update the information of the opponent in the battle.

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        require(msg.sender == _gameServerAddress);
        require(mergingPortion <= 100);
        address fighterOwner = _fighterFarmInstance.ownerOf(tokenId);
        require(
            !initiatorBool ||
            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
        );

        _updateRecord(tokenId, battleResult);
        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
        }
        totalBattles += 1;
    }

The statement totalBattles += 1 will be executed twice.
So, totalBattles will be twice the correct number of battles.

Tools Used

Manual audit

Recommended Mitigation Steps

Move the statement totalBattles += 1; inside the if (initiatorBool) { }block.

        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
            totalBattles += 1;
        }

Low[2] - The "attributeProbabilities" is initialized twice in the constructor function.

Impact

In the AiArenaHelper contract, the "attributeProbabilities" is initialized redundantly.

Proof of Concept

In the constructor of the AiArenaHelper contract, the function addAttributeProbabilities(0, probabilities) is called, initializing the attributeProbabilities array. However, within the loop, the addAttributeProbabilities array is redundantly initialized again. Github:[44]

   // Initialize the probabilities for each attribute
    addAttributeProbabilities(0, probabilities);

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

    function addAttributeProbabilities(uint256 generation, uint8[][] memory probabilities) public {
        require(msg.sender == _ownerAddress);
        require(probabilities.length == 6, "Invalid number of attribute arrays");

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

Tools Used

Manual audit

Recommended Mitigation Steps

Please choose to remove either addAttributeProbabilities(0, probabilities) or attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i];
I suggest maintaining a consistent style.
Change addAttributeDivisor to public. Then:

    constructor(uint8[][] memory probabilities) {
        _ownerAddress = msg.sender;
        // Initialize the probabilities for each attribute
        addAttributeProbabilities(0, probabilities);
        addAttributeDivisor(defaultAttributeDivisor);
    } 

or

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

Low[3] - Consider using a two-step transfer for ownership

Impact

A copy-paste error or a typo could potentially replace the owner with an address not controlled by the sponsor, resulting in the sponsor losing some control over the contract's permissions.

Proof of Concept

In the robot competition, a question was raised suggesting a two-step update for the protocol address, but it did not mention updating the owner.
A copy-paste error or a typo could potentially replace the owner with an address not controlled by the sponsor, resulting in the sponsor losing some control over the contract's permissions.

Tools Used

Manual audit

Recommended Mitigation Steps

Consider implementing a two-step procedure for updating protocol addresses, where the recipient is set as pending, and must 'accept' the assignment by making an affirmative call.

L[4] - Once set, the allowedBurningAddresses cannot be revoked.

Impact

The authorized Burning Addresses have significant permissions, enabling them to potentially destroy GameItems from any address. Once an authorized contract is attacked, the destruction cannot be prevented.

Proof of Concept

The contract can only permit Burning Addresses and cannot be revoked. Once a contract is authorized and subsequently attacked, it cannot be stopped. GitHub:[185]

    /// @notice Sets the allowed burning addresses.
    /// @dev Only the admins are authorized to call this function.
    /// @param newBurningAddress The address to allow for burning.
    function setAllowedBurningAddresses(address newBurningAddress) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = true;
    }

    function burn(address account, uint256 tokenId, uint256 amount) public {
        require(allowedBurningAddresses[msg.sender]);
        _burn(account, tokenId, amount);
    }

Tools Used

Manual audit

Recommended Mitigation Steps

    function setAllowedBurningAddresses(address newBurningAddress,bool state) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[newBurningAddress] = state;
    }

N[1]- Consider implementing more flexible management for GameItems.

Once a GameItem is added, it cannot be deleted, and only the transferability of the game item can be modified by administrators. Other attributes such as daily supply cannot be changed. Please consider flexible management for GameItem and its supply.

For instance, administrators could update the dailyAllowance and itemPrice based on the actual gaming circumstances.

Github:[208]

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 
        public 
    {
        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
                transferable,
                itemsRemaining,
                itemPrice,
                dailyAllowance
            )
        );
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

N[2] Potential overflow risk.

The contract uses totalNumTrained to record the total number of training sessions for all fighters. Its type is uint32.
Each owner of a fighter can update its model by calling the updateModel function at any time, without any restrictions on the number of updates or time limits.
If maliciously attacked, there is a risk of overflow for totalNumTrained. In that case, all fighters will be unable to call updateModel to update ModelType and ModelHash.

    /// @notice Aggregate number of training sessions recorded.
    uint32 public totalNumTrained;

 function updateModel(
        uint256 tokenId,
        string calldata modelHash,
        string calldata modelType
    ) external {
        require(msg.sender == ownerOf(tokenId));
        fighters[tokenId].modelHash = modelHash;
        fighters[tokenId].modelType = modelType;
        numTrained[tokenId] += 1;
        totalNumTrained += 1;
    }

Analysis Report

Table of Contents

• Analysis of the codebase
• Architecture feedback
• Centralization risk
• Systemic risks
• Other recommendations
• Audit Process

Analysis of the codebase <a id="analysis-of-the-codebase"></a>

Overview

The entire codebase predominantly utilizes imports instead of inheritance, enabling most instance contracts to change instance addresses through specific permission roles, thereby enhancing the system's upgradability. Below, I will describe some notable aspects of the code:

• Neuron.sol
  The Neuron contract serves as the native ERC20 token for the entire system, utilizing AccessControl for role management. Certain roles are granted the authority to transfer NRN tokens to any address. These roles' owners are audited contracts.

• AiArenaHelper.sol, MergingPool.sol, FighterFarm.sol, FighterOps.sol
  These contracts define the minting and circulation rules for fighter NFTs. Each minted NFT corresponds to a fighter struct. Since the contracts do not provide a burn function, minted fighters cannot be destroyed but can be transferred to other addresses under specific conditions. Additionally, the sponsors opted out of using Chainlink VRF and instead chose to control fighter attributes using predictable pseudo-random numbers during minting. When asked about this decision, the sponsors explained that true randomness is not crucial for their game as training is the most significant factor determining outcomes. They also mentioned that after testing the game for about 2.5 years on the testnet, players did not actually care about true randomness. Regarding the NFT lottery controlled by MergingPool, the contract only records points and lottery results, while the complex lottery calculation process is handled off-chain. This design is deemed reasonable as it executes only critical parts on-chain, reducing certain risks and on-chain computational burdens. However, it sacrifices some transparency in the lottery. Winners claim their NFTs by calling relevant functions themselves, rather than the contract actively distributing them, effectively preventing malicious DOS attacks.

• RankedBattle.sol, StakeAtRisk.sol, GameItems.sol, VoltageManager.sol
  These contracts govern the rules related to game battles and game items. Similar to the lottery, RankedBattle only records battle results and changes in staking status. Game outcomes are predicted using off-chain gaming oracles. GameItems utilize ERC1155, where each token corresponds to a GameItems struct, and the struct's information determines the token's supply rules.

Architecture feedback <a id="architecture-feedback"></a>

The entire contract system is divided into two modules: fighter acquisition (NFT minting) and staking battles.

Fighter Acquisition:

• There are three methods to mint fighter NFTs:
  1. Minting through off-chain acquisition of signature and minting information provided by sponsors, followed by calling claimFighters.
  2. Minting by redeeming AAMintPass NFTs through redeemMintPass.
  3. Obtaining through MergingPool lottery.
• Note: If the recipient's total number of fighters is less than the maximum quantity, and the fighters to be sent are not staked, fighters can be transferred to other addresses.

Battle:

1. Administrators can advance to the next round by calling setNewRound, provided that the current round has accumulated points.
2. During a round, players can stake NRN without limitation (stakeNRN) until they cancel their stake (unstakeNRN).
3. After a battle concludes, the gameserver invokes updateBattleRecord twice to update information regarding both the initiator and the target of the battle. If there is no stake, it does not affect points; however, if there is a stake, it affects both points and staked assets. If the performance is poor, a portion of the assets will become "at risk," and to earn points in subsequent battles, the player must first redeem all at-risk assets through victories in battles. Only then will victories in subsequent battles increase points.
4. After a round ends, all at-risk assets are transferred to the treasury. Owners of fighters with points can call claimNRN to claim NRN from the prize pool based on the total points ratio.

However, I discovered a high-risk vulnerability in the GameItems contract.The contract does not override the safeBatchTransferFrom function, allowing players to freely transfer their game items regardless of whether they are transferable.

Centralization risk <a id="centralization-risk"></a>

Every major contract has an _ownerAddress to wield supreme authority, including updating protocol addresses, changing contract states, and assigning admin roles downward.

A. transferOwnership is provided to transfer _ownerAddress, but a one-step transfer of ownership could pose risks. A copy-paste error or a typo could potentially replace the owner with an address not controlled by the sponsor, resulting in the sponsor losing some control over the contract's permissions. This is particularly serious due to the extensive permissions held by the owner, including:

1. Loss of the ability to update contract instance addresses, which would affect system upgrades.
2. Loss of the ability to set certain basic calculation parameters, such as attributeDivisors and attributeProbabilities.
3. Loss of the ability to update delegated permission roles, which are generally contracts necessary for inter-contract interactions, such as adjustAllowedVoltageSpenders in the VoltageManager contract. This further impacts the system's upgradability.
4. If admin permissions are not delegated in RankedBattle, loss of owner control could prevent rounds from progressing.

B. Regarding other admins below the owner:

• Admins in RankedBattle could potentially be externally owned accounts (EOA) used to advance rounds and set prize pools.
• Admins in MergingPool could potentially be EOAs used to update the number of winners per round and update the list of winners.
• Admins in createGameItem could potentially be EOAs used to create and manage game items.
  Malicious behavior by these admins could lead to:

1. Incorrect progression of rounds in the battle contract, such as initiating battles to gain points at the start of a new round when the total points are still low, or setting a large total prize pool maliciously to gain a large amount of NRN.
2. Malicious adjustment of the prize pool at the end of a battle round, potentially resulting in players not receiving the expected amount of NRN rewards.
3. Malicious tampering with the list of winners in the MergingPool contract.
4. Malicious creation of useless game items that cannot be revoked (as the GameItems struct array can only add elements and not delete them).
   Other admins or roles with special permissions should ideally be contracts within the system necessary to ensure normal functioning and interactions between different parts of the system. For instance, adjustAllowedVoltageSpenders in the VoltageManager contract should be set to the RankedBattle contract.

Systemic risks <a id="systemic-risks"></a>

• I believe the most significant systemic potential risk lies within Neuron, which serves as the native token of the entire system, but lacks ROLE management. Details are as follows:

  • In the Neuron contract, the owner can assign SPENDER_ROLE, STAKER_ROLE, and MINTER_ROLE. Among these, SPENDER_ROLE and STAKER_ROLE have immense control over tokens for any address. However, due to the absence of an ADMIN_ROLE function, these roles cannot be effectively managed. Once assigned, only the roles themselves can relinquish their permissions, and newly added contract privilege roles do not override old ones. Consequently, even _ownerAddress cannot control them to revoke permissions.
  • Although the sponsors mentioned that the contracts granted with privilege roles are audited and trustworthy, it is well-known that even rigorously audited contracts may still contain vulnerabilities. If exploited, these vulnerabilities could have catastrophic effects on Neuron (SPENDER_ROLE and STAKER_ROLE having control over NRN tokens for any address). However, there is no method to revoke their permissions to terminate their influence on the system. Additionally, I observed that the contract roles that have been confirmed (FighterFarm.sol, GameItems.sol, RankedBattle.sol) do not encapsulate the renounceRole(bytes32 role, address account) function, so they themselves cannot relinquish ROLE privileges.
  • Consider implementing ADMIN_ROLE to address this issue.

• Secondly, the Admin privileges at the bottom of the Battle contract are extensive and may lead to a DOS attack on the staking reward system.

  • Firstly, Battle mints new NRN tokens as rewards for each round. However, it is important to note that when the total supply exceeds MAX_SUPPLY, the mint function will stop minting. Therefore, an admin could maliciously set the staking rewards for a round to exceed the total supply, allowing players in that round to receive a large amount of NRN, preventing subsequent round players from successfully receiving their rewards. This situation can only be rectified by the treasury or other roles burning their NRN. Alternatively, incentivizing players who receive a large amount of NRN to destroy unlawfully obtained NRN is difficult.
  • Consider setting a maximum reward per round, which only the owner can modify, to prevent such occurrences.

• some parameters in the contracts are hardcoded, reducing contract upgradability. While system upgrades can be accomplished by changing contract addresses, during upgrades, not only must the contract addresses be changed, but also the settings of some contract privilege roles, which is complex and prone to oversight.

  • Consider adopting a proxy contract pattern.

• Lastly, In GameItems contract, once set, the allowedBurningAddresses cannot be revoked.The authorized Burning Addresses have significant permissions, enabling them to potentially destroy GameItems from any address. Once an authorized contract is attacked, the destruction cannot be prevented.Please endeavor to avoid this pattern as much as possible.

Other recommendations <a id="other-recommendations"></a>

• In the staking system, under similar conditions, staking amounts ranging from 1 to 1 ether yield the same number of points. For stakes below 1000 NRN, due to precision issues with division, NRN will not be at risk even when their points are 0.Additionally, regardless of how low the points are, they can offset any amount of asset loss, which could potentially be exploited. Further details are available in my other issue report.

• Due to the omission of the NRN at risk part, certain situations may cause updateBattleRecord to revert. Further details are available in my other issue report.

• I believe the system's management of GameItems lacks flexibility. Once a game item is added, it cannot be deleted, and none of its attributes other than "transferable" can be modified. However, the system is subject to change, and the data set during addition, such as supply and price, may become obsolete later. Please consider allowing administrators to set new dailyAllowance, itemPrice, or even remove game items based on the system's actual needs.

• There are some minor errors in the contracts. For instance, the view function (getFighterPoints) does not initialize the array length correctly, attributeProbabilities are initialized twice in the constructor, and totalBattle fails to accurately represent the total number of battles.

Audit Process <a id="audit-process"></a>

I spent five days auditing this code repository. With the addition of the report writing time, it may take up to five days.

I learned about the overall functionality of the project from the readme file, divided the contract into functional modules, and read the complete code in the order of Neuron -> (AiArenaHelper, FighterFarm, FighterOps) -> (GameItems, VoltageManager) -> (RankedBattle, StackAtRisk, MergingPool).

After the first reading, I marked my existing problems and some minor findings. Afterwards, I rechecked the various functions of each contract and asked sponsors some questions. Some of the questions were confirmed and written into a report.

Some questions I asked

1. Question: Hello, I have a question about why the contract abandoned VRF and used pseudo-random numbers. Is it because the importance of elements, weight, and Physical Attribute has decreased, so predicting the forging results in advance is encouraged?

   • Answer: Yea it’s not important in our game in terms of getting an advantage. By far the most important determinant of the outcome is training. The elements and weights simple affect the way you train and play style. Also we’ve been testing the game for about 2.5 years on testnet and players don’t actually care about true randomness.

2. Question: I'm wondering if I can find out how much NUN players typically stake during testing.

   • Answer: Up to 10k.

3. Question: Is it allowed for a fighter to withdraw their stake once they have achieved their satisfactory points, resulting in their points being locked in this round? In this way, they won't lose points even if they fail the attack battle.

   • Answer: Yes, this is allowed.

4. Question: I think initiating a battle, which the gameserver calls the updateBattleRecord function twice, so totalBattles will increase twice. Is this considered an issue?

   • Answer: Great catch - yes, this is an issue.

Thank you to @ guppy and @ Sonny for their careful answers.

Time spent:

45 hours