Ironsidesec - wstETH.wrap will revert most of the times

[sherlock-admin3]

Ironsidesec

High

wstETH.wrap will revert most of the times

Summary

This issue is about fewer shares minted when steth.submit which reverts on wsteth.wrap.
Also, there is another issue on steth.transferfrom which is different from this. This issue's root cause and fix are on steth.submit and wsteth.wrap.

Issue occurs on both stakingmodule._wethToWSteth and DepositModule._ethToWsteth

Vulnerability Detail

Issue flow :

1. DepositWrapper.deposit converts eth to wstEth or stETH to wstETH or WETH to wstEth. But the flow will revert when wrapping from stETH to wstETH due to fee on transfer kind of bug.
2. when stETH.submit is called with 1 eth as input, it will mint 1steth - 1 wei. This is due to internal shares rounding on Lido's stETH contract. Check this submit transaction https://etherscan.io/tx/0xcef25ddf24a038764525d090e839cc9619a7fae2fb4a24ce6485379828e1f173 getting minted 1 wei less than the submitted value. It submits 3.05 ETH but gets 3.049999999999999999 stETH
3. The issue is in line 676 below onwsteth.wrap which is where it reverts. Look at the flow, 1e18 ETH gets submitted to stETH but minted 1e18 -1 wei stETH. Now we are trying to wrap 1e18 stETH instead of 1e18 - 1.
4. The fix is to see how many shares were minted and pass it as wrap amount instead of submitted amount from input param.

https://github.com/lidofinance/lido-dao/blob/5fcedc6e9a9f3ec154e69cff47c2b9e25503a78a/contracts/0.6.12/WstETH.sol#L57

File: wstETH.sol

672:     function wrap(uint256 _stETHAmount) external returns (uint256) {
673:         require(_stETHAmount > 0, "wstETH: can't wrap zero stETH");
674:         uint256 wstETHAmount = stETH.getSharesByPooledEth(_stETHAmount);
675:         _mint(msg.sender, wstETHAmount);
676:  >>>    stETH.transferFrom(msg.sender, address(this), _stETHAmount);
677:         return wstETHAmount;
678:     }

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

File: 2024-06-mellow/mellow-lrt/src/modules/obol/StakingModule.sol

80:     function _wethToWSteth(uint256 amount) private {
81:         IWeth(weth).withdraw(amount);
82:   >>>>  ISteth(steth).submit{value: amount}(address(0));
83:         IERC20(steth).safeIncreaseAllowance(address(wsteth), amount);
84:   >>>>  IWSteth(wsteth).wrap(amount);
85:     }

https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/utils/DepositWrapper.sol#L25-L42

File: 2024-06-mellow/mellow-lrt/src/utils/DepositWrapper.sol

    function _ethToWsteth(uint256 amount) private returns (uint256) {
 >>>>  ISteth(steth).submit{value: amount}(address(0));
        return _stethToWsteth(amount);
 }

    function _stethToWsteth(uint256 amount) private returns (uint256) {
        IERC20(steth).safeIncreaseAllowance(wsteth, amount);
 >>>>  IWSteth(wsteth).wrap(amount);
        return IERC20(wsteth).balanceOf(address(this));
 }

Impact

Reverts the deposit action with a high likelihood of causing DOS. So, high

Code Snippet

https://github.com/sherlock-audit/2024-06-mellow/blob/26aa0445ec405a4ad637bddeeedec4efe1eba8d2/mellow-lrt/src/utils/DepositWrapper.sol#L31-L37

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

https://github.com/lidofinance/lido-dao/blob/5fcedc6e9a9f3ec154e69cff47c2b9e25503a78a/contracts/0.6.12/WstETH.sol#L57

Tool used

Manual Review

Recommendation

Use stakeswap recommendation : https://github.com/stakeswap/stakeswap-contracts/blob/d727040cd4a259e0bf914ba8e914372a15834488/src/adaptor/LidoAdaptor.sol#L68-L72

Duplicate of #299

[IronsideSec]

Escalate
This is not dup of #299.

#299 talks about DOS reverts on DepositWrapper.deposit due to 1wei rounding on safeTransferfrom.
But, this issue will mint less steth on (steth).submit and will revert on (wsteth).wrap(amount) due to less than amount of steth being minted and reverts on wrap call.

