AI Arena - pkqs90's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L539-L545 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L333-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/RankedBattle.sol#L440-L499

Vulnerability details

Impact

The core issue is users can freely transfer fighter NFTs regardless of the _ableToTransfer() validation. This leads to two significant impacts, with the first being of high severity:

1. A malicious user could transfer staked fighters between accounts and participate in battles without risking NRNs, by exploiting a DoS in updateBattleRecord.
2. A single account could accumulate more than the MAX_FIGHTERS_ALLOWED limit of NFT fighters, contravening game rules.

Bug Description

1. How to bypass _ableToTransfer check

FighterFarm.sol, which extends OpenZeppelin's ERC721 implementation, modifies the transferFrom() and safeTransferFrom() functions by incorporating a _ableToTransfer() validation. This check ensures that:

1. The recipient does not exceed the MAX_FIGHTERS_ALLOWED limit for NFT ownership.
2. The NFT being transferred is currently not staked.

However, within OpenZeppelin's ERC721 implementation, there exists another public safeTransferFrom() function that includes an additional bytes memory data parameter. This means users can bypass the _ableToTransfer() validation by using this variant of the function.

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

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

From OpenZeppelin ERC721: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC721/ERC721.sol#L159-L162.

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

2. How transferring staked fighters may cause DoS and benefit the attacker

Understanding that a staked NFT fighter can be transferred, we can create an attack vector:

1. Account A stakes NRN into an NFT fighter, which then wins a match, leading to an increase in both accumulatedPointsPerFighter and accumulatedPointsPerAddress.
2. Account A transfers the NFT fighter to Account B (controlled by the same user).
3. If the NFT then loses a match, the record for Account B cannot be updated due to accumulatedPointsPerAddress for Account B being zero, resulting in an integer underflow when attempting to subtract points.

As a consequence, Account B will never enter a risk state again because a portion of accumulatedPointsPerFighter has been "taken profit" in Account A and will never return to zero. This allows Account B to continue playing the game without the risk associated with its staked NRNs, unlike other users, leading to an unfair advantage.

        if (battleResult == 0) {
            /// If the user won the match
            /// If the user has no NRNs at risk, then they can earn points
            if (stakeAtRisk == 0) {
                points = stakingFactor[tokenId] * eloFactor;
            }
            ...
            /// Add points to the fighter for this round
>           accumulatedPointsPerFighter[tokenId][roundId] += points;
>           accumulatedPointsPerAddress[fighterOwner][roundId] += points;
            ...
        } else if (battleResult == 2) {
            /// If the user lost the match
            /// Do not allow users to lose more NRNs than they have in their staking pool
            if (curStakeAtRisk > amountStaked[tokenId]) {
                curStakeAtRisk = amountStaked[tokenId];
            }
            if (accumulatedPointsPerFighter[tokenId][roundId] > 0) {
                /// If the fighter has a positive point balance for this round, deduct points 
                points = stakingFactor[tokenId] * eloFactor;
                if (points > accumulatedPointsPerFighter[tokenId][roundId]) {
                    points = accumulatedPointsPerFighter[tokenId][roundId];
                }
                accumulatedPointsPerFighter[tokenId][roundId] -= points;
>               accumulatedPointsPerAddress[fighterOwner][roundId] -= points;
                totalAccumulatedPoints[roundId] -= points;
                ...
            } else {
                ...
            }
        }

Proof of Concept

Run the following test in FighterFarm.t.sol. This proves that staked NFTs can still be transferred.

    function testSafeTransferFromNew() public {
        // 1. Staked fighters can still be transferred.
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        // 2. More than 10 fighters can be transferred to single account.
        for (uint i = 1; i <= 20; ++i) {
            _mintFromMergingPool(_ownerAddress);
            _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, i, "");
        }
        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 21);
    }

Run the following test in RankedBattle.t.sol. This proves the DoS attack vector mentioned above.

    function testDoSAttack() public {
        address accountA = vm.addr(3);
        address accountB = vm.addr(4);
        uint8 tokenId = 0;
        _mintFromMergingPool(accountA);
        _fundUserWith4kNeuronByTreasury(accountA);

        // 1. Account A stakes NRN.
        vm.prank(accountA);
        _rankedBattleContract.stakeNRN(3_000 * 10 ** 18, tokenId);

        // 2. The first battle wins.
        vm.prank(address(_GAME_SERVER_ADDRESS));
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);

        // 3. Account A transfer NFT to Account B.
        vm.prank(accountA);
        _fighterFarmContract.safeTransferFrom(accountA, accountB, tokenId, "");

        // 4. The next battle loses, but ends up in a revert due to underflow.
        vm.prank(address(_GAME_SERVER_ADDRESS));
        vm.expectRevert();
        _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, false);
    }

Tools Used

Foundary.

Recommended Mitigation Steps

Override the second safeTransferFrom() function that includes the bytes memory data parameter, incorporating the require(_ableToTransfer(tokenId, to)); check.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

In the GameItems.sol contract, items remain transferrable even when the transferable flag is set to false, undermining the intended functionality of the transferable attribute.

Bug Description

GameItems.sol, which inherits from ERC1155, serves as a collection of game items for AI Arena. Each item features a transferable boolean flag indicating its transferability. A check has been incorporated into the safeTransferFrom() function, inherited from ERC1155, to enforce this flag.

