AI Arena - soliditywala's results

Lines of code

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

Vulnerability details

Impact

Following function ensures that a user can not transfer the staked fighter.

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

The above function is used in FighterFarm.transferFrom() and FighterFarm.safeTransferFrom(address from,address to, uint256 tokenId) to restrict the transferability of a staked fighter.

Since FighterFarm is ERC721, so it contains a function safeTransferFrom(address from,address to,uint256 tokenId,bytes memory data) with an extra param of data.

The transferability check is not enforced for this function hence a user can use this function in order to transfer the staked Fighter.

POC

Add this as first test in FighterFarm.t.sol;

  function test_stakedFighterTransferability() public {
        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";

        // Right sender of signature should be able to claim fighter
        _fighterFarmContract.claimFighters(
            numToMint,
            claimSignature,
            claimModelHashes,
            claimModelTypes
        );
        assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.totalSupply(), 1);

        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);
        vm.prank(_ownerAddress);
        vm.expectRevert();
        // reverting with simple transfer
        _fighterFarmContract.transferFrom(address(this), address(1), 0);
        vm.prank(_ownerAddress);
        // allows transfer with this version of safeTransferFrom
        _fighterFarmContract.safeTransferFrom(address(this), address(1), 0,"0x00");
        assertEq(_fighterFarmContract.ownerOf(0),address(1));

    }

Tools Used

Manual review.

Recommended Mitigation Steps

Also override this version of safeTranferFrom like below.

 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/main/src/GameItems.sol#L301 https://github.com/code-423n4/2024-02-ai-arena/blob/main/src/GameItems.sol#L291

Vulnerability details

Impact

There are two types of GameItems. Transferable and Non-transferable (also, admin can adjust transferability of any GameItem). To ensure that an item is transferable or not, following check is enforced in GameItems.safeTransferFrom()

         require(allGameItemAttributes[tokenId].transferable);

Since GameItems.sol is ERC1155, a standard ERC1155 contains a function safeBatchTransferFrom() that can be used by the user to transfer the non-transferable token since the above check is not enforced in safeBatchTransferFrom()

Proof of Concept

Add this as first test in GameItems.t.sol.

function test_buypassTransferability() public {
        _fundUserWith4kNeuronByTreasury(address(this));
        _gameItemsContract.mint(0, 10);
        // making the item non-transferable
        _gameItemsContract.adjustTransferability(0, false);
        uint[] memory ids = new uint[](1);
        uint[] memory amount = new uint[](1);
        ids[0] = 0;
        amount[0] = 10;
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(
            address(this),
            address(this),
            0,
            10,
            "0x00"
        );
        // bypassing non-transferability
        _gameItemsContract.safeBatchTransferFrom(
            address(this),
            address(1),
            ids,
            amount,
            "0x00"
        );
        assertEq(_gameItemsContract.balanceOf(address(1), 0), 10);
    }

Tools Used

Manual analysis.

Recommended Mitigation Steps

Also override the safeBatchTransferFrom() like below.

 function safeBatchTransferFrom(
        address from,
        address to,
        uint256[] memory ids,
        uint256[] memory amounts,
        bytes memory data
    ) public override {
        for (uint i; i < ids.length; i++) {
            require(allGameItemAttributes[ids[i]].transferable);
        }
        super.safeBatchTransferFrom(from, to, ids, amounts, data);
    }

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

FighterFarm.redeemMintPass() takes input from user to redeem the mint pass and mint the fighter against each mint pass. However, user can mint fighters with the attributes they want due to the control of input params. That should not be the case as all users will try to mint fighters with best attribues/qualities and that will basically to some extent cancels out the rarity of fighters as fighter will be a super hero.

Proof of Concept

For example user can input such DNA that earn him a fighter with best weight and element. Also user can control the fighter type he wants to have.

Tools Used

Manual Review

Recommended Mitigation Steps

Maybe input data can be generated signed from server that will ensure fair input params and then verified on-chain like we did here in FighterFarm.claimFighters()

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

FighterFarm.reRoll() expects the user to correctly input fighter type of a Fighter NFT.

However, this can be miss-used by the malicious user to change the fighter type of the Fighter that should not be possible (sponsor also confirmed that change of fighter type is not allowed by design).

This will allow users to make their average NFT worth more by changing their NFT's fighter type.

Proof of Concept

