nobody2018 - When DestinationVault is at loss, LMPVault._withdraw may revert in some cases due to underflow

[sherlock-admin]

nobody2018

medium

When DestinationVault is at loss, LMPVault._withdraw may revert in some cases due to underflow

Summary

[_calcUserWithdrawSharesToBurn](https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/libs/LMPDebt.sol#L231) is used to figure out how many shares we can burn from the destination as well as what our totalDebt deduction should be (totalDebt being a cached value). If its at a loss, they can only burn an amount proportional to their ownership of LMPVault. totalDebtBurn is the return value of this function, which is calculated based on the cached values. Since DestinationVault is at a loss, totalDebtBurn represents past debts, which may be higher than totalAssetsToPull. This will cause the subtraction underflow in [L475](https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L475) and revert. Users cannot withdraw funds.

Vulnerability Detail

This problem can occur when totalIdle has funds, but not enough to cover the funds the user wants to withdraw. _calcUserWithdrawSharesToBurn returns two values: sharesToBurn and totalDebtBurn. totalDebtBurn represents the past debt, and sharesToBurn represents the share to be withdrawn in DestinationVault. When DestinationVault is at a loss, the assetPulled by destVault.withdrawBaseAsset(sharesToBurn, address(this)) may be less than totalDebtBurn. If totalDebtBurn is greater than info.totalAssetsToPull, then subtraction underflow may occur at L475. In other words, this condition is met: assetPulled < info.totalAssetsToPull < totalDebtBurn. Because when i=1, the info.debtDecrease and info.totalAssetsPulled of L475 are equivalent to totalDebtBurn and assetPulled obtained when i=0 (L488-489).

File: v2-core-audit-2023-07-14\src\vault\LMPVault.sol
465:         if (info.totalAssetsToPull > 0) {
466:             uint256 totalVaultShares = totalSupply();
467: 
468:             // Using pre-set withdrawalQueue for withdrawal order to help minimize user gas
469:             uint256 withdrawalQueueLength = withdrawalQueue.length;
470:             for (uint256 i = 0; i < withdrawalQueueLength; ++i) {
471:                 IDestinationVault destVault = IDestinationVault(withdrawalQueue[i]);
472:->               (uint256 sharesToBurn, uint256 totalDebtBurn) = _calcUserWithdrawSharesToBurn(
473:                     destVault,
474:                     shares,
475:->                   info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled),//maybe underflow
476:                     totalVaultShares
477:                 );
478:                 if (sharesToBurn == 0) {
479:                     continue;
480:                 }
481: 
482:                 uint256 assetPreBal = _baseAsset.balanceOf(address(this));
483:->               uint256 assetPulled = destVault.withdrawBaseAsset(sharesToBurn, address(this));
484: 
485:                 // Destination Vault rewards will be transferred to us as part of burning out shares
486:                 // Back into what that amount is and make sure it gets into idle
487:                 info.idleIncrease += _baseAsset.balanceOf(address(this)) - assetPreBal - assetPulled;
488:->               info.totalAssetsPulled += assetPulled;
489:->               info.debtDecrease += totalDebtBurn;
.....
493:                 if (info.totalAssetsPulled > info.totalAssetsToPull) {
.....
496:                     break;
497:                 }
.....
502:                 if (info.totalAssetsPulled == info.totalAssetsToPull) {
503:                     break;
504:                 }
505:             }
506:         }

Impact

When DestinationVault is at a loss, users may not be able to withdraw funds due to underflow.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L475

Tool used

Manual Review

Recommendation

File: v2-core-audit-2023-07-14\src\vault\LMPVault.sol
470:             for (uint256 i = 0; i < withdrawalQueueLength; ++i) {
471:                 IDestinationVault destVault = IDestinationVault(withdrawalQueue[i]);
472:                 (uint256 sharesToBurn, uint256 totalDebtBurn) = _calcUserWithdrawSharesToBurn(
473:                     destVault,
474:                     shares,
475:-                    info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled),
475:+                    info.totalAssetsToPull >= Math.max(info.debtDecrease, info.totalAssetsPulled) ? info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled) : info.totalAssetsToPull - Math.min(info.debtDecrease, info.totalAssetsPulled),
476:                     totalVaultShares
477:                 );
478:                 if (sharesToBurn == 0) {
479:                     continue;
480:                 }

Duplicate of #519

[securitygrid]

Escalate
This is dup of #519

[sherlock-admin2]

Escalate
This is dup of #519

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Trumpero]

Agree with the escalation, this issue is a duplicate of #519. It was my mistake when moving issues into the folders.

[Evert0x]

Planning to accept escalation

[Evert0x]

Result:
Medium
Duplicate of #519

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• securitygrid: accepted