infect3d - Users can arbitrage assets into a vault despite the presence of a withdrawal queue

[sherlock-admin2]

infect3d

High

Users can arbitrage assets into a vault despite the presence of a withdrawal queue

Summary

The withdrawal queue do not prevent malicious users to deposit and withdraw in the same block, opening-up possibilities for arbitrage.

Vulnerability Detail

In order to limit arbitrage of the different assets of the vault, Mellow has implemented a withdrawal queue as it is common practice.

Each user can only have one withdrawal request at a time, which is stored inside a mapping at _withdrawalRequest[sender]
If a user has created a withdrawal request, its address gets addd to the _pendingWithdrawers set.
Then, operators can process users request by calling processWithdrawals(address[] memory users) with the array of users they want to process the requests.

This system should logically prevent a user to withdraw in the same block as a deposit, or at least make it really unlikely as the operator (which is trusted) must send the tx to process the request of the user in the same block, after the withdrawal request tx.

But there a way for a malicious user (Bob) to easily get around that protection following these steps:

1. Bob deposit some funds into the vault (the amount to deposit isn't important, can be the minimum required)
2. Bob create withdrawal requests regularly so that his address is always present in the queue (the value to withdraw isn't important, can be very low). Because the request is in the queue, there's chances that the operator will process it.
3. Bob listen to the mempool and see there is an opportunity to arbitrage the vault at the same time that the operator decided to process its waiting request with Bob's address in the input array of processWithdrawals(address[] memory users)
4. Bob front-run the operator and cancels its previous request
5. Still front-running, he deposit a large amount of funds to increase the profitability of the operation
6. still front-running, he create a new withdrawal request for the deposit he made in (5)
7. the operator tx is executed with Bob's address

Bob was able to deposit and withdraw in a same block and profit from a price change, also avoiding risk of slashing for LRT vaults like wstETH

Please note that this is even easier for vaults using the defaultBondStrategy as there is a processAll() function, in this case there is no need to keep a request alive in the queue

Impact

Users can circumvent the withdrawal queue and deposit>withdraw in the same block, opening possibilities for arbitrage

Code Snippet

The function analyzeRequest perform sanity checks against deadline and amounts to be withdrawn:
https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/Vault.sol#L536

File: src/Vault.sol
536:     function processWithdrawals(
537:         address[] memory users
538:     ) external nonReentrant returns (bool[] memory statuses) {
539:         _requireAtLeastOperator();
540:         statuses = new bool[](users.length);
541:         ProcessWithdrawalsStack memory s = calculateStack();
542:         uint256 burningSupply = 0;
543:         for (uint256 i = 0; i < users.length; i++) {
544:             address user = users[i];
545:             if (!_pendingWithdrawers.contains(user)) continue;
546:             WithdrawalRequest memory request = _withdrawalRequest[user];
547:             (
548:                 bool isProcessingPossible,
549:                 bool isWithdrawalPossible,
550:                 uint256[] memory expectedAmounts
551:⚠>>		 ) = analyzeRequest(s, request);
552: 
553:             if (!isProcessingPossible) {
554:                 _cancelWithdrawalRequest(user);
555:                 continue;
556:             }
557: 
558:             if (!isWithdrawalPossible) continue;
559: 

But no check of request timestamp creation against block.timestamp, which would prevent such operations:
https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/Vault.sol#L482

File: src/Vault.sol
476:     function analyzeRequest(
477:         ProcessWithdrawalsStack memory s,
478:         WithdrawalRequest memory request
479:     ) public pure returns (bool, bool, uint256[] memory expectedAmounts) { //* isProcessingPossible, isWithdrawalPossible, expectedAmounts
480:         uint256 lpAmount = request.lpAmount;
481:         if (
482:⚠>>          request.tokensHash != s.tokensHash || request.deadline < s.timestamp
483:         ) return (false, false, expectedAmounts);

Tool used

Manual review

Recommendation

Check request creation timestamp against block.timestamp and act accordingly if equal

[InfectedIsm]

Escalate

This should be a valid issue.

This report shows a path allowing a user to circumvent the withdrawal queue mechanism, and thus depositing/withdrawing in a same block, front-running the price update with a deposit, and back-running it with a withdraw
This allow the user to generate an immediate profit off the vault and its participants, without the slashing risk associated with this profit.

The implementation of withdrawal queue is a well-known protection against this type of exploit, but as demonstrated, this mechanism can be avoided by the user.

Applying the recommandation will fix this and withdrawal queue will effectively prevent this arbitrage.

[sherlock-admin3]

Escalate

This should be a valid issue.

This report shows a path allowing a user to circumvent the withdrawal queue mechanism, and thus depositing/withdrawing in a same block, front-running the price update with a deposit, and back-running it with a withdraw
This allow the user to generate an immediate profit off the vault and its participants, without the slashing risk associated with this profit.

The implementation of withdrawal queue is a well-known protection against this type of exploit, but as demonstrated, this mechanism can be avoided by the user.

Applying the recommandation will fix this and withdrawal queue will effectively prevent this arbitrage.

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.

[debugging3]

This issue is a duplicate of dany.armstrong90 - User can continue to earn points during withdrawal lockup period. #68

[z3s]

Hey, @InfectedIsm I asked the protocol team about these front-running issues, and their answer was:

processWithdrawals will occur through flashbots.

[InfectedIsm]

Hey @z3s , thank you for your swift reply, much appreciated

I agree with protocol team that operating through Flashbots would prevent this from happening.
However, nowhere in the documentation, nor the contest's readme, this attack was highlighted as a known issue, and its mitigation (using a private RPC) described.

Thus, this issue should be considered valid and not excluded, as the mitigation measures are shared post-audit.

[WangSecurity]

I agree with the escalation, flashbots were not mentioned anywhere and without them I agree the issue possible. The only consideration I've now got is would the deposit queue fix this vulnerability. It will make the scenario in the report impossible, but is it possible to make a small withdrawal request, then front-run the processWithdrawals call and make a larger withdrawal?

[InfectedIsm]

but is it possible to make a small withdrawal request, then front-run the processWithdrawals call and make a larger withdrawal?

Yes, when user call Vault::registerWithdrawal with closePrevious == true then its withdrawalRequest is canceled L451, and the new request is written in _withdrawalRequest[sender] L496
Then, Vault::processWithdrawals process the withdrawalRequest by simply reading _withdrawalRequest[user] for each user of the input array.
While the withdrawalRequest has a timestamp parameter written at the time of the registering, the only check that is made is against the request deadline (as shown in the initial report)

The only consideration I've now got is would the deposit queue fix this vulnerability

This would require the deposit (register) queue to be correctly implemented, i.e ensuring that a malicious user cannot deposit and withdraw in a same block.
This can be achieved either by checking registration timestamp against block.timestamp, or ensuring that deposit queue is processed after withdrawal queue, otherwise, users could place the deposit tx before the processDeposit call, and the withdraw tx before processWithdrawals

[debugging3]

I agree with the escalation, flashbots were not mentioned anywhere and without them I agree the issue possible. The only consideration I've now got is would the deposit queue fix this vulnerability. It will make the scenario in the report impossible, but is it possible to make a small withdrawal request, then front-run the processWithdrawals call and make a larger withdrawal?

I recommend you read #68.
The example of the issue shows the possibility of it and the recommendation is just for your consideration, I think.

[WangSecurity]

Thank you for the answers. I agree it's a valid issue cause it allows to significantly increase the withdrawal for the user if they see a significant price decrease of LST/LRT token in the next block (assuming the withdrawal is also in the next block). This big withdrawal (that was initially small), allows the attack to decrease the price even lower and then re-deposit in the next block at lower price, resulting in extracting tokens from users who kept their funds in the protocol.

But I believe, medium severity is more appropriate, since for this to be profitable, there should be a big price decrease of LST/LRT and withdrawal in the same block, since there is a withdrawal fee that may be even bigger than the profit from such arbitrage. Planning to accept the escalation.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• InfectedIsm: accepted

[Slavchew]

Thank you for the answers. I agree it's a valid issue cause it allows to significantly increase the withdrawal for the user if they see a significant price decrease of LST/LRT token in the next block (assuming the withdrawal is also in the next block). This big withdrawal (that was initially small), allows the attack to decrease the price even lower and then re-deposit in the next block at lower price, resulting in extracting tokens from users who kept their funds in the protocol.

But I believe, medium severity is more appropriate, since for this to be profitable, there should be a big price decrease of LST/LRT and withdrawal in the same block, since there is a withdrawal fee that may be even bigger than the profit from such arbitrage. Planning to accept the escalation.

How is this valid if you deposit at time = 0, amount = 100e18 at a price of $1 for 1e18 = $100, and right after that request a withdrawal. Then at time = 100, the operator processes the request at a price of $0.95 for 100e18, you will receive $95 but 100e18 tokens.

Also the upper example - If you front-run and deposit more tokens before the price drops at the end, you will get the same amount of tokens that you deposited. The price has nothing to do with Mellow's contract.

This can be seen in the tests also - https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/tests/mainnet/unit/VaultTest.t.sol#L2996-L3052

The whole idea of the deposit and withdrawal request has nothing to do with the price change.

@WangSecurity Isn't it appropriate to ask the Watson for a test on this to show what bonus he will get from doing this, without it there is no main context to the problem, only that you can front-run the operator, which is not a wrong thing, so as you still get all your tokens not more.

[debugging3]

The issue didn't explicitly prove how arbitrage become profitable.
In order to arbitrage become profitable, the deposit and registerWithdrawal have to sandwich certain price changes of underlying tokens or amount changes of TVL modules.
However, in order to this issue become valid, the deposit and registerWithdrawal should front-run the processWithdrawRequests in the same block.
Therefore, the deposit and registerWithdrawal have to sandwich some change of TVL and at the same time have to front-run processWithdrawRequests. And this is almost not possible because the registerWithdrawal have to back-run the change of TVL and front-run the processWithdrawRequests at the same time.
That is, the attacker have to set low gas price for front-run and set high price for back-run, is it possibe to do them simultaneously ?

[InfectedIsm]

@Slavchew

Allowing user to withdraw right after its deposit in a same block ensure he is not exposed to any infortunate event (as slahsing), thus securing its profit, avoiding losses.

While vault are subject to price increase of shares, they can also incur price decrease through slashing for example, or other decreasing events depending on the used strategy.

For recall, the emergencyWithdrawalDelay for all vault is configured to 7776000 or 90 days (e.g : [Configurator contract](https://etherscan.io/address/0xe6180599432767081beA7deB76057Ce5883e73Be#readContract)), so users must expect to be able withdraw in-between those timebounds, but here this can be avoided.

Regarding the profit, front-running the price increase of the LP allow the user to deposit the assets right before, getting LP for “cheap”, then instantly redeem the LP for a greater amount of assets.

It must not be forgotten that vaults can have as underlying tokens USDC and USDT, as it was updated afterward by the sponsors. Those vaults will likely use other kind of strategies to generate profit for depositors, where share price increase can be captured too.

Finally, must also be noted that deposit and withdraw ratios can be configured differently as there is a isDeposit parameter when updating or getting ratios. This create even more possible configurations where its possible to take advantage of those ratios, for example when the ‘deposit ratio’ of the asset increasing in value is lower than its ‘withdraw ratio’.

@debugging3

I am not an expert about flashbots, but I think that a user using flashbots do not have to take care of gas price ordering inside the bundle when proposing a tx bundle to be executed.
Also, even though, even outside of the use of flashbots, this doesn’t make it “almost not possible”, as this is hypothetical to the gas configuration/strategy each actor is using for its tx.

[debugging3]

As I know, the flashbots are for avoiding front-run attack, so the attacker can't use flashbots for front-run attack.
Moreover, in addition to the previous mentioned conditions, the possibility of the change of TVL and the processWithdrawalRequests being executed in the same block is very low.

[WangSecurity]

@Slavchew how I see the attacker profiting from the price increase here:

The attacker sees a price sharp price increase in the underlying LRT/LST and the operator processing withdrawals in the same block. They deposit more funds into the protocol at price X, then a price increase takes place and they withdraw all the deposited amounts at a price of 1.1X (I know 10% is a very large increase, but it's an example to show the profit here, also they still withdraw the underlying token LST/LRT, but they can swap it on the secondary market). Essentially, the attacker made a free trade here. Yes, they can be subject to withdrawal fees. Additionally, a similar scenario was mentioned here. But, in the linked issue the attacker is also subject to a withdrawal queue, where here the attacker almost bypasses the withdrawal queue.

As for avoiding the slashing event. The attacker can bypass the queue, but still, they would withdraw the underlying token, i.e. the LRT/LST (e.g. wstETH) and would still suffer a loss due to the slashing event. On the other hand, in that case, the attacker can swap wstETH they got out of the Vault before the slashing event.

@debugging3 yep, there are quite a lot of constraints for this issue and the probability may be low, but it's still possible and we don't judge the severity based on the likelihood since it's quite a subjective parameter.

With that said, I agree it's a very borderline issue, but I lean towards it being medium since the attack is still possible, even though the constraints are high.

[IlIlHunterlIlI]

Just a small point, we really need the operator to process withdrawals in the same time of slashing event but with higher gas, so that the attack is successful, right?

The corner stone of the attack is that the operator process withdrawals(front run his own protocol of slashing or price changes).

Doesn't this seems involving some acts from trusted roles?

[WangSecurity]

I don't think it involves "some acts from trusted roles" as it may happen unintentionally. Again, the scenario with the slashing event is not the only one and I think the scenario with the price increase is more viable, but of course, it's subjective. Still, there's not only one scenario and considering both of them, I believe it qualifies for the Medium sevrity.

[debugging3]

@WangSecurity
Thank you for your kind explanation.
Only one more problemIf this issue is valid despite its many constraints, why is #68 invalid?
As I see, #68 has the same cause and recommendation with this issue.

[WangSecurity]

I don’t have enough context on #68 because I didn’t invalidate it and it wasn’t escalated:

Any request to resolve issues outside of the escalation period is not guaranteed to be addressed.

[debugging3]

I have stated that #68 is a duplicate of this issue during escalation period.
I misunderstood the juding rule at that time.
Now, I realized the rule "No escalation, no rewards" disregarding it's validity.
Thank you for your reply.

[WangSecurity]

Oh, it was misunderstanding from my side, I'll check if it's a duplicate

[Slavchew]

Here is a PoC showing that a user cannot get more tokens after a price increase, because the amount they receive is the amount they deposited, and the price increase/decrease does nothing. In case he deposits 1000 wsteth tokens and the price increases, he will withdraw 1000 tokens again and of course, he will have more money, but he will also have the same amount if he waits for the price to increase and does not interact with Mellow.

function testPriceIncreaseDoesNothing() 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);

        //

        VaultConfigurator configurator = VaultConfigurator(
            address(vault.configurator())
        );

        ChainlinkOracle pricesOracle = ChainlinkOracle(address(configurator.priceOracle()));
        ChainlinkOracle.AggregatorData memory agg = pricesOracle.aggregatorsData(address(vault), Constants.WSTETH);

        WStethRatiosAggregatorV3 aggAdd = WStethRatiosAggregatorV3(agg.aggregatorV3);

        uint256 priceBefore = uint256(aggAdd.getAnswer());
        console.log("WSTETH price", priceBefore);

        address depositor = address(bytes20(keccak256("depositor")));
        address depositor2 = address(bytes20(keccak256("depositor2")));
        address depositor3 = address(bytes20(keccak256("depositor3")));

        vm.startPrank(depositor);
        deal(Constants.WSTETH, depositor, 100 ether);

        console.log("Balance Before price drop", IERC20(Constants.WSTETH).balanceOf(depositor));

        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            100 ether
        );
        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 100 ether;
        uint256[] memory minAmounts = amounts;
        vault.deposit(depositor, amounts, 0, type(uint256).max);

        vm.startPrank(depositor2);
        deal(Constants.WSTETH, depositor2, 180 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            180 ether
        );

        uint256[] memory amounts2 = new uint256[](1);
        amounts2[0] = 180 ether;

        vault.deposit(depositor2, amounts2, 0, type(uint256).max);
        vm.stopPrank();

        vm.startPrank(depositor3);
        deal(Constants.WSTETH, depositor3, 86 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(
            address(vault),
            86 ether
        );

        uint256[] memory amounts3 = new uint256[](1);
        amounts3[0] = 86 ether;

        vault.deposit(depositor3, amounts3, 0, type(uint256).max);
        vm.stopPrank();

        aggAdd.setPrice(priceBefore + (priceBefore * 1000 / 10000));
        // CASE with Decrease - aggAdd.setPrice(priceBefore - (priceBefore * 1000 / 10000));
        (, int256 priceAfter , , ,) = aggAdd.latestRoundData();
        console.log("WSTETH price after", uint(priceAfter));

        vm.startPrank(depositor);
        vault.registerWithdrawal(
            depositor,
            100 ether,
            minAmounts,
            type(uint256).max,
            type(uint256).max,
            false
        );
        vm.stopPrank();


        
        vm.startPrank(operator);
        address[] memory users = new address[](1);
        users[0] = depositor;
        vault.processWithdrawals(users);
        {
            address[] memory withdrawers = vault.pendingWithdrawers();
            assertEq(withdrawers.length, 0);
        }
        console.log("Balance After", IERC20(Constants.WSTETH).balanceOf(depositor));
    }

OUTPUT:

Logs:
  WSTETH price 1174146665442558861
  Balance Before price drop 100000000000000000000
  WSTETH price after 1291561331986814747
  Balance After 100000000000000000000

The following files must also be changed because the test setUp is wrong.

function _setUp(Vault vault) internal {
        ERC20TvlModule erc20TvlModule = new ERC20TvlModule();
        vault.addTvlModule(address(erc20TvlModule));

        vault.addToken(Constants.WSTETH);
        VaultConfigurator configurator = VaultConfigurator(
            address(vault.configurator())
        );

        // oracles setup
        {
            ManagedRatiosOracle ratiosOracle = new ManagedRatiosOracle();

            uint128[] memory ratiosX96 = new uint128[](1);
            ratiosX96[0] = 2 ** 96;
            ratiosOracle.updateRatios(address(vault), true, ratiosX96);
            ratiosOracle.updateRatios(address(vault), false, ratiosX96);

            configurator.stageRatiosOracle(address(ratiosOracle));
            configurator.commitRatiosOracle();

            ChainlinkOracle chainlinkOracle = new ChainlinkOracle();
            chainlinkOracle.setBaseToken(address(vault), Constants.WETH);
            address[] memory tokens = new address[](2);
            tokens[0] = Constants.WSTETH;
            tokens[1] = Constants.WETH;

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

            configurator.stagePriceOracle(address(chainlinkOracle));
            configurator.commitPriceOracle();
        }

        configurator.stageMaximalTotalSupply(1000 ether);
        configurator.commitMaximalTotalSupply();
    }

    function _initialDeposit(Vault vault) internal {
        vm.startPrank(admin);
        _setupDepositPermissions(vault);
        vm.stopPrank();

        vm.startPrank(operator);
        deal(Constants.WSTETH, operator, 10 gwei);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(address(vault), 10 gwei);

        uint256[] memory amounts = new uint256[](1);
        amounts[0] = 10 gwei;
        vault.deposit(address(vault), amounts, 10 gwei, type(uint256).max);

        assertEq(IERC20(Constants.WSTETH).balanceOf(address(vault)), 10 gwei);
        assertEq(IERC20(Constants.RETH).balanceOf(address(vault)), 0);
        assertEq(IERC20(Constants.WETH).balanceOf(address(vault)), 0);
        assertEq(vault.balanceOf(address(vault)), 10 gwei);
        assertEq(vault.balanceOf(operator), 0);

        vm.stopPrank();
    }

As well as adding a way to change the price in the test

// SPDX-License-Identifier: BSL-1.1
pragma solidity 0.8.23;

import "../interfaces/external/chainlink/IAggregatorV3.sol";
import "../interfaces/external/lido/IWSteth.sol";

contract WStethRatiosAggregatorV3 is IAggregatorV3 {
    uint8 public constant decimals = 18;
    address public immutable wsteth;
+    uint256 public price;
+    bool public isCustomPrice;

    constructor(address wsteth_) {
        wsteth = wsteth_;
    }

+    function setPrice(uint256 _price) external {
+       price = _price;
+       isCustomPrice = true;
+   }

    function getAnswer() public view returns (int256) {
        return int256(IWSteth(wsteth).getStETHByWstETH(10 ** decimals));
    }

    function latestRoundData()
        public
        view
        override
        returns (uint80, int256, uint256, uint256, uint80)
    {
-        return (0, getAnswer(), block.timestamp, block.timestamp, 0);
+        return (0, isCustomPrice ? int256(price) : getAnswer(), block.timestamp, block.timestamp, 0);
    }
}

[WangSecurity]

With that test, I would agree the issue is low severity. Yes, it allows for the attacker to get essentially a free trade, but it doesn't lead to a loss of funds for other users or a protocol. It also allows to bypass the withdrawal queue, but I don't see it as medium severity:

Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.
Breaks core contract functionality, rendering the contract useless or leading to loss of funds.

Hence, will invalidate it tomorrow morning to give time to @InfectedIsm to provide counterarguments.

[InfectedIsm]

Hi @WangSecurity, I think I basically shared how this can lead to a profit for the user (and thus a loss for the other depositors) in this comment: #119 (comment)

To summarize the comment, there can be profit in the following situations:

1. Vaults can be based on other assets than rebasing assets, i.e USDT and USDC as stated.
   In those cases, vault will not earn yield through rebasing, but through strategies where they will be deposited. When yield is earned by the vault, the LP value will increase. Here, we are in a typical LP value increase capture, as user will deposit X USDC and get Y USDC where X>Y.
   There is no such example in the codebase though, as only strategies based on LRT has been developed yet, so this is hypothetical to how would work USDC/USDT based vaults, but I think the hypothesis is fair knowing how existing vault already work.

2. Deposit ratio and withdraw ratio can be different as it can be seen in the ManagedRatiosOracle. We see that the isDeposit parameter is set differently if its a deposit or a withdrawal. The reason for allowing such possibilities can be diverse, but one could simply based on a complex dynamic strategy based on multiple LRT. This is up to vault owner to decide how to use that built-in available capability.

To develop further example 2, here a scenario:

1. Vault with 2 LRTs, [wstETH, rETH]
2. price of both tokens is $1000
3. depositRatio = [50, 50] and withdrawalRatio = [60, 40]
4. user deposit 10 wstETH and 10 rETH which is worth $20000
5. wstETH value increase to $1010 through rebasing, but as rETH rebase is not synchronous with stETH, its price do not change
6. user immediately withdraw and get 12 wstETH and 8 rETH
7. User has now $20120 worth of assets

Using easy numbers to make the scenario easily understandable, but numbers are available here
This is one example amongst all possibilities, but the idea is here I think.

[Slavchew]

Totally wrong

First read about wstETH, the token is not rebasing. There are no rebasing tokens in the allowed tokens. Then increasing the price of an underlying do not change the value the user receives.

If you have any contra-arguments provide coded PoC showing how price increase change the user token balance, not the price.

[InfectedIsm]

Totally wrong

First read about wstETH, the token is not rebasing. There are no rebasing tokens in the allowed tokens. Then increasing the price of an underlying do not change the value the user receives.

If you have any contra-arguments provide coded PoC showing how price increase change the user token balance, not the price.

I'm not sure why this remark about wstETH, as stETH rebase => wstETH value increase.
Then following my scenario of a [wstETH,rETH] vault, being able to sandwich wstETH value increase with such deposit/withdrawal ratio, user will get more of the appreciated token, and less of the still not appreciated one, generating a profit.
In your example, you show a vault with only wstETH as the underlying token, and I agree, in this case this is not possible to profit from the price increase.
Also, I've shown 2 paths for which this make a profit possible.

[WangSecurity]

@InfectedIsm as I understand from Lido docs, wstETH is indeed not a rebase token:

For easier DeFi integrations, stETH has a non-rebasable value-accruing (non-rebasable) counterpart called 'wrapped stETH' (or just wstETH).

And other underlying tokens from the README are not rebase tokens, are they?

[Slavchew]

Below is the changed test with RETH as underlying as well.

@WangSecurity Just to mention something, @InfectedIsm stated depositRatio = [50, 50] and withdrawalRatio = [60, 40]. This will indeed give you back more than one of the tokens when you withdraw, but as an amount, although it is NEVER mentioned anywhere what the withdrawal ratios will be, as the vault only works with wstETH for now. Furthermore, the cases below show that even with this ratio, the user (attacker) will receive fewer tokens after the price increase.

  function testC111() 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);

      //

      VaultConfigurator configurator = VaultConfigurator(
          address(vault.configurator())
      );

      ChainlinkOracle pricesOracle = ChainlinkOracle(address(configurator.priceOracle()));
      ChainlinkOracle.AggregatorData memory agg = pricesOracle.aggregatorsData(address(vault), Constants.WSTETH);

      WStethRatiosAggregatorV3 aggAdd = WStethRatiosAggregatorV3(agg.aggregatorV3);

      uint256 priceBefore = uint256(aggAdd.getAnswer());
      console.log("WSTETH price", priceBefore);

      address depositor = address(bytes20(keccak256("depositor")));
      address depositor2 = address(bytes20(keccak256("depositor2")));
      address depositor3 = address(bytes20(keccak256("depositor3")));

      vm.startPrank(depositor);
      deal(Constants.WSTETH, depositor, 100 ether);
      deal(Constants.RETH, depositor, 100 ether);

      console.log("Depositor WSTETH balance before deposit", IERC20(Constants.WSTETH).balanceOf(depositor));
      console.log("Depositor RETH balance before deposit", IERC20(Constants.RETH).balanceOf(depositor));

      IERC20(Constants.WSTETH).safeIncreaseAllowance(
          address(vault),
          100 ether
      );
      IERC20(Constants.RETH).safeIncreaseAllowance(
          address(vault),
          100 ether
      );
      uint256[] memory amounts = new uint256[](2);
      amounts[0] = 100 ether;
      amounts[1] = 100 ether;
      vault.deposit(depositor, amounts, 0, type(uint256).max);

      vm.startPrank(depositor2);
      deal(Constants.WSTETH, depositor2, 20 ether);
      deal(Constants.RETH, depositor2, 20 ether);
      IERC20(Constants.WSTETH).safeIncreaseAllowance(
          address(vault),
          20 ether
      );
      IERC20(Constants.RETH).safeIncreaseAllowance(
          address(vault),
          20 ether
      );

      uint256[] memory amounts2 = new uint256[](2);
      amounts2[0] = 20 ether;
      amounts2[1] = 20 ether;

      vault.deposit(depositor2, amounts2, 0, type(uint256).max);
      vm.stopPrank();

      vm.startPrank(depositor3);
      deal(Constants.WSTETH, depositor3, 10 ether);
      deal(Constants.RETH, depositor3, 10 ether);
      IERC20(Constants.WSTETH).safeIncreaseAllowance(
          address(vault),
          10 ether
      );
      IERC20(Constants.RETH).safeIncreaseAllowance(
          address(vault),
          10 ether
      );

      uint256[] memory amounts3 = new uint256[](2);
      amounts3[0] = 10 ether;
      amounts3[1] = 10 ether;

      vault.deposit(depositor3, amounts3, 0, type(uint256).max);
      vm.stopPrank();

      aggAdd.setPrice(priceBefore + (priceBefore * 1000 / 10000));
      // CASE with Decrease - aggAdd.setPrice(priceBefore - (priceBefore * 1000 / 10000));
      (, int256 priceAfter , , ,) = aggAdd.latestRoundData();
      console.log("WSTETH price after", uint(priceAfter));

      uint256[] memory minAmounts = new uint256[](2);
      minAmounts[0] = 0;
      minAmounts[1] = 0;
      vm.startPrank(depositor);
      vault.registerWithdrawal(
          depositor,
          1000 ether,
          minAmounts,
          type(uint256).max,
          type(uint256).max,
          false
      );
      vm.stopPrank();


      
      vm.startPrank(operator);
      address[] memory users = new address[](1);
      users[0] = depositor;
      vault.processWithdrawals(users);
      {
          address[] memory withdrawers = vault.pendingWithdrawers();
          assertEq(withdrawers.length, 0);
      }
      console.log("Balance After", IERC20(Constants.WSTETH).balanceOf(depositor));
      console.log("Balance After", IERC20(Constants.RETH).balanceOf(depositor));
  }

    function _setUp(Vault vault) internal {
        ERC20TvlModule erc20TvlModule = new ERC20TvlModule();
        vault.addTvlModule(address(erc20TvlModule));

        vault.addToken(Constants.WSTETH);
        vault.addToken(Constants.RETH);
        VaultConfigurator configurator = VaultConfigurator(
            address(vault.configurator())
        );

        // oracles setup
        {
            ManagedRatiosOracle ratiosOracle = new ManagedRatiosOracle();

            uint128[] memory ratiosX96 = new uint128[](2);
            ratiosX96[0] = 2 ** 95;
            ratiosX96[1] = 2 ** 95;
            uint128[] memory ratiosX96Withdraw = new uint128[](2);
            ratiosX96Withdraw[0] = 47536897508558602556126370201;
            ratiosX96Withdraw[1] = 31691265005705735037417580135;
            ratiosOracle.updateRatios(address(vault), true, ratiosX96);
            ratiosOracle.updateRatios(address(vault), false, ratiosX96Withdraw);

            configurator.stageRatiosOracle(address(ratiosOracle));
            configurator.commitRatiosOracle();

            ChainlinkOracle chainlinkOracle = new ChainlinkOracle();
            chainlinkOracle.setBaseToken(address(vault), Constants.WETH);
            address[] memory tokens = new address[](3);
            tokens[0] = Constants.WSTETH;
            tokens[1] = Constants.RETH;
            tokens[2] = Constants.WETH;

            IChainlinkOracle.AggregatorData[]
                memory data = new IChainlinkOracle.AggregatorData[](3);
            data[0] = IChainlinkOracle.AggregatorData({
                aggregatorV3: address(
                    new WStethRatiosAggregatorV3(Constants.WSTETH)
                ),
                maxAge: 30 days
            });
            data[1] = IChainlinkOracle.AggregatorData({
                aggregatorV3: address(new ConstantAggregatorV3(1 ether)), // RETH price
                maxAge: 30 days
            });
            data[2] = IChainlinkOracle.AggregatorData({
                aggregatorV3: address(new ConstantAggregatorV3(1 ether)),
                maxAge: 30 days
            });
            chainlinkOracle.setChainlinkOracles(address(vault), tokens, data);

            configurator.stagePriceOracle(address(chainlinkOracle));
            configurator.commitPriceOracle();
        }

        configurator.stageMaximalTotalSupply(1000 ether);
        configurator.commitMaximalTotalSupply();
    }

    function _initialDeposit(Vault vault) internal {
        vm.startPrank(admin);
        _setupDepositPermissions(vault);
        vm.stopPrank();

        vm.startPrank(operator);
        deal(Constants.WSTETH, operator, 10 gwei);
        deal(Constants.RETH, operator, 10 gwei);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(address(vault), 10 gwei);
        IERC20(Constants.RETH).safeIncreaseAllowance(address(vault), 10 gwei);

        uint256[] memory amounts = new uint256[](2);
        amounts[0] = 10 gwei;
        amounts[1] = 10 gwei;
        vault.deposit(address(vault), amounts, 10 gwei, type(uint256).max);

        assertEq(IERC20(Constants.WSTETH).balanceOf(address(vault)), 10 gwei);
        assertEq(IERC20(Constants.RETH).balanceOf(address(vault)), 10 gwei);
        assertEq(IERC20(Constants.WETH).balanceOf(address(vault)), 0);
        assertEq(vault.balanceOf(address(vault)), 10 gwei);
        assertEq(vault.balanceOf(operator), 0);

        vm.stopPrank();
    }

Here is the output when 3 users deposit 50% wstETH and 50% rETH, and one of them withdraw immediately after wstETH price increase.

// Without changing the price
 WSTETH price 1174146665442558861
  Depositor WSTETH balance before deposit 100000000000000000000
  Depositor RETH balance before deposit 100000000000000000000
  Balance After 118107937807653537938
  Balance After 78738625205102358625
  
  ----------------------------------------------------------------------------------------------
// With changing the price
  WSTETH price 1174146665442558861
  Depositor WSTETH balance before deposit 100000000000000000000
  Depositor RETH balance before deposit 100000000000000000000
  WSTETH price after 1291561331986814747
  Balance After 117022192184083789778
  Balance After 78014794789389193185
  

This is the result when using the same 50%/50% ratio for withdrawal as well.

Logs:
  WSTETH price 1174146665442558861
  Depositor WSTETH balance before deposit 100000000000000000000
  Depositor RETH balance before deposit 100000000000000000000
  WSTETH price after 1291561331986814747
  Balance After 100000000000000000000
  Balance After 100000000000000000000

@WangSecurity There are no examples of how withdrawal ratios will be configured for multiple tokens, and even with that these examples show that a user cannot get a free token via a sandwich price increase, especially since the report does not mention a different withdrawal ratio anywhere. This should be treated as configured and with the current configuration after price increase/decrease, users receive 0 more tokens.

[WangSecurity]

Yep, I agree that ratios are not mentioned anywhere and after reading the report I understand the problem is not in them and the attack should be possible with the same ratios for deposit and withdrawal. 2 underlying (or more) are not mentioned in the report and it talks solely about the price increase of an asset. As was proven the price increase doesn't affect the tokens at all and won't lead to a loss of funds. Additionally, as I understand the attack is possible with rebasing tokens, but they're not used.

Hence, the decision is to invalidate this report tomorrow morning, since no other users lose funds.

[InfectedIsm]

This is a test modified with different ratio:

Click to expand

	function testPriceIncreaseDoesNothing() 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);


        VaultConfigurator configurator = VaultConfigurator(
            address(vault.configurator())
        );

        ChainlinkOracle pricesOracle = ChainlinkOracle(address(configurator.priceOracle()));
        ChainlinkOracle.AggregatorData memory agg = pricesOracle.aggregatorsData(address(vault), Constants.WSTETH);
        ChainlinkOracle.AggregatorData memory aggRETH = pricesOracle.aggregatorsData(address(vault), Constants.RETH);

        WStethRatiosAggregatorV3[] memory aggAdd = new WStethRatiosAggregatorV3[](2); 
		aggAdd[0] = WStethRatiosAggregatorV3(agg.aggregatorV3);

        uint256 priceBefore = uint256(aggAdd[0].getAnswer());
        console.log("WSTETH price", priceBefore);

        address depositor = address(bytes20(keccak256("depositor")));
        address depositor2 = address(bytes20(keccak256("depositor2")));
        address depositor3 = address(bytes20(keccak256("depositor3")));

        vm.startPrank(depositor);
        deal(Constants.WSTETH, depositor, 100 ether);
        console.log("Balance Before price drop", IERC20(Constants.WSTETH).balanceOf(depositor));
        IERC20(Constants.WSTETH).safeIncreaseAllowance(address(vault), 100 ether);

        deal(Constants.RETH, depositor, 100 ether);
        console.log("Balance Before price drop", IERC20(Constants.RETH).balanceOf(depositor));
        IERC20(Constants.RETH).safeIncreaseAllowance(address(vault), 100 ether);
		
        uint256[] memory amounts = new uint256[](3);
        amounts[0] = 100 ether;
        amounts[1] = 100 ether;
        vault.deposit(depositor, amounts, 0, type(uint256).max);
		vm.stopPrank();

        vm.startPrank(depositor2);
        deal(Constants.WSTETH, depositor2, 100 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(address(vault), 100 ether);
        deal(Constants.RETH, depositor2, 100 ether);
        IERC20(Constants.RETH).safeIncreaseAllowance(address(vault), 100 ether);
        amounts[0] = 100 ether;
        amounts[1] = 100 ether;
        vault.deposit(depositor2, amounts, 0, type(uint256).max);
		vm.stopPrank();


        vm.startPrank(depositor3);
        deal(Constants.WSTETH, depositor3, 100 ether);
        IERC20(Constants.WSTETH).safeIncreaseAllowance(address(vault), 100 ether);
        deal(Constants.RETH, depositor3, 100 ether);
        IERC20(Constants.RETH).safeIncreaseAllowance(address(vault), 100 ether);
        amounts[0] = 100 ether;
        amounts[1] = 100 ether;
        vault.deposit(depositor3, amounts, 0, type(uint256).max);
		vm.stopPrank();


        aggAdd[0].setPrice(priceBefore + (priceBefore * 1000 / 10000));
        // CASE with Decrease - aggAdd.setPrice(priceBefore - (priceBefore * 1000 / 10000));
        (, int256 priceAfter , , ,) = aggAdd[0].latestRoundData();
        console.log("WSTETH price after", uint(priceAfter));


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

        vm.startPrank(depositor);
        vault.registerWithdrawal(
            depositor,
            IERC20(address(vault)).balanceOf(depositor),
            minAmounts,
            type(uint256).max,
            type(uint256).max,
            false
        );
        vm.stopPrank();

        vm.startPrank(operator);
        address[] memory users = new address[](1);
        users[0] = depositor;
        vault.processWithdrawals(users);
        {
            address[] memory withdrawers = vault.pendingWithdrawers();
            assertEq(withdrawers.length, 0);
        }

        console.log("Balance After", IERC20(Constants.WSTETH).balanceOf(depositor));
		console.log("Balance After", IERC20(Constants.RETH).balanceOf(depositor));
    }

And output:

  WSTETH price 1166209160223626863
  Balance Before price drop 100000000000000000000
  Balance Before price drop 100000000000000000000
  WSTETH price after 1282830076245989549
  Balance After 144638007597241338868
  Balance After 48212669199080446289

If rETH suffer slashing, the user will have made a profitable operation.
But I can hear that this is a pretty very low likelihood scenario.
And I understand @Slavchew argument, and why he insisted on the difference between rebasing and value accruing nature of wstETH and stETH.

Nevertheless, scenario (1) based on USDT/USDC vaults will work as a standard vault as those are not LST. Yield is generated through strategies and increasing LP value in a way that can be extracted by depositing right before yield is sent to vault, and LP value increase.

@WangSecurity Please note that the initial submission isn't focused solely on LST vaults even though the escalation has revolved around that specific case, and that both type of vaults are in scope.

[WangSecurity]

@InfectedIsm but what would be the estimated loss that other users would suffer?

[InfectedIsm]

@InfectedIsm but what would be the estimated loss that other users would suffer?

On scenario (1), user will capture a share of the yield through the increase of the vault LP token value, based on the additional USDT/USDC accrued into the vault by the strategy, without the risk of incurring a loss from the strategy.
While other depositors subject to the withdrawal queue, would experience both LP price increase, and LP price decrease.
So over time, the user is extracting value by getting a share of the yield when LP increase, but letting other depositor suffer the whole loss when LP decrease.

[WangSecurity]

But the attacker would suffer the withdrawal fee themselves. Additionally, the yield is sent to the vault as a one-time transaction, correct? So for example, there's a one-time injection of the yield of 2%, correct?

[InfectedIsm]

A withdrawal fee would lower the profit surely, and if set higher than expected yield from the strategy, then user would be able to profit from the vault only in cases where the generates yield is greater than the withdrawal fee.

[WangSecurity]

Thank you, but I don't think you answered my questions, the question wasn't about the withdrawal fee, but how the yield is accrued:

But the attacker would suffer the withdrawal fee themselves. Additionally, the yield is sent to the vault as a one-time transaction, correct? So for example, there's a one-time injection of the yield of 2%, correct?

[InfectedIsm]

Sorry I've missed to answer that.
There is no clear evidence in the codebase how would the yield accrue, as implemented strategies are only about LRTs.
The one possibilities would be either through the deposit/withdraw callbacks or through operator acting manually.
In both case, there is a possibility to frontrun the yield.
If deposit callback, as the lpAmount is calculated before calling the callback, the user could deposit when the yield would be harvested depending on the strategy condition.
If withdraw callback, in case the operator withdraw in multiple batches, user could deposit before a callback harvesting yield.
If done manually by the operator, same thing can happen.

[WangSecurity]

@InfectedIsm so you mean the admins will inject yield via deposit and withdraw callbacks? Wouldn't the yield come from Mellow investing in other protocols, such as Symbiotic now, so the yield comes from the symbiotic bonds and it will steadily accrue?

[InfectedIsm]

@WangSecurity can you share a GMT time until when I can defend?

I'm right now not in a position to correctly research as I only have access to my phone due to constraints.

But I'm interested about you point, I'm not sure if USDT/USDC tokens are eligible for Symbiotic vaults. If it is the case, then we would expect admins to use them, yes.

[WangSecurity]

Honestly, further thinking about the issue, I don't think it's defendable. Of course, I'm giving you time until 11 am GMT (hope that's enough). But, this issue is about the LP share value increasing. It's quite fair to assume the larger the vault TVL the larger will be the share value increase (since more tokens to accrue yield). But the larger the pool, the larger capital the attack has to have. The larger the capital, the larger the withdrawal fee that has to be paid (but it can be 0, of course). Moreover, it depends on a one-time yield injection to increase the LP share value, if the yield accrues steadily with time, this won't be possible.

With that being said, I don't see how the attack could cause a considerable loss of funds with the attacker ending up with a profit or at least breakeven.

Again, you have a bit of time to defend it, but I can't give you much time, unfortunately, and will invalidate the issue if no new concrete arguments are present.

[InfectedIsm]

Thanks @WangSecurity for having letting me this time, really appreciate.
I have nothing to add to your points, so you can proceed with your decision.