eeyore - Corrupted oracle system if more than 2 underlying tokens are used and one of them is WSTETH

[sherlock-admin3]

eeyore

Medium

Corrupted oracle system if more than 2 underlying tokens are used and one of them is WSTETH

Summary

The priceX96() function always enforces a cross-oracle price check, even when a direct price feed is provided by Chainlink.

Vulnerability Detail

There is no option to use direct price feeds like RETH/ETH or USDC/ETH without an extra step using ConstantAggregatorV3 or another Chainlink price feed. The cross-oracle price check is required for WSTETH/STETH and then STETH/ETH price check but should not be enforced for every underlying token used in the protocol.

The current system works with only one underlying token, WSTETH, but with the introduction of, for example, rETH as a second underlying token, there is no option to correctly configure the current Oracle system for both tokens to be denominated in the same base token.

In such a scenario, if ETH is used as the base token, the price feed for stETH/ETH needs to be used, or ConstantAggregatorV3 with a constant 10**18, which will lead to price corruption and indicate that 1 stETH = 1 ETH.

Impact

There is no possibility to use the current oracle system and its priceX96() function correctly if there is more than one underlying token and one of those tokens is WSTETH.

Code Snippet

https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/oracles/ChainlinkOracle.sol#L89-L98
https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/oracles/WStethRatiosAggregatorV3.sol#L16
https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/oracles/ConstantAggregatorV3.sol#L20

Tool used

Manual Review

Recommendation

There should be two different options to acquire the price for a token: direct and cross-oracle. The system should be flexible to use them in parallel. A good example is Euler with its Base and Cross adapters.

[0xklapouchy]

@z3s

As Vault.sol is designed to support more than one underlying token, and given that [rETH, USDC, USDT] are in scope, there is a possibility of adding new tokens as underlying tokens alongside wstETH.

It is logical to assume that if support for multiple underlying tokens exists, the current oracle system should accommodate this. However, it does not.

As pointed out in the issue, there is no way to configure the current oracle system to support multiple underlying tokens, where one of them is wstETH.

Therefore, the only way for the system to function correctly is to have only wstETH as an underlying token.

I see this issue as a medium risk because any action taken by the admin to introduce a new token into a Vault with wstETH will lead to incorrect behavior. In the live contracts, the addToken() function should not be touched.

More or less, what this issue indicates is that, even though Vault.sol looks like it supports multiple underlying tokens, in fact, this part of the system is broken in many places, one of which is the oracle system.

[Senya123]

Escalate.

Escalating this issue on behalf of the submitter. Please review the above comments.

[sherlock-admin3]

Escalate.

Escalating this issue on behalf of the submitter. Please review the above comments.

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.

[InfectedIsm]

If I understand correctly, this means that for this issue to be valid, #197 (comment) must be validated too ?

As #266 rely on the fact that 1 stETH != 1 ETH

[WangSecurity]

As Vault.sol is designed to support more than one underlying token, and given that [rETH, USDC, USDT] are in scope, there is a possibility of adding new tokens as underlying tokens alongside wstETH

Do I understand correctly that you have this assumption based on the code, is it somewhere in the docs?

[0xklapouchy]

@WangSecurity

Let me rephrase it:
As Vault.sol can support more than one underlying token, and given that [rETH, USDC, USDT] are in scope, there is a possibility of adding new tokens (by admin) as underlying tokens alongside wstETH

    /// @notice Adds a new token to the list of underlying tokens in the vault.
    /// @dev Only accessible by an admin.
    /// @param token The address of the token to add.
    function addToken(address token) external;

That was the initial meaning, I can see now it could be misinterpreted.

[IlIlHunterlIlI]

this is the comment by the sponsor in discord about oracles problems

[0xklapouchy]

@IlIlHunterlIlI

I assume you are referring to this, as you linked to the incorrect message:
we assume 1 steth == 1 weth, at least for current deployments

To showcase why this is Medium, I will refer to the following:

(External) Admin trust assumptions: When a function is access restricted, only values for specific function variables mentioned in the README can be taken into account when identifying an attack path.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.

Note: if the attack path is possible with any possible value, it will be a valid issue.

And to the currently live code that assume 1 steth == 1 weth:

Mellow currently has multiple live Vaults with a combined TVL of around $550M+. They are using wstETH as the underlying asset. Any action on addToken() at this point leads to the corruption of the system. ANY addition will lead to protocol misconfiguration and users losing money.

[IlIlHunterlIlI]

quoting this from the submission,

