VotingEscrowUpgradeableV1_2::onAttachToManagedNFT can overflow

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x8f6669292fda0a615d8ce64a263d71792b026d6e4b8649085f99716ce40c7217 Severity: medium

Description: Description\ Due to int128 used to represent the locked amount of given tokens, onAttachToManagedNFT can overflow when amounts of both tokendId and managedTokenId are being summed:

function onAttachToManagedNFT(uint256 tokenId_, uint256 managedTokenId_) external nonReentrant returns (uint256) {
      _onlyManagedNFTManager();
      _onlyNormalNFT(tokenId_);

      require(!voted[tokenId_], "voted");

      require(IManagedNFTManager(managedNFTManager).isManagedNFT(managedTokenId_), "not managed nft");

      require(_balanceOfNFT(tokenId_, block.timestamp) > 0, "zero balance");

      int128 amount = locked[tokenId_].amount;
      uint256 cAmount = uint256(int256(amount));

      if (locked[tokenId_].isPermanentLocked) {
          permanentTotalSupply -= cAmount;
      }

      _checkpoint(tokenId_, locked[tokenId_], LockedBalance(0, 0, false));
      locked[tokenId_] = LockedBalance(0, 0, false);

      permanentTotalSupply += cAmount;

      LockedBalance memory newLocked = locked[managedTokenId_];
      newLocked.amount += amount;//ISSUE this can overflow?

      _checkpoint(managedTokenId_, locked[managedTokenId_], newLocked);

      locked[managedTokenId_] = newLocked;

      return cAmount;
  }

There is no validation whether the sum is less than int128 max and if it’s bigger the number become negative, indicating no voting power (bias will remain unchanged in _checkpoint, since there is check for positive new_locked::amount):

function _checkpoint(uint _tokenId, LockedBalance memory old_locked, LockedBalance memory new_locked) internal {
      Point memory u_old;
      Point memory u_new;
      int128 old_dslope = 0;
      int128 new_dslope = 0;
      uint _epoch = epoch;
      int128 permanent;

      if (_tokenId != 0) {
          permanent = new_locked.isPermanentLocked ? new_locked.amount : int128(0);

          // Calculate slopes and biases
          // Kept at zero when they have to
          if (old_locked.end > block.timestamp && old_locked.amount > 0) {
              u_old.slope = old_locked.amount / iMAXTIME;
              u_old.bias = u_old.slope * int128(int256(old_locked.end - block.timestamp));
          }
          if (new_locked.end > block.timestamp && new_locked.amount > 0) {//@audit if balance is less than 0 this won't be re-calculated
              u_new.slope = new_locked.amount / iMAXTIME;
              u_new.bias = u_new.slope * int128(int256(new_locked.end - block.timestamp));
          }

          // Read values of scheduled changes in the slope
          // old_locked.end can be in the past and in the future
          // new_locked.end can ONLY by in the FUTURE unless everything expired: than zeros
          old_dslope = slope_changes[old_locked.end];
          if (new_locked.end != 0) {
              if (new_locked.end == old_locked.end) {
                  new_dslope = old_dslope;
              } else {
                  new_dslope = slope_changes[new_locked.end];
              }
          }
      }

Attack Scenario\ User holding significant amount intentionally can brick managed token with big amount in locked[managedTokenId].amount by simply attaching his token to it, then after the vote delay passes he can again perform this action on another managed token.

Or many issue can arise even unintentionally when multiple tokens with significant amount decide to manage to one token.

Attachments

1. Proof of Concept (PoC) File
2. Revised Code File (Optional) Use int258 or apply check whether the summed amount of 2 tokens do not exceed the max amount.

[0xmahdirostami]

for now, I will judge these submissions as well

[BohdanHrytsak]

@0xmahdirostami

Solidity version 0.8.19 will prevent overflows in these places.

And the token of this contract in the context of the project is FNX, which does not provide for the supply of more than int128.max. (fnx deciamls = 18).

Therefore, in my opinion, this submission is not valid

[0xmahdirostami]

After re-evaluating the issue and taking into account the locked time, the block time, and the total supply of the 18-decimal token, it appears that this issue may not be valid as the downcasting here seems to be safe. (The max value of int128 is at order of ~1e38)