Add this as first test in FighterFarm.t.sol that shows user can change the fighter type of the fighter NFT.

 function testReroll() public {
        _mintFromMergingPool(_ownerAddress);
        // get 4k neuron from treasury
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        // after successfully minting a fighter, update the model
        if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
            uint256 neuronBalanceBeforeReRoll = _neuronContract.balanceOf(
                _ownerAddress
            );
            uint8 tokenId = 0;
            uint8 fighterType = 0;

            _neuronContract.addSpender(address(_fighterFarmContract));
            // first reRoll with fighter type 0
            _fighterFarmContract.reRoll(tokenId, fighterType);
            // then reRoll with fighter type 1
            _fighterFarmContract.reRoll(tokenId, uint8(1));
        }
    }

Tools Used

Manual Review

Recommended Mitigation Steps

1 - Create a global mapping that keeps track of tokenId -> fighterType upon minting of fighters. 2 - Fetch fighterType of tokenId from that global mapping rather than taking input from user.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Generation of a fighertype can be increased by FighterFarm.incrementGeneration() whose logic is below:

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

However, when a fighter's generation is incremented, its numElements are not being set.

So consider the following scenario:

Generation of fightertype CHAMPION has been incremented using incrementGeneration(0).

Now the generation of CHAMPION fightertype becomes from 0 to 1.

But numElement[1] = 0 because it is not set when incrementing the generation.

Now the following line inside FigherFarm._createFighterBase() will cause revert due to % with 0 because generation[CHAMPION] = 1 and a numElements[1] = 0.

        uint256 element = dna % numElements[generation[fighterType]];

_createFighterBase() is used by _createNewFighter() that is used to create new fighter and reRoll(). So minting of new fighter and reRoll of existing fighter is not possible and MergingPool.claimRewards() won't work as well blocking the claim of rewards.

Proof of Concept

Make this first test in FigherFarm.t.sol;

function test_incrementGenertionBug() public {
        // normal mint working fine
        vm.prank(address(_mergingPoolContract));
         _fighterFarmContract.mintFromMergingPool(
            address(this),
            "_neuralNetHash0",
            "original0",
            [uint256(100), uint256(100)]
        );
        // incrementing generation
        _fighterFarmContract.incrementGeneration(0);
        vm.prank(address(_mergingPoolContract));
        vm.expectRevert();
        _fighterFarmContract.mintFromMergingPool(
            address(this),
            "_neuralNetHash",
            "original",
            [uint256(100), uint256(100)]
        );
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Also set numElements when incrementing the generation of a fightertype like below.

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

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

There is only way to claim NRN that is through claimNRN().

If a user have not claimNRN for a long while, he may be unable to claim because claimNRN() will calculate NRN from the last round he claimed to the current-1 round, because if the difference between numRoundsClaimed[user] and current-1 round is too large, then the transaction may reach the block gas limit and user won't be able to claimNRN().

Tools Used

Manual review

Recommended Mitigation Steps

Add another function that allows user to claim for any previous round he wishes and keep track in a mapping that user has claimed for which round. That way even if he claimNRN after a long while, he may be able to claim individually if faces max block gas limit issue.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

DNA plays an important part in determining that what element and weight to give to a Fighter. However, inside FighterFarm.claimFighters(), DNA is calculated as below

                uint256(keccak256(abi.encode(msg.sender, fighters.length))),

Since it takes msg.sender as well as fighters.length into account, a claimer can wait enough time so that fighters.length comes to a value that would yield user such a DNA whose elementand weight is more useful for the fighter.

Consider the following example: 1 - address(123) is allowed to claim a fighter. 2 - Suppose fighters.length = 10, so the owner of address(123) can simulate the transaction to find what DNA would uint256(keccak256(abi.encode(address(123), fighters.length))) will yield and what will be the element and weight against that DNA for the fighter. 3 - He can continue simulate after each time fighters.length changes to mint a fighter with more useful weight and element.

Below is the coded POC which shows how fighters.length influence weight and element

Proof of Concept

element = 2 and weight = 93 will be assigned to the fighter when fighter.length = 0. Below POC verifies that.

function test_whenLengthIsZero() public {
        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";
        // @audit minting fighter when fighter.lenght = 0
        _fighterFarmContract.claimFighters(
            numToMint,
            claimSignature,
            claimModelHashes,
            claimModelTypes
        );
      
    }

element = 0 and weight = 67 will be assigned to the fighter when length will be 1. Below POC verifies that.

 function test_whenLengthIsOne() public {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(
            _ownerAddress,
            "_neuralNetHash",
            "original",
            [uint256(100), uint256(100)]
        );
        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";
        // @audit minting fighter when fighter.lenght = 1
        _fighterFarmContract.claimFighters(
            numToMint,
            claimSignature,
            claimModelHashes,
            claimModelTypes
        );
      
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Use Chainlink VRF for the source of DNA randomness rather then depending of user controlled params.

Assessed type

Other