AI Arena - Timenov's results

Lines of code

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

Vulnerability details

Impact

In the current context of the protocol, the game items are represented as ERC1155 tokens. These items have a property that determines if they are transferable. This property is set upon creation of the item and later can be changed by the owner.

    function createGameItem(
        string memory name_,
        string memory tokenURI,
        bool finiteSupply,
        bool transferable,
        uint256 itemsRemaining,
        uint256 itemPrice,
        uint16 dailyAllowance
    ) 
        public 
    {
        require(isAdmin[msg.sender]);
        allGameItemAttributes.push(
            GameItemAttributes(
                name_,
                finiteSupply,
                transferable,
                itemsRemaining,
                itemPrice,
                dailyAllowance
            )
        );
        if (!transferable) {
          emit Locked(_itemCount);
        }
        setTokenURI(_itemCount, tokenURI);
        _itemCount += 1;
    }

    function adjustTransferability(uint256 tokenId, bool transferable) external {
        require(msg.sender == _ownerAddress);
        allGameItemAttributes[tokenId].transferable = transferable;
        if (transferable) {
          emit Unlocked(tokenId);
        } else {
          emit Locked(tokenId);
        }
    }

If we take a look at the safeTransferFrom function, we can see that there is a validation if the item is transferable.

    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 problem is that there is no such validation for safeBatchTransferFrom. Therefore users can still transfer their items no matter the transferable property.

Proof of Concept

Follow the following steps:

1. Since there is no test for validating if the safeTransferFrom reverts when transferable == false, add the following test case to GameItems.t.sol:

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

        vm.prank(_ownerAddress);
        _gameItemsContract.adjustTransferability(0, false);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
    }

2. Run forge test --match-contract GameItems --match-test testSafeTransferFromRevertsIfTokenIsNotTransferable. The test passes:

[PASS] testSafeTransferFromRevertsIfTokenIsNotTransferable() (gas: 170574)

3. Add the real test case that demonstrates the attack to GameItems.t.sol:

    function testSafeBatchTransferFromBypassCheck() public {
        uint[] memory ids = new uint[](1);
        uint[] memory amounts = new uint[](1);
        ids[0] = 0;
        amounts[0] = 1;

        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);

        vm.prank(_ownerAddress);
        _gameItemsContract.adjustTransferability(0, false);

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

4. Run forge test --match-contract GameItems --match-test testSafeBatchTransferFromBypassCheck. The test passes:

