AI Arena - aslanbek's results

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC721/ERC721.sol#L175-L183

Vulnerability details

Impact

ERC721 has 3 public functions that transfer tokens between accounts:

1. function transferFrom(address from, address to, uint256 tokenId)
2. function safeTransferFrom(address from, address to, uint256 tokenId)
3. function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data)

FighterFarm overrides 1st and 2nd functions with _ableToTransfer check, which reverts the transfer if the receiver would exceed MAX_FIGHTERS_ALLOWED, or if the transferred Fighter is staked.

But, because ERC721's safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) is inherited but not overridden, both of these checks can be bypassed.

By transferring fighters between accounts, owners of staked FighterNFTs can initiate unlimited number of games for free, as every address has 100 free daily voltage, and the number of addresses is practically unlimited.

Recommended Mitigation Steps

Override safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) with _ableToTransfer check.

Assessed type

ERC721

Lines of code

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/token/ERC1155/ERC1155.sol#L134

Vulnerability details

Impact

Every GameItem can be made "untransferable" via createGameItem or adjustTransferability. To achieve that, ERC1155's safeTransferFrom is overridden to check for transferable value of the item:

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

But the same was not done for safeBatchTransferFrom, so it will be possible to transfer tokens that should not be transferable.

Users are expected to be limited in daily number of initiated games by their voltage, and voltage could be replenished only by batteries. In the deployment script, Batteries will be non-transferrable and have a daily limit of 5. So each user should be able to initiate only (100 + 5*100) / 10 = 60 battles per day regardless of their capital. However, because of this vulnerability, users will be able to initiate unlimited number of games per day (as long as they have NRN to buy batteries).

Recommended Mitigation Steps

Override safeBatchTransferFrom with transferrable check, same as in safeTransferFrom.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

reRoll, instead of setting traits pseudo-randomly, always sets them to 1, if the caller specified fighterType = 1. This can be done regardless if the rerolled fighter is a rare Dendroid (fighterType 1) or a normal Champion (fighterType 0).

Let's trace the behaviour of reRoll, if the user specifies fighterType = 1:

    function reRoll(uint8 tokenId, uint8 fighterType) public {
            /*...*/
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            /*...*/

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

As you can see, newDna is set to 1 if the caller specified fighterType = 1.

Then, reRoll calls createPhysicalAttributes with the newDna, generates traits based on the dna modulo, which will always be 1.

Proof of Concept

    function testRerollPoc() public {
        _mintFromMergingPool(_ownerAddress);
        _fundUserWith4kNeuronByTreasury(_ownerAddress);

        uint8 tokenId = 0;

        // normal Champion, not a Dendroid
        (, , , , , , , , bool isDendroid) = _fighterFarmContract.fighters(
            tokenId
        );
        assertEq(isDendroid, false);
        _neuronContract.addSpender(address(_fighterFarmContract));

        _fighterFarmContract.reRoll(tokenId, 1);
        (
            ,
            ,
            FighterOps.FighterPhysicalAttributes memory physicalAttributes,
            ,
            ,
            ,
            ,
            ,

        ) = _fighterFarmContract.fighters(tokenId);

        assertEq(physicalAttributes.head, 1);
        assertEq(physicalAttributes.eyes, 1);
        assertEq(physicalAttributes.mouth, 1);
        assertEq(physicalAttributes.body, 1);
        assertEq(physicalAttributes.hands, 1);
        assertEq(physicalAttributes.feet, 1);
    }

Tools Used

Foundry

Recommended Mitigation Steps

a) If only Dendroids are supposed to have all physical attributes == 1, fighterType should be validated in reRoll:

function reRoll(uint8 tokenId, uint8 fighterType) public {
+       require(
+           fighters[tokenId].dendroidBool ? fighterType == 1 : fighterType == 0
+       );

b) If neither Dendroids nor Champions should have all physical attributes equal to 1, then _createFighterBase should not alter the dna.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

In RankedBattle#updateBattleRecord, _addResultPoints is only entered if the user has positive stake or stakeAtRisk:

        if (amountStaked[tokenId] + stakeAtRisk > 0) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }

With bpsLostPerLoss = 10, staking 1-999 wei results in curStakeAtRisk rounding down 0 due to integer division:

        curStakeAtRisk = (bpsLostPerLoss * (amountStaked[tokenId] + stakeAtRisk)) / 10**4;

So if a user stakes 1 wei of NRN (mergingPortion = 0), all his battles will have one of these 3 outcomes:

1. Won - points are increased
2. Lost & positive points - points are decreased
3. Lost & zero points - no change in points

Which is unfair towards other stakers, whose wins and losses are always rewarded / penalized symmetrically.

Moreover, if instead the user decides to forward all points to the merging pool, there will only be two possible outcomes:

1. Won - get merging points
2. Lost - no penalty

These strategies can be employed either by low-budget / low-winrate players; or by high-winrate / high-budget players after they are done for the round and simply want to preserve their current accumulatedPoints.

The only users who do not want to employ this strategy are active high-winrate high-budget players, as they are the only ones who would gain more by staking significant NRN amounts and playing fair.

