Bauchibred - Querying `depositBufferedEther()` via the deposit security module is not done correctly

[sherlock-admin3]

Bauchibred

High

Querying depositBufferedEther() via the deposit security module is not done correctly

Summary

Querying depositBufferedEther() via the deposit security module is not done correctly.

Vulnerability Detail

To deposit there is a need to convert the weth amount into wsteth, see https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/modules/obol/StakingModule.sol#L64-L74

    function convertAndDeposit(
..
        _wethToWSteth(amount);
        depositSecurityModule.depositBufferedEther(
            blockNumber,
            blockHash,
            depositRoot,
            stakingModuleId,
            nonce,
            depositCalldata,
            sortedGuardianSignatures
        );
..
}

Now take a look at https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/modules/obol/StakingModule.sol#L77-L83

    function _wethToWSteth(uint256 amount) private {
        IWeth(weth).withdraw(amount);
        ISteth(steth).submit{value: amount}(address(0));//@audit protocol assumes steth <-> eth is 1:1,
        IERC20(steth).safeIncreaseAllowance(address(wsteth), amount);
        IWSteth(wsteth).wrap(amount);
    }
}

This function is used for conversion from weth to steth, issue however is that protocol assumes the rate for steth <-> eth is always 1:1, however this is inaccurate, going to the docs for lido's submit(): https://docs.lido.fi/contracts/lido#submit-1

We can see that the function sends funds to the pool and then mints StETH tokens, would be key to note that the docs also clearly state that this function returns the number of StETH shares generated, which would mean that the conversion rate is not 1:1, and after the deposits to the pool, submit() would correctly return the amount of stETH shares/tokens generated.

Problem however is that protocol does not take this factor into account and instead directly attempts to wrap the amount attached from the weth value to wsteth, in this line IWSteth(wsteth).wrap(amount);, as previously explained however, this value would either be deflated or inflated, depending on the eth <-> steth going rate.

Impact

As hinted under Proof of Concept, when converting weth to wsteth, protocol assumes a 1:1:1 rate for the conversion from weth <-> eth <-> steth, which would be wrong and would lead to different impact:

• In the case the exchange rate is > 1, this would mean a loss of funds/ unaccounted funds, cause the amount of tokens that end up being attached to depositBufferedEther() would be deflated, cause the amount used while wrapping steth to wsteth via IWSteth(wsteth).wrap(amount); has also been deflated.
• Alternatively, in the case there is no previous balance of steth in the contract as is expected and the rate of eth <-> steth is < 1, then the attempt at depositing would encounter a revert cause the amount of tokens steth tokens sent from Lido after the submitquery is less than the amount of tokens the execution attempts to wrap via IWSteth(wsteth).wrap(amount);, which would prompt a revert in Lido's code here: https://github.com/lidofinance/lido-dao/blob/5fcedc6e9a9f3ec154e69cff47c2b9e25503a78a/contracts/0.6.12/WstETH.sol#L57 since the amount attempted to be transferred is more than is present in the contract.

Code Snippet

https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/modules/obol/StakingModule.sol#L77-L83

https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/modules/obol/StakingModule.sol#L64-L74

Tool used

Manual Review

Recommendation

A common logic used by protocol when submitting eth to Lido is to then query the difference in balance of the contract's steth tokens before and after the deposit, a similar code example can be seen from the just concluded Sophon contest on Sherlock here: https://github.com/sherlock-audit/2024-05-sophon/blob/05059e53755f24ae9e3a3bb2996de15df0289a6c/farming-contracts/contracts/farm/SophonFarming.sol#L808-L813.

So apply these changes:

    function _wethToWSteth(uint256 amount) private {
        IWeth(weth).withdraw(amount);
+        uint256 balanceBefore = IERC20(stETH).balanceOf(address(this));
        ISteth(steth).submit{value: amount}(address(0));
+        uint256 amountReceived = IERC20(stETH).balanceOf(address(this)) - balanceBefore);
-        IERC20(steth).safeIncreaseAllowance(address(wsteth), amount);
+        IERC20(steth).safeIncreaseAllowance(address(wsteth), amountReceived);
-        IWSteth(wsteth).wrap(amount);
+        IWSteth(wsteth).wrap(amountReceived);
    }

Duplicate of #299

[Bauchibred]

This is not a dup of 299.

This report is not about the 1-2 wei corner case, but rather the fact that the protocol currently assumes a 1:1:1 rate for the conversion from weth <-> eth <-> steth, which is wrong.

Report hinting something similar is: #285.

[Al-Qa-qa]

Escalate.

On @Bauchibred Behalf.

[sherlock-admin3]

Escalate.

On @Bauchibred Behalf.

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.

[IWildSniperI]

197 is not the same issue as ours sir

he is talking clearly about the oracle and we are talking about another edge case

[z3s]

