AI Arena - peter's results

Lines of code

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

Vulnerability details

Impact

Specific game items can be locked/unlocked by the contract owner, with the intention of preventing transfers of these game items during periods when they are locked. This constraint is correctly added to the overwritten (inherited) ERC1155 function safeTransferFrom, which requires the item be unlocked in order to allow a transfer:

    /// @dev Added a check to see if the game item is transferable.
    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);
    }

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

However the contract does not also overwrite the inherited ERC1155 function safeBatchTransferFrom, as it should. Therefore the entire locking/unlocking logic is rendered defunct, as locked items can be as freely transferred as unlocked items. This may have significant ramifications for the protocol, as core game item logic doesn't work as intended.

Proof of Concept

Add the following test to the GameItemsTest test suite and run:

forge test --match-test testCircumventLockWithBatchTransfer -vvv

    /// @notice Test cirumventing the lock by using a batch transfer
    function testCircumventLockWithBatchTransfer() public {
        _fundUserWith4kNeuronByTreasury(_ownerAddress);
        _gameItemsContract.mint(0, 1);
        console.log("make the game item non transferable");
        _gameItemsContract.adjustTransferability(0, false);     // make the game item non transferable
        (,, bool transferable,,,) = _gameItemsContract.allGameItemAttributes(0);
        assertEq(transferable, false);
        console.log("expect revert since the game item is non transferable");
        vm.expectRevert();       // expect revert since the game item is non transferable
        _gameItemsContract.safeTransferFrom(_ownerAddress, _DELEGATED_ADDRESS, 0, 1, "");
        console.log("the game item was not transferred as safe transfer lock check works as expected");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 0);   // the game item was not transferred
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 1);

        console.log("transfer the same locked item with a batch transfer instead");
        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, "");
        console.log("the game item was transferred successfully as no lock check is performed for ERC1155 batch transfers");
        assertEq(_gameItemsContract.balanceOf(_DELEGATED_ADDRESS, 0), 1);   // the game item was transferred
        assertEq(_gameItemsContract.balanceOf(_ownerAddress, 0), 0);
    }

Output:

Running 1 test for test/GameItems.t.sol:GameItemsTest
[PASS] testCircumventLockWithBatchTransfer() (gas: 201580)
Logs:
  make the game item non-transferable
  expect revert since the game item is non-transferable
  the game item was not transferred as safe transfer lock check works as expected
  transfer the same locked item with a batch transfer instead
  the game item was transferred successfully as no lock check is performed for ERC1155 batch transfers

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.78ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual inspection, Foundry

Recommended Mitigation Steps

Add the same transferability lock/unlock logic to safeBatchTransferFrom in GameItems.sol:

diff --git a/src/GameItems.sol b/src/GameItems.sol
index 4c810a8..900466d 100644
--- a/src/GameItems.sol
+++ b/src/GameItems.sol
@@ -302,6 +302,25 @@ contract GameItems is ERC1155 {
         super.safeTransferFrom(from, to, tokenId, amount, data);
     }
 
+    /// @notice Safely transfers an NFT from one address to another.
+    /// @dev Added a check to see if the game item is transferable.
+    function safeBatchTransferFrom(
+        address from, 
+        address to, 
+        uint256[] memory tokenIds,
+        uint256[] memory amounts,
+        bytes memory data
+    ) 
+        public 
+        override(ERC1155)
+    {
+        for (uint256 i = 0; i < tokenIds.length;) {
+            require(allGameItemAttributes[tokenIds[i]].transferable);
+            unchecked {++i;}
+        }
+        super.safeBatchTransferFrom(from, to, tokenIds, amounts, data);
+    }
+
     /*//////////////////////////////////////////////////////////////
                             PRIVATE FUNCTIONS
     //////////////////////////////////////////////////////////////*/    

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

As long as a user has a valid mint pass, they can give any arbitrary values for modelHashes, modelTypes, fighterTypes, iconsTypes and mintPassDnas. During protocol launch, FighterFarm::redeemMintPass will be the only mechanism to get a fighter and play the game, so this function will be used extensively.

With arbitrary fighterTypes, users can mint themselves any fighter type they desire, including a Dendroid, which the protocol intends to be:

"very rare and more difficult to obtain"

