AI Arena - 0xE1's results

Lines of code

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

Vulnerability details

Impact

Players can get unfair advantage by owning more than 10 fighters in the game, hence stake, sell, trade more and benefit from the distribution of features.

The problem was caused, because the developers forgot to override safeTransferFrom method from the ERC721.

They have a function called _ableToTransfer, which checks whether a player is able to transfer its fighter. It reverts when the balance for the recipient is 10 or more, however this function is not used when safeTransferFrom with data is used.

In the ERC721 code implementation there are 2 safeTransferFrom:

safeTransferFrom(address from, address to, uint256 tokenId) and safeTransferFrom(address from, address to, uint256 tokenId, bytes _data)

The developers have confirmed that they override the first one, but forgot to override the second one, hence users can use it to avoid the _ableToTransfer method

This error was confirmed as high by the project developers, see my gist: https://gist.github.com/Ivan-Dosev/f62488e745ba72cfb2ad2c6e041fa569

Proof of Concept

try it in RankedBattleTest

  function testSafeTransferFromWithVulnerability() public {

    address player1 = vm.addr(11);
    address player2 = vm.addr(12);

    for (uint8 i = 0; i < 10; i++) {
        _mintFromMergingPool(player1);
    }
    assertEq(_fighterFarmContract.balanceOf(player1), 10);
    assertEq(_fighterFarmContract.balanceOf(player2), 0);

   for (uint8 i = 0; i < 10; i++) {
        _mintFromMergingPool(player2);
    }
    assertEq(_fighterFarmContract.balanceOf(player2), 10);

    vm.startPrank(player1);
    vm.expectRevert();
   _fighterFarmContract.safeTransferFrom(player1, player2, 0); // this reverts because it is an override function

   _fighterFarmContract.safeTransferFrom(player1, player2, 0, ""); // however this was missed by the developers therefore it doesn't check the _ableToTransfer function hence allow players to own more than 10 players in game
   assertEq(_fighterFarmContract.balanceOf(player2), 11);
   console.log("Player1 balance after transfer", _fighterFarmContract.balanceOf(player1));

    _fighterFarmContract.safeTransferFrom(player1, player2, 1, "");
   assertEq(_fighterFarmContract.balanceOf(player2), 12);
   console.log("Player2 balance after transfer", _fighterFarmContract.balanceOf(player2));

}

Tools Used

Foundry test

Recommended Mitigation Steps

Simply, override safeTransferFrom(address from, address to, uint256 tokenId, bytes _data) like the other.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Some of the game items should not be transferrable that is why the developers have implemented a check to make sure that if the item's transferrable property is marked as false it shouldn't be able to be transferred. However the ERC1155 token code implements two functions for transferring items:

safeTransferFrom(address from, address to, uint256 id, uint256 amount, bytes data) and safeBatchTransferFrom(address from, address to, uint256[] ids, uint256[] amounts, bytes data)

The SmartContract developers override safeTransferFrom, but forgot to override the safeBatchTransferFrom, which leads to vulnerability, where players can use it to transfer game items, which shouldn't be allowed to.

According to the developers, this is a High severity, please see: https://gist.github.com/Ivan-Dosev/4a3b788272352efac0ea5d06ba498356

There will be soulbound tokens and batteries which shouldn't be transferred however this vulnerability make that possible activity.

Proof of Concept

copy this into GameItemsTest

   function testTransferingFalseItems() public {

     address player1 = vm.addr(15);
     address player2 = vm.addr(16);

    _fundUserWith4kNeuronByTreasury(player1);
    vm.prank(player1);
    _gameItemsContract.mint(0, 2); // player buys 2 batteries

    _gameItemsContract.adjustTransferability(0, false);
    (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
    assertEq(transferable, false);

    vm.prank(player1);
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(player1, player2, 0, 1, ""); // this fails because the item is not transferable
    assertEq(_gameItemsContract.balanceOf(player1, 0), 2);

    uint256[] memory ids = new uint256[](2); 
    uint256[] memory amounts = new uint256[](2); 

    ids[0] = 0; 

    amounts[0] = 1;

    vm.prank(player1);
    _gameItemsContract.safeBatchTransferFrom(player1, player2, ids, amounts, ""); // this succeeds because the requirement statements is missing
    assertEq(_gameItemsContract.balanceOf(player2, 0), 1);
}

Tools Used

Foundry test

Recommended Mitigation Steps

Override and add requirement

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Looking in Neuron contract we can see that 3 roles as Spender, Minter and Staker were distributed, however those roles can't be revoked. Consider a use case where those roles are malicious accounts and their access need to be restricted and new roles assigned. That is not possible, hence they will always posses their roles. The only chance to be revoked is to be revoked by themselves, but if they are malicious that is not going to happen.

I consider this as Medium, because at the moment those roles are assigned to smart contracts that are deployed by the developers of the protocol. However in future updates that can be ignored, hence causing some major issues in the control of the protocol.

Proof of Concept

Just look at the implementation of AccessControl.sol

Tools Used

Manual check

Recommended Mitigation Steps

Consider implementing a revocation of roles.

Assessed type

Access Control