H-08 MitigationConfirmed

[c4-bot-7]

Lines of code

Vulnerability details

Lines of code

Old lines of code

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

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

Mitigated lines of code

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/fix-47/src/MergingPool.sol#L5 https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/fix-47/src/MergingPool.sol#L11 https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/fix-47/src/MergingPool.sol#L142-L149

Vulnerability details

The issue was reported in #37.

The issue is because MergingPool.claimRewards() is vulnerable to reentrancy.

MergingPool.sol#L139-L167

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      }

A malicious player could call MergingPool.claimRewards() using a contract. This function mints multiple fighter using FighterFarm.mintFromMergingPool(). A malicious user can build an attacker contract that implements Openzeppelin.IERC721Receiver overriding the onERC721Received() which is triggered by _mint() action. This malicious method could reenter calling MergingPool.claimRewards() again, before the first call is finished, obtaining more fighters than he should have.

Recommended Mitigation proposed by wardens

The mitigation proposal is to use the nonReentrant modifier on MergingPool.claimRewards():

    @@ -2,11 +2,13 @@
    pragma solidity >=0.8.0 <0.9.0;

    import { FighterFarm } from "./FighterFarm.sol";
+   import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+

    /// @title MergingPool
    /// @author ArenaX Labs Inc.
    /// @notice This contract allows users to potentially earn a new fighter NFT.
-   contract MergingPool {
+   contract MergingPool is ReentrancyGuard{

        /*//////////////////////////////////////////////////////////////
                                    EVENTS
    @@ -141,7 +143,7 @@ contract MergingPool {
            string[] calldata modelTypes,
            uint256[2][] calldata customAttributes
        ) 
-           external 
+           external nonReentrant
        {
            uint256 winnersLength;
            uint32 claimIndex = 0;

This solution was implemented by the Ai Arena team.

Comment about the Mitigation Proposal

This mitigation is commonly used to avoid Reentrancy attacks. According to OpenZeppelin Documentation:

/**
* @dev Prevents a contract from calling itself, directly or indirectly.
* Calling a `nonReentrant` function from another `nonReentrant`
* function is not supported. It is possible to prevent this from happening
* by making the `nonReentrant` function external, and making it call a
* `private` function that does the actual work.
*/

So, it will not be possible to call

`MergingPool.claimRewards()` -> `FighterFarm.mintFromMergingPool()` -> malicious contract -> `MergingPool.claimRewards()`

We consider this issue mitigated. We want to add the we found another core functions that should be protected against reentrancy: FighterFarm.claimFighters() and FighterFarm.redeemMintPass()

FighterFarm.sol#L218-L249

    218      function claimFighters(
    219          uint8[2] calldata numToMint,
    220          bytes calldata signature,
    221          string[] calldata modelHashes,
    222          string[] calldata modelTypes
    223      ) 
-   224          external 
+                external nonReentrant
    225      {
    226          bytes32 msgHash = bytes32(keccak256(abi.encode(
    227              msg.sender, 
    228              numToMint[0], 
    229              numToMint[1],
    230              nftsClaimed[msg.sender][0],
    231              nftsClaimed[msg.sender][1]
    232          )));
    233          require(Verification.verify(_delegatedAddress, msgHash, signature));
    234          uint16 totalToMint = uint16(numToMint[0] + numToMint[1]);
    235          require(modelHashes.length == totalToMint && modelTypes.length == totalToMint);
    236          nftsClaimed[msg.sender][0] += numToMint[0];
    237          nftsClaimed[msg.sender][1] += numToMint[1];
    238          for (uint16 i = 0; i < totalToMint; i++) {
    239              _createNewFighter(
    240                  msg.sender, 
    241                  uint256(keccak256(abi.encode(msg.sender, nftsClaimed[msg.sender][0], nftsClaimed[msg.sender][1]))),
    242                  modelHashes[i], 
    243                  modelTypes[i],
    244                  i < numToMint[0] ? 0 : 1,
    245                  0,
    246                  [uint256(100), uint256(100)]
    247              );
    248          }
    249      }

FighterFarm.sol#L261-L304

    261      function redeemMintPass(
    262          uint256[] calldata mintpassIdsToBurn,
    263          uint8[] calldata fighterTypes,
    264          uint8[] calldata iconsTypes,
    265          string[] calldata mintPassDnas,
    266          string[] calldata modelHashes,
    267          string[] calldata modelTypes,
    268          bytes calldata signature
    269      ) 
-   270          external 
+                external nonReentrant
    271      {
    272          require(
    273              mintpassIdsToBurn.length == mintPassDnas.length && 
    274              mintPassDnas.length == fighterTypes.length && 
    275              fighterTypes.length == modelHashes.length &&
    276              modelHashes.length == modelTypes.length &&
    277              modelTypes.length == iconsTypes.length
    278          );
    279  
    280           bytes32 msgHash = bytes32(keccak256(abi.encode(
    281            mintpassIdsToBurn,
    282            fighterTypes,
    283            iconsTypes,
    284            mintPassDnas,
    285            modelHashes,
    286            modelTypes
    287          )));
    288  
    289          require(Verification.verify(_delegatedAddress, msgHash, signature));
    290  
    291          for (uint16 i = 0; i < mintpassIdsToBurn.length; i++) {
    292              require(msg.sender == _mintpassInstance.ownerOf(mintpassIdsToBurn[i]));
    293              _mintpassInstance.burn(mintpassIdsToBurn[i]);
    294              _createNewFighter(
    295                  msg.sender, 
    296                  uint256(keccak256(abi.encode(mintPassDnas[i]))), 
    297                  modelHashes[i], 
    298                  modelTypes[i],
    299                  fighterTypes[i],
    300                  iconsTypes[i],
    301                  [uint256(100), uint256(100)]
    302              );
    303          }
    304      }

They are both protected against reentrancy: FighterFarm.claimFighters() thanks to [L230-L231] which changes at every call and avoids using the same signature twice, FighterFarm.redeemMintPass() thanks to the fact that the used mintPass is burned. However, we suggest protecting both using OpenZeppelin ReentrancyGuard, because they mint a new ERC721 token and so they both could trigger the onERC721Received method of a malicious contract. In future development, without this protection, some vulnerabilities could be introduced.

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as confirmed for report