ctf_sec - remaining amount AssetManager.sol#withdraw is not handled properly when user unstake in UserManager.sol#unstake

[sherlock-admin]

ctf_sec

medium

remaining amount AssetManager.sol#withdraw is not handled properly when user unstake in UserManager.sol#unstake

Summary

remaining amount is not handled properly when user unstake in AssetManager.sol#withdraw

Vulnerability Detail

When the user unstake, the UserManager.sol calls AssetManager.sol then withdraw from the money Market.sol

However, the desired unstake balance may not match the actually fund unstaked.

In the function AssetManager.sol#withdraw,

the code first check if there are available fund in AssetManager.sol, if it is, transfer the fund out and update remaining amount.

  uint256 remaining = amount;

  // If there are tokens in Asset Manager then transfer them on priority
  uint256 selfBalance = IERC20Upgradeable(token).balanceOf(address(this));
  if (selfBalance > 0) {
      uint256 withdrawAmount = selfBalance < remaining ? selfBalance : remaining;
      remaining -= withdrawAmount;
      IERC20Upgradeable(token).safeTransfer(account, withdrawAmount);
  }

if the remaining amount is not 0, we loop over all the money market and keep withdraw fund.

if (isMarketSupported(token)) {
      uint256 withdrawSeqLength = withdrawSeq.length;
      // iterate markets according to defined sequence and withdraw
      for (uint256 i = 0; i < withdrawSeqLength && remaining > 0; i++) {
          IMoneyMarketAdapter moneyMarket = moneyMarkets[withdrawSeq[i]];
          if (!moneyMarket.supportsToken(token)) continue;

          uint256 supply = moneyMarket.getSupply(token);
          if (supply == 0) continue;

          uint256 withdrawAmount = supply < remaining ? supply : remaining;
          remaining -= withdrawAmount;
          moneyMarket.withdraw(token, account, withdrawAmount);
      }
  }

However, at this point, the remaining amount does not guarantee to be 0, the remaining amount can be positive, means the desired unstaking amount does not match the actual staking amount.

Impact

User may get less fund then they expeceted when unstaking because even though there are remaining. The code updated the unstaked amount as if there is no remaining amount.

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L700-L707

Code Snippet

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L700-L707

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L335-L367

Tool used

Manual Review

Recommendation

I think the code can return the remaining amount and not deducting the remaining from the staking balance.

We change from

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/asset/AssetManager.sol#L361-L364

to

        if (!_isUToken(msg.sender, token)) {
            balances[msg.sender][token] = balances[msg.sender][token] - amount + remaining;
            totalPrincipal[token] = totalPrincipal[token] - amount + remaining;
            return 0;
        } else {
           return remaining;
        }

then in UserManager.sol

we change from

https://github.com/sherlock-audit/2022-10-union-finance/blob/main/union-v2-contracts/contracts/user/UserManager.sol#L700-L708

to

        staker.stakedAmount -= amount;
        totalStaked -= amount;

       uint256 remaining = AssetManager(assetManager).withdraw(stakingToken, msg.sender, amount);

        staker.stakedAmount += remaining;
        totalStaked += amount;

        emit LogUnstake(msg.sender, amount - remaining);

we update the state after external call but the function unstake is marked as nonReentrant, so reentrancy is not a problem.

Duplicate of #27