but with the introduction of, for example, rETH as a second underlying token, there is no option to correctly configure the current Oracle system for both tokens to be denominated in the same base token.

where is the corruption coming from when adding another underlying?

sorry but i think there is something missing

[0xklapouchy]

The current ChainlinkOracle.sol always forces you to determine the base token, and the priceX96() will be based on its price. This requirement forces you to always use cross price querying Z/Y -> Y/X.

Preconditions:

• The base token is X (it can be WETH or USDC; querying ETH/USD directly is not possible).
• The vault wants to use two underlying tokens, Y and Z.

Let's consider we want X to be WETH and query the price in USD. We set X to use the ETH/USD price feed. At this point, if any of the Y or Z tokens don't have Y/X or Z/X price feeds, we can't use them in combination. This appears to be a misconfiguration by the admin, since the admin is trusted, it should not be an issue.

Now, let's look at a real scenario with rETH and wstETH, where we want to price them in ETH:

1. If we want to set up rETH first, we need to use the rETH/ETH oracle and set the base token as WETH. Since we don't have a WETH/ETH Chainlink price feed, we will use ConstantAggregatorV3 for a 1-to-1 conversion. If we then try to set up wstETH, we can't do it correctly. There is no wstETH/ETH Chainlink oracle, and if we use WStethRatiosAggregatorV3, our return price will be in stETH, which will then be converted by ConstantAggregatorV3 for a 1-to-1 conversion to ETH.

2. If we want to set up wstETH first, we need to use WStethRatiosAggregatorV3 and set the base token as stETH. There is a stETH/ETH Chainlink price feed, which we will use to query the price for the base token. If we then try to set up rETH, we need to use the rETH/ETH oracle. Since there is no rETH/stETH oracle, our price will be returned in ETH, and querying it against the stETH/ETH price will give an incorrect price.

3. Now, we have a corner case from live contracts:

• WETH is set as the base token, with ConstantAggregatorV3 as the address to query its price.
• wstETH is set in ChainlinkOracle with WStethRatiosAggregatorV3 as the address to query its price.

Such a setup can be correct, but it needs to block any future interaction with addToken(). Since it does not block interaction with addToken(), any interaction will break the system. As this corner case is possible and any interaction by the admin with the trusted function leads to breaking the system, I see this as a Medium issue.

[IlIlHunterlIlI]

okey, thx so much for clarification, i think this should be my last comment
if the attack path is possible with any possible value, it will be a valid issue.
i think i understand now where the problem coming from, the problem here is the fact that any act from admin will cause the issue. (always corrupted design)

but isn't this statement assume 1 steth == 1 weth from sponsor totally accepts the risk associated with using steth -> weth interchangeably during the interactions with oracles, cause from what i see from the above comment show the overhead of dealing with those discrepancies through 2 oracles price retrievals.

[0xklapouchy]

As I mentioned in another comment on a different issue, we are now trying to find all possible edge cases where the system works, and I can confirm it will work with a single underlying token.

However, the issue is not about a single underlying token but the possibility of adding a new token and how the system reacts to it.

[WangSecurity]

I agree with the escalation, indeed the protocol wants to integrate 2 underlying tokens in their system, while this report shows how it would break the system. Planning to accept the escalation and leave the issue as it is.

[WangSecurity]

Result:
Medium
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Senya123: accepted

[WangSecurity]

@z3s @0xklapouchy are there any duplicates?

[IlIlHunterlIlI]

is this message have been considered?

[Slavchew]

@IlIlHunterlIlI

This is not appropriate for this discussion and you have already commented this on it. Please refrain from such comments because they do not help in any case.

[0xklapouchy]

Wrong, adding more tokens will work fine because all the tokens announced by the sponsor (wstETH, rETH, USDC, USDT) have a token/ETH price feed.

You are wrong here, in current live code and in code in audit, adding any of (rETH, USDC, USDT) to the system that have wstETH as underlying will brick the system. (Not to mention USDC, USDT decimal problem).

Wrong, like I said, the base token IS WETH, not stETH.

You missed the real part, the real base token is stETH. @IlIlHunterlIlI This is exactly this assumption that protocol have 1 steth == 1 weth.

In current live code the addToken() function will break the system, you can add any token that have token/ETH price feed and you still will not be able to configure current oracle system correctly to work with wstETH.

[Slavchew]

Wrong, adding more tokens will work fine because all the tokens announced by the sponsor (wstETH, rETH, USDC, USDT) have a token/ETH price feed.

