AI Arena - sobieski's results

Lines of code

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

Vulnerability details

Details

FigtherFarm contract, an ERC721 token, implements custom check if the token can be transferred. This check is executed in the overridden ERC721::transferFrom() and ERC721::safeTransferFrom() methods. However, the contract does not override the 4-parameter version of ERC721::safeTransferFrom() method, which can effectively be used to bypass the check.

Impact

The FighterFarm::_ableToTransfer() method determines if the token transfer can be executed. The method is implemented as such:

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

As mentioned before, ERC721::safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) is not overridden in FighterFarm contract, which means this method can be used to bypass the _isAbleToTransfer() check.

The first check of _isAbleToTransfer() method - whether or not the caller is approved or owner of the token - is implemented in the ERC721::safeTransferFrom() method already, so it will make no difference. However, the remaining 2 checks are not implemented there. This means that the staked Fighter can still be transferred, and the MAX_FIGHTERS_ALLOWED invariant can be breached.

Effectively, this bug allows the users to perform two prohibited actions:

1. Own more than MAX_FIGHTERS_ALLOWED fighters (uncapped number of fighters), which violates game design choice
2. Transfer Fighters that have been staked upon (the ownership of the stake will also be transferred) - this can be particularly damaging as the user may transfer their Fighter without knowing that the stake will be transferred too (or not remembering about the stake). In this case the user will loose money.

Proof of Concept

In order to demonstrate the vulnerability, please make the following change to FighterFarm.t.sol::testTransferringFighterWhileStakedFails() test:

function testTransferringFighterWhileStakedFails() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        // check that i'm unable to transfer since i staked
        vm.expectRevert();
-       _fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
+       _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
}

The test will fail since the expected revert won't happen - the transfer will be successful.

Tools Used

Manual Review

Recommended Mitigation Steps

Override the ERC721:ERC721::safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) method in the FighterFarm contract:

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

Assessed type

ERC721

Lines of code

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

Vulnerability details

Details

The GameItems.sol contract is an ERC1155 token representing the in-game items. The individual item has the following attributes:

struct GameItemAttributes {
        string name;
        bool finiteSupply;
        bool transferable;
        uint256 itemsRemaining;
        uint256 itemPrice;
        uint256 dailyAllowance;
}  

One of the attributes is the transferable boolean flag, indicating whether or not this item can be transferred between users. This flag is checked inside the GameItems::safeTransferFrom() method to ensure that trying to transfer non-transferable items reverts.

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

However, even though the contract correctly overrides the ERC1155::safeTransferFrom() method to honor the flag, it does not override the ERC1155::safeBatchTransferFrom() method. It means that the non-transferable items can still be transferred with safeBatchTransferFrom() method.

Impact

The concept of non-transferable items is critical to the AI Arena economy. The transferable flag was introduced to prevent users from transferring some of the items between each other, thus enforcing them to buy/mint them directly from the GameItems contract. The possibility of bypassing the restriction allows the users to create secondary markets and trading the items. As the users may prefer to buy the items cheaper from each other, instead of minting them from the contract, it can seriously affect the economy of the Protocol and directly impact the profit it will be making.

Proof of Concept

The following test demonstrates that the transferable flag can be bypassed. Please paste it in the GameItems.t.sol test suite and run it with the following command: forge test --match-test testTransferabilityCanBeBypassed

function testTransferabilityCanBeBypassed() public {
        /* 1. Mint 1 battery to the _ownerAddress */
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);   
        /* 2. Set the battery as non-transferable */
        _gameItemsContract.adjustTransferability(0, false);
        /* 3. Test that safeTransferFrom reverts */
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        /* 4. Test that safeBatchTransferFrom still works */
        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        ids[0] = 0;
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);  
}

Tools Used

Manual Review

Recommended Mitigation Steps

Inside the GameItems.sol contract override the ERC1155::safeBatchTransferFrom() method to honor the transferable flag:

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

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L149-L163 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/RankedBattle.sol#L294-L311

Vulnerability details

Details

Winners may not be able to claim their NFTs from MergingPool contract due to the out of gas error, since the method iterates through the two-dimensional array of all the winners from every round. The same issue may occur when user will try to claim the NRN rewards from RankedBattle contract.

Impact

To claim the NFT from MergingPool, the winner has to call the MergingPool::claimRewards() method. The method is implemented as such:

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
        external 
    {
        uint256 winnersLength;
        uint32 claimIndex = 0;
        uint32 lowerBound = numRoundsClaimed[msg.sender];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (msg.sender == winnerAddresses[currentRound][j]) {
                    _fighterFarmInstance.mintFromMergingPool(
                        msg.sender,
                        modelURIs[claimIndex],
                        modelTypes[claimIndex],
                        customAttributes[claimIndex]
                    );
                    claimIndex += 1;
                }
            }
        }
        if (claimIndex > 0) {
            emit Claimed(msg.sender, claimIndex);
        }
 }

The for loops iterate through the rounds (outer loop) and winners (inner loop). The outer loop's lower bound is numRoundsClaimed[msg.sender] - the number of last round for which the user has claimed the reward.

Let's consider a scenario where the new user, who has never claimed any rewards from MintingPool, wins a round. The loop will iterate from round 0 to the current round, for each round checking all of the winners via costly operation of reading the state variable winnerAddresses. If the current round number is high enough, the method is very likely to revert with out of gas error. Therefore the winner will not be able to claim the reward.

When the round number will be high enough, the same scenario may also occur in RankedBattle::claimNRN() method here, locking the users from their NRN prizes.

Tools Used

Manual Review

Recommended Mitigation Steps

Instead of iterating through the winners from every round, add roundId as a parameter to MergingPool::claimRewards() and RankedBattle::claimNRN() methods, so that they will perform the computations only for specific round.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/70b73ce5acaf10bc331cf388d35e4edff88d4541/src/GameItems.sol#L185-L188

Vulnerability details

Details

There is no option to revoke previously granted burning privilege in the GameItems.sol contract. As the address with this privilege can burn any amount of any in-game items from an arbitrary address, not having an option to revoke the rights from a malicious/compromised address can lead to severe consequences.

Impact

In-game items, handled in the GameItems.sol contract, are essential to the Protocol's economy as the source of income. The privileged roles in the Protocol are generally handled with caution, and for every privileged role there is a mechanism to revoke it. The burner role in GameItems contract is an exception, as there is no way to revoke the previously granted privilege.

In a scenario where the address with burning privileges was compromised, a malicious actor can burn an arbitrary amount of any token from an address of choice. Effectively he will have a total control over the GameItems economy and will be able to keep burning all the tokens and DOS the contract's functionality.

Proof of Concept

Admins of the protocol can grant the burning privilege by calling the GameItems::setAllowedBurningAddress() method:

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

This role is checked inside the GameItems::burn() method:

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

There are no other restrictions to burn GameItems tokens - the address allowed inside the allowedBurningAddresses array can burn any amount of any token from anyone.

There is currently no way to revoke this privilege.

Tools Used

Manual Review

Recommended Mitigation Steps

Change the GameItems::setAllowedBurningAddress() method to accept a second parameter bool allowed, that can be used to revoke the access.

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

Assessed type

Access Control