ZeroTrust - EtherFiLib::_initiateWithdrawImpl will revert because rebase tokens transfer 1-2 less wei

[sherlock-admin3]

ZeroTrust

High

EtherFiLib::_initiateWithdrawImpl will revert because rebase tokens transfer 1-2 less wei

Summary

EtherFiLib::_initiateWithdrawImpl will revert because rebase tokens transfer 1-2 less wei

Vulnerability Detail

The protocol always assumes that the amount of tokens received is equal to the amount of tokens transferred.
This is not the case for rebasing tokens, such as stETH and eETH, because internally they transfer shares which generally results in the received amount of tokens being lower than the requested one by a couple of wei because of roundings: transferring 1e18 eETH tokens from A to B, will may result in B receiving 0.99999e18 eETH tokens.

function _initiateWithdrawImpl(uint256 weETHToUnwrap) internal returns (uint256 requestId) {
@>        uint256 eETHReceived = weETH.unwrap(weETHToUnwrap);
        eETH.approve(address(LiquidityPool), eETHReceived);
@>        return LiquidityPool.requestWithdraw(address(this), eETHReceived);
    }

• 1 Unwraps weETHToUnwrap weETH, meaning Transfers eETHReceived of eETH from the weETH contract to this contract itself
• 2 RequestWithdraw eETHReceived, which will attempt to transfer eETHReceived from the contract to the Etherfi protocol
  But the actual amount of eETH received may be eETHReceived - 1~2 wei.

Step 2 will fail, because the contract doesn't have enough eETH. The issue lies in attempting to transfer eETHReceived of eETH in step 2 instead of wrapping the actual amount of tokens received.

Impact

Contract functionality DoS

Code Snippet

https://github.com/sherlock-audit/2024-06-leveraged-vaults/blob/main/leveraged-vaults-private/contracts/vaults/staking/protocols/EtherFi.sol#L24

Tool used

Manual Review

Recommendation

function _initiateWithdrawImpl(uint256 weETHToUnwrap) internal returns (uint256 requestId) {
+       uint256 balanceBefore = eETH.balanceOf(address(this));
        uint256 eETHReceived = weETH.unwrap(weETHToUnwrap);
+       uint256 balanceAfter = eETH.balanceOf(address(this));
-        eETH.approve(address(LiquidityPool), eETHReceived);
-        return LiquidityPool.requestWithdraw(address(this), eETHReceived);
+         eETH.approve(address(LiquidityPool), balanceAfter - balanceBefore);
+        return LiquidityPool.requestWithdraw(address(this), balanceAfter - balanceBefore);
    }

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

0xmystery commented:

It concerns deposit(), NOT wrap/unwrap

[ZeroTrust01]

Escalate
This should be considered a valid issue.

function unwrap(uint256 _weETHAmount) external returns (uint256) {
        require(_weETHAmount > 0, "Cannot unwrap a zero amount");
        uint256 eETHAmount = liquidityPool.amountForShare(_weETHAmount);
        _burn(msg.sender, _weETHAmount);
@>>        eETH.transfer(msg.sender, eETHAmount);
        return eETHAmount;
    }

The weETH::unwrap() function includes a transfer operation, so this involves the transfer of rebase tokens in this contract. In the LiquidityPool.requestWithdraw function, rebase tokens are transferred out of this contract because internally they transfer shares, which generally results in the received amount of tokens being lower than the requested one by a couple of wei due to rounding, So it will revert.

This is exactly the same issue as the one confirmed in the contest a few weeks ago.
sherlock-audit/2024-05-sophon-judging#63
sherlock-audit/2024-05-sophon-judging#119

[sherlock-admin3]

Escalate
This should be considered a valid issue.

function unwrap(uint256 _weETHAmount) external returns (uint256) {
        require(_weETHAmount > 0, "Cannot unwrap a zero amount");
        uint256 eETHAmount = liquidityPool.amountForShare(_weETHAmount);
        _burn(msg.sender, _weETHAmount);
@>>        eETH.transfer(msg.sender, eETHAmount);
        return eETHAmount;
    }

The weETH::unwrap() function includes a transfer operation, so this involves the transfer of rebase tokens in this contract. In the LiquidityPool.requestWithdraw function, rebase tokens are transferred out of this contract because internally they transfer shares, which generally results in the received amount of tokens being lower than the requested one by a couple of wei due to rounding, So it will revert.

