scammed - User can benefit from emergencyWithdrawal in case other tokens are added

[sherlock-admin4]

scammed

Medium

User can benefit from emergencyWithdrawal in case other tokens are added

Summary

emergencyWithdrawal doesn’t check for the request’s token hash and will transfer from all the currently active underlying tokens in case new ones are added, despite he deposited in a different set.

Vulnerability Detail

Emergency withdrawal can be used when requests cannot be satisfied in a normal way, but due to missing checks whether the underlying tokens at the time of emergencyWithdrawal execution are the same as when the withdrawal request was created. As a result, all the current underlying tokens + bond tokens received from deposits will be transferred to him, eventually leading to more profit than what he would have received from processWithdrawal.

function emergencyWithdraw(
        uint256[] memory minAmounts,
        uint256 deadline
    )
        external
        nonReentrant
        checkDeadline(deadline)
        returns (uint256[] memory actualAmounts)
    {
        uint256 timestamp = block.timestamp;
        address sender = msg.sender;
        if (!_pendingWithdrawers.contains(sender)) revert InvalidState();
        WithdrawalRequest memory request = _withdrawalRequest[sender];
        if (timestamp > request.deadline) {
            _cancelWithdrawalRequest(sender);
            return actualAmounts;
        }

        if (
            request.timestamp + configurator.emergencyWithdrawalDelay() >
            timestamp
        ) revert InvalidState();

        uint256 totalSupply = totalSupply();
        (address[] memory tokens, uint256[] memory amounts) = baseTvl();
        if (minAmounts.length != tokens.length) revert InvalidLength();
        actualAmounts = new uint256[](tokens.length);
        for (uint256 i = 0; i < tokens.length; i++) {
            if (amounts[i] == 0) {
                if (minAmounts[i] != 0) revert InsufficientAmount();
                continue;
            }
            uint256 amount = FullMath.mulDiv(
                IERC20(tokens[i]).balanceOf(address(this)),
                request.lpAmount,
                totalSupply
            );
            if (amount < minAmounts[i]) revert InsufficientAmount();
            IERC20(tokens[i]).safeTransfer(request.to, amount);
            actualAmounts[i] = amount;
        }
        delete _withdrawalRequest[sender];
        _pendingWithdrawers.remove(sender);
        _burn(address(this), request.lpAmount);
        emit EmergencyWithdrawal(sender, request, actualAmounts);
    }

As we can see baseTvl is called that will take both all the underlying token balances in the vault itself + all the bonds lpTokens balances per underlying tokens.

For example user has entered only in wstETH and later on decides to create withdrawal request:

function registerWithdrawal(
      address to,
      uint256 lpAmount,
      uint256[] memory minAmounts,
      uint256 deadline,
      uint256 requestDeadline,
      bool closePrevious
  )
      external
      nonReentrant
      checkDeadline(deadline)
      checkDeadline(requestDeadline)
  {
      uint256 timestamp = block.timestamp;
      address sender = msg.sender;
      if (_pendingWithdrawers.contains(sender)) {
          if (!closePrevious) revert InvalidState();
          _cancelWithdrawalRequest(sender);
      }
      uint256 balance = balanceOf(sender);
      if (lpAmount > balance) lpAmount = balance;
      if (lpAmount == 0) revert ValueZero();
      if (to == address(0)) revert AddressZero();

      address[] memory tokens = _underlyingTokens;
      if (tokens.length != minAmounts.length) revert InvalidLength();

      WithdrawalRequest memory request = WithdrawalRequest({
          to: to,
          lpAmount: lpAmount,
          tokensHash: keccak256(abi.encode(tokens)),
          minAmounts: minAmounts,
          deadline: requestDeadline,
          timestamp: timestamp
      });
      _withdrawalRequest[sender] = request;
      _pendingWithdrawers.add(sender);
      _transfer(sender, address(this), lpAmount);
      emit WithdrawalRequested(sender, request);
  }

Knowing that there is a possibility more underlying tokens to be added he specifies requestDeadline above the emergencyWithdrawalDelay in order to make it possible to execute emergencyWithdraw in a latter stage. No normal withdrawals are being executed from operators because of either the low withdrawal requests or simply to maximise the yield from the underlying strategies by keeping the deposited funds inside bonds and even adding another underlying tokens (rETH, weth, etc). Then emergencyWithdraw will be the better option as it will give to the user from all of the circulating tokens in the system, then he can simply withdraw bond tokens in the Symbiotic and take the profit.

Impact

EmergencyWithdrawal will transfer more tokens than normally processed withdraw in case there are underlying tokens added after request creation.

Code Snippet

https://github.com/mellow-finance/mellow-lrt/blob/8191087682cc6a7f36c1c6390e37eb308953b11a/src/Vault.sol#L434-L473

https://github.com/mellow-finance/mellow-lrt/blob/8191087682cc6a7f36c1c6390e37eb308953b11a/src/Vault.sol#L371-L431

Tool used

Manual Review

Recommendation

Consider checking the underlying tokens hash in vault::emergencyWithdraw also.

[z3s]

Low/Info; addToken() is an Admin function, they can resolve all withdraws before add new tokens.

[ctf-sec]

Escalate

resolve all withdraws before add new tokens.

This should be a valid issue.

if the admin can always process all withdraws, there will be no emergency withdraw function in the first place.

the emergency withdraw's design is the let user process withdraw themselves, but the way that user can withdraw more than their entitled token makes the code works not intended and leads to loss of fund.

this is still a bug that must be fixed.

in reality, add new tokens has nothing to do with withdraw, " resolve all withdraws before add new tokens" is not a documented assumption at all.

again, the statement below is not possible..

resolve all withdraws before add new tokens.

how about sandwich the add token / create a emergency withdraw right before the add token via frontrunning,

[sherlock-admin3]

Escalate

resolve all withdraws before add new tokens.

This should be a valid issue.

if the admin can always process all withdraws, there will be no emergency withdraw function in the first place.

the emergency withdraw's design is the let user process withdraw themselves, but the way that user can withdraw more than their entitled token makes the code works not intended and leads to loss of fund.

this is still a bug that must be fixed.

in reality, add new tokens has nothing to do with withdraw, " resolve all withdraws before add new tokens" is not a documented assumption at all.

again, the statement below is not possible..

resolve all withdraws before add new tokens.

how about sandwich the add token / create a emergency withdraw right before the add token via frontrunning,

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.

[0xklapouchy]

@z3s