contract GameItems is ERC1155 {
    ...
}

    /// @notice Safely transfers an NFT from one address to another.
    /// @dev Added a check to see if the game 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);
    }

However, within ERC1155, there exists another function safeBatchTransferFrom(), which users can utilize to circumvent the transferable check, thus allowing the transfer of items regardless of their transferable flag setting.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC1155/ERC1155.sol#L120-L132

    function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public virtual override {
        require(
            from == _msgSender() || isApprovedForAll(from, _msgSender()),
            "ERC1155: caller is not token owner nor approved"
        );
        _safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Proof of Concept

Run the following test in GameItems.t.sol.

    function testSafeBatchTransferFrom() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        // 1. Set item 0 to non-transferrable.
        _gameItemsContract.adjustTransferability(0, false);
        // 2. safeTransferFrom should revert.
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        // 3. safeBatchTransferFrom doesn't revert.
        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

Foundary.

Recommended Mitigation Steps

Override the safeBatchTransferFrom() function from ERC1155 to include the require(allGameItemAttributes[tokenId].transferable); check.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Users redeeming mint passes for fighter NFTs have the ability to arbitrarily set fighterTypes[], enabling them to freely obtain dendroids, which are intended to be extremely rare.

Bug Description

Users are able to claim their mint pass through AAMintPass.sol and use this mint pass to claim a fighter NFT in FighterFarm.sol by utilizing the redeemMintPass() function.

The problem arises from the lack of validation within this function, allowing users to specify any fighterTypes[] they choose. Specifically, if fighterType == 1, it results in the creation of a rare dendroid. After discussion with the protocol team, it was clarified that allowing users to freely generate dendroid types was not the intended design, as it could lead to an oversupply of the rare dendroid types.

    function redeemMintPass(
        uint256[] calldata mintpassIdsToBurn,
        uint8[] calldata fighterTypes,
        uint8[] calldata iconsTypes,
        string[] calldata mintPassDnas,
        string[] calldata modelHashes,
        string[] calldata modelTypes
    ) 
        external 
    {
        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++) {
            require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
            _mintpassInstance.burn(mintpassIdsToBurn[i]);
            _createNewFighter(
                msg.sender, 
                uint256(keccak256(abi.encode(mintPassDnas[i]))), 
                modelHashes[i], 
                modelTypes[i],
                fighterTypes[i],
                iconsTypes[i],
                [uint256(100), uint256(100)]
            );
        }
    }

>       bool dendroidBool = fighterType == 1;
        FighterOps.FighterPhysicalAttributes memory attrs = _aiArenaHelperInstance.createPhysicalAttributes(
            newDna,
            generation[fighterType],
            iconsType,
            dendroidBool
        );

https://docs.aiarena.io/gaming-competition/nft-makeup

Proof of Concept

Call redeemMintPass() function with fighterType == 1 would create a rare dendroid type NFT.

Tools Used

Manual review.

Recommended Mitigation Steps

Add some sort of signature validation for redeemMintPass() function, or simply set the fighterType to 0.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

For generations >= 1, the numElements[] will always be 0, preventing the creation of any fighters.

Bug Description

numElements[] represents the number of elements available for each generation. For the initial generation (generation == 0), it is set to numElements[0] = 3, corresponding to fire, water, and electricity. However, in the FighterFarm.sol contract, there's no mechanism to assign numElements[] for subsequent generations, implying it remains at 0.

Furthermore, because the value of numElements[] is utilized as the divisor in modulo operations for fighter creation, no fighters can be generated for newer generations. This is due to the fact that a modulo operation with 0 will always result in a revert.

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
>       uint256 element = dna % numElements[generation[fighterType]];
        uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Proof of Concept

N/A

Tools Used

Manual review.

Recommended Mitigation Steps

Add a function to set the numElements[].

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/MergingPool.sol#L154-L159 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L313-L331 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/FighterFarm.sol#L502-L506

Vulnerability details

Impact

New fighter NFTs may be minted with invalid element and weight value.

Bug Description

When a user wins in the MergingPool, they are entitled to mint a new fighter NFT from FighterFarm.sol. This process allows the user to specify any customAttributes for the fighter, such as its element and weight, which the FighterFarm.sol will then create.

The problem arises from the lack of validation for these input attributes. Specifically, the element attribute should not exceed the numElements[] for the current generation, and there should be a data range on the weight attribute.

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

        if (customAttributes[0] == 100) {
            (element, weight, newDna) = _createFighterBase(dna, fighterType);
        }
        else {
>           element = customAttributes[0];
>           weight = customAttributes[1];
            newDna = dna;
        }

In other methods of minting fighter NFTs, the constraints for element and weight attributes are as follows:

1. The element should be within the range of [0, numElements[generation[fighterType]]].
2. The weight should be within the range of [65, 95].

Additionally, it's important to note that the range has been confirmed by the project sponsor in the public Discord channel.

    function _createFighterBase(
        uint256 dna, 
        uint8 fighterType
    ) 
        private 
        view 
        returns (uint256, uint256, uint256) 
    {
>       uint256 element = dna % numElements[generation[fighterType]];
>       uint256 weight = dna % 31 + 65;
        uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
        return (element, weight, newDna);
    }

Proof of Concept

N/A

Tools Used

Manual review.

Recommended Mitigation Steps

Element/weight validations should be added for custom attribute inputs.

Assessed type

Invalid Validation