Nyx - Hard-coded slippage may freeze user funds during market turbulence

[sherlock-admin2]

Nyx

high

Hard-coded slippage may freeze user funds during market turbulence

Summary

If volatile market conditions happen, users can't withdraw their funds from the vault.

Vulnerability Detail

function withdraw(
        ILMPVault vault,
        address to,
        uint256 amount,
        uint256 maxSharesOut,
        bool unwrapWETH
    ) public virtual override returns (uint256 sharesOut) {
        address destination = unwrapWETH ? address(this) : to;

        sharesOut = vault.withdraw(amount, destination, msg.sender);
        if (sharesOut > maxSharesOut) {
            revert MaxSharesError();
        }

        if (unwrapWETH) {
            _processWethOut(to);
        }
    }

When the user withdraws, the user can use maxSharesOut parameter for slippage protection. There is one more slippage protection inside the withdrawing process.

uint256 actualAssets = _withdraw(assets, shares, receiver, owner);

        if (actualAssets < assets) {
            revert TooFewAssets(assets, actualAssets);
        }

If the actualAssests that returns from _withdraw() function is less than the user input assets parameter, the function will revert. If there are not enough idle funds in the LMPVault, the rest of the funds are withdrawn from Destination Vaults.

uint256 withdrawalQueueLength = withdrawalQueue.length;
            for (uint256 i = 0; i < withdrawalQueueLength; ++i) {
                IDestinationVault destVault = IDestinationVault(withdrawalQueue[i]);
                (uint256 sharesToBurn, uint256 totalDebtBurn) = _calcUserWithdrawSharesToBurn(
                    destVault,
                    shares,
                    info.totalAssetsToPull - Math.max(info.debtDecrease, info.totalAssetsPulled),
                    totalVaultShares
                );
                if (sharesToBurn == 0) {
                    continue;
                }

                uint256 assetPreBal = _baseAsset.balanceOf(address(this));
                uint256 assetPulled = destVault.withdrawBaseAsset(sharesToBurn, address(this));

If one destination vault is not enough, funds are withdrawn from other destination vaults as well. (withdrawalQueue.length)

The problem is, let's say there are 3 destination vaults. (withdrawalQueue.length == 3) The volatile market condition happens, and funds coming from destination vaults are less than usual.(due to swaps) There is a chance that actualAssests won't equal user input assets parameter.

Because of too strict or hard-coded slippage, the user can't withdraw his funds and his funds are locked.

Impact

Users funds may be locked.

Code Snippet

https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L400-L419

https://github.com/sherlock-audit/2023-06-tokemak/blob/5d8e902ce33981a6506b1b5fb979a084602c6c9a/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L469-L480

Tool used

Manual Review

Recommendation

Pass a slippage parameter for the assets or remove the check.

Duplicate of #450

[berndartmueller]

Escalate

I do not consider this submission a duplicate of #519, which demonstrates an arithmetic underflow error rendering withdrawals impossible.

While this submission here highlights a slippage issue in https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L416-L418, reverting if the withdrawn assets are less than the anticipated withdrawal amount.

[sherlock-admin2]

Escalate

I do not consider this submission a duplicate of #519, which demonstrates an arithmetic underflow error rendering withdrawals impossible.

While this submission here highlights a slippage issue in https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/src/vault/LMPVault.sol#L416-L418, reverting if the withdrawn assets are less than the anticipated withdrawal amount.

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]

This issue seems a dup of #450. withdraw function will only work as written when there isn't any slippage. Users should use redeem function to avoid reverting due to slippage of withdrawal.

[Evert0x]

Planning to accept escalation and duplicate with #450

[Evert0x]

Result: Invalid Duplicate of #450

Escalation is accepted even if issue is invalid as main issue was valid when the escalation was created.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• berndartmueller: accepted