Ch_301 - Swap fees will lead the withdraw to revert

[sherlock-admin]

Ch_301

medium

Swap fees will lead the withdraw to revert

Summary

This issue is a result of two problems
1- This check
2- The fact that any swap has swap fees and some slippage, so the max here is always info.debtDecrease

Vulnerability Detail

Impact

• users are not able to use withdraw() to get their funds from the LMP Vault

Code Snippet

Please copy the following POC in LMPVault-Withdraw.t.sol

    /*to run this POC successfully  
    we need to add `- 1` in LMPVault.sol
    like this:
    uint256 assetPulled = destVault.withdrawBaseAsset(sharesToBurn, address(this)) - 1;

    we are doing that to simulate swap fees or slippage*/

    function test_POC_11() public {
        address user_01 = vm.addr(101);
        address user_02 = vm.addr(102);
        address solver = vm.addr(23_423_434);
        vm.label(user_01, "User_01");
        vm.label(user_02, "User_02");
        vm.label(solver, "solver");
        _accessController.grantRole(Roles.SOLVER_ROLE, solver);
        _accessController.grantRole(Roles.LMP_FEE_SETTER_ROLE, address(this));


        //User_01 `deposit()`
        vm.startPrank(user_01);
        _asset.mint(user_01, 500);
        _asset.approve(address(_lmpVault), 500);
        _lmpVault.deposit(500, user_01);
        vm.stopPrank();

        //User_02 `deposit()`
        vm.startPrank(user_02);
        _asset.mint(user_02, 500);
        _asset.approve(address(_lmpVault), 500);
        _lmpVault.deposit(500, user_02);
        vm.stopPrank();

        // Token prices
        // _asset - 1:1 ETH
        // _underlyer1 - 1:2 ETH
        // _underlyer2 - 1:2.5 ETH
        
        _mockRootPrice(address(_underlyerTwo), 2.5 ether);


        _underlyerOne.mint(solver, 100);
        _underlyerTwo.mint(solver, 320);

        vm.startPrank(solver);
        _underlyerOne.approve(address(_lmpVault), 100);
        _lmpVault.rebalance(
            address(_destVaultOne),
            address(_underlyerOne), // tokenIn
            100,
            address(0), // destinationOut, none when sending out baseAsset
            address(_asset), // baseAsset, tokenOut
            200
        );
        _underlyerTwo.approve(address(_lmpVault), 320);
        _lmpVault.rebalance(
            address(_destVaultTwo),
            address(_underlyerTwo), // tokenIn
            320,
            address(0), // destinationOut, none when sending out baseAsset
            address(_asset), // baseAsset, tokenOut
            800
        );
        vm.stopPrank();

        // At this point we've transferred 1000 idle out, which means we
        // should have 0 left
        assertEq(_lmpVault.totalIdle(), 0);
        assertEq(_lmpVault.totalDebt(), 1000);


        _mockRootPrice(address(_underlyerTwo), 2 ether);
        _lmpVault.updateDebtReporting(_destinations);

        vm.startPrank(user_02);
        vm.expectRevert(abi.encodeWithSelector(LMPVault.TooFewAssets.selector, 200, 199));
        _lmpVault.withdraw(200, user_02, user_02);//<==== [FAIL. Reason: TooFewAssets(200, 199)]
        console.log("user_02 balance after redeem all: ",_asset.balanceOf(user_02) );
        vm.stopPrank();

    }

Tool used

Manual Review - Foundry

Recommendation

            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),
+                     info.totalAssetsToPull - info.totalAssetsPulled,

                    totalVaultShares
                );

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 is very vague, it highlights an 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. However, due to the lack of detail in the submission, I have a hard time evaluating the validity of this finding here.

Edit: This submission here is likely rather a duplicate of #289

