AI Arena - agadzhalov's results

Lines of code

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

Vulnerability details

[M-2] Winner from MergingPool can pass any amount of weight custom attribute to _createNewFighter

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

Summary

Players are able to divert part of their points to MergingPool and potentially earn a new fighter NFT. There's withdrawal mechanism for a player to claim his prize figther NFT by calling claimRewards(). A player who claims his prize can easily pass any number for the weight custom attribute if he passes element custom attribute less than 100 to bypass validation in FighterFarm._createNewFighter().

Details

A player who claims his prize can easily pass customAttribute = [99, type(uint256).max]. Passing element of 99 would bypass the validation if (customAttributes[0] == 100) executing the else logic and set the weight to his desirable amount. https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L499

if (customAttributes[0] == 100) {
       (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else { // it will eneter here 
        element = customAttributes[0];
        weight = customAttributes[1]; // any number up to type(uint256).max
        newDna = dna;
    }

PoC

function testClaimRewardsForWinnerWithAnyWeight() public {
        address playerOne = makeAddr("playerOne");
        address playerTwo = makeAddr("playerTwo");
        _mintFromMergingPool(playerOne);
        _mintFromMergingPool(playerTwo);
        assertEq(_fighterFarmContract.ownerOf(0), playerOne);
        assertEq(_fighterFarmContract.ownerOf(1), playerTwo);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);
        assertEq(_mergingPoolContract.isSelectionComplete(0), true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 0) == playerOne, true);
        assertEq(_mergingPoolContract.winnerAddresses(0, 1) == playerTwo, true);

        string[] memory _modelURIs = new string[](2);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](2);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](2);
        _customAttributes[0][0] = uint256(1);
        // Claimer can pass any value up to type(uint256).max
        _customAttributes[0][1] = UINT256_MAX;
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = UINT256_MAX;

        // claimRewards can be executed by any user
        vm.prank(playerOne);
        // playerOne calaims his reward
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        // Get the first of the claimed NFTs and checks its weight is type(uint256).max
        uint256 indexOfClaimedNft = 2;
        (address a ,uint256[6] memory b, uint256 weight, uint256 element,string memory c,string memory d,uint16 e) = _fighterFarmContract.getAllFighterInfo(indexOfClaimedNft);
        assertEq(UINT256_MAX, weight);
        assertEq(_fighterFarmContract.ownerOf(indexOfClaimedNft), playerOne);
    }

How to run: Copy and paste the test into MergingPool.t.sol and execute forge test --match-test testClaimRewardsForWinnerWithAnyWeight -vvv

Impact

Player can gain unfair advantege over other players with fighters with less weight.

Recommended Mitigation Steps

Implement validation in _createNewFighter for checking maximum of weight custom attribute.

Assessed type

Invalid Validation

[QA-1] attributeProbabilities is updated twice

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

Summary

In constructor attributeProbabilities are updated twice. Once by calling addAttributeProbabilities(0, probabilities) and once by implementing for loop.

Details

constructor(uint8[][] memory probabilities) {
        _ownerAddress = msg.sender;

        // Initialize the probabilities for each attribute
->        addAttributeProbabilities(0, probabilities); // first update is here

        uint256 attributesLength = attributes.length;
->        for (uint8 i = 0; i < attributesLength; i++) { // second update is here
            attributeProbabilities[0][attributes[i]] = probabilities[i];
->            attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i]; // this is missed in addAttributeProbabilities()
        }
    } 

Recommended Mitigation Steps

Update attributeProbabilities only once by calling addAttributeProbabilities(0, probabilities); and most likely also implementing attributeToDnaDivisor[attributes[i]] = defaultAttributeDivisor[i] in addAttributeProbabilities()

[QA-2] attributesLength is uint256 and i is uint8

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

Summary

attributesLength is from type uint256 and i is from type uint8.

Details

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

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

PoC

function testArr() public view {
        uint256 l = 256;
        vm.expectRevert()
        for(uint8 i=0; i < l; i++) {}
    }

Impact

In case attributesLength is bigger than 255 the method will revert

Recommended Mitigation Steps

Keep attributesLength and i from the same type or use casting.

[QA-3] Type used for generation is not consistent

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

Summary

generation is set to be of type uint256 and there are multiple instances where generation is used as uint8

/// @notice Mapping tracking fighter generation to attribute probabilities
mapping(uint256 => mapping(string => uint8[])) public attributeProbabilities;

PoC

-> function deleteAttributeProbabilities(uint8 generation) public { //@audit-issue QA generation here is also uint8
        require(msg.sender == _ownerAddress); //@ok

        uint256 attributesLength = attributes.length; //@ok
        for (uint8 i = 0; i < attributesLength; i++) { //@ok
            attributeProbabilities[generation][attributes[i]] = new uint8[](0);
        }
    }

It doesn't make sense to have generation of uint256 and then pass only uint8 to get attributeProbabilities[generation].

Recommended Mitigation Steps

Keep generation from the same type or use casting.

[QA-4] Passing an array of uint256 to a loop iterating only to type(uint16).max

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

Summary

Array of is passed to In case the array is bigger consists of elements more than type(uint16).max method would revert.

Impact

Revert

[QA-5] Misleading comment

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

Summary

Mapping is not of address but of tokenId.

-> /// @notice Mapping of address to fighter points.
mapping(uint256 => uint256) public fighterPoints;

Recommended Mitigation Steps

Changed comment Mapping of address to fighter points. -> Mapping of tokenId to fighter points.

[QA-5] Refactor comment docs for getUnclaimedRewards

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

Summary

It's not getting the unclaimed rewards, but it gets the amount (uint256) of unclaimed rewards. Also the method is view and users can mistake it for a method that changes the state.

->  /// @notice Gets the unclaimed rewards for a specific address.
    /// @param claimer The address of the claimer.
    /// @return numRewards The amount of unclaimed fighters.
->    function getUnclaimedRewards(address claimer) external view returns(uint256) {
        uint256 winnersLength;
        uint256 numRewards = 0;
        uint32 lowerBound = numRoundsClaimed[claimer];
        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            winnersLength = winnerAddresses[currentRound].length;
            for (uint32 j = 0; j < winnersLength; j++) {
                if (claimer == winnerAddresses[currentRound][j]) {
                    numRewards += 1;
                }
            }
        }
        return numRewards;
    }

Recommended Mitigation Steps

Use better naming for both method name and refactor comment to be more meaninfut that actually we retrieve the number of unclaimed rewards.

[QA-6] Functionality doesn't work as explained docs/comments

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/Neuron.sol#L62

Summary

This comment is wrong. The roles are not granted by the constructor but by methods like addMinter, addStaker, addSpender etc

->  /// @notice Grants roles to the ranked battle contract. 
    /// @notice Mints the initial supply of tokens.
    /// @param ownerAddress The address of the owner who deploys the contract
    /// @param treasuryAddress_ The address of the community treasury
    /// @param contributorAddress The address that will distribute NRNs to early contributors when 
    /// the initial supply is minted.
    constructor(address ownerAddress, address treasuryAddress_, address contributorAddress)
        ERC20("Neuron", "NRN")
    {
        _ownerAddress = ownerAddress;
        treasuryAddress = treasuryAddress_;
        isAdmin[_ownerAddress] = true;
        _mint(treasuryAddress, INITIAL_TREASURY_MINT);
        _mint(contributorAddress, INITIAL_CONTRIBUTOR_MINT);
    }

Recommended Mitigation Steps

Refactor comment.