berndartmueller - Edge case scenario during LMPVault withdrawals results in the inability to withdraw

[sherlock-admin2]

berndartmueller

medium

Edge case scenario during LMPVault withdrawals results in the inability to withdraw

Summary

During LMPVault withdrawals, the amount of destination vault shares and debt to burn is potentially rounded up, leading to an underflow error when the amount of assets to pull is less than the amount of debt to decrease. This results in the inability to withdraw from the LMPVault under specific conditions.

Vulnerability Detail

Withdrawals from the LMPVault contract utilize the currently available idle funds and withdraw the remaining needed funds from the destination vaults, configured via the withdrawalQueue.

Destination vaults are iterated and withdrawn from as long as the total assets to pull (totalAssetsToPull) are not reached. The specific amount of destination vault shares to burn to receive the anticipated amount of base assets is calculated with the _calcUserWithdrawSharesToBurn function, which calls the LMPDebt._calcUserWithdrawSharesToBurn function. As a return value, the LMPDebt._calcUserWithdrawSharesToBurn function returns the amount of destination vault shares to burn (sharesToBurn) and the amount of debt to decrease (totalDebtBurn).

The maxAssetsToPull parameter of the LMPDebt._calcUserWithdrawSharesToBurn function is set to info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled). This limits the amount of assets to withdraw from the destination vault.

However, the subtraction within info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled) can possibly revert if the subtrahend is larger than the minuend. Specifically, this can happen if info.debtDecrease is larger than info.totalAssetsPulled and even larger than info.totalAssetsToPull.

To cause such a scenario, the LMPDebt._calcUserWithdrawSharesToBurn function needs to return a totalDebtBurn value that is larger than the provided maxAssetsToPull parameter, and the then withdrawn base assets need to be less than the anticipated totalAssetsToPull, to ensure the for loop continues to iterate. The next iteration, info.debtDecrease is potentially greater than info.totalAssetsToPull, causing a revert due to the underflow error.

How can the returned totalDebtBurn value from the LMPDebt._calcUserWithdrawSharesToBurn function be larger than the provided maxAssetsToPull parameter?

Observing the internals of the LMPDebt._calcUserWithdrawSharesToBurn function, we notice that the totalDebtBurn value is the result of two calculations:

Line 291: sharesToBurn = currentDvShares.mulDiv(maxAssetsToPull, currentDvDebtValue, Math.Rounding.Up);
Line 296: totalDebtBurn = cachedCurrentDebt.mulDiv(sharesToBurn, cachedDvShares, Math.Rounding.Up);

Both calculations use the Math.Rounding.Up rounding mode, rounding the result up to the next integer.

To demonstrate this behavior, we can use the following, very simplified, example (the same issue persists with large, realistic values as well):

Assume the following:

• currentDvShares = 1
• maxAssetsToPull = 1
• currentDvDebtValue = 2
• cachedCurrentDebt = 2
• cachedDvShares = 1

Calculating sharesToBurn:

$$ \begin{align} sharesToBurn &= currentDvShares \cdot \frac{maxAssetsToPull}{currentDvDebtValue} \\ &= 1 \cdot \frac{1}{2} \\ &= 0.5 \end{align} $$

0.5 is now rounded up to 1 due to the Math.Rounding.Up rounding mode.

Calculating totalDebtBurn:

$$ \begin{align} totalDebtBurn &= cachedCurrentDebt \cdot \frac{sharesToBurn}{cachedDvShares} \\ &= 2 \cdot \frac{1}{1} \\ &= 2 \end{align} $$

As we can see, the totalDebtBurn value (2) is larger than the provided maxAssetsToPull parameter (1).

In words, this basically means that the destination vault's full debt (2) and all shares (1) should be burned.

Let's now assume that this example was demonstrated with larger and realistic values (i.e., such as 1.000219247063460994160469e24). If the actual received base assets, received after swapping the underlyer tokens to base asset, is not sufficient to cover the funds needed to pull (info.totalAssetsToPull), the loop will continue with the next destination vault.