Furthermore, arbitrary iconsTypes allow users to acquire unique custom icon attributes that they shouldn't be allowed through the FighterFarm::redeemMintPass user flow. And arbitrary mintPassDnas allows users to give themselves custom rarity attributes, as these become deterministic if one is able to set the dna value (see AiArenaHelper::createPhysicalAttributes).

Beyond simply exploiting the FighterFarm::redeemMintPass intended functionality and giving oneself especially rare fighters, this exploit has 2 downstream financial consequences:

1. It lowers the market value of rare NFTs owned by users that derived them legitimately, thus causing financial losses for honest users
2. It effectively allows users to dictate the value of the NFT they mint, thus allowing malicious users to indirectly financially exploit the protocol

In summary, users can set any values for key parameters within FighterFarm::redeemMintPass. This allows malicious actors to exploit the intended functionality of the protocol by giving themselves arbitrarily rare fighter NFTs, which has significant negative consequences for the protocol.

Tools Used

Manual inspection, Foundry

Recommended Mitigation Steps

Incorporate the following diff file instead of the original FigherFarm::redeemMintPass. The changes are as follows:

1. Add public state mapping to track number of redemptions a given address has made
2. Require a signature from the delegated address for each redemption, similar to the logic in FigherFarm::claimFighters
3. Each signature message should include: msg.sender, fighterType, iconsType and how many redemptions the address has already made. This ensures no same-chain replay, and ensures the integrity of the fighter and icons type, as this has to be approved by the delegator.

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..b924a23 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -87,6 +87,9 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @notice Maps address to fighter type to return the number of NFTs claimed.
     mapping(address => mapping(uint8 => uint8)) public nftsClaimed;
 
+    /// @notice Mapping of address to the number of NFTs redeemed.
+    mapping(address => uint8) public nftsRedeemed;
+
     /// @notice Mapping of tokenId to number of times trained.
     mapping(uint256 => uint32) public numTrained;
 