You are wrong here, in current live code and in code in audit, adding any of (rETH, USDC, USDT) to the system that have wstETH as underlying will brick the system. (Not to mention USDC, USDT decimal problem).

Wrong, like I said, the base token IS WETH, not stETH.

You missed the real part, the real base token is stETH. @IlIlHunterlIlI This is exactly this assumption that protocol have 1 steth == 1 weth.

In current live code the addToken() function will break the system, you can add any token that have token/ETH price feed and you still will not be able to configure current oracle system correctly to work with wstETH.

No examples and no facts, you just say you can't add new tokens and that the base token is stETH, which is wrong and you can't prove it. As I said I am open to discussion, but if you give facts where and how it is written that stETH is a base token, until then everything written in the issue is wrong.

[0xklapouchy]

3. Now, we have a corner case from live contracts:

• WETH is set as the base token, with ConstantAggregatorV3 as the address to query its price.
• wstETH is set in ChainlinkOracle with WStethRatiosAggregatorV3 as the address to query its price.

@Slavchew I think you did not read my explanations.

Here are the tx from live code for the Steakhouse vault:

1. https://etherscan.io/tx/0x5dbd039e9774bbe28cdf52b55c9657381bc042763db99ffd3f0aacfd1bc1d0fa

WETH - set as base token.

2. https://etherscan.io/tx/0x21f4cc7f85a9896800a545b8ba3d5e74b71d7c51525e8942c9707693df477822

Oracle configured as described. No use of real Chainlink price feed.

aggregatorV3 for WETH used: 0x6A8d8033de46c68956CCeBA28Ba1766437FF840F (ConstantAggregatorV3)
aggregatorV3 for wstETH used: 0x94336dF517036f2Bf5c620a1BC75a73A37b7bb16 (WStethRatiosAggregatorV3)

Please reread the #266 (comment)

The case 1 and case 2 are the possible approaches how admin can start configuring the oracle system, and why he is unable to do it.

Case 3 is real configuration of the Mellow Steakhouse vault, and it is showing why addToken() will brake the system.

[Slavchew]

3. Now, we have a corner case from live contracts:

• WETH is set as the base token, with ConstantAggregatorV3 as the address to query its price.
• wstETH is set in ChainlinkOracle with WStethRatiosAggregatorV3 as the address to query its price.

@Slavchew I think you did not read my explanations.

Here are the tx from live code for the Steakhouse vault:

1. https://etherscan.io/tx/0x5dbd039e9774bbe28cdf52b55c9657381bc042763db99ffd3f0aacfd1bc1d0fa

WETH - set as base token.

2. https://etherscan.io/tx/0x21f4cc7f85a9896800a545b8ba3d5e74b71d7c51525e8942c9707693df477822

Oracle configured as described. No use of real Chainlink price feed.

aggregatorV3 for WETH used: 0x6A8d8033de46c68956CCeBA28Ba1766437FF840F (ConstantAggregatorV3) aggregatorV3 for wstETH used: 0x94336dF517036f2Bf5c620a1BC75a73A37b7bb16 (WStethRatiosAggregatorV3)

Please reread the #266 (comment)

The case 1 and case 2 are the possible approaches how admin can start configuring the oracle system, and why he is unable to do it.

Case 3 is real configuration of the Mellow Steakhouse vault, and it is showing why addToken() will brake the system.

Indeed you also confirm that the baseToken is WETH, wstETH is the underlying and of course WStethRatiosAggregatorV3 is used.

By this comment, you agree that the baseToken is WETH and only showing the aggregators linked to different tokens which is known to me.The base token is WETH, wstETH is underlying using WStethRatiosAggregatorV3()

No use of real Chainlink price feed.
Yes, there is no WETH/ETH priceFeed.

That being said, I'm stopping responding to your comments as none of them provide facts and an example of where and when baseToken will be stETH as you stated - "Most of the live contracts are set up like this, as in such cases the real base token is stETH". You just keep throwing out comments without actual facts and examples, only increasing the complexity and time of escalations, as they are a place to give you facts, not assumptions without understandings.

@WangSecurity I hope this #266 (comment) of mine is the last to provide facts, that accepting a different baseToken is not specified anywhere and not true, I will only answer to you if have questions or if I can help you understand the situation. Thank you.

[WangSecurity]