I duplicated this and #299 because they have same root cause that protocol assumed received stETH amount == submitted/transferd amount, they even have same fix, treat stETH as a FoT.

Duplication Rules

[Bauchibred]

197 is not the same issue as ours sir

he is talking clearly about the oracle

and we are talking about another edge case

Edited my original comment to reflect this, thanks for bringing it up.

[IWildSniperI]

@z3s received amount through transfer is case different from loss of funds and dos as long as the returned value from submit is lower

sponsor was clearly talking about oracle prices (that fetches CEX and DEX data) ratio

our case here talks about Coverting from ETH to stETH which shouldn't be a transfer (not a case of rebasing token or parallel FOT case) but about the shared Pool ETH in lido protocol that leads the discrepancy in the returned tokens

to elaborate more

• even if stETH is not rebasing the returned value from submit would be a problem (so not same root cause at all)

for the solution:

• will checking balance in depositWrapper prevent this problem? you dupped the issue like its a slippage issue right?

[IlIlHunterlIlI]

hello sirs, i have same issue reported about same edge case in #280 #283 about stackingModule and depositWrapper

and i think both issues should be treated separately, cause they are in different places

[Audinarey]

I duplicated this and #299 because they have same root cause that protocol assumed received stETH amount == submitted/transferd amount, they even have same fix, treat stETH as a FoT.

Duplication Rules

@z3s See also #202

[IWildSniperI]

@Audinarey your issue is correctly grouped with #299 our case is different

299 on of their dups issues got escalated and if it gets accepted, then the whole grouped issues will get validated

[WangSecurity]

I believe this issue and #299 have the same root cause of not correctly working with stETH. In both cases the problem is that the actual amount of stETH minted would result in less than amount variable causing a revert on the subsequent calls with amount variable. The reason for this (in both cases) is because stETH conversion from ETH to shares (stETH) has precision loss of 1-2 wei.

Yes, this issue and #299 happen in different parts of the code, but the root cause is the same as well as the fix.

Planning to reject the escalation and leave the issue as it is.

[Bauchibred]

I believe this issue and #299 have the same root cause of not correctly working with stETH. In both cases the problem is that the actual amount of stETH minted would result in less than amount variable causing a revert on the subsequent calls with amount variable. The reason for this (in both cases) is because stETH conversion from ETH to shares (stETH) has precision loss of 1-2 wei.

I tend to disagree with this reasoning, the root cause here is not on the grounds of "not correctly working with stETH". This is because in the case of #299 no "mints" of stETH is happening, but rather conversion from the steth "transferred" in, which would be 1-2 less wei, and would potentially cause the revert, and this is the novel logic with steth, so the issue there does have the root cause of "not correctly working with stETH". This is also the reason why #299 only hints the _stethToWsteth() function in the report cause that's where the bug occurs, i.e when converting the same amount of transferred steth to wsteth.

Here the issue is that the rate is assumed to be 1:1:1 for fresh "mints" from weth <-> eth <-> steth, this is not novel to steth since this logic is just like any other classic "exchangeRate", so we can't conclude the root cause to be "not correctly working with stETH".

And this issue would not be fixed if only #299 was reported, cause the fix protocol would apply would be to the _stethToWsteth() and any other instance where transfer/transferFrom() is queried on the steth token.

Also CC @z3s's comment, I believe there is an exception in the rules for duplications when the code implementations are different which I assume is the case here, protocol has two implementations that are wrong:

• When transferring steth via ( transfer/transferFrom() ), they assume the amount specified is what's received.
• When staking ether, they assume a 1:1 rate.

Fixing one and not the other, leaves the unfixed one problematic for the protocol.

[WangSecurity]

Excuse me for a vague root cause, let me re-iterate. Both submit and transfer convert the input amount into stETH shares (basically stETH), both have the precision loss which results in 1-2 less wei transferred/minted. Hence, I believe they have the same root cause. Yes, it happens in different functions, but the underlying issue is the same. Yes, it requires to fix the issues separately, but the fix can be the same. In this case, as you've said, they assume the amount specified is what's received applies to both transfer and submit.

Hence, the decision remains the same, reject the escalation and keep this issues as duplicates.

[Bauchibred]

Excuse me for a vague root cause, let me re-iterate. Both submit and transfer convert the input amount into stETH shares (basically stETH), both have the precision loss which results in 1-2 less wei transferred/minted. Hence, I believe they have the same root cause. Yes, it happens in different functions, but the underlying issue is the same. Yes, it requires to fix the issues separately, but the fix can be the same. In this case, as you've said, they assume the amount specified is what's received applies to both transfer and submit.

Fairs, your argument's got weight, I concur with the decision to keep the issues as duplicates.

[WangSecurity]

Result: Invalid Duplicate of #299

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• Al-Qa-qa: rejected

[WangSecurity]

It's invalid for now, cause I'm still sorting out the #299 and duplicates.