ADD-04 MitigationConfirmed

[c4-bot-5]

Lines of code

Vulnerability details

Vulnerability details

The mitigation purpose of ADD-04 is composed of 3 parts:

1) Mitigated unstakeNRN 2) Mitigated setNewRound 3) Mint upto MAX_SUPPLY

1) Mitigated unstakeNRN

The mitigation applied in RankedBattle.unstakeNRN() was suggested by #137 and was reported as M-06. We describe the vulnerability and comment on the mitigation there. We don't find any suggestion about it in #1490, which should be the report relative to ADD-04.

2) Mitigated setNewRound

This issue was reported by #1490 L-6. The issue is in the RankedBattle. This contract uses a state variable called globalStakedAmount to track the overall staked amount. Each player can stake an amount of NRN on one of his/her fighters and try to win battles to gain points. When a fighter, i.e., a tokenId with an amount of NRN staked on it loses a battle, that amount is at risk. If the player is not able to win back that amount, at the end of the round, it will be permanently lost. For this reason, when admin role starts the new round calling RankedBattle.setNewRound(), the globalStakedAmount should be updated removing the totalStakeAtRisk amount: at the start of a new round, globalStakedAmount should be equal to the sum of all elements in amountStaked.

Because this value isn't used anywhere, this issue doesn't introduce a security vulnerability. This is the mitigation proposed by wardens:

RankedBattle.sol#L233-L239

    233      function setNewRound() external {
    234          require(isAdmin[msg.sender]);
    235          require(totalAccumulatedPoints[roundId] > 0);
+                globalStakedAmount = globalStakedAmount - _stakeAtRiskInstance.totalStakeAtRisk(roundId);
    236          roundId += 1;
    237          _stakeAtRiskInstance.setNewRound(roundId);
    238          rankedNrnDistribution[roundId] = rankedNrnDistribution[roundId - 1];
    239      }

This solution was implemented by the Ai Arena team. We consider this issue mitigated. Furthermore, we don't think it could introduce new vulnerabilities. The only problem we found is that if the new line underflowed, it would be not possible to start a new round by the admin role. We want to show that this can't happen. To reach such a state, it should be:

globalStakedAmount < _stakeAtRiskInstance.totalStakeAtRisk(roundId)

The _stakeAtRiskInstance.totalStakeAtRisk can be increased only using StakeAtRisk.updateAtRiskRecords(), which is called in RankedBattle.sol#L537 when a fighter loses a battle and it doesn't have accumulated point during this round.

Because globalStakedAmount is increased every time amountStaked is increased:

RankedBattle.sol#L289-L290

289              amountStaked[tokenId] += amount;
290              globalStakedAmount += amount;

except when amountStaked is increased in RankedBattle._addResultPoints:

RankedBattle.sol#L504

504                  amountStaked[tokenId] += curStakeAtRisk;

So, during a round, this invariant should always hold:

globalStakedAmount = sum(amountStaked) + totalStakeAtRisk(currentRound)

When a new round starts, totalStakeAtRisk(currentRound) is 0, and the invariant above becomes:

globalStakedAmount = sum(amountStaked)

Before mitigation, the value of totalStakeAtRisk for the previous round wasn't removed. So during the new round, we had:

globalStakedAmount = sum(amountStaked) + totalStakeAtRisk(previousRound) + totalStakeAtRisk(currentRound)

This breaks the invariant above.

Now, after mitigation, the value of totalStakeAtRisk(previousRound) is removed at the beginning of the round. For the reason above, we think the right invariant is respected and the issue is mitigated.

3) Mint upto MAX_SUPPLY

This issue was reported by #1490 L-5. The issue is in the Neuron.mint()

Neuron.sol#L151-L159

151      /// @notice Mints the specified amount of tokens to the given address.
152      /// @dev The caller must have the minter role.
153      /// @param to The address to which the tokens will be minted.
154      /// @param amount The amount of tokens to be minted.
155      function mint(address to, uint256 amount) public virtual {
156          require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
157          require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
158          _mint(to, amount);
159      }

In the line #156 the require statement checks if the caller is not "Trying to mint more than the max supply". However, this check excludes the case where the caller is trying to mint exactly the max supply. So it could be better (and gas-safe) to add equality:

    151      /// @notice Mints the specified amount of tokens to the given address.
    152      /// @dev The caller must have the minter role.
    153      /// @param to The address to which the tokens will be minted.
    154      /// @param amount The amount of tokens to be minted.
    155      function mint(address to, uint256 amount) public virtual {
-   156          require(totalSupply() + amount < MAX_SUPPLY, "Trying to mint more than the max supply");
+                require(totalSupply() + amount <= MAX_SUPPLY, "Trying to mint more than the max supply");
    157          require(hasRole(MINTER_ROLE, msg.sender), "ERC20: must have minter role to mint");
    158          _mint(to, amount);
    159      }

This solution was implemented by the Ai Arena team. We consider this issue mitigated.

[L-0] iconsTypes length should also be checked in FighterFarm::redeemMintPass(...)

#1490 also reported this issue which doesn't appear in ADD-04 purpose, but it was mitigated in Fix 1490 #14.

The FighterFarm.redeemMintPass takes many arrays as input parameters. There was a lack of check on the length of iconsTypes. After mitigation, all arrays must have the same length:

FighterFarm.sol#L261-L278

    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 
    271      {
    272          require(
    273              mintpassIdsToBurn.length == mintPassDnas.length && 
    274              mintPassDnas.length == fighterTypes.length && 
    275              fighterTypes.length == modelHashes.length &&
-                    modelHashes.length == modelTypes.length
+   276              modelHashes.length == modelTypes.length &&
+   277              modelTypes.length == iconsTypes.length
    278          );

This additional check doesn't introduce any new vulnerability and mitigates the initial issue.

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 marked the issue as confirmed for report