H-07 MitigationConfirmed

[c4-bot-1]

Lines of code

Vulnerability details

Lines of code

Mitigated lines of code

https://github.com/ArenaX-Labs/2024-02-ai-arena-mitigation/blob/fix-47/src/FighterFarm.sol#L137-L144

Vulnerability description

The issue was reported in #45.

This issue is due to the missing initialization of numElements mapping with an index greater than 0. numElements describe the number of elements for each generation and it is defined in FighterFarm.sol#L84-L85:

FighterFarm.sol#L84-L85

84      /// @notice Mapping of number elements by generation.
85      mapping(uint8 => uint8) public numElements;

This means that each index of numElements is initialized with value 0. Then, inside FighterFarm.constructor(), numElements[0] is defined in FighterFarm.sol#L110:

FighterFarm.sol#L104-L111

104      constructor(address ownerAddress, address delegatedAddress, address treasuryAddress_)
105          ERC721("AI Arena Fighter", "FTR")
106      {
107          _ownerAddress = ownerAddress;
108          _delegatedAddress = delegatedAddress;
109          treasuryAddress = treasuryAddress_;
110          numElements[0] = 3;
111      } 

There is no other function that can set numElements for generations > 0. This means that numElements[i]=0 for i > 0. numElements is used only inside FighterFarm._createFighterBase() at FighterFarm.sol#L470:

FighterFarm.sol#L462-L474

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      }

So, when generations > 0, numElements=0, and the formula at FighterFarm.sol#L470 tries to apply modulo 0, which reverts. This cause a DoS of two core functions: FighterFarm._createNewFighter() (when it is used with param customAttributes[0] = 100), and FighterFarm.reRoll()

Recommended Mitigation proposed by wardens

The mitigation proposal is to add a function usable just by the admin to update the numElements mapping.

@@ -133,6 +133,15 @@ contract FighterFarm is ERC721, ERC721Enumerable {
        return generation[fighterType];
    }

+   /// @notice Updates the number of elements for a given generation.
+   /// @dev Only the owner address is authorized to call this function.
+   /// @param newNumElements number of elements for the generation.
+   /// @param generation_ generation to be updated.
+   function setNumElements(uint8 newNumElements, uint8 generation_) external {
+       require(msg.sender == _ownerAddress);
+       numElements[generation_] = newNumElements;
+   }
+
    /// @notice Adds a new address that is allowed to stake fighters on behalf of users.
    /// @dev Only the owner address is authorized to call this function.
    /// @param newStaker The address of the new staker

This solution was implemented by the Ai Arena team

Comment about the Mitigation Proposal

The solution above mitigates the issue. Now, the admin can set numElements for a specific generation. However, we suggest some improvements.

1) It should not be possible to update numElements for the current generation because otherwise, we could have fighter of the same generation with different numElements. Because the generation is defined for each fighterType and can only increase, we propose to make updatable only numElements for generations that have not been reached yet by both fighterType:

FighterFarm.sol#L137-L144

137      /// @notice Updates the number of elements for a given generation.
138      /// @dev Only the owner address is authorized to call this function.
139      /// @param newNumElements number of elements for the generation.
140      /// @param generation_ generation to be updated.
141      function setNumElements(uint8 newNumElements, uint8 generation_) external {
142          require(msg.sender == _ownerAddress);
+            uint256 maxGeneration = generation[0] > generation[1] ? generation[0] : generation[1]
+            require(generation_ > maxGeneration)
143          numElements[generation_] = newNumElements;
144      }

2) It is still possible that admin sets numElements=0. We propose to avoid this, and to add a safe mechanism to the FighterFarm._createFighterBase() function:

FighterFarm.sol#L137-L144, FighterFarm.sol#L508-L520

137      /// @notice Updates the number of elements for a given generation.
138      /// @dev Only the owner address is authorized to call this function.
139      /// @param newNumElements number of elements for the generation.
140      /// @param generation_ generation to be updated.
141      function setNumElements(uint8 newNumElements, uint8 generation_) external {
142          require(msg.sender == _ownerAddress);
+            require(newNumElements >= 1);
             uint256 maxGeneration = generation[0] > generation[1] ? generation[0] : generation[1]
             require(generation_ > maxGeneration)
143          numElements[generation_] = newNumElements;
144      }
[...]
508      function _createFighterBase(
509          uint256 dna, 
510          uint8 fighterType
511      ) 
512          private 
513          view 
514          returns (uint256, uint256, uint256) 
515      {
+            uint256 element = dna % (numElements[generation[fighterType]]+1);
-516         uint256 element = dna % numElements[generation[fighterType]];
517          uint256 weight = dna % 31 + 65;
518          uint256 newDna = fighterType == 0 ? dna : uint256(fighterType);
519          return (element, weight, newDna);
520      }

The proposed safe mechanism above is to be sure numElements>0, if it doesn't care about the modulo used on dna.

3) The proposed mitigation forces the admin to remember to update numElements for future generations. To be safer against centralization risk and not depend on manual mechanism, we suggest updating numElements>0 when a generation is increased, in this way:

FighterFarm.sol#L125-L135

125      /// @notice Increase the generation of the specified fighter type.
126      /// @dev Only the owner address is authorized to call this function.
127      /// @param fighterType Type of fighter either 0 or 1 (champion or dendroid).
128      /// @return Generation count of the fighter type.
129      function incrementGeneration(uint8 fighterType) external returns (uint8) {
130          require(msg.sender == _ownerAddress);
131          require(fighterType == 0 || fighterType == 1);
132          generation[fighterType] += 1;
133          maxRerollsAllowed[fighterType] += 1;
+            if(numElements[generation[fighterType] + 1] == 0) numElements[generation[fighterType] + 1] = numElements[generation[fighterType]]
134          return generation[fighterType];
135      }

In this way, numElements for the next generation is always initialized.

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as confirmed for report