@@ -230,28 +233,37 @@ contract FighterFarm is ERC721, ERC721Enumerable {
     /// @param fighterTypes Array of fighter types corresponding to the fighters being minted.
     /// @param modelHashes Array of ML model hashes corresponding to the fighters being minted. 
     /// @param modelTypes Array of ML model types corresponding to the fighters being minted.
+    /// @param signatures Array of signatures corresponding to the mint pass IDs to be burned.
     function redeemMintPass(
         uint256[] calldata mintpassIdsToBurn,
         uint8[] calldata fighterTypes,
         uint8[] calldata iconsTypes,
-        string[] calldata mintPassDnas,
         string[] calldata modelHashes,
         string[] calldata modelTypes,
+        bytes[] calldata signatures
     ) 
         external 
     {
         require(
-            mintpassIdsToBurn.length == mintPassDnas.length && 
-            mintPassDnas.length == fighterTypes.length && 
+            mintpassIdsToBurn.length == fighterTypes.length && 
             fighterTypes.length == modelHashes.length &&
             modelHashes.length == modelTypes.length
         );
+        
         for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
+            bytes32 msgHash = bytes32(keccak256(abi.encode(
+                msg.sender, 
+                fighterTypes[i], 
+                iconsTypes[i],
+                nftsRedeemed[msg.sender]
+            )));
+            require(Verification.verify(msgHash, signatures[i], _delegatedAddress));
+            nftsRedeemed[msg.sender] += 1;
             require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
             _mintpassInstance.burn(mintpassIdsToBurn[i]);
             _createNewFighter(
                 msg.sender, 
-                uint256(keccak256(abi.encode(mintPassDnas[i]))), 
+                uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
                 modelHashes[i], 
                 modelTypes[i],
                 fighterTypes[i],

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

FighterFarm.sol provides _ownerAddress the ability to increment fighter generations, meaning the contract is designed to handle generations > 0. However nowhere in the code can the owner or any other user actually add elements for future generations ('Elemental Powers' are a key component in each fighter NFT). Only generation 0 elements are defined as they are hard-coded in the constructor:

    constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
        ERC721("AI Arena Fighter", "FTR")
    {
        _ownerAddress = ownerAddress;
        _delegatedAddress = delegatedAddress;
        treasuryAddress = treasuryAddress_;
        numElements[0] = 3;
    } 

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

This means that as fighter type generations are incremented in FighterFarm.sol (as intended), the key NFT fighter minting functions will always revert. This is because both FighterFarm::claimFighters and FighterFarm::redeemMintPass use the internal FighterFarm::_createFighterBase function. When the elements in a generation > 0 are not defined (as they can't be), the line uint256 element = dna % numElements[generation[fighterType]]; will always revert, as num % 0 will always give EVM divide by 0 error.

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

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

In summary, essential fighter NFT parameters can't be defined as non-zero in a contract that is clearly intended to handle generations > 0, causing core minting functionality to always fail for generations > 0.

Tools Used

Manual inspection

Recommended Mitigation Steps

Add the following function to FighterFarm.sol to add elements for future generations:

    /// @notice Adds a new element number to the specified generation.
    function addElements(uint8 generation, uint8 _numElements) external {
        require(msg.sender == _ownerAddress);
        numElements[generation] = _numElements;
    }

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

NFT fighters in generation 0 are explicitly limited to 3 elements: fire, water and electricity. Further there are 3 weight classes: striker, scrapper and slugger. Elements are strictly bounded between uint values [0,2] and weight classes [65,95], where each fighter weight class increases in increments of 10 (i.e. striker is 65-74). These limitations are clearly laid out in FighterFarm. From the protocol documentation, a fighter’s weight:

"is the primary determinant of its other relative strength and weaknesses"

"Additionally, it is used to calculate how far the fighter moves when being knocked back"

For elements:

"There are three featured elements, each with corresponding elemental moves. These moves have relative strengths and weaknesses depending on pairing"

However when a user mints an NFT through MergingPool::claimRewards, they can choose any arbitrary values for elements and weights in customAttributes:

    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]

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

This means that NFT fighters with undefined attributes can be created. As a fighter's weight and element type are very important in combat, allowing arbitrary values for these key parameters at best creates invalid fighters, at worst may significantly damage and exploit gameplay.

Proof of Concept

Add the following test to MergingPoolTest test suite and run:

forge test --match-test testSetIncorrectCustomAttributes -vvv

    function testSetIncorrectCustomAttributes() external {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;
        _mergingPoolContract.pickWinner(_winners);
        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] = type(uint256).max;        // Elements should be bounded between 0-2
        _customAttributes[0][1] = type(uint256).max;        // Weight should be bounded between 65-95 (3 weight classes)
        // other winner claims rewards for previous roundIds
        vm.prank(_DELEGATED_ADDRESS);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        (, , uint256 weight, uint256 element, , ,) = _fighterFarmContract.getAllFighterInfo(2);
        assertEq(element, type(uint256).max);
        assertEq(weight, type(uint256).max);
    }

The test passes, confirming that type(uint256).max has been successfully set for both element type and weight, highlighting an extreme out-of-bounds scenario.

Tools Used

Manual inspection, Foundry

Recommended Mitigation Steps

Bound the element type and weight by the modulo operator in the desired range, potentially just making use of the already existing FighterFarm::_createFighterBase logic.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The DNA of a fighter is very important when determining rarity attributes. If one knows the DNA, one can deterministically calculate the rarity of a future NFT to mint. The protocol generally uses a pseudorandom method for calculating DNA with 2 degrees of freedom:

1. The NFT mint recipient address
2. The length of the fighters array

This method works sufficiently well to prevent users from playing timing games in order to maximize the mint NFT rarity, as each user has a different address, and hence the DNA would be different for different users (with the same fighters length). So there is basically no competition/MEV to mint a certain fighter at a certain fighters array length, as it would be different for each user anyway.

However this is not the case with the mintFromMergingPool user flow. We can see in FighterFarm::mintFromMergingPool below, the DNA is set as uint256(keccak256(abi.encode(msg.sender, fighters.length))). This is a mistake because in this particular function msg.sender will always be MergingPool, hence the pseudorandom 2 degrees of freedom for DNA has now collapsed to only 1: the length of the fighters array.

This means that, say, 20 unclaimed winners from MergingPool will all be competing for the exact same DNA (and hence fighter rarity) for a given fighter length, as there is only 1 degree of freedom. This introduces front-running from users to compete for deterministic rarity traits, and users will hold out from claiming their rewards until a sufficiently rare NFT is able to be claimed:

e.g. Fighter length from 200-350 may not produce any sufficiently rare fighter, but fighter length 351 gives a very rare NFT when claimed through MergingPool. Hence users will delay claiming their rewards until the next fighter length gets to 351, at which point all users will compete for the same, highly valuable NFT.

Overall this allows users to manipulate the rarity and hence value of their NFT (to a limited degree) by selectively choosing when to claim their reward (based on fighters length). This introduces undesirable timing/front-running games and rarity manipulation into the ecosystem. It also hurts fighter NFT liquidity, as sophisticated users will hold off minting fighters until they can maximise their rarity/value, with the most optimal fighters length.

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

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

Tools Used

Manual inspection

Recommended Mitigation Steps

Change the DNA formula in FighterFarm::mintFromMergingPool from uint256(keccak256(abi.encode(msg.sender, fighters.length))) to uint256(keccak256(abi.encode(to, fighters.length))), i.e. use to instead of msg.sender. This maintains 2 degrees of freedom within the DNA pseudorandom formula (consistent with other minting functions in FighterFarm), and prevents the exploitative timing and rarity games:

diff --git a/src/FighterFarm.sol b/src/FighterFarm.sol
index 06ee3e6..a90d03e 100644
--- a/src/FighterFarm.sol
+++ b/src/FighterFarm.sol
@@ -321,7 +321,7 @@ contract FighterFarm is ERC721, ERC721Enumerable {
         require(msg.sender == _mergingPoolAddress);
         _createNewFighter(
             to, 
-            uint256(keccak256(abi.encode(msg.sender, fighters.length))), 
+            uint256(keccak256(abi.encode(to, fighters.length))), 
             modelHash, 
             modelType,
             0,

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

MergingPool::claimRewards and FighterFarm::mintFromMergingPool function input argument tuples both contains a dynamic type, and the last element is a fixed size uint256 array with the calldata keyword (see below). This exactly fits the criteria for the Solidity compiler bug AbiReencodingHeadOverflowWithStaticArrayCleanup, which the Solidity team assigned as medium severity. This bug was resolved in solc v0.8.16, however this project currently uses solc 0.8.13:

    function claimRewards(
        string[] calldata modelURIs, 
        string[] calldata modelTypes,
        uint256[2][] calldata customAttributes
    ) 

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

    function mintFromMergingPool(
        address to, 
        string calldata modelHash, 
        string calldata modelType, 
        uint256[2] calldata customAttributes
    ) 

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

The bug effectively deletes the data in the first dynamic type, which would be modelHash/modelURIs in this case. Although this bug will only directly manifest in situations where abi-encoding reoccurs, i.e. external call, event, error etc. This happens with MergingPool::claimRewards (external call to FighterFarm.sol), meaning that 100% of the time a user calls MergingPool::claimRewards, all of their modelURIs values will be deleted to become empty strings.

Overall this bug causes unintended NFT data corruption in 100% of cases, when any user calls MergingPool::claimRewards. Future changes to code that include events or errors relating to this parameter would also trigger this bug, causing misleading logs/debugging.

Proof of Concept

Run testClaimRewardsForWinnersOfMultipleRoundIds from the MergingPoolTest test suite and add the following to MergingPoolTest::testClaimRewardsForWinnersOfMultipleRoundIds. Note we can demonstrate this bug on FighterFarm::mintFromMergingPool as well, although explicit ABI-reencoding would need to be added, as this function doesn't currently do this.

diff --git a/test/MergingPool.t.sol b/test/MergingPool.t.sol
index 7ad2ba6..349ef3f 100644
--- a/test/MergingPool.t.sol
+++ b/test/MergingPool.t.sol
@@ -190,6 +190,23 @@ contract MergingPoolTest is Test {
         // other winner claims rewards for previous roundIds
         vm.prank(_DELEGATED_ADDRESS);
         _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
+        // Getting attributes for claimed fighters, demonstrating data corruption of '_modelURIs'
+        (, , , , string memory _modelHash, string memory _modelType, ) = _fighterFarmContract.getAllFighterInfo(2);
+        console.log("modelHash: ", _modelHash);
+        console.log("modelType: ", _modelType);
+        assertEq(_modelHash, "");
+        (, , , , _modelHash, _modelType, ) = _fighterFarmContract.getAllFighterInfo(3);
+        console.log("modelHash: ", _modelHash);
+        console.log("modelType: ", _modelType);
+        assertEq(_modelHash, "");
+        (, , , , _modelHash, _modelType, ) = _fighterFarmContract.getAllFighterInfo(4);
+        console.log("modelHash: ", _modelHash);
+        console.log("modelType: ", _modelType);
+        assertEq(_modelHash, "");
+        (, , , , _modelHash, _modelType, ) = _fighterFarmContract.getAllFighterInfo(5);
+        console.log("modelHash: ", _modelHash);
+        console.log("modelType: ", _modelType);
+        assertEq(_modelHash, "");
         uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(_DELEGATED_ADDRESS);
         emit log_uint(numRewards);
         assertEq(numRewards, 0);

forge test --match-test testClaimRewardsForWinnersOfMultipleRoundIds -vvv

Output:

Compiling 1 files with 0.8.13
Solc 0.8.13 finished in 7.24s
Compiler run successful!

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testClaimRewardsForWinnersOfMultipleRoundIds() (gas: 2711770)
Logs:
  modelHash:  
  modelType:  original
  modelHash:  
  modelType:  original
  modelHash:  
  modelType:  original
  modelHash:  
  modelType:  original
  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.30ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As we can observe from the output, modelHash/_modelURIs from all 4 new fighter NFTs have been entirely corrupted to become empty strings (they should be ipfs strings from test), as function is currently susceptible to this bug.

Tools Used

Manual inspection, Foundry

Recommended Mitigation Steps

Compile code with >= solc 0.8.16 or otherwise rearrange FigherFarm::mintFromMergingPool and MergingPool::claimRewards input args so uint256[2] calldata customAttributes and uint256[2][] calldata customAttributes are not the last arguments respectively.

Assessed type

en/de-code

Winners of later rounds in MergingPool contract are charged more gas to claim their winnings

Explanation

The MergingPool::claimRewards function is not gas efficient as each loop through another round requires 2 extra SHA3 opcodes, 2 extra SLOADs and 1 extra SSTORE. This results in approximately 3000 extra gas per round (see PoC below) and is effectively an arbitrary penalty for winners of later rounds.

Since each round is intended to last 1 week, after 2 years there will have been +100 rounds. If a user wins round 99 instead of round 0, they must pay an additional arbitrary penalty of ~300,000 gas, which is inequitable.

This can be seen in the snippet and full function code below:

        for (uint32 currentRound = lowerBound; currentRound < roundId; currentRound++) {
            numRoundsClaimed[msg.sender] += 1;
            winnersLength = winnerAddresses[currentRound].length;

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

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

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

PoC

Add the following test to the MergingPoolTest test suite:

    function testDemonstrateGasChange() external {
        _mintFromMergingPool(_ownerAddress);
        _mintFromMergingPool(_DELEGATED_ADDRESS);
        assertEq(_fighterFarmContract.ownerOf(0), _ownerAddress);
        assertEq(_fighterFarmContract.ownerOf(1), _DELEGATED_ADDRESS);
        uint256[] memory _winners = new uint256[](2);
        _winners[0] = 0;
        _winners[1] = 1;

        // Moving to round 99. Comment out if doing round 0
        vm.store(address(_mergingPoolContract), bytes32(uint256(1)), bytes32(uint256(99)));

        // winners of roundId 99 are picked, or otherwise
        _mergingPoolContract.pickWinner(_winners);
        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);

        // other winner claims rewards for previous roundIds
        vm.prank(_DELEGATED_ADDRESS);
        _mergingPoolContract.claimRewards(_modelURIs, _modelTypes, _customAttributes);
        uint256 numRewards = _mergingPoolContract.getUnclaimedRewards(_DELEGATED_ADDRESS);
        emit log_uint(numRewards);
        assertEq(numRewards, 0);
    }

Run:

forge test --match-test testDemonstrateGasChange -vvv

Output for round 99 and round 0 respectively:

No files changed, compilation skipped

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testDemonstrateGasChange() (gas: 1640704)
Logs:
  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.30ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
Compiling 1 files with 0.8.13
Solc 0.8.13 finished in 6.44s
Compiler run successful!

Running 1 test for test/MergingPool.t.sol:MergingPoolTest
[PASS] testDemonstrateGasChange() (gas: 1361907)
Logs:
  0

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.33ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As we can see from the output, round 99 winner test cost 1640704 gas, whereas exact same setup except for round 0 cost only 1361907 gas, a 278,797 gas difference. Thus a winner at round 99 had to pay 278,797 more gas than winner 0, which should not occur.

Recommended Mitigation Steps

Use a storage pointer for numRoundsClaimed[msg.sender] to avoid an extra SHA3 opcode each round, and otherwise refactor the code to avoid unnecessary extra storage reads and writes