Works as expected.

    /// @notice Handles emergency withdrawals, proportionally withdrawing all tokens in the system (not just the underlying).
    /// @dev Transfers tokens based on the user's share of lpAmount / totalSupply.

Additionally, new tokens deposited after such a deposit partially belong to the old user at the moment of the new deposit, as LP minting is calculated based on the TVL of the vault per deposit value, not the amount or type of token.

The entire contract flow allows for one token to be a deposit token and another to be a withdrawal token if the ratiosX96 for deposit and withdrawal differ. Therefore, there is no issue when emergency tokens differ from what the user initially deposited, as long as they receive them in proportion to their LP / Total supply (which is correct in an emergency withdrawal).

[WangSecurity]

@0xklapouchy as I understand you mean that this report in fact described the intended design of the protocol (as evident by the code comments), correct?

[0xklapouchy]

@WangSecurity

Yes, to my understanding, it is intended by protocol that as a last resort, if the system collapses and users are able to perform an emergencyWithdraw(), they can grab what is left in any tokens (from baseTvl() list) proportionally to the LP amount they are withdrawing.

Plus there is no benefit from new token as LP amount is calculated from deposit value / total TVL of the Vault, so if new token is added to a list of (X, Y) tokens, and new list is (X, Y, Z) my LP is now valued in (X, Y, Z).

Tokens hash comparison and minAmounts in a normal withdrawal are needed for slippage protection. In such cases, the minAmounts need to match the token list. However, this is not needed in an emergency withdrawal, as here we got new minAmount for slippage.

[JeffCX]

That is not the intended design at all and clearly a bug.

Adding a new token by admin does not increase the lp total balance and only result in one more additional loop running

(before adding token user withdraw 2 tokens, after adding token user withdraw 3 tokens, the request.lpAmount from user does not really change.)

   for (uint256 i = 0; i < tokens.length; i++) {
            if (amounts[i] == 0) {
                if (minAmounts[i] != 0) revert InsufficientAmount();
                continue;
            }
            uint256 amount = FullMath.mulDiv(
                IERC20(tokens[i]).balanceOf(address(this)),
                request.lpAmount,
                totalSupply
            );
            if (amount < minAmounts[i]) revert InsufficientAmount();
            IERC20(tokens[i]).safeTransfer(request.to, amount);
            actualAmounts[i] = amount;
        }

the action sequence below is very practical to make user lose fund if not addressed.

1. user A create withdraw with 2 tokens.
2. withdraw does not get processed and user can can trigger emergency withdraw.
3. admin add new token.
4. user B deposit new token.
5. user A trigger emergency withdraw and withdraw 3 tokens.

[0xklapouchy]

Let me update this flow:

1. User A creates a withdrawal with 2 tokens.
   1.1. The user's withdrawal is processed in processWithdrawals().
   1.2. If the withdrawal is not processed, proceed to Point 2.
2. The withdrawal does not get processed, and the user can trigger an emergency withdrawal.
   2.1. If the system is operational, the withdrawal is cancelled in processWithdrawals().
   2.2. The user decides to withdraw the 2 tokens.
   2.3. The user decides to wait even longer, proceed to Point 3.
3. The admin adds a new token.
   3.1. If the admin is able to add the new token, system should be operational go back to 2.1.
   3.2. If system is not operational, proceed to Point 4.
4. User B deposits the new token.
   4.1. Before User B can deposit the new token, the admin needs to update ratiosX96, or it will revert on getTargetRatiosX96(). From now on, all LP tokens are valued in (old token, old token, new token).
   For simplicity, I will not mention that deposit vs. withdrawal ratiosX96 can differ.
   4.2. Let's assume the withdrawal at this point is still not cancelled by processWithdrawals(), as the system is not operational.
   4.3. User B can now deposit. Their deposit is correctly valued in (old token, old token, new token), the same as for all users.
5. User A triggers an emergency withdrawal and withdraws 3 tokens.
   5.1. This is exactly as designed. The system is not operational at this moment, and based on all the needed steps to be in here, the admin looks compromised. If anyone deposits into the system at this moment, they can incur a loss, but it still works as designed. User A will receive their tokens proportionally to withdrawn LP/Total supply, as LP is backed by 3 tokens.

[JeffCX]

there is no document saying that the admin looks compromised / system is not operational when emergency withdraw is triggered...

as long as the withdraw is not processed within time window (could be admin has too many withdraw to process), the user can trigger emergency withdraw themselves.

[0xklapouchy]

Where is the impact?

1. User A, decides to emergency withdraw, taking the loss if there are no tokens compared to his LP value. He can withdraw less than his LP market value. But to ensure he does not withdraw less than the 'market value', he can use slippage protection in the form of the minAmounts argument of the emergencyWithdraw() function. Or cancel withdrawal and create new one with new 3 tokens minAmount.

2. User B, can create a withdrawal request, and if processed, will receive all his tokens back based on the ratiosX96 for withdrawal.

[blckhv]

Here is what happens:

As we can see baseTvl is called that will take both all the underlying token balances in the vault itself + all the bonds lpTokens balances per underlying tokens.

This is what makes the opportunity since a bond is the symbiotic representation of the LST token that is deposited - a completely different token.
Knowing that we can visualize how many iterations the for loop will perform:

• underlyingAssets: [wstETH, rETH]
• assets: [symbioticTokenWstETH, symbioticTokenRETH], they are minted to the Vault when the LST tokens are deposited in Symbiotic - here (important to note that they can be withdrawn at a 1:1 ratio from Symbiotic directly)

And since there are 4 different tokens calculateTVLs will return amounts = [wstETH, rETH, symbioticTokenWstETH, symbioticTokenRETH], which will then transfer the respective amount not of the underlying assets (wstETH, rETH) but also for the bond assets as well.

[WangSecurity]

The question I've got is this indeed true?

As we can see baseTvl is called that will take both all the underlying token balances in the vault itself + all the bonds lpTokens balances per underlying tokens

As I see baseTvl only returns base tokens, not underlying ones, or at least I'm missing where it returns both. If I'm mistaken here, please refer me to the part of the code, that will return both in baseTvl.

Therefore, the following is also incorrect:

And since there are 4 different tokens calculateTVLs will return amounts = [wstETH, rETH, symbioticTokenWstETH, symbioticTokenRETH], which will then transfer the respective amount not of the underlying assets (wstETH, rETH) but also for the bond assets as well.

The emegrencyWithdrawal will use only base tokens and their amounts, so there will be 2 loops in this example and not 4. In that case, I believe this is not an issue, since the report is incorrect.

[blckhv]