Proof of Concept

1. Alice stakes 1 wei of NRN.
2. Alice sets her mergingPortion to 100 off-chain.
3. If Alice wins - she gets merging pool points.
4. If Alice loses - she loses nothing, as _addResultPoints will not change her stakeAtRisk, because curStakeAtRisk rounds down to zero.

Alice ends up accumulating more merging pool points, and has a higher chance of winning the new fighter in comparison to other players.

Recommended Mitigation Steps

In updateBattleRecord, _addResultPoints should be entered only if user's stake + stakeAtRisk are above a certain value, which would prevent rounding down to zero.

-       if (amountStaked[tokenId] + stakeAtRisk > 0) {
+       if (amountStaked[tokenId] + stakeAtRisk > MIN_STAKE) {
            _addResultPoints(battleResult, tokenId, eloFactor, mergingPortion, fighterOwner);
        }

Assessed type

Other

Lines of code

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

Vulnerability details

Vulnerability Details

1. FighterType's gen is increased via incrementGeneration:

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

2. numElements doesn't have a setter. Only numElements[0] is set in the constructor:

    constructor(
        address ownerAddress,
        address delegatedAddress,
        address treasuryAddress_
    ) ERC721("AI Arena Fighter", "FTR") {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
        numElements[0] = 3;
    }

3. Fighter's traits can be rerolled via reRoll, which calls _createFighterBase:

    function _createFighterBase(
        uint256 dna,
        uint8 fighterType
    ) private view returns (uint256, uint256, uint256) {
        uint256 element = dna % numElements[generation[fighterType]]; 

Once the generation for a FighterType is increased to 1, all reRolls for this type will revert in _createFighterBase with 0x12 panic due to modulo by zero, as numElements[generation[fighterType]] = numElements[1] = 0.

Impact

Fighters with gen > 0 can not be rerolled.

Proof of Concept

    function testPanic() public {
        deal(address(_neuronContract), _ownerAddress, 1000e18);
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(
            _ownerAddress,
            "_neuralNetHash",
            "original",
            [uint256(1), uint256(80)]
        );
        vm.startPrank(_ownerAddress);
        _fighterFarmContract.incrementGeneration(0);
        _neuronContract.addSpender(address(_fighterFarmContract));
        vm.expectRevert(stdError.divisionError);
        _fighterFarmContract.reRoll(0, 0);
    }

Recommended Mitigation Steps

Add a separate setter for numElements mapping; or set it from incrementGeneration.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

MergingPool#claimRewards allows the winner to specify customAttirubutes that the new fighter will have, which are element and weight. Then, FighterFarm#mintFromMergingPool calls _createNewFighter, which checks for customAttributes:

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

According to sponsor, the elements should be in range [0;2] (for the generation 0), and weight should be in range of [65; 95]. However, there's no such check in the else bracket. As these factors directly affect the outcome of battles, the winner of merging pool NFT can choose any value for element and weight, getting a fighter with extreme competitive advantage.

Proof of Concept

    function testClaimRewardsPoC() public {
        testPickWinner();
        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(1000);
        _customAttributes[0][1] = uint256(1000);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _mergingPoolContract.claimRewards(
            _modelURIs,
            _modelTypes,
            _customAttributes
        );
        (uint256 weight, uint256 element, , , , , , , ) = _fighterFarmContract
            .fighters(2);
        assertEq(weight, 1000);
        assertEq(element, 1000);
    }

Recommended Mitigation Steps

        else {
+           require(customAttributes[0] < numElements[generation[fighterType]], "invalid element");
+           require(weight >= 65 && weight <= 95, "invalid weight");
            element = customAttributes[0];
            weight = customAttributes[1];
            newDna = dna;
        }

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Summary

According to docs,

There are 8 categories of randomized attributes which make up the appearance

fighter traits are meant to be pseudo-random, uncontrolled by users. However, this is not the case.

Vulnerability Details

FighterFarm allows users to reroll the traits for their FighterNFT. The seed for the reroll is derived from msg.sender:

            uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
            (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
            fighters[tokenId].element = element;
            fighters[tokenId].weight = weight;
            fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
                newDna,
                generation[fighterType],
                fighters[tokenId].iconsType,
                fighters[tokenId].dendroidBool
            );

If the NFT is not staked, it can be transferred to another address, for which reroll would yield a different result.

Therefore, users can bruteforce EOA addresses of their mnemonic phrase, or bruteforce create2 addresses of their accounts, until they get the address that would get the desirable traits from reRoll. Then, they would send their Fighter to that address and execute reRoll.

Thus, sophisticated users will get an unfair advantage over the normal users, as they will be extremely efficient in getting the desirable traits.

Essentially, reRoll will be used the following way:

1. Ordinary players - spend NRN on one or multiple rerolls, have no control over their traits.

2. Sophisticated players - compute the address off-chain, spend NRN on one reroll, get the desired traits.

Recommended Mitigation Steps

It is impossible to make randomness non-manipulatable with on-chain seeds. Consider returning to Chainlink VRF.

Assessed type

Other