So fix and impacts are different, #299's fix cannot fix this and this one's fix cannot fix #299. And impact is different due to reverts happen at different place, #299 is at steth.safeTransferfrom and this one's at (steth).submit

[sherlock-admin3]

Escalate
This is not dup of #299.

#299 talks about DOS reverts on DepositWrapper.deposit due to 1wei rounding on safeTransferfrom.
But, this issue will mint less steth on (steth).submit and will revert on (wsteth).wrap(amount) due to less than amount of steth being minted and reverts on wrap call.

So fix and impacts are different, #299's fix cannot fix this and this one's fix cannot fix #299. And impact is different due to reverts happen at different place, #299 is at steth.safeTransferfrom and this one's at (steth).submit

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.

[IlIlHunterlIlI]

a dup #280

[IronsideSec]

#280 combined the issues of both .submit(this #80) and .safetransferfrom (#299) bugs

if #280 gets duped into #299, then dont dup this with #280. Read my escalated comment again.

[IlIlHunterlIlI]

@IronsideSec sorry if i didn't say it in a good way

i meant to say that my issue is dupped wrong too and i think my issue should be with your family

in fact i didn't talk at all about safeTransferFrom, at all. only i made a wrong recommendation about deposit cause i forgot to delete that recommendation from the 1-2wei corner case

but issue body and impact only talks about submit

[WangSecurity]

I disagree it's a different issue. The root cause is basically the same, but happens in a different part of the code than #299. Moreover, the fix is the same (get the amount of actually minted shares), while they still have to be fixed separately. Hence, I believe it should remain duplicates.

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

[IronsideSec]

@WangSecurity

The root cause is basically the same, but happens in a different part of the code than #299.

If the root cause were similar on mellow itself, then agree with you to dup. Fixes here are similar like you said (shares minted use)
But root cause on external protocol (stETH) are not similar. One is on .submit call and another on transferFrom call

Are these DUPable ?

[IlIlHunterlIlI]

to add Up, since the concept behind is different, then protocol wouldn't have noticed or solved it from the primary issue of this family

[IlIlHunterlIlI]

get the amount of actually minted shares

this is false sir,
we don't use shares as shares != stETH balance

instead we use balance change of stETH in the contract

there is big problem that may not be considered into judging

1- the ratio can decrease or increase
2- the probability is low for those edge cases other than the 1 - wei case
if the ration increases this will be even worth as the extra stETH will be permanently freezed in that wrapper contract

mentioned by lido Docs here caused by

1. pooled ETH decrease with supply and shares stays the same (slashing) since stETH has centralised validators, then slashing will be reflected into stETH users
2. vice versa

[WangSecurity]

Excuse me for vagueness in the previous comment. 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 differently, but the fix can be the same (getting the difference between contract balance after and before the call to stETH will solve both issues). That is why I believe they have to remain duplicates:

If the following issues appear in multiple places, even in different contracts. In that case, they may be considered to have the same root cause.
Issues with the same logic mistake.

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

[WangSecurity]

Result:
Invalid
Duplicate of #299

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ironsidesec: rejected

[IlIlHunterlIlI]

@WangSecurity Hello sir, my comment above is talking about different edge case other than the precision loss with lido docs as a reference,

i don't see my arguments mentioned in your last comment while rejecting the escalation. Yes same solution but different root cause

Just wanted to make sure that they are considered.

[WangSecurity]

Excuse me if it seemed I ignored you. The problem is that this edge case is not mentioned in the report. Moreover, the bug described in the report is invalid as proven here and here, both submit and wrap work as intended, without reverts and lost tokens.

Hence, there's no need to use the balance change of stETH in the contract and this code functions perfectly. Moreover, about the Lido docs, with slashing the pooled ETH is reduced, while as you said the share amount remains the same, so again, there shouldn't be problems with that, but if I'm mistaken and it doesn't seem like I addressed all of your points, please correct me.

[IlIlHunterlIlI]

I referred to what my report wrote so that we don't do double escalations on the same problem which is wrong dups of #299

The issue here as said from lido docs low probability is that in some cases every time you submit 1ETH you get 1.1 stETH or in some cases 0.9 stETH