I agree with @Slavchew arguments. Indeed, WETH is used as a base token, we don't have any information in the README and code comments that there will be other base tokens, only the script linked in the README which shows WETH is used as the base token every time. Based on @Slavchew comments I don't think there will be any problems with adding more underlying tokens and the admin can set custom oracles in ChainlinkOracle::setChainlinkOracles, since as I understand ConstantAggregator and WStethRatiosAggregatorV3 are specific to WETH and wstETH, respectively. I won't repeat the arguments exactly since there is no point in repeating the same information.

Hence, I'm planning to revert the decision and invalidate this issue. @0xklapouchy if still disagree and believe there will be an issue, I'll need you to show a coded POC or an extremely detailed written POC with exact functions and values that would cause an issue. I'll revert the decision on Sunday morning (CEST timezone).

[0xklapouchy]

@WangSecurity

As mentioned in the issue description, a potential scenario in the Mellow protocol involves a Vault with multiple underlying tokens. Let's consider a Vault with [wstETH, rETH].

The issue arises in an edge case where wstETH is the first underlying token added. Any subsequent use of the addToken() function leads to incorrect operation of the Oracle system.

There is no mechanism to prevent the Admin from using the addToken() function, and its use results in a loss of funds.

If no values are provided, the (external) admin is trusted to use values that will not cause any issues.
Note: if the attack path is possible with any value, it is a valid issue.

Example Scenario

Now, let's consider a live Vault, Steakhouse: 0xBEEF69Ac7870777598A04B2bd4771c71212E6aBc. We'll add rETH as a second underlying token and configure it to have a 50/50 ratio with wstETH for deposits and withdrawals. After the setup, a user will deposit tokens worth 100 ETH into the Vault.

• Vault: 0xBEEF69Ac7870777598A04B2bd4771c71212E6aBc
• Configurator: 0xe6180599432767081beA7deB76057Ce5883e73Be
• Ratios Oracle: 0x955Ff4Cc738cDC009d2903196d1c94C8Cfb4D55d
• Price Oracle: 0x1Dc89c28e59d142688D65Bd7b22C4Fd40C2cC06d
• rETH: 0xae78736Cd615f374D3085123A210448E74Fc6393

Following the deployment script steps:

1. Add rETH as a new underlying token.

   vault.addToken(rETH);

2. Update the ratios for deposits and withdrawals.

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

3. Update the price oracle.

   address[] memory tokens = new address[](2);
   tokens[0] = weth; // 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2
   tokens[1] = rETH; // 0xae78736Cd615f374D3085123A210448E74Fc6393
   
   IChainlinkOracle.AggregatorData[] memory data = new IChainlinkOracle.AggregatorData[](2);
   data[0].aggregatorV3 = 0x6A8d8033de46c68956CCeBA28Ba1766437FF840F; // ConstantAggregatorV3
   data[0].maxAge = 0;
   data[1].aggregatorV3 = 0x536218f9E9Eb48863970252233c8F271f554C2d0; // rETH/ETH Chainlink price feed
   data[1].maxAge = 86400; // 24h
   
   priceOracle.setChainlinkOracles(vault, tokens, data);

4. User deposits 50 wstETH and 50 rETH:

Based on the Mellow.finance page, the Vault currently has 51,528 wstETH, used to calculate the current TVL. As of 26.07.2024, 51,528 wstETH ≈ 60,487.1086820949 stETH ≈ 60,440.6545826271 ETH.

    vault.deposit(user, [50000000000000000000, 50000000000000000000], minLpAmount, deadline); // user address, minLpAmount, and deadline are not important

Calculation in the Deposit Function

Current Oracle System:

• priceX96 (wstETH): ((1173868744800786254 * 10**18) * 2**96) / (10**18 * 10**18) = 93003463683492183526026379957
  • Overvalued as it should be in WETH but it is in stETH since WStethRatiosAggregatorV3 is used, base token is WETH.
• priceX96 (rETH): ((1119033350396089900 * 10**18) * 2**96) / (10**18 * 10**18) = 88658956144063119450293135167
  • Correctly returned as WETH.

Total Value:

totalValue = (51528000000000000000000 * 93003463683492183526026379957) / 2**96 + 0 
           = 60487108682094914096111 
           ≈ 60487.1086820949 WETH (but overvalued)

Deposit Value:

depositValue = (50000000000000000000 * 93003463683492183526026379957 / 2**96) + 
               (50000000000000000000 * 88658956144063119450293135167 / 2**96) 
             = 114645104759843807698 (rETH deposit undervalued)

Current Total Supply:

currentTotalSupply = 13137111513365174591322

LP Amount:

lpAmount = 114645104759843807698 * 13137111513365174591322 / 60487108682094914096111 
         = 24899611809967870953 
         ≈ 24.899611809967870953 LP

Correct Oracle System:

• stETH / ETH price from roundId 18446744073709552703: 999232162227420800

• priceX96 (wstETH): (((1173868744800786254 * 999232162227420800) / 10**18) * 10**18 * 2**96) / (10**18 * 10**18) = 92932052111095300295443068851

  • Correctly returned as WETH.

• priceX96 (rETH): ((1119033350396089900 * 10**18 * 2**96) / (10**18 * 10**18) = 88658956144063119450293135167)

  • Correctly returned as WETH.

Total Value:

totalValue = (51528000000000000000000 * 92932052111095300295443068851) / 2**96 + 0 
           = 60440664395294698300127 
           = 60440.664395294698300127

Deposit Value:

depositValue = (50000000000000000000 * 92932052111095300295443068851 / 2**96) + 
               (50000000000000000000 * 88658956144063119450293135167 / 2**96) 
             = 114600037721728398798

LP Amount:

lpAmount = 114600037721728398798 * 13137111513365174591322 / 60440664395294698300127 
         = 24908949794790898821 
         ≈ 24.908949794790898821 LP

Difference in LP Amounts

The difference in LP amounts is:

9337984823027868 = 0.009337984823027868 LP 
                 = 9337984823027868 * 60487108682094914096111 / 13137111513365174591322 
                 ≈ 0.042994816804866599 ETH loss for the user

[WangSecurity]

@0xklapouchy I see what the problem is now, since the protocol assumes 1 stETH == 1 ETH, then it just gets the value of wstETH in stETH and counts it as a value in WETH. Hence, the deposit amounts are calculated incorrectly, since the current stETH price is 1.000580 ETH. Is it correct (I know it may sound silly to confirm it but to make sure I understand your argument)?

[0xklapouchy]

@WangSecurity
yes, this is correct.

I just want to add why this is not the same issue as other with '1 stETH == 1 WETH', it explain the root problem.

The problem here is that current oracle system always force you to use to use cross-price fetch.

It would be perfectly fine if it will allow to use Chainlink price feed directly, then you will be able to configure wstETH -> stETH -> WETH as cross, plus rETH / ETH directly.

But as it is not allowing that, the base token needs to be the same. And as such it lead to a problem that 1 stETH == WETH if first underlying is wstETH.

[WangSecurity]

About the other 1 stETH == 1 WETH issue, is it about #299? @0xklapouchy

[0xklapouchy]

#29

[WangSecurity]

Thank you, but in that sense, I believe these are duplicates. The root cause of assuming 1 stETH == 1 WETH gives two vulnerability paths. The first is in case of an increased difference between the prices of 2 assets, leading to an incorrect share amount minted and a loss for a user. The second path is in this report where this assumption would break the protocol when the second token is added, cause the exchange rate would be incorrect, thus, the loss for a user.

Both can be fixed with the same fix, of using the chainlink stETH/ETH price feed, for example. In that sense, I believe these issues can be duplicated together because the root cause is the same, the fix is the same, and the impact and the vulnerability path are different, which is sufficient to consider them duplicates.

[0xklapouchy]

I agree it is similar to #29 I can even agree it is duplicate of it, but the issue is only valid when you add additional underlying to wstETH. Most of the duplicates of #29 don't even mention a valid path to this edge case issue.

Having 1 stETH == 1 WETH is perfectly fine if you only use 1 underlying token, it is even desired to not fetch stETH / ETH price then, and use constant 1e18 instead.

@WangSecurity if you plan to confirm that this issue is valid, and #29 is a duplicate or this issue a duplicate of #29, please recheck on all of #29 duplicates to determine if the explanations were sufficient.

[WangSecurity]

Thank you for pointing out, I'll check out the duplicates and comment on the ones I don't find sufficient.

[0x3b33]

Besides the above 5 issues I think 29 can also be considered a duplicate.

[WangSecurity]

@0x3b33 I think I’ve already set #29 as duplicate, or I’m missing something (#29 has Duplicate of #266 in the bottom of the report)?

[0x3b33]

It still had excluded so I wanted to mention it just in case. Sorry if I opened more work.

[WangSecurity]

Oh, I see, the excluded label doesn't mean it's not duplicated or not rewarded. This label is set if the issue was invalidated initially during the judging contest (or initial phase of judging, when there's no judging contest as for Mellow).