However, now it will revert due to the above mentioned subtraction issue!

The intended behaviour would be that the value of maxAssetsToPull, passed to the _calcUserWithdrawSharesToBurn function in the current iteration evaluates to 0, leading to no further withdrawal from the destination vault. Depending in the user's configured slippage control in the LMPVaultRouterBase contract, the withdrawal would then succeed.

P.S.: This issue was detected by fuzzing the LMPDebt._calcUserWithdrawSharesToBurn function. Here you can find the used fuzz test (please note that for simplicity, the affected code was taken out of context and simplified - however, the vulnerable logic remains the same):

https://gist.github.com/berndartmueller/932942d01a8e7d3c8e102eec9c8bbde0

Impact

Inability to withdraw from the LMPVault in the demonstrated scenario.

Code Snippet

src/vault/LMPVault.sol#L475

File: LMPVault.sol
448: function _withdraw(
449:     uint256 assets,
450:     uint256 shares,
451:     address receiver,
452:     address owner
453: ) internal virtual returns (uint256) {
454:     uint256 idle = totalIdle;
455:     WithdrawInfo memory info = WithdrawInfo({
456:         currentIdle: idle,
457:         assetsFromIdle: assets >= idle ? idle : assets,
458:         totalAssetsToPull: assets - (assets >= idle ? idle : assets),
459:         totalAssetsPulled: 0,
460:         idleIncrease: 0,
461:         debtDecrease: 0
462:     });
463:
464:     // If not enough funds in idle, then pull what we need from destinations
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),
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;
490:
491:             // It's possible we'll get back more assets than we anticipate from a swap
492:             // so if we do, throw it in idle and stop processing. You don't get more than we've calculated
493:             if (info.totalAssetsPulled > info.totalAssetsToPull) {
494:                 info.idleIncrease = info.totalAssetsPulled - info.totalAssetsToPull;
495:                 info.totalAssetsPulled = info.totalAssetsToPull;
496:                 break;
497:             }
498:
499:             // No need to keep going if we have the amount we're looking for
500:             // Any overage is accounted for above. Anything lower and we need to keep going
501:             // slither-disable-next-line incorrect-equality
502:             if (info.totalAssetsPulled == info.totalAssetsToPull) {
503:                 break;
504:             }
...      // [...]

src/vault/libs/LMPDebt.sol#L291-L296

232: function _calcUserWithdrawSharesToBurn(
233:     DestinationInfo storage destInfo,
234:     IDestinationVault destVault,
235:     uint256 userShares,
236:     uint256 maxAssetsToPull,
237:     uint256 totalVaultShares
238: ) external returns (uint256 sharesToBurn, uint256 totalDebtBurn) {
...      // [...]
282:
283:     // Shouldn't pull more than we want
284:     // Or, we're not in profit so we limit the pull
285:     if (currentDvDebtValue < maxAssetsToPull) {
286:         maxAssetsToPull = currentDvDebtValue;
287:     }
288:
289:     // Calculate the portion of shares to burn based on the assets we need to pull
290:     // and the current total debt value. These are destination vault shares.
291: ❌  sharesToBurn = currentDvShares.mulDiv(maxAssetsToPull, currentDvDebtValue, Math.Rounding.Up);
292:
293:     // This is what will be deducted from totalDebt with the withdrawal. The totalDebt number
294:     // is calculated based on the cached values so we need to be sure to reduce it
295:     // proportional to the original cached debt value
296: ❌  totalDebtBurn = cachedCurrentDebt.mulDiv(sharesToBurn, cachedDvShares, Math.Rounding.Up);
297: }

Tool used

Manual Review

Recommendation

Consider checking if the totalDebtBurn value in the LMPDebt._calcUserWithdrawSharesToBurn is greater than the provided maxAssetsToPull value, and if so, limit the returned totalDebtBurn value to the maxAssetsToPull value.

Duplicate of #519