[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 is very vague, it highlights an 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. However, due to the lack of detail in the submission, I have a hard time evaluating the validity of this finding here.

Edit: This submission here is likely rather a duplicate of #289

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.

[Evert0x]

@Trumpero any comment on this?

[Trumpero]

I agree with the escalation that this issue isn't a duplicate of #519; I believe it is similar to #450. It doesn't have a detailed description, and the summary doesn't mention the risk of a revert due to underflow in the subtraction. The PoC describes a revert with the reason 'TooFewAssets' in the 'withdraw' function, which is intended by the protocol for this function and shouldn't be considered a valid vector (as commented on #450)

[Ch-301]

Yeah, this isn't a dup of #519. But also not similar to #450 at all, it talks about receiving a lower amount than expected due to the Oracle price.

However, This issue reverts with 'TooFewAssets' in the 'withdraw' function, but you need to mention why (I left the Vulnerability Detail empty by mistake) then you can decide if it a intended by the protocol or not.
I don't think that the protocol dev designed a whole withdraw() function to just keep reverting every time a user invokes it.

This check is correct and should stay there to make sure that the function _withdraw() will provide the exact asked amount by the user.

Everyone in this space knows that every swap needs to pay a swap fee and this is what I show in the POC by reducing one wei from the Pulled Asset to simulate the swap fees.

This is not an issue with future integrations You can check the SwapRouter.sol where all the swaps get executed. the mapping swapRoutes[ ] will select one from all the swapper adapters (Balancer, Uni V3, Curve V1 and V2 ) to execute this transaction.

Also, the sponsor says
users will only be using the redeem() path via the front-end
Our mission is to find issues on the smart contract level. what you going to use on the front-end is another story.

[Trumpero]

@Ch-301 Both the withdraw and redeem functions can be used for user withdrawals. The withdraw function includes an additional check (strict slippage check) to ensure the withdrawn amount aligns with the price from the oracle. Therefore, users should utilize this function when they require a stricter slippage check for their withdrawals and anticipate a higher outcome, thus understanding that the potential for reverting is intended. The withdraw function will still succeed when there isn't any slippage. On the other hand, if users aim to avoid unexpected reverting, they should use the redeem function, even when they are using front-end or not. Hence, the final check is intended for the withdraw functionality and does not pose any issues, as reverting is anticipated in the case of slippage. Therefore, I believe this issue is a duplicate of #450 and should be considered invalid.
Leave it to @Evert0x

[Ch-301]

1 - This issue is not about slippage.
2 - I guarantee you this withdraw function will keep reverting if there is just one swap (if is this condition (info.totalAssetsToPull > 0) is true).

[Trumpero]

1 - This issue is not about slippage. 2 - I guarantee you this withdraw function will keep reverting if there is just one swap (if is this condition (info.totalAssetsToPull > 0) is true).

@Ch-301 If the withdrawal doesn't utilize any swap (withdraw directly from idle) or the swaps during withdrawals don't incur any slippage, it implies that _withdraw will acquire an amount of assets greater than or equal to the expected assets from the calculation. In this case, the info.totalAssetsToPull amount will be withdrawn successfully without reverting (info.totalAssetsToPull will be zero after loops).

There are multiple tests of the protocol that don't result in a revert when calling the withdraw function of LMPVault. For instance, you can check this: https://github.com/sherlock-audit/2023-06-tokemak/blob/main/v2-core-audit-2023-07-14/test/vault/LMPVault-Withdraw.t.sol#L850-L876.

Therefore, if users want to avoid unexpected reverts, they should use the redeem function since it doesn't have the additional check. The withdraw function has a stricter check for the withdrawn amount, which is intended by the protocol to revert if the returned amount is less than the calculation from the oracle price.

[Trumpero]

Additionally, this issue lacks vulnerable details, and all the information from the summary and PoC revolves around the revert in the withdraw function, which is similar to #450. The statement 'The fact that any swap has swap fees and some slippage' in the report isn't correct, as there are rare instances of swaps that return more than the expected amount of the oracle price (called positive slippage). The withdraw function is designed to withdraw assets without any swaps (directly from idle) or with non-slippage swaps. Therefore, I believe this issue should be considered invalid.

I would prefer not to continue discussing this matter further.

[Evert0x]

Planning to accept escalation and duplicate with 450

[Evert0x]

Result:
Invalid
Duplicate of #450

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• berndartmueller: accepted