This is exactly the same issue as the one confirmed in the contest a few weeks ago.
sherlock-audit/2024-05-sophon-judging#63
sherlock-audit/2024-05-sophon-judging#119

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.

[mystery0x]

Escalate This should be considered a valid issue.

function unwrap(uint256 _weETHAmount) external returns (uint256) {
        require(_weETHAmount > 0, "Cannot unwrap a zero amount");
        uint256 eETHAmount = liquidityPool.amountForShare(_weETHAmount);
        _burn(msg.sender, _weETHAmount);
@>>        eETH.transfer(msg.sender, eETHAmount);
        return eETHAmount;
    }

The weETH::unwrap() function includes a transfer operation, so this involves the transfer of rebase tokens in this contract. In the LiquidityPool.requestWithdraw function, rebase tokens are transferred out of this contract because internally they transfer shares, which generally results in the received amount of tokens being lower than the requested one by a couple of wei due to rounding, So it will revert.

This is exactly the same issue as the one confirmed in the contest a few weeks ago. sherlock-audit/2024-05-sophon-judging#63 sherlock-audit/2024-05-sophon-judging#119

Here's the selected report for Sophon contest that's related to your finding on eETH:

sherlock-audit/2024-05-sophon-judging#4

Read through the report carefully, and you will notice wrap/unwrap is always 1:1. It's deposit() that's causing the 1-2 wei issue which does not apply to your report context.

[ZeroTrust01]

Escalate This should be considered a valid issue.

function unwrap(uint256 _weETHAmount) external returns (uint256) {
        require(_weETHAmount > 0, "Cannot unwrap a zero amount");
        uint256 eETHAmount = liquidityPool.amountForShare(_weETHAmount);
        _burn(msg.sender, _weETHAmount);
@>>        eETH.transfer(msg.sender, eETHAmount);
        return eETHAmount;
    }

The weETH::unwrap() function includes a transfer operation, so this involves the transfer of rebase tokens in this contract. In the LiquidityPool.requestWithdraw function, rebase tokens are transferred out of this contract because internally they transfer shares, which generally results in the received amount of tokens being lower than the requested one by a couple of wei due to rounding, So it will revert.
This is exactly the same issue as the one confirmed in the contest a few weeks ago. sherlock-audit/2024-05-sophon-judging#63 sherlock-audit/2024-05-sophon-judging#119

Here's the selected report for Sophon contest that's related to your finding on eETH:

sherlock-audit/2024-05-sophon-judging#4

Read through the report carefully, and you will notice wrap/unwrap is always 1:1. It's deposit() that's causing the 1-2 wei issue which does not apply to your report context.

You mixed up the link I provided. I pointed out that this issue is exactly the same as issue group 1 (in the picture), but your link is to issue group 2 (in the picture). I never said it was the same as issue group 2.

[WangSecurity]

@ZeroTrust01 could you provide the link to the eETH contract please, so I can see how they've implemented their transfer function with fees.

[ZeroTrust01]

@ZeroTrust01 could you provide the link to the eETH contract please, so I can see how they've implemented their transfer function with fees.

This is the address of the eETH contract.
https://etherscan.io/address/0x35fA164735182de50811E8e2E824cFb9B6118ac2

This issue is not about the transfer function with fees; it is about rebase token transfer using shares. If the transfer amount is 1e18, the received amount might be (1e18 - 1 wei).

I learned this from a report where the issue was exactly the same as this one.
sherlock-audit/2024-05-sophon-judging#119

[WangSecurity]

Thank you. For future reference, historical decisions are not sources of truth and each issue is different, so I would like to ask you instead of saying "this issue is the exact same as in the previous contest", prove that your issue is valid, regardless of other contests and decisions. Often it's the case that the same issue is valid in one contest, but invalid in an another.

About the issue. Even though, Notional's code fetches the eEthReceived from weEth.unwrap the eEthReceived represents the amount before eEth.transfer. eETH.transfer makes a conversion from input amount to token shares and sends the amount of shares received, which can be lower due to precision loss. Later, this will revert since the contract tries to send eEthreceived.

Planning to accept the escalation and validate the report with high severity since it can happen on every withdrawal from EtherFi. @mystery0x @ZeroTrust01 are there any duplicates?

[wtp0x]

I believe it is unique.

[WangSecurity]

Result:
High
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• ZeroTrust01: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/notional-finance/leveraged-vaults/pull/94/files

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.