Those cases are not due to shares rounding down but don't want to repeat my above argument.

The case is constrained areound the depeg of eth to stETH, which a case actually acknowledged by Lido and not impossible

[IlIlHunterlIlI]

Now when 1 ETH = 1.1 stETH

loss of the extra 0.1 stETH and freeze in the contract.

If its the other way (if its equal 0.9 stETH) then always revert at wrapping

[WangSecurity]

Firstly, you mean about report #280, correct?

Secondly, stETH is not pegged to ETH, hence, receiving 0.9 stETH or 1.1 ETH for 1 ETH is fine.

But, this 0.1 stETH won't be lost in the contract, as proven in the POC linked in the previous comment, or check this out:

POC and output

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";

interface IStETH {
    function transfer(address to,uint amount) external returns(bool) ;
    function submit(address referral) external payable returns(uint) ;
}

interface IWeth {
    function withdraw(uint amount) external ;
    function transfer(address to,uint amount) external returns(bool) ;

}

interface IWSteth {
    function wrap(uint amount) external returns(uint) ;
}

interface IERC20 {
    function approve(address to,uint amount) external returns(bool) ;
    function transfer(address to, uint256 amount) external returns (bool);
    function balanceOf(address account) external view returns (uint256);
}


contract stETHTransferTest is Test {
    address stETH = 0xae7ab96520DE3A18E5e111B5EaAb095312D7fE84;
    address wsteth = 0x7f39C581F595B53c5cb19bD0b3f8dA6c935E2Ca0;
    address weth = 0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2;


    // function testStETHTransfer()  public {
    //     vm.createSelectFork(vm.envString("eth_rpc"));

    //     address stETHWhale = 0x43594da5d6A03b2137a04DF5685805C676dEf7cB;

    //     IStETH(stETH).transfer(address(this),0);
    //     IStETH(stETH).transfer(stETH,0);
    // }

    function testWethToWstETH() public {
        vm.createSelectFork(vm.envString("eth_rpc"));

        address wethWhale = 0x1681195C176239ac5E72d9aeBaCf5b2492E0C4ee;

        uint amount = 1 ether;
        address(0).call{value: address(this).balance}(""); // without this step address(this) has ETH, so for a better test, let's send it away

        vm.prank(wethWhale);
        IWeth(weth).transfer(address(this),amount);
        

        IWeth(weth).withdraw(amount);
        console.log(address(this).balance);
        IStETH(stETH).submit{value: amount}(address(0));
        console.log("Contract Balance stETH 1: ",IERC20(stETH).balanceOf(address(this)));
        IERC20(stETH).approve(address(wsteth), amount);
        uint256 amountWSTETH = IWSteth(wsteth).wrap(amount);
        console.log("Received wstETH: ",amountWSTETH);
        IERC20(wsteth).transfer(wethWhale, amountWSTETH);
        console.log("User Balance: ",IERC20(wsteth).balanceOf(wethWhale));
        console.log("Contract Balance wstETH: ",IERC20(wsteth).balanceOf(address(this)));
        console.log("Contract Balance stETH: ",IERC20(stETH).balanceOf(address(this)));
        console.log("Contract Balance ETH: ",address(this).balance);
        console.log("Contract Balance WETH: ",IERC20(weth).balanceOf(address(this)));
    }


    receive() external payable{

    }
}

Output:

Ran 1 test for test/TestStWETH.t.sol:stETHTransferTest
[PASS] testWethToWstETH() (gas: 227597)
Logs:
  1000000000000000000
  Contract Balance stETH 1:  999999999999999999
  851818410885481494
  User Balance:  851818410885481494
  Contract Balance wstETH:  0
  Contract Balance stETH:  0
  Contract Balance ETH:  0
  Contract Balance WETH:  0

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

As you can see, we use the same amount variable for both WETH to ETH, then ETH to stETH and then stETH to wstETH, all with the same amount variable.

In the very end, when we wrap stETH to wstETH, we get the balance of the contract, just like the DepositWrapper at L38 and send it back to the user (yes in Mellow we sent it to a different contract, but it doesn't matter here). If you look at the console logs you can see that in the end, the contract has zero funds and all of them are transferred to the user.

To run the POC, you need to create .env file, eth_rpc = and enter eth mainnet RPC (e.g. from alchemy).

Hence, the current implementation won't have funds stuck in the contract or reverts. If you still think differently, please send a POC proving your point.

[IlIlHunterlIlI]

No sir,

1 ETH -> 1.1 stETH
Then we wrap 1stETH to the corresponding wstETH (0.1 stETH is not wrapped)

Then we check any extra balance of wstETH but not the extra 0.1 stETH that will never be returnable.

[WangSecurity]

The output in the POC shows there is no 0.1 stETH trapped and the user received all the funds. The POC checks for balances of WETH, stETH, ETH and wstETH and there are no tokens left inside the contract.

As I’ve said, to prove your point please send a POC, cause in mine there are no “extra” inside the contract.

[IlIlHunterlIlI]

@WangSecurity i understand your concern, external constraints can't be applied through foundry, although the 1 to 2 wei edge case didn't even show up in the test suit, so same problem applies here.

the only thing i have is that 1 to 1 ETH to stETH can sometimes not hold

[WangSecurity]

i understand your concern, external constraints can't be applied through foundry, although the 1 to 2 wei edge case didn't even show up in the test suit, so same problem applies here.

1-2 wei edge case is shown in the POC. As you can see in the output logs, after the call to stETH.submit the contract has 1e18-1 stETH, i.e. 999999999999999999.

the only thing i have is that 1 to 1 ETH to stETH can sometimes not hold

As I've said, stETH is not pegged to ETH, hence, it shouldn't hold a 1:1 ratio. For example, as of the time of writing, chainlink returns stETH price is 0.999,746,984,366,609,000 ETH. The exchange rate shouldn't hold 1:1, since even though the price of stETH is connected to ETH, it's not a peg like USDC to USD, hence, not holding is fine.

[IlIlHunterlIlI]

1-2 wei edge case is shown in the POC. As you can see in the output logs, after the call to stETH.submit the contract has 1e18-1 stETH, i.e. 999999999999999999

i was confused that the tests are successful only to see that they are checking balance - 2 wei ( so my above statement is wrong

As I've said, stETH is not pegged to ETH, hence, it shouldn't hold a 1:1 ratio. For example, as of the time of writing, chainlink returns stETH price is 0.999,746,984,366,609,000 ETH. The exchange rate shouldn't hold 1:1, since even though the price of stETH is connected to ETH, it's not a peg like USDC to USD, hence, not holding is fine.

Using chainLink oracle is false statement to evaluate the situation, cause it fetches the prices from multiple cexes, so the price =! Exchange rate

we see here that stETH is of type Price

so i think there may be confusion with Proof of reserve Feed

Lido docs saying its always 1 to 1

current chainlink price of stETH

we see here almost 1 to 1 conversion from ETH to stETH (only with 1 wei loss edge case) in this txn Hash which is recent

• Staking 4.39274306 ETH, receiving 4.392743059999999999 stETH

the above txn and oracle only to prove that price =! conversion rate, supported by lido Docs statement and live example

[WangSecurity]

Fair point about Chainlink Oracle, and indeed the docs say the approach is to treat them 1:1 even though this ration doesn't always hold.

But, Lido docs don't say the user can receive more, at least I don't see it. Maybe you can refer me to it, or find an example where Lido indeed minted larger than expected where the loss would be a considerable value (i.e. loss is >=0.01% of the deposit). I assume there may be such a case due to division during conversion, even though I'm unsure, but I don't think such a case will be large enough as 0.1 stETH for example.

Excuse me for overlooking your points initially and I see the difference in the scenario where Lido returns more stETH than expected, but I'm not sure if it's possible and if such loss exceeds small and finite amounts for Medium severity. Would be glad if you could refer me to it in the Lido docs or find an example of such conversion would be even better.

[IlIlHunterlIlI]

@WangSecurity

The example of 1ETH -> 1.1 stETH is only to show the underlying idea, so maybe it may look inflated example

I will have the burden to provide you the information till all escalation resolved, so that i don't delay results due to some misunderstanding.

Even if its valid, if i don't provide enough info till then i totally accept the invalidation.

[WangSecurity]

thank you very much, I get your idea, but without sufficient info/example, I can't consider it as a valid medium cause I cannot see the real impact of such case.