[PASS] testSafeBatchTransferFromBypassCheck() (gas: 184504)

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Implement the safeBatchTransferFrom the same way as safeTransferFrom:

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override(ERC1155) {
        for (uint i = 0; i < ids.length; i++) {
            require(allGameItemAttributes[ids[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/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L139-L142 https://github.com/code-423n4/2024-02-ai-arena/blob/f2952187a8afc44ee6adc28769657717b498b7d4/src/FighterFarm.sol#L268-L276

Vulnerability details

Impact

In FighterFarm::addStaker the owner can update the hasStakerRole mapping.

    function addStaker(address newStaker) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = true;
    }

This mapping is used to check if the msg.sender has permission to update the staking status of a fighter.

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

However addStaker() can only give access and there is no way to revoke that access.

Proof of Concept

Consider the following scenario. The owner calls addStaker() with the wrong address. Now this address can change the staking status of any fighter and nobody can remove that address from the mapping.

Tools Used

Manual Review

Recommended Mitigation Steps

Add bool parameter for the access.

    function addStaker(address newStaker, bool access) external {
        require(msg.sender == _ownerAddress);
        hasStakerRole[newStaker] = access;
    }

Assessed type

Access Control

Lines of code

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

Vulnerability details

Impact

In GameItems::setAllowedBurningAddresses the admin can update the allowedBurningAddresses mapping.

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

This mapping is used to check if the msg.sender has permission to burn specific amount of game items from an account. This can happen through the burn function.

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

However setAllowedBurningAddresses() works only one way. I mean that the admin can give permission, but not revoke it.

Proof of Concept

Imagine the following scenario. Admin calls setAllowedBurningAddresses with wrong address. Now this address has the permission to burn any token from any user. Now no one can remove the wrong address from the mapping.

Tools Used

Manual Review

Recommended Mitigation Steps

Use adjustAdminAccess function as an example:

    function adjustAdminAccess(address adminAddress, bool access) external {
        require(msg.sender == _ownerAddress);
        isAdmin[adminAddress] = access;
    }

So the new setAllowedBurningAddresses function should look something like this:

    function adjustBurningAccess(address burningAddress, bool access) public {
        require(isAdmin[msg.sender]);
        allowedBurningAddresses[burningAddress] = access;
    }

Now even if the admin sets the wrong address, he can immediately change it.

Assessed type

Access Control

AI Arena QA report by Timenov

Summary

[I-01] State variables can be set in constructor.

In some contracts, state variable that are referencing other contracts are set in functions(usually their names start with instantiate). This can be removed so that the deployer will not need to call these functions on deployment, but only when these variables need to be updated. How each contract can be optimized:

• FighterFarm.sol -> Set _aiArenaHelperInstance and _neuronInstance in the constructor.
• GameItems.sol -> Set _neuronInstance in the constructor.
• MergingPool.sol -> Set _fighterFarmInstance in the constructor(this must be deployed after FighterFarm contract).
• RankedBattle.sol -> Set _neuronInstance and _mergingPoolInstance in the constructor.

The other contracts have done this already. Also consider renaming all the functions that will be affected from these changes. Currently they start with instantiate. A better naming will be set or update. All these changes will increase the code readability, remove deployment errors and cost less gas on deployment.

[I-02] Process failed transfers.

The boolean success is taken after NRN transfer. However only the true value is processed. Consider reverting when the result is false so that the users are notified when the token transfer has failed.

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

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

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

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

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

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

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

[I-03] No way to change _neuronInstance.

In all contracts that have state variable _neuronInstance that references to the Neuron contract, have functions to change the address. However in StakeAtRisk.sol this is not true. The variable there is set in the constructor and there is no other function to allow the admin to change it.

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

[L-01] Wrong ERC1155 uri.

In GameItems.sol constructor the uri value https://ipfs.io/ipfs/ is passed to the ERC1155. This does not seem to be valid uri as we can see the comment in ERC1155.sol.

    // Used as the URI for all token types by relying on ID substitution, e.g. https://token-cdn-domain/{id}.json
    string private _uri;

There is no way to change this after deployment.

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

[L-02] Function will return wrong value if item has infinite supply.

The function remainingSupply in GameItems is used to return the remaining supply of a game item.

    function remainingSupply(uint256 tokenId) public view returns (uint256) {
        return allGameItemAttributes[tokenId].itemsRemaining;
    }

This can be misleading if !allGameItemAttributes[tokenId].finiteSupply. In this case the itemsRemaining does not matter. Consider rewriting the function to avoid this type of confusion.

    function remainingSupply(uint256 tokenId) public view returns (uint256) {
        return allGameItemAttributes[tokenId].finiteSupply 
            ? allGameItemAttributes[tokenId].itemsRemaining 
            : type(uint256).max;
    }

[L-03] Mint will never get to the MAX_SUPPLY.

The mint() in Neuron.sol will not allow totalSupply == MAX_SUPPLY. This means 1 token will never be minted.

    function mint(address to, uint256 amount) public virtual {
        require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
        require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
        _mint(to, amount);
    }

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

This is because a strict check is used < instead of <=. Add the following test case to Neuron.t.sol:

    function testDoesNotMintWholeSupply() public {
        address minter = vm.addr(3);
        _neuronContract.addMinter(minter);
        uint256 difference = _neuronContract.MAX_SUPPLY() - _neuronContract.totalSupply();

        vm.prank(minter);
        vm.expectRevert("Trying to mint more than the max supply");
        _neuronContract.mint(minter, difference);
    }

Run forge test --match-contract Neuron --match-test testDoesNotMintWholeSupply. Test passes:

[PASS] testDoesNotMintWholeSupply() (gas: 41181)

AI Arena Gas report by Timenov

Summary

[G-01] Array's cached length not used optimally.

The protocol has done a good job on caching the length of an array before a loop. However there are places that this can be even more gas efficient.

    function addAttributeDivisor(uint8[] memory attributeDivisors) external {
        require(msg.sender == _ownerAddress);
+       uint256 attributesLength = attributes.length;
-       require(attributeDivisors.length == attributes.length);
+       require(attributeDivisors.length == attributesLength);

-       uint256 attributesLength = attributes.length;
        for (uint8 i = 0; i < attributesLength; i++) {
            attributeToDnaDivisor[attributes[i]] = attributeDivisors[i];
        }
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L70-L72

+       uint256 attributesLength = attributes.length;
+       uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributesLength);
-       uint256[] memory finalAttributeProbabilityIndexes = new uint[](attributes.length);

-       uint256 attributesLength = attributes.length;

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/AiArenaHelper.sol#L96-L98

+       uint256 mintPassIdsToBurnLength = mintpassIdsToBurn.length;
+       require(
+           mintPassIdsToBurnLength == mintPassDnas.length && 
+           mintPassIdsToBurnLength == fighterTypes.length && 
+           mintPassIdsToBurnLength == modelHashes.length &&
+           mintPassIdsToBurnLength == modelTypes.length
+       );
+       for (uint16 i = 0; i < mintPassIdsToBurnLength; i++) {
        
-       require(
-           mintpassIdsToBurn.length == mintPassDnas.length && 
-           mintPassDnas.length == fighterTypes.length && 
-           fighterTypes.length == modelHashes.length &&
-           modelHashes.length == modelTypes.length
-       );
-       for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {

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

+       uint256 winnersLength = winners.length;
+       require(winnersLength == winnersPerPeriod, "Incorrect number of winners");
-       require(winners.length == winnersPerPeriod, "Incorrect number of winners");
        require(!isSelectionComplete[roundId], "Winners are already selected");
-       uint256 winnersLength = winners.length;

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L120-L122

[G-02] Smaller uint can be used.

At the beggining winnersPerPeriod = 2. Later this can change, but still I think uint8 will be sufficient instead of uint256.

+       for (uint8 i = 0; i < winnersLength; i++) {
-       for (uint256 i = 0; i < winnersLength; i++) {

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

[G-03] State variable can be removed.

In RankedBattle.sol there is a state variable _stakeAtRiskAddress of type address. Its purpose is to hold the address of the StakeAtRisk contract. However there is another variable _stakeAtRiskInstance of type StakeAtRisk. They both are doing the same thing. The first one is used only once. So you can just remove it and when you need the address of the contract just cast it:

    /// The StakeAtRisk contract address.
-   address _stakeAtRiskAddress;

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

    function setStakeAtRiskAddress(address stakeAtRiskAddress) external {
        require(msg.sender == _ownerAddress);
-       _stakeAtRiskAddress = stakeAtRiskAddress;
-       _stakeAtRiskInstance = StakeAtRisk(_stakeAtRiskAddress);
+       _stakeAtRiskInstance = StakeAtRisk(stakeAtRiskAddress);
    }

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

+        bool success = _neuronInstance.transfer(address(_stakeAtRiskInstance), curStakeAtRisk);
-        bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);

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

[G-04] State variable can updated after loop.

In 2 functions, numRoundsClaimed[msg.sender] is increment by 1 every loop. This can be optimized so that the state variable is update after the loop.

        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;
                }
            }
        }
+       numRoundsClaimed[msg.sender] = roundId - 1;

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

        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            nrnDistribution = getNrnDistribution(currentRound);
            claimableNRN += (
                accumulatedPointsPerAddress[msg.sender][currentRound] * nrnDistribution   
            ) / totalAccumulatedPoints[currentRound];
-           numRoundsClaimed[msg.sender] += 1;
        }
+       numRoundsClaimed[msg.sender] = roundId - 1;

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

[G-05] Unnecessary validation.

In GameItems::mint there is a validation:

    require(
        allGameItemAttributes[tokenId].finiteSupply == false || 
        (
            allGameItemAttributes[tokenId].finiteSupply == true && 
            quantity <= allGameItemAttributes[tokenId].itemsRemaining
        )
    );

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

allGameItemAttributes[tokenId].finiteSupply is first checked for false and then for true. The second check(for true) can be removed since if the value is not false, it will be true. The require should look like this:

    require(
        allGameItemAttributes[tokenId].finiteSupply == false ||
        quantity <= allGameItemAttributes[tokenId].itemsRemaining
    );

[G-06] Operation can be skipped.

In AiArenaHelper::dnaToIndex we get the attributeProbabilityIndex.

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
        public 
        view 
        returns (uint256 attributeProbabilityIndex) 
    {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex;
    }

There are a few things that can be done. No need for return value and the break statement can be removed.

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
        public 
        view 
-       returns (uint256 attributeProbabilityIndex) 
+       returns (uint256) 
    {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
+               return i + 1;
-               attributeProbabilityIndex = i + 1;
-               break;
            }
        }
-       return attributeProbabilityIndex;
+       return 0;
    }

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L169-L186