As I see baseTvl only returns base tokens, not underlying ones, or at least I'm missing where it returns both. If I'm mistaken here, please refer me to the part of the code, that will return both in baseTvl.

underlyingTVLs (function called from the normal withdrawal) calls _calculateTVL with isUnderlying == true, and then in DefaultBondTvlModule it takes underlyingToken which is the same underlying token as in the Vault.
You can also check this line here: https://github.com/mellow-finance/mellow-lrt/blob/8191087682cc6a7f36c1c6390e37eb308953b11a/src/modules/symbiotic/DefaultBondTvlModule.sol#L18

emergencyWithdraw

Unlike the normal withdrawal, here baseTvl we can see that
_calculateTVL with isUnderlying == false is called:

 function baseTvl()
        public
        view
        returns (address[] memory tokens, uint256[] memory amounts)
    {
...
        amounts = _calculateTvl(tokens, false); //HERE
    }

Then in DefaultBondTvlModule data[i].token = bonds[i]; is what is taken as token, which is the LST token of Symbiotic, not wstETH or any other underlying token used in the Vault. Then the second Tvl module is the ERC20TvlModule.sol that returns only the underlying tokens that Vault uses, not any of the strategy tokens.

Symbiotic code:

contract DefaultCollateral is ERC20Upgradeable, ReentrancyGuardUpgradeable, IDefaultCollateral {
    using SafeERC20 for IERC20;
    using Permit2Lib for IERC20;

    uint8 private DECIMALS;

    /**
     * @inheritdoc ICollateral
     */
    address public asset;
    
    function deposit(address recipient, uint256 amount) public nonReentrant returns (uint256) {
        uint256 balanceBefore = IERC20(asset).balanceOf(address(this));
        IERC20(asset).transferFrom2(msg.sender, address(this), amount);
        amount = IERC20(asset).balanceOf(address(this)) - balanceBefore;

        if (amount == 0) {
            revert InsufficientDeposit();
        }

        if (totalSupply() + amount > limit) {
            revert ExceedsLimit();
        }

        _mint(recipient, amount);

        emit Deposit(msg.sender, recipient, amount);

        return amount;
    }

The entire contract is the bonds[i] since it is an ERC20 and is what is minted to the Vault (these tokens stay in the Vault and can be emergency withdrawn) against deposits to this strategy, while asset is what underlyingToken is in Mellow.

That's why I said 4 loops will be performed, not 2.

I really hope that clears the confusion, thanks!

[Slavchew]

@WangSecurity

The emegrencyWithdrawal will use only base tokens and their amounts, so there will be 2 loops in this example and not 4. In that case, I believe this is not an issue, since the report is incorrect.

You agree that there will be more tokens than the initial ones. Exactly what the report says.

Knowing that there is a possibility more underlying tokens to be added he specifies requestDeadline above the emergencyWithdrawalDelay in order to make it possible to execute emergencyWithdraw in a latter stage. No normal withdrawals are being executed from operators because of either the low withdrawal requests or simply to maximise the yield from the underlying strategies by keeping the deposited funds inside bonds and even adding another underlying tokens (rETH, weth, etc). Then emergencyWithdraw will be the better option as it will give to the user from all of the circulating tokens in the system, then he can simply withdraw bond tokens in the Symbiotic and take the profit.

[WangSecurity]

You agree that there will be more tokens than the initial ones. Exactly what the report says

Yep, I didn't say here that only 1 token will be used.

That's why I said 4 loops will be performed, not 2.
I really hope that clears the confusion, thanks!

Thank you for that response, but I think I still need additional clarification. What I don't understand is that the problem is in new underlying tokens being added, but emergencyWithdraw accounts only for the value of not underlying tokens but bonds/strategy tokens. So, how adding an underlying token would lead to getting more funds from emergencyWithdraw if it accounts for bonds? Maybe you could make a numerical example of how much more exactly the user would get?

[JeffCX]

Emm I think a coded POC is more persuasive then the numerial analysis.

I think the severity is high then because it leads to straight loss of fund,

basically user can deposit one token and withdraw two or three token, and indeed when a new token is added the user can withdraw more.

https://github.com/mellow-finance/mellow-lrt/blob/8191087682cc6a7f36c1c6390e37eb308953b11a/tests/mainnet/unit/VaultTest.t.sol#L3334

there is a straightford test case

it just deposit one token and emergency withdraw above

we can add the above test:

   function testEmergencyWithdraw_POC() external {
        Vault vault = new Vault("Mellow LRT Vault", "mLRT", admin);
        vm.startPrank(admin);
        vault.grantRole(vault.ADMIN_DELEGATE_ROLE(), admin);
        vault.grantRole(vault.OPERATOR(), operator);
        _setUp(vault);
        vm.stopPrank();
        _initialDeposit(vault);

        address depositor = address(bytes20(keccak256("depositor")));
        vm.startPrank(depositor);
        deal(Constants.WSTETH, depositor, 10 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );

        uint256[] memory amounts = new uint256[](3);
    
        amounts[0] = 10 ether;

        uint256[] memory minAmounts = new uint256[](3);

        vault.deposit(depositor, amounts, 10 ether, type(uint256).max);

        vault.registerWithdrawal(
            depositor,
            10 ether,
            minAmounts,
            type(uint256).max,
            type(uint256).max,
            false
        );

        // vm.stopPrank();

        uint256 WSETH_Balance = IERC20(Constants.WSTETH).balanceOf(address(depositor));

        uint256 RETH_Balance = IERC20(Constants.RETH).balanceOf(address(depositor));

        uint256 USDT_Balance = IERC20(Constants.USDT).balanceOf(address(depositor));

        console.log("WSETH_Balance : ", RETH_Balance);

        console.log("RETH_Balance : ", RETH_Balance);

        console.log("USDT Balance: ", USDT_Balance);

        deal(Constants.RETH, address(vault), 10 ether);

        (address[] memory tokens, uint256[] memory vals) = vault.baseTvl();

        console.log(tokens.length);
        for(uint i = 0; i < tokens.length; ++i) {
            console.log("val", vals[i]);
        }

        vm.stopPrank();

        vm.prank(admin);
        vault.addToken(Constants.USDT);

        deal(Constants.USDT, address(vault), 10 ** 6 * 100000);

        vm.startPrank(depositor);
        
        vault.emergencyWithdraw(new uint256[](4), type(uint256).max);

        (tokens, vals) = vault.baseTvl();

        console.log(tokens.length);
        for(uint i = 0; i < tokens.length; ++i) {
            console.log("val", vals[i]);
        }

        WSETH_Balance = IERC20(Constants.WSTETH).balanceOf(address(depositor));
        RETH_Balance = IERC20(Constants.RETH).balanceOf(address(depositor));
        USDT_Balance = IERC20(Constants.USDT).balanceOf(address(depositor));


        console.log("WSTETH_Balance : ", WSETH_Balance);
        console.log("RETH_Balance : ", RETH_Balance);
        console.log("USDT Balance: ", USDT_Balance);
        // IVault.WithdrawalRequest memory request = vault.withdrawalRequest(
        //     depositor
        // );
        // assertEq(request.lpAmount, 0);

        // assertEq(IERC20(Constants.WSTETH).balanceOf(address(vault)), 10 gwei);
        // assertEq(
        //     IERC20(Constants.WSTETH).balanceOf(address(depositor)),
        //     10 ether
        // );

        // assertEq(vault.balanceOf(address(vault)), 10 gwei);
        // assertEq(vault.balanceOf(address(depositor)), 0);
        // vm.stopPrank();
    }

basically user only deposit WSETH, but because admin is adding a new token and also the RETH token presents in the contract,

the user can withdraw WSETH, RETH and USDT even when the user never deposit USDT.

running the test command is:

forge test -vv --match-test "testEmergencyWithdraw_POC" --fork-url "https://rpc.ankr.com/eth"

output:

Ran 1 test for tests/mainnet/unit/VaultTest.t.sol:VaultTestUnit
[PASS] testEmergencyWithdraw_POC() (gas: 13019938)
Logs:
  WSETH_Balance :  0
  RETH_Balance :  0
  USDT Balance:  0
  3
  val 10000000010000000000
  val 10000000000000000000
  val 0
  4
  val 10000000000
  val 9999999991
  val 0
  val 100
  WSTETH_Balance :  10000000000000000000
  RETH_Balance :  9999999990000000009
  USDT Balance:  99999999900

[JeffCX]

   deal(Constants.USDT, address(vault), 10 ** 6 * 100000);

mimic other user's deposit after the token is added.

[JeffCX]

I do appreciate lead judge's efforts to try to apply technical rigor to resolve escalation because there are a lot of them.

can @z3s judge please also help lead judge interpret the POC? thank you!

[WangSecurity]

Thank you for the POC, but I found it's a bit incorrect. The main problem is that instead of making a second user actually deposit into the vault, you deal the tokens directly into the vault. That leads to the amount of underlying tokens increasing, but the share amount remains the same. That is the reason, why the depositor who had all the shares received all the tokens in the vault, even though new tokens were sent in the vault before their withdrawal. And here's no issue since the number of tokens withdrawn by the user is proportional to their shares, i.e. 100% of shares == 100% of tokens.

Here's the updated POC where I added the second user who deposits into the vault. Small scenario:

1. D1 (depositor 1) deposits into the vault and makes a withdrawal request.
2. Admin adds new token.
3. D2 (deposit2) deposits into the vault and makes a withdrawal request.
4. D1 calls emergencyWithdraw and gets the same amount of tokens as they had initially, even though a new token was added.
5. D2 calls ``emergencyWithdraw` and gets the same amount of tokens as they had initially.
   No one was given more tokens and no one lost tokens.

Here's the POC

       function testEmergencyWithdraw_POC() external {
        Vault vault = new Vault("Mellow LRT Vault", "mLRT", admin);
        vm.startPrank(admin);
        vault.grantRole(vault.ADMIN_DELEGATE_ROLE(), admin);
        vault.grantRole(vault.OPERATOR(), operator);
        _setUp(vault);
        vm.stopPrank();
        _initialDeposit(vault);

        address depositor = address(bytes20(keccak256("depositor")));
        vm.startPrank(depositor);
        deal(Constants.WSTETH, depositor, 10 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );

        // @Wang just additional console logs
        uint256 WSETH_Initial = IERC20(Constants.WSTETH).balanceOf(address(depositor));
        console.log("WSETH_Initial D1: ", WSETH_Initial);

        uint256[] memory amounts = new uint256[](3);
    
        amounts[0] = 10 ether;

        uint256[] memory minAmounts = new uint256[](3);

        vault.deposit(depositor, amounts, 10 ether, type(uint256).max);

        vault.registerWithdrawal(
            depositor,
            10 ether,
            minAmounts,
            type(uint256).max,
            type(uint256).max,
            false
        );

        vm.stopPrank();

        //@Wang adding new token
        vm.startPrank(admin);
        vault.addToken(Constants.STETH); // adding a fourth token, but decided to add the one with 18 decimals
        IVaultConfigurator configurator = vault.configurator(); // get the config to change the ratios, otherwise, the second user cannot deposit
        uint128[] memory ratiosX96 = new uint128[](4);
        ratiosX96[0] = 2 ** 96; // from VaultTestCommon, the only change is adding a fourth token, the ratio remains unchanged
        ManagedRatiosOracle(configurator.ratiosOracle())
           .updateRatios(address(vault), true, ratiosX96);

        // adding a Chainlink Oracle for stETH for the deposit to go through. Using the same as wstETH
        ChainlinkOracle chainlinkOracle = new ChainlinkOracle();
        chainlinkOracle.setBaseToken(address(vault), Constants.WSTETH);
        address[] memory tokensNew = new address[](4);
        tokensNew[0] = Constants.WSTETH;
        tokensNew[1] = Constants.RETH;
        tokensNew[2] = Constants.WETH;
        tokensNew[3] = Constants.STETH;

        IChainlinkOracle.AggregatorData[]
            memory data = new IChainlinkOracle.AggregatorData[](4);
        data[0] = IChainlinkOracle.AggregatorData({
            aggregatorV3: address(
                new WStethRatiosAggregatorV3(Constants.WSTETH)
            ),
            maxAge: 30 days
        });
        data[1] = IChainlinkOracle.AggregatorData({
            aggregatorV3: Constants.RETH_CHAINLINK_ORACLE,
            maxAge: 30 days
        });
        data[2] = IChainlinkOracle.AggregatorData({
            aggregatorV3: address(new ConstantAggregatorV3(1 ether)),
            maxAge: 30 days
        });
        data[3] = IChainlinkOracle.AggregatorData({
            aggregatorV3: address(
                new WStethRatiosAggregatorV3(Constants.WSTETH)
            ),
            maxAge: 30 days
        });

        chainlinkOracle.setChainlinkOracles(address(vault), tokensNew, data);

        configurator.stagePriceOracle(address(chainlinkOracle));
        configurator.commitPriceOracle();
        // @Wang not dealing tokens directly
        //deal(Constants.USDT, address(vault), 10 ** 6 * 100000);

        // @Wang adding a second actual depositor, since just dealing to the vault doesn't increase the shares amount
        address depositor2 = address(0x69);
        
        uint256[] memory amounts2 = new uint256[](4);
    
        amounts2[0] = 10 ether;

        uint256[] memory minAmounts2 = new uint256[](4);

        vm.startPrank(depositor2);

        deal(Constants.WSTETH, depositor2, 10 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );
        uint256 WSETH_D2_Balance_Initial = IERC20(Constants.WSTETH).balanceOf(address(depositor2));
        console.log("WSTETH_Balance D2 before deposit: ", WSETH_D2_Balance_Initial);
        vault.deposit(depositor2, amounts2, 10 ether, type(uint256).max);
        vault.registerWithdrawal(
            depositor2,
            10 ether,
            minAmounts2,
            type(uint256).max,
            type(uint256).max,
            false
        );   
        vm.stopPrank();


        vm.startPrank(depositor);
        
        vault.emergencyWithdraw(new uint256[](4), type(uint256).max);
        

        uint256 WSETH_D1_Balance_Final = IERC20(Constants.WSTETH).balanceOf(address(depositor));

        console.log("WSTETH_Balance D1 after withdrawal: ", WSETH_D1_Balance_Final);
        vm.stopPrank();
        // @Wang second depositor withdrawing
        vm.startPrank(depositor2);
        vault.emergencyWithdraw(new uint256[](4), type(uint256).max);

        uint256 WSETH_D2_Balance_Final = IERC20(Constants.WSTETH).balanceOf(address(depositor2));

        console.log("WSTETH_Balance D2 after withdrawal: ", WSETH_D2_Balance_Final);
        vm.stopPrank();
    }

You can search for @Wang to see the changes and my comments. I reduced the amount of console logs cause I was getting a Stack too deep issue and kept only four. 2 logs for the initial balances of the depositors and 2 logs for their final balances after the withdrawals.

Output

Ran 1 test for tests/mainnet/unit/VaultTest.t.sol:VaultTestUnit
[PASS] testEmergencyWithdraw_POC() (gas: 14413378)
Logs:
  WSETH_Initial D1:  10000000000000000000
  WSTETH_Balance D2 before deposit:  10000000000000000000
  WSTETH_Balance D1 after withdrawal:  10000000000000000000
  WSTETH_Balance D2 after withdrawal:  10000000000000000000

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.18s (7.91s CPU time)

As proven with the POC, even though another token was added after the user deposited, they didn't receive any additional tokens and got the very same amount they deposited. Hence, I believe there is no issue and the report is incorrect. Of, course if my POC is wrong, please correct it. However, the current decision is to reject the escalation and leave the issue as it is.

[0xklapouchy]

LP minted are based on the TVL of provided assets, in the oracle system you configured the prices of tokens are messed up, but let recheck:

WSTETH priceX96 = 79228162514264337593543950336 = 1 WSTETH
RETH priceX96 = (1119391666562886000 * 10**18 * 2**96) / (1174146665442558861 * 10**18) = 75533446958374261408753191664 ~= 0,9533661335 WSTETH
WETH priceX96 = (1000000000000000000 * 10**18 * 2**96) / (1174146665442558861 * 10**18) = 67477228225488927592052155914 ~= 0,8516823574 WSTETH
STETH priceX96 = (999883973062914300 * 10**18 * 2**96) / (1174146665442558861 * 10**18) = 67469399049374891367997310915 ~= 0,8515835393 WSTETH

I don't need to even count to notice that (10 WSTETH/10 RETH/10 WETH) deposit in terms of LP will give me more at the withdrawal in terms of new token in case he is worth less then my initial 10 WSTETH in proportion I added.

And you are expecting exact 10/10/10/10 withdrawals.

[WangSecurity]

@blckhv also could send instead of coded POC, a numerical POC which shows how an emergency withdrawal results in a user getting more tokens, so the discussion doesn't circle test configuration issues.

[0xklapouchy]

@blckhv and please recheck how much of LP tokens are you withdrawing for depositor2 as for 40e18 it is more then 10e18.

[0xklapouchy]

I did calculations for you, based on the 10e18 LP you are withdrawing for both users, the end result is the same:

you should change in PoC the registerWithdrawal() and use vault.balanceOf(depositor2) in place of 10e18

D1 deposit TVL:
10000000000000000000 * 953366133500000000 / 1000000000000000000 + 10000000000000000000 + 10000000000000000000 * 851682357400000000 / 1000000000000000000 = 28050484909000000000

D1 withdraw TVL:
  RETH_Balance :  8682102068338637266 * 953366133500000000 / 1000000000000000000 = 8277222079544359378
  WETH Balance:  8682102068338637266 * 851682357400000000 / 1000000000000000000 = 7394393156750066488
  bond Balance:  8682102068338637265
  STETH Balance:  4341051031998793115 * 851583539300000000 / 1000000000000000000 = 3696767602111449794

Total:  28050484906744512925 for 10e18 LP


D2 deposit TVL:
10000000000000000000 * 953366133500000000 / 1000000000000000000 + 10000000000000000000 + 10000000000000000000 * 851682357400000000 / 1000000000000000000 + 10000000000000000000 * 851583539300000000 / 1000000000000000000 = 36566320302000000000

 D2 withdraw TVL:
  RETH_Balance :  8682102068338637265 * 953366133500000000 / 1000000000000000000 = 8277222079544359377
  WETH Balance:  8682102068338637265 * 851682357400000000 / 1000000000000000000 = 7394393156750066487
  bond Balance:  8682102068338637265
  STETH Balance:  4341051031998793116 * 851583539300000000 / 1000000000000000000 = 3696767602111449795

Total:  28050484906744512924 for 10e18 LP

Total withdraw for depositor2 is below deposit as he got more than 10e18 LP and is not withdrawing all LP.

[0xklapouchy]

@WangSecurity Based on the TVL value from the deposit, the withdrawal amounts are perfectly fine

[Slavchew]

@WangSecurity please do not allow this type of conversation, since if anyone has any arguments can provide them in one comment and with facts, not to post countless comments and only numbers without contraarguments of our state.

We will provide the PoC again with examples later, and will ask you to specify the way it can be argued by someone, because just dropping comments lacking facts and explanation will goes to nothing.

Thanks

[WangSecurity]

@Slavchew I think @0xklapouchy is correct at some points. Firstly, indeed the second depositor deposits only 4e18 tokens and not 31 ether, since the target ratio is 25% for each token, but they deposit only 1 ether of stETH. Hence, for each token, the vault claims only 1 ether as well.

Secondly, even though the token amounts, in the end, may be different to what the users got initially, if we calculate the final value by prices, i.e. rethAmount * rethPrice + wstethAmount * wstETHPrice + ..., wouldn't the value be the same?

[blckhv]

@WangSecurity

Secondly, even though the token amounts, in the end, may be different to what the users got initially, if we calculate the final value by prices, i.e. rethAmount * rethPrice + wstethAmount * wstETHPrice + ..., wouldn't the value be the same?

This is not true because the second depositor who deposited 4 tokens totaled at X price and then he got less from these 4 tokens, so less than X amount.

Regarding the test:
I don't know from where he got these numbers, but here is the POC, exactly as he wanted it:

• WETH as base token
• 10 stETH deposit, instead of 1
• depositor2 performs full withdrawal (not only 10e18)
• It was stated that all tokens should have the same price, which is wrong, the test uses the real price of the tokens and does not set them all 1:1 with ETH. We will not be applying this change to the test, since it will lead to completely wrong state of the Vault.

depositor 1 lp: 10000000000623781598
  depositor 2 lp: 13035895965181913903

  RETH_Balance :  8682102068443850464
  WETH Balance:  8682102068443850464
  bond Balance:  8682102068443850463
  STETH Balance:  4341051032051399713

  RETH_Balance :  11317897932332473916
  WETH Balance:  11317897932332473916
  bond Balance:  11317897932332473916
  STETH Balance:  5658948963336762475

Really, @0xklapouchy if you don't have concrete arguments to prove what you state, we should stop unnecessary extending the escalation process.
We have explained numerous times where is the issue, provided POC, then you wanted to perform some modifications, we did it. Here is the result.

@WangSecurity although it is good to have discussions about issue validity I think this is getting out of control, if he wants to prove something, the same effort should be put into, it instead of throwing random numbers.

[WangSecurity]

@blckhv I think to finally settle the discussion, could show the balances of the users if we call processWithdrawals instead of emergencyWithdrawal to see the difference, cause as I see in your report, the problem is exactly in emergencyWithdrawal.

[blckhv]

@WangSecurity, It won't be possible since the tokensHash will be different because there is a new underlying token added.

[WangSecurity]

So processWithdrawal is not possible at all if the new token is added? Or the first depositor will have to create a new request?

[0xklapouchy]

To let you know what those random numbers ware:

• 1119391666562886000 - RETH/ETH price
• 1174146665442558861 - WSTETH/STETH exchange ratio
• 999883973062914300 - STETH/ETH price

With WETH as base token we currently have:

WSTETH priceX96 = (1174146665442558861 * 10**18 * 2**96) / (1000000000000000000 * 10**18) = 93025482825264612275329476316 ~= 1,1741466654 WETH
RETH priceX96 = (1119391666562886000 * 10**18 * 2**96) / (1000000000000000000 * 10**18) = 88687344875557529108227789341 ~= 1,1193916666 WETH
WETH priceX96 = 79228162514264337593543950336 = 1 WETH
STETH priceX96 = (999883973062914300 * 10**18 * 2**96) / (1000000000000000000 * 10**18) = 67469399049374891367997310915 ~= 0,9998839731 WETH

D1 deposit TVL:
10000000000000000000 * 1174146665442558861 / 1000000000000000000 + 10000000000000000000 * 1119391666562886000 / 1000000000000000000 + 10000000000000000000 = 32935383320054448610 WETH [Wei] ~= 32.93538332005444861 WETH

D1 withdraw TVL:
  RETH_Balance :  8682102068443850464 * 1119391666562886000 / 1000000000000000000 = 9718672703664441503
  WETH Balance:  8682102068443850464
  bond Balance:  8682102068443850463 * 1174146665442558861 / 1000000000000000000 = 10194061192695289963
  STETH Balance:  4341051032051399713 * 999883973062914300 / 1000000000000000000 = 4340547353196418072

Total:  9718672703664441503 + 8682102068443850464 + 10194061192695289963 + 4340547353196418072 = 32935383318000000002 ~= 32.935383318000000002


D2 deposit TVL:
10000000000000000000 * 1174146665442558861 / 1000000000000000000 + 10000000000000000000 * 1119391666562886000 / 1000000000000000000 + 10000000000000000000 + 10000000000000000000 * 999883973062914300 / 1000000000000000000 = 42934223050683591610 WETH [Wei] ~= 42.93422305068359161 WETH

 D2 withdraw TVL:
  RETH_Balance :  11317897932332473916 * 1119391666562886000 / 1000000000000000000 = 12669160628462289538
  WETH Balance:  11317897932332473916
  bond Balance:  11317897932332473916 * 1174146665442558861 / 1000000000000000000 = 13288872117067405937
  STETH Balance:  5658948963336762475 * 999883973062914300 / 1000000000000000000 = 5658292372821422213

Total: 12669160628462289538 + 11317897932332473916 + 13288872117067405937 + 5658292372821422213 = 42934223050683591604 ~= 42.934223050683591604 WETH

Once again deposited token value is same as value of tokens after withdrawal.

[WangSecurity]

@0xklapouchy but as we know Mellow assumes 1 STETH == 1 ETH, could you adjust the calculations?

[0xklapouchy]

It uses real Chainlink values and also 1 STETH = 1 ETH for WSTETH. The PoC is correct, values are correct. The conclusion the @Slavchew and @blckhv are making from it is incorrect.

They are expecting the same amounts to be withdraw as deposited, but LP amount is minted based on Value of the tokens deposited not the amount of them.

LP = Deposit Value / Total Value, not Deposit Amount / Total Amount.

The total value of tokens deposited and withdrawn is correct in terms of the its value in WETH.

[0xklapouchy]

This is why I also recommended using ConstantAggregatorV3 for all tokens for PriceOracle, as then the Value equals the Amount of tokens. You can quickly see that the withdrawal amount will match the value of the deposited tokens, so they will receive 40e18 tokens as expected.

[WangSecurity]

Thank you for the calculations. Indeed, the users receive different amounts of tokens, while they can receive the amount even fewer than they had. But, in ETH value (or WETH to be precise), the token amounts match (with slight discrepancies).

That is why depositor 1 received less RETH and WETH, but more STETH than they deposited. Also, depositor 2 received more RETH and WETH, but less STETH, than they deposited, but the difference in the price of these assets mitigates it. Hence, the initial and the final values of the tokens are matching.

Planning to reject the escalation tomorrow morning CEST timezone (approx. noon), cause I think all the arguments have been said and there's nothing more to add.

[blckhv]

They are matching and issue won't occur unless there are both bond and wstETH tokens in the Vault, this is completely viable situation as currently Symbiotic has no more limit and the situation where both tokens have non-zero balance in the system is possible - there are some wstETH deposited in Symbiotic that received bond asset and wstETH idling in the Vault. This was mentioned in the original submission and can be seen. So if we modify the test to contain both tokens + other underlying tokens and since the emergencyWithdraw uses baseTvl function:

POC

function test_BLCKHVEmergencyWithdraw() external {
        DefaultBondTvlModule bondModule = new DefaultBondTvlModule();

        Vault vault = new Vault("Mellow LRT Vault", "mLRT", admin);
        vm.startPrank(admin);
        vault.grantRole(vault.ADMIN_DELEGATE_ROLE(), admin);
        vault.grantRole(vault.OPERATOR(), operator);
        _setUp(vault);
         //TODO add bond module:
        vault.addTvlModule(address(bondModule));
        //TODO deploy bond contract and configure
        BondCtr bondCtr = new BondCtr();
        address[] memory bonds = new address[](1);
        bonds[0] = address(bondCtr);
        bondModule.setParams(address(vault), bonds);
        vm.stopPrank();
        _initialDeposit(vault);

        address depositor = address(bytes20(keccak256("depositor")));
        vm.startPrank(depositor);
        deal(Constants.WSTETH, depositor, 10 ether);
        deal(Constants.RETH, depositor, 10 ether);
        deal(Constants.WETH, depositor, 10 ether);
        // deal(Constants.USDC, depositor, 1 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );         IERC20(Constants.RETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );        IERC20(Constants.WETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );

        uint256[] memory amounts = new uint256[](3);
        amounts[0] = 10 ether;
        amounts[1] = 10 ether;
        amounts[2] = 10 ether;
        uint256[] memory minAmounts = new uint256[](3);
        vault.deposit(depositor, amounts, 10 ether, type(uint256).max);
         uint vault_Balance = vault.balanceOf(address(depositor));
        // console.log("depositor 1 lp:", vault_Balance);
        vault.registerWithdrawal(
            depositor,
            10 ether,
            minAmounts,
            type(uint256).max,
            type(uint256).max,
            false
        );
        vm.stopPrank();
       
        //TODO add another token: USDC
        addStEthAsUnderlying(vault);
        //TODO create another user and deposit from 4 tokens:
        address depositor2 = makeAddr("depositor2");
        deal(Constants.WSTETH, depositor2, 10 ether);
        deal(Constants.RETH, depositor2, 10 ether);
        deal(Constants.WETH, depositor2, 10 ether);
        //TODO deal doesn't work, mock stETH whale:
        //deal(Constants.STETH, depositor2, 10 ether);
        vm.startPrank(0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0);
        IERC20(Constants.STETH).transfer(depositor2, 10 ether);
        vm.stopPrank();
        vm.startPrank(depositor2);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );         IERC20(Constants.RETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );        IERC20(Constants.WETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );
        IERC20(Constants.STETH).safeIncreaseAllowance(
            address(vault),
            10 ether
        );

        uint256[] memory amounts2 = new uint256[](4);
        amounts2[0] = 10 ether;
        amounts2[1] = 10 ether;
        amounts2[2] = 10 ether;
        amounts2[3] = 10 ether;
        vault.deposit(depositor2, amounts2, 0, type(uint256).max);
         vault_Balance = vault.balanceOf(address(depositor2));
         //console.log("depositor 2 lp:", vault_Balance);
        vm.stopPrank();
        //TODO instead of depositing WSTETH directly, simulate by automatically minting Bond assets to the Vault
        vm.startPrank(address(vault));
        bondCtr.mint(address(vault), IERC20(Constants.WSTETH).balanceOf(address(vault)));
        //TODO if you want to see how output is being affected modify the amount transferred:
        //IERC20(Constants.WSTETH).transfer(address(1), IERC20(Constants.WSTETH).balanceOf(address(vault)) / 4);
        vm.stopPrank();

        vm.prank(depositor);
        vault.emergencyWithdraw(new uint256[](5), type(uint256).max);

        (address[] memory tokens, uint256[] memory vals) = vault.baseTvl();

         uint RETH_Balance = IERC20(Constants.RETH).balanceOf(address(depositor));
         uint WETH_Balance = IERC20(Constants.WETH).balanceOf(address(depositor));
         uint bond_Balance = IERC20(bondCtr).balanceOf(address(depositor));
         uint STETH_Balance = IERC20(Constants.STETH).balanceOf(address(depositor));
         uint WSTETH_Balance = IERC20(Constants.WSTETH).balanceOf(address(depositor));

         console.log("RETH_Balance : ", RETH_Balance);
         console.log("WETH Balance: ", WETH_Balance);
         console.log("bond Balance: ", bond_Balance);
         console.log("STETH Balance: ", STETH_Balance);
         console.log("WSTETH Balance: ", WSTETH_Balance);

        console.log("D1 balance:", RETH_Balance + WETH_Balance + bond_Balance + STETH_Balance + WSTETH_Balance);

        vm.startPrank(depositor2);
        // uint256[] memory minAmounts2 = new uint256[](4);
         minAmounts = new uint256[](4);
        vault.registerWithdrawal(
            depositor2,
            100 ether,
            minAmounts,
            type(uint256).max,
            type(uint256).max,
            false
        );
        vault.emergencyWithdraw(new uint256[](5), type(uint256).max);
        vm.stopPrank();
         RETH_Balance = IERC20(Constants.RETH).balanceOf(address(depositor2));
         WETH_Balance = IERC20(Constants.WETH).balanceOf(address(depositor2));
         bond_Balance = IERC20(bondCtr).balanceOf(address(depositor2));
         STETH_Balance = IERC20(Constants.STETH).balanceOf(address(depositor2));
         WSTETH_Balance = IERC20(Constants.WSTETH).balanceOf(address(depositor2));

         console.log("RETH_Balance : ", RETH_Balance);
         console.log("WETH Balance: ", WETH_Balance);
         console.log("bond Balance: ", bond_Balance);
         console.log("STETH Balance: ", STETH_Balance);
         console.log("WSTETH Balance: ", WSTETH_Balance);

        console.log("D2 balance:", RETH_Balance + WETH_Balance + bond_Balance + STETH_Balance);
   }

D1:
  RETH_Balance :  8681880049760031104
  WETH Balance:  8681880049760031104
  bond Balance:  8681880049760031103
  STETH Balance:  4340940022709545539
  WSTETH Balance:  8681880049760031104
  Total: 39068460221749669954
  
D2:
  RETH_Balance :  11318119951149700131
  WETH Balance:  11318119951149700131
  bond Balance:  11318119951149700131
  STETH Balance:  5659059972745320079
  WSTETH Balance:  11318119951149700131

 Total: 39613419826194420472

Then this bond asset can be exchanged for 1:1 ratio with wstETH. This situation is profitable even for the relatively small amounts of wstETH and bond tokens, the only requirements is that both tokens to be in the Vault.

Important note is that whether there will be wstETH or not in the Vault is not access controlled, even on the contrary users can directly influence with fulfilling the Symbiotic limits (but not obligated to).

[WangSecurity]

The value of the D1 with prices from here = 43128341604690656809 = 43.1e18. For example, if we take the bond value, i.e. 8681880049760031103 * 1174146665442558861 / 1000000000000000000 = 10193800510198017516 and deduct it from the final value:

43128341604690656809 - 10193800510198017516 = 32934541094492639293 which is almost the same as the deposit value, counted here.

Now the same for the D2. Total value is 56224198074418346192 = 56.2e18. Now, let's deduct the bond value, i.e. 11318119951149700131 * 1174146665442558861 / 1000000000000000000 = 13289132799721317598 and deduct here:

56224198074418346192 - 13289132799721317598 = 42935065274697028594 which is again almost the same as the initial value.

Hence, the only additional tokens the users withdraw are the bond assets. The second depositor got more of them since they LP share is larger than the 1st depositor's one. Additionally, the depositors just received them because they were directly minted to the vault without additional shares being minted to anyone.

Hence, there's no loss, everyone is getting as much as they should. The decision remains the same, if you want me to share the full calculations, please ask.

[blckhv]

Hence, the only additional tokens the users withdraw are the bond assets.

Yes, this is the idea of using baseTVL in emergencyWithdraw it returns both underlying tokens and bond tokens in this case and then this bond token can be exchanged at 1:1 ratio with the bond::asset() - https://github.com/symbioticfi/collateral/blob/main/src/contracts/defaultCollateral/DefaultCollateral.sol#L155-L157

Additionally, the depositors just received them because they were directly minted to the vault without additional shares being minted to anyone

This is how the strategy deposits work, users deposit wstETH and receive lp tokens based on this token, then it is deposited by the depositCallback - https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/strategies/DefaultBondStrategy.sol#L51-L81. And then bond token is minted to the Vault. No need to mint additional lp tokens since this is how the code works now.

I've researched a bit and even the first Vault that I've checked contains both wstETH and SymbioticWstETH.

• wstETH
  
• symbioticWstETH
  

Reference: https://docs.mellow.finance/mellow-lrt-primitive/contract-deployments

[0xklapouchy]

@blckhv

You are aware that symbioticWstETH is part of the TVL calculation?

https://etherscan.io/address/0x1E1d1eD64e4F5119F60BF38B322Da7ea5A395429#readContract

Therefore depositing and LP minting is correct, based on Value of the deposit.

And when withdrawing via emergencyWithdraw it withdraw this bond to user based on Lp amount / Total Lp, exactly as expected, and as designed:

    /// @notice Handles emergency withdrawals, proportionally withdrawing all tokens in the system (not just the underlying).
    /// @dev Transfers tokens based on the user's share of lpAmount / totalSupply.

[blckhv]

Can you point me where do the TVL calculation is being used in emergencyWithdrawal?

function emergencyWithdraw(
        uint256[] memory minAmounts,
        uint256 deadline
    )
        external
        nonReentrant
        checkDeadline(deadline)
        returns (uint256[] memory actualAmounts)
    {
        uint256 timestamp = block.timestamp;
        address sender = msg.sender;
        if (!_pendingWithdrawers.contains(sender)) revert InvalidState();
        WithdrawalRequest memory request = _withdrawalRequest[sender];
        if (timestamp > request.deadline) {
            _cancelWithdrawalRequest(sender);
            return actualAmounts;
        }

        if (
            request.timestamp + configurator.emergencyWithdrawalDelay() >
            timestamp
        ) revert InvalidState();

        uint256 totalSupply = totalSupply();
        (address[] memory tokens, uint256[] memory amounts) = baseTvl();
        if (minAmounts.length != tokens.length) revert InvalidLength();
        actualAmounts = new uint256[](tokens.length);
        for (uint256 i = 0; i < tokens.length; i++) {
            if (amounts[i] == 0) {
                if (minAmounts[i] != 0) revert InsufficientAmount();
                continue;
            }
            uint256 amount = FullMath.mulDiv(
                IERC20(tokens[i]).balanceOf(address(this)),
                request.lpAmount,
                totalSupply
            );
            if (amount < minAmounts[i]) revert InsufficientAmount();
            IERC20(tokens[i]).safeTransfer(request.to, amount);
            actualAmounts[i] = amount;
        }
        delete _withdrawalRequest[sender];
        _pendingWithdrawers.remove(sender);
        _burn(address(this), request.lpAmount);
        emit EmergencyWithdrawal(sender, request, actualAmounts);
    }

[0xklapouchy]

@blckhv
You need to understand that emergencyWithdraw withdraws tokens proportionally to the LP tokens you have. TVL is used in the deposit() function to mint those LP tokens, and this is working correctly.

@WangSecurity
I have nothing to add anymore to this invalid issue. Hope you will reject it shortly.

[blckhv]

I see that deposit() uses underlyingTVL, not baseTVL and SymbioticWstETH is not used in the deposit TVL calculation, only the balance of wstETH that is in the Vault currently, otherwise normal withdrawals will suffer from this TVL calculation.

[0xklapouchy]

https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/modules/symbiotic/DefaultBondTvlModule.sol#L34

https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/Vault.sol#L96-L98

Please stop waisting my time. You have no idea how this works.

[WangSecurity]

Comments above still don’t show if there’s a loss of funds for any user. Emergency withdrawal should send all tokens in the system as said in its description. Hence, the users get the bond assets as they should.

The decision remains the same, planning to reject the escalation and leave the issue as it is.

[WangSecurity]

Will proceed with my decision in approx. 2 hours

[WangSecurity]

Result:
Invalid
Has duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ctf-sec: rejected