0xBhumii - Potential Denial of Service (DoS) in DepositWrapper Contract Due to Rounding Errors in stETH Transfers

[sherlock-admin4]

0xBhumii

High

Potential Denial of Service (DoS) in DepositWrapper Contract Due to Rounding Errors in stETH Transfers

Summary

The deposit function in the DepositWrapper contract is susceptible to a known issue with the stETH token where rounding errors can cause transferred shares to be 1-2 wei less than the intended _amount. This discrepancy can lead to subsequent function calls failing due to insufficient balance, resulting in a potential denial of service (DoS) for users attempting to deposit stETH.

Vulnerability Detail

The issue arises when the deposit function processes stETH tokens. The stETH token uses shares for tracking balances, and due to rounding errors, the transferred shares may be 1-2 wei less than the _amount passed. This can cause a mismatch in the expected amount of stETH transferred and wrapped into wstETH, leading to failures in subsequent operations.

Detailed Flow:

1. The deposit function receives an amount parameter, which is passed to IERC20(steth).safeTransferFrom(sender, wrapper, amount).
2. The _stethToWsteth function is called to convert steth to wsteth.
3. The _stethToWsteth function may end up with 1-2 wei less stETH due to rounding issues.
4. This discrepancy can cause subsequent function calls, such as wrapping into wstETH, to fail because the actual balance is slightly less than expected.

Impact

The probability of issue appearing is high and you can check in the following discussion. It has also been classified as a High severity on past contests:
lidofinance/core#442
The impact can be Contract DOS and incorrect balance handling

Code Snippet

https://github.com/sherlock-audit/2024-06-mellow/blob/main/mellow-lrt/src/utils/DepositWrapper.sol#L55C1-L58C1

Tool used

Manual Review

Recommendation

Use lido recommendation to utilize transferShares function, so the _amount is realistic, or implement FoT approach, which compares the balance before and after the transfer.

Duplicate of #299

[recursiveEth]

Escalate
the loss of 1-2 WEI is not a low/invalid issue, the same issue has been considered in past audits as well thank you

[sherlock-admin3]

Escalate
the loss of 1-2 WEI is not a low/invalid issue, the same issue has been considered in past audits as well thank you

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.

[z3s]

#181

[WangSecurity]

I believe #181 is the duplicate of #299 that was escalated the earliest, so it will be the one to decide if this family is valid. Hence, this escalation is about the same and doesn't bring new information. Planning to reject it, cause regardless of it being accepted/rejected, the earlier escalation on #181 will give the same outcome.

[WangSecurity]

After looking deeper into #181, I see it's not a sufficient duplicate, hence, the escalation there will be rejected either way. Hence, I'll make this escalation as the main one for #299 issue family.

Plan to accept the escalation and validate with high seveirty.

[z3s]

Plan to accept the escalation and validate with high severity.

I think this family should be Medium. because there is no fund loss, and only stETH deposits get DoSed most of time. users can still convert stETH to wstETH themselves.

[WangSecurity]

Fair point, but I believe this issue qualifies for high severity:

Inflicts serious non-material losses (doesn't include contract simply not working).

In this case, major part of the deposits will be DOSed and since the protocol specifically allows to deposit stETH and will convert it for the user, I believe this is a serious non-material loss.

The decision remains the same, planning to accept the escalation and validate with high severity.

[z3s]

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

I think you mean accept the escalation.

[10xhash]

Fair point, but I believe this issue qualifies for high severity:

Inflicts serious non-material losses (doesn't include contract simply not working).

In this case, major part of the deposits will be DOSed and since the protocol specifically allows to deposit stETH and will convert it for the user, I believe this is a serious non-material loss.

The decision remains the same, planning to accept the escalation and validate with high severity.

This issue is invalid as I have explained in detail in the comment here #205 (comment)
Wrapping will work with shares of the amount which remains the same for an amount of steth tokens unless the shares/underlying balance in lido changes. Please attach a POC if otherwise, I am attaching one proving my point

// 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) ;
}


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

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

        address wethWhale = 0x1681195C176239ac5E72d9aeBaCf5b2492E0C4ee;

        uint amount = 3854154965781563458935;

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

        IWeth(weth).withdraw(amount);
        IStETH(stETH).submit{value: amount}(address(0));
        IERC20(stETH).approve(address(wsteth), amount);
        IWSteth(wsteth).wrap(amount);

    }

    receive() external payable{

    }
}

[WangSecurity]

And further tweaking the POC, I've got the following two scenarios:

POC 2

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

        address wethWhale = 0x1681195C176239ac5E72d9aeBaCf5b2492E0C4ee;

        uint amount = 1e18;

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

        IWeth(weth).withdraw(amount);
        uint256 stETHValue = IStETH(stETH).submit{value: amount}(address(0));
        IERC20(stETH).transfer(wethWhale, amount);

        console.log("stETHValue",stETHValue);
        console.log("amountOfWeth",amount);
    }

POC 3

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

        address wethWhale = 0x1681195C176239ac5E72d9aeBaCf5b2492E0C4ee;

        uint amount = 1e18;

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

        IWeth(weth).withdraw(amount);
        uint256 stETHValue = IStETH(stETH).submit{value: amount}(address(0));
        IERC20(stETH).transfer(wethWhale, amount);
        vm.prank(wethWhale);
        IWeth(stETH).transfer(address(this),amount);
        uint256 stETHValue2 = IERC20(stETH).balanceOf(address(this));
        //IERC20(stETH).approve(address(wsteth), amount);
        //uint amountofWstETH = IWSteth(wsteth).wrap(amount);

        console.log("stETHValue",stETHValue);
        console.log("stETHValue2",stETHValue2);
        console.log("amountOfWeth",amount);

    }

And none of the reverts at any point. In the end, regardless of which token you deposit (ETH, WETH, stETH or wstETH), each of them leads to calling _stethToWsteth which returns the amount of wstETH inside the contract and deposits this amount into Vault. Hence, this issue wouldn't cause any reverts or any accounting issues. Planning to reject the escalation and leave the issue as it is.

[xKeywordx]

Disregard my previous comment. I double-checked some things and it makes sense now why this doesn't happen.

[WangSecurity]

Result:
Invalid
Duplicate of #299

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• recursiveEth: rejected