AI Arena - KupiaSec's results

Lines of code

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

Vulnerability details

Impact

In FighterFarm contract, before transfering the ownership of the NFT, there is a check, _ableToTransfer. For this, transferFrom and safeTransferFrom function override the standard functions. However the safeTransferFrom function with data is not overrided and users can use this function to bypass the _ableToTransfer check.

Proof of Concept

By using safeTransferFrom with data, users can bypass the _ableToTransfer check. In the following code, the number of fighters for the _DELEGATED_ADDRESS can be greater than 10.

    function testSafeTransferFromWithData() public {
        for (uint16 i = 0; i < 10; i++) {
            _mintFromMergingPool(_ownerAddress);
        }
        _mintFromMergingPool(alice);

        for (uint16 i = 0; i < 10; i++) {
            assertEq(_fighterFarmContract.ownerOf(i), _ownerAddress);
        }
        assertEq(_fighterFarmContract.ownerOf(10), alice);

        for (uint16 i = 0; i < 10; i++) {
            _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, i);
        }
        vm.startPrank(alice);
        vm.expectRevert(); // can't transfer token because of check and this will revert
        _fighterFarmContract.safeTransferFrom(alice, _DELEGATED_ADDRESS, 10);
        assertEq(_fighterFarmContract.ownerOf(10), alice);
        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 10);
        _fighterFarmContract.safeTransferFrom(alice, _DELEGATED_ADDRESS, 10, ""); // but safeTransferFrom with data bypasses the check
        assertEq(_fighterFarmContract.ownerOf(10), _DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.balanceOf(_DELEGATED_ADDRESS), 11);
        // assertEq(_fighterFarmContract.ownerOf(0), _DELEGATED_ADDRESS);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Should override the safeTransferFrom (with data), too.

+   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/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L470

Vulnerability details

Impact

numElements is used to determine the element of fighter for the fighterType. However, numElements is only initialized once in the constructor and there is no update progress for numElements. So after the generation for the fighterType is increased and the corresponding numElements is always 0 and this makes the _createFighterBase function revert. Also _createNewFighter, reRoll and claimFighters, redeemMintPass, mintFromMergingPool, functions will always revert becasue these functions call _createFighterBase function directly or indirectly.

Proof of Concept

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

numElements is used to determine the element for the fighter. And in the incrementGeneration function, the generation for the given fighterType is increased by 1. But the numElements is never updated and numElements for the increased generation of the fighterType is always 0, and this makes the _createFighterBase function revert because of modulo by 0

I made a simpe test as follows

• Increase generation for fighterType 0.
• Try to call claimFighters and the check the function that revert

    function testIncrementGenerationAndClaimFightersFail() public {
        _fighterFarmContract.incrementGeneration(0);
        assertEq(_fighterFarmContract.generation(1), 0);
        assertEq(_fighterFarmContract.generation(0), 1);

        uint8[2] memory numToMint = [1, 0];
        bytes memory claimSignature = abi.encodePacked(
            hex"407c44926b6805cf9755a88022102a9cb21cde80a210bc3ad1db2880f6ea16fa4e1363e7817d5d87e4e64ba29d59aedfb64524620e2180f41ff82ca9edf942d01c"
        );
        string[] memory claimModelHashes = new string[](1);
        claimModelHashes[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

        string[] memory claimModelTypes = new string[](1);
        claimModelTypes[0] = "original";

        vm.expectRevert();
        _fighterFarmContract.claimFighters(numToMint, claimSignature, claimModelHashes, claimModelTypes);
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

After increase the generation for fighterType, should update the numElements

    function incrementGeneration(uint8 fighterType) external returns (uint8) {
        require(msg.sender == _ownerAddress);
        generation[fighterType] += 1;
+       numElements[generation[fighterType]] = 3;
        maxRerollsAllowed[fighterType] += 1;
        return generation[fighterType];
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If the accumulatedPointsPerFighter[tokenId][roundId] is not removed before transfer, accumulatedPointsPerFighter[tokenId][roundId] could be greater than the accumulatedPointsPerAddress[fighterOwner][roundId] and in case of the user loses the match, it causes underflow and it is impossible to update the battle record.

Proof of Concept

Users can transfer the fighters to other addresses in the middle of the round if the fighters are not locked. But when users transfer the fighters to other users, there is not any mechanism to adjust the accumulatedPointsPerFighter[tokenId][roundId]. And there is possibility that accumulatedPointsPerFighter[tokenId][roundId] could be greater than the accumulatedPointsPerAddress[fighterOwner][roundId] of the new owner. And if the the new owner loses the match the points should be reduced from the new owner.

    function updateBattleRecord(
        uint256 tokenId, 
        uint256 mergingPortion,
        uint8 battleResult,
        uint256 eloFactor,
        bool initiatorBool
    ) 
        external 
    {   
        require(msg.sender == _gameServerAddress);
        require(mergingPortion <= 100);
        address fighterOwner = _fighterFarmInstance.ownerOf(tokenId);
        require(
            !initiatorBool ||
            _voltageManagerInstance.ownerVoltageReplenishTime(fighterOwner) <= block.timestamp || 
            _voltageManagerInstance.ownerVoltage(fighterOwner) >= VOLTAGE_COST
        );

        _updateRecord(tokenId, battleResult);
        uint256 stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);
        if (amountStaked[tokenId] + stakeAtRisk > 0) {
@>          _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }
        if (initiatorBool) {
            _voltageManagerInstance.spendVoltage(fighterOwner, VOLTAGE_COST);
        }
        totalBattles += 1;
    }

    function _addResultPoints(
        uint8 battleResult, 
        uint256 tokenId, 
        uint256 eloFactor, 
        uint256 mergingPortion,
        address fighterOwner
    ) 
        private 
    {
        uint256 stakeAtRisk;
        uint256 curStakeAtRisk;
        uint256 points = 0;

        /// Check how many NRNs the fighter has at risk
        stakeAtRisk = _stakeAtRiskInstance.getStakeAtRisk(tokenId);

        /// Calculate the staking factor if it has not already been calculated for this round 
        if (_calculatedStakingFactor[tokenId][roundId] == false) {
            stakingFactor[tokenId] = _getStakingFactor(tokenId, stakeAtRisk);
            _calculatedStakingFactor[tokenId][roundId] = true;
        }

        /// Potential amount of NRNs to put at risk or retrieve from the stake-at-risk contract
        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;
        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;
            }

            /// Divert a portion of the points to the merging pool
            uint256 mergingPoints = (points * mergingPortion) / 100;
            points -= mergingPoints;
            _mergingPoolInstance.addPoints(tokenId, mergingPoints);

            /// Do not allow users to reclaim more NRNs than they have at risk
            if (curStakeAtRisk > stakeAtRisk) {
                curStakeAtRisk = stakeAtRisk;
            }

            /// If the user has stake-at-risk for their fighter, reclaim a portion
            /// Reclaiming stake-at-risk puts the NRN back into their staking pool
            if (curStakeAtRisk > 0) {
                _stakeAtRiskInstance.reclaimNRN(curStakeAtRisk, tokenId, fighterOwner);
                amountStaked[tokenId] += curStakeAtRisk;
            }

            /// Add points to the fighter for this round
            accumulatedPointsPerFighter[tokenId][roundId] += points;
            accumulatedPointsPerAddress[fighterOwner][roundId] += points;
            totalAccumulatedPoints[roundId] += points;
            if (points > 0) {
                emit PointsChanged(tokenId, points, true);
            }
        } 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;
@>486           accumulatedPointsPerAddress[fighterOwner][roundId] -= points; // underflow can occur
                totalAccumulatedPoints[roundId] -= points;
                if (points > 0) {
                    emit PointsChanged(tokenId, points, false);
                }
            } else {
                /// If the fighter does not have any points for this round, NRNs become at risk of being lost
                bool success = _neuronInstance.transfer(_stakeAtRiskAddress, curStakeAtRisk);
                if (success) {
                    _stakeAtRiskInstance.updateAtRiskRecords(curStakeAtRisk, tokenId, fighterOwner);
                    amountStaked[tokenId] -= curStakeAtRisk;
                }
            }
        }
    }

At L486, the points could be greater than the accumulatedPointsPerAddress[fighterOwner][roundId] and underflow will occur and it will revert. So it is impossible to update the battle record

Tools Used

Manual review

Recommended Mitigation Steps

Should remove the accumulated points of the fighter before token transfer.

Assessed type

Other