Staking ETH incorrectly assumes revert bubbling

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-zap-router/src/ZapRouterBase.sol#L38-L38

Vulnerability details

Impact

When EbtcLeverageZapRouter::openCdpWithEth and EbtcLeverageZapRouter::adjustCdpWithEth stake ETH with Lido, the underlying function performing the call assumes that any exceptions (from Lido) will be bubbled up, but with call being a low level rather than a Solidity function, it is not the case.

Lido stETH has two cases where a revert on depositing ETH for staking may be encountered, when deposits are paused or when staking limits are enabled and it is exceeded.

If Lido returns a revert it is ignored and the entire CDP flow continues (permits, flash loans, account synching, CDP initialization, eBTC minting), with a revert only being cause only by one of the last step of the flow, moving the collateral to the active pool.

Proof of Concept

The two parts to the issue:

1. Any possibility of reverts during staking ETH with Lido
2. Incorrect assumption that revert would bubble

1. Lido deposit could revert

Lido has pausable staking on [Ethereum stETH]](https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code#F1#L324)

    /**
     * @notice Stops accepting new Ether to the protocol
     *
     * @dev While accepting new Ether is stopped, calls to the `submit` function,
     * as well as to the default payable function, will revert.
     *
     * Emits `StakingPaused` event.
     */
    function pauseStaking() external {

Lido can impose a per block stake limit on Ethereum stETH

     * @dev Reverts if:
     * - `_maxStakeLimit` == 0
     * - `_maxStakeLimit` >= 2^96
     * - `_maxStakeLimit` < `_stakeLimitIncreasePerBlock`
     * - `_maxStakeLimit` / `_stakeLimitIncreasePerBlock` >= 2^32 (only if `_stakeLimitIncreasePerBlock` != 0)
     *
     * Emits `StakingLimitSet` event
     *
     * @param _maxStakeLimit max stake limit value
     * @param _stakeLimitIncreasePerBlock stake limit increase per single block
     */
    function setStakingLimit(uint256 _maxStakeLimit, uint256 _stakeLimitIncreasePerBlock) external {

Both these cause the same effect; a possible revert during the submit of ETH for staking with Ethereum stETH

    function _submit(address _referral) internal returns (uint256) {
        require(msg.value != 0, "ZERO_DEPOSIT");

        StakeLimitState.Data memory stakeLimitData = STAKING_STATE_POSITION.getStorageStakeLimitStruct();
        // There is an invariant that protocol pause also implies staking pause.
        // Thus, no need to check protocol pause explicitly.
@>      require(!stakeLimitData.isStakingPaused(), "STAKING_PAUSED");

        if (stakeLimitData.isStakingLimitSet()) {
            uint256 currentStakeLimit = stakeLimitData.calculateCurrentStakeLimit();

@>          require(msg.value <= currentStakeLimit, "STAKE_LIMIT");

2. Incorrect assumption that revert would bubble

EbtcLeverageZapRouter and EbtcZapRouter both use the same ancestor function to stake ETH with Lido, that performs the call operation on a address in ZapRouterBase::_depositRawEthIntoLido

    function _depositRawEthIntoLido(uint256 _initialETH) internal returns (uint256) {
        // check before-after balances for 1-wei corner case
        uint256 _balBefore = stEth.balanceOf(address(this));
        // TODO call submit() with a referral?
@>      payable(address(stEth)).call{value: _initialETH}("");
        uint256 _deposit = stEth.balanceOf(address(this)) - _balBefore;
        return _deposit;
    }

As outlined in Solidity by Example - call

call is a low level function to interact with other contracts.

This is the recommended method to use when you're just sending Ether via calling the fallback function.

However it is not the recommend way to call existing functions.
Few reasons why low-level call is not recommended

    Reverts are not bubbled up
    Type checks are bypassed
    Function existence checks are omitted

EbtcZapRouter does check the collateral balance in the function that follows the staking of ETH with Lido in EbtcZapRouter::_openCdpWithPermit()

        require(
            stEth.balanceOf(address(this)) >= _stEthBalance,
            "EbtcZapRouter: not enough collateral for open!"
        );

EbtcLeverageZapRouter performs no similar checks on the collateral balance after the staking, rather failing on a ERC20 transfer after another ~320K gas of other operations.

Test Case

Adds a pause to the CollateralTokenTester and demonstrates the non-propagation of the inital revert, instead relying on the later ERC20 revert.

Code

The `CollateralTokenTester` mocks `Lido stETH`, add the following to the version used by the `ebtc-zap-router` tests (should be at `/lib/ebtc/packages/contracts/TestContracts/CollateralTokenTester.sol`) ```diff - function deposit() public payable { + bool private depositsArePaused; + + function pauseDeposits() external { + depositsArePaused = true; + } + + function deposit() public payable { + require(!depositsArePaused, "STAKING_PAUSED"); ``` Add to [LeverageZaps](https://github.com/code-423n4/2024-06-badger/blob/9173558ee1ac8a78a7ae0a39b97b50ff0dd9e0f8/ebtc-zap-router/test/LeverageZaps.t.sol#L534) ```solidity function test_ZapOpenCdp_WithEth_LidoRReverts() external { seedActivePool(); // Pausing deposits to mimic Lido's pauseStaking() or stakeLimit being set & exceeded CollateralTokenTester(collateral).pauseDeposits(); // Logic equivalent to that from createLeveragedPosition(MarginType.ETH) // Extracted to test correctly using expectRevert() address user = vm.addr(userPrivateKey); uint256 _debt = 1e18; uint256 flAmount = _debtToCollateral(_debt); uint256 marginAmount = 5 ether; vm.deal(user, type(uint96).max); IEbtcZapRouter.PositionManagerPermit memory pmPermit = createPermit(user); vm.prank(user); // Fails on the last step of BorrowerOperations::_openCdp(); transfer of collateral to the active pool vm.expectRevert("ERC20: transfer amount exceeds balance"); _openTestCdp(MarginType.ETH, _debt, flAmount, marginAmount, pmPermit); } ``` Run with `forge test --match-contract LeverageZaps --match-test test_ZapOpenCdp_WithEth_LidoRReverts`

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Use submit instead, as that will propogate any revert.

In ZapRouterBase::_depositRawEthIntoLido()

    function _depositRawEthIntoLido(uint256 _initialETH) internal returns (uint256) {
        // check before-after balances for 1-wei corner case
        uint256 _balBefore = stEth.balanceOf(address(this));
        // TODO call submit() with a referral?
-       payable(address(stEth)).call{value: _initialETH}("");
+       stETH.submit{value: _initialETH}(address(0));
        uint256 _deposit = stEth.balanceOf(address(this)) - _balBefore;
        return _deposit;
    }

The msg.sender could also be included for the referral, as that will be included by Lido their emitted event.

    function _depositRawEthIntoLido(uint256 _initialETH, address _referral) internal returns (uint256) {
        // check before-after balances for 1-wei corner case
        uint256 _balBefore = stEth.balanceOf(address(this));
        // TODO call submit() with a referral?
-       payable(address(stEth)).call{value: _initialETH}("");
+       stETH.submit{value: _initialETH}(_referral);
        uint256 _deposit = stEth.balanceOf(address(this)) - _balBefore;
        return _deposit;
    }

Assessed type

Other

[GalloDaSballo]

We agree that the finding is valid, but believe it should be downgraded to QA

The check is missing and will cause the delta returned to be 0

The stETH amount is checked a few lines after, and a 0 amount, or a 0 change will cause every operation to revert due to insufficient change amount:

Open Cdp would revert here:

https://github.com/ebtc-protocol/ebtc-zap-router/blob/4b37cce03c16b5e5caf5a9aab0bab5d344a587d2/src/EbtcLeverageZapRouter.sol#L217-L218

        _requireAtLeastMinNetStEthBalance(_stEthDepositAmount - LIQUIDATOR_REWARD);

On adjustCDP

https://github.com/ebtc-protocol/ebtc-zap-router/blob/4b37cce03c16b5e5caf5a9aab0bab5d344a587d2/src/EbtcLeverageZapRouter.sol#L411-L414

        _requireZeroOrMinAdjustment(_params.debtChange);
        _requireZeroOrMinAdjustment(_params.stEthBalanceChange);
        _requireZeroOrMinAdjustment(_params.stEthMarginBalance);

The operation will also revert later once it tries to send an insufficient amount of tokens to the protocol

For these reasons, we will fix the bug, but believe it's QA

[alex-ppg]

The Warden has demonstrated how a low-level interaction with the stETH system has not adequately handled the bool flag yielded which would indicate whether the call was successful or not, causing revert errors to be ignored.

I am not sure I fully agree with the Sponsor, as I believe certain code paths and CDP operations would result in the vulnerability regardless of the additional security checks imposed in the routers.

Specifically, a healthy CDP that has its debt increased and collateral added to it simultaneously via the ETH or WETH to stETH deposit flows would satisfy all conditions and be executed. Here's a quick PoC that can be added to the LeverageZaps.t.sol file:

function test_marginErrorSilent() public {
    seedActivePool();

    (address user, bytes32 cdpId) = createLeveragedPosition(MarginType.stETH);

    IEbtcZapRouter.PositionManagerPermit memory pmPermit = createPermit(user);

    uint256 debtChange = 1e18;
    uint256 marginIncrease = 0.5e18;
    uint256 collValue = _debtToCollateral(debtChange) * COLLATERAL_BUFFER / 10000;
    uint256 flAmount = _debtToCollateral(debtChange);

    _before();
    vm.startPrank(user);
    leverageZapRouter.adjustCdpWithEth{value: marginIncrease}(
        cdpId, 
        _getAdjustCdpParams(flAmount, int256(debtChange), int256(collValue), int256(marginIncrease), false),
        abi.encode(pmPermit), 
        _getExactInDebtToCollateralTradeData(debtChange)
    );
    vm.stopPrank();
    _after();

    // Test zap fee
    assertEq(eBTCToken.balanceOf(testFeeReceiver), (1e18 + debtChange) * defaultZapFee / 10000); 

    // Demonstrate a non-zero balance remains
    assertEq(address(leverageZapRouter).balance, 0);

    _checkZapStatusAfterOperation(user);
}

I believe that the above PoC sufficiently justifies a high severity risk rating as the funds transmitted alongside the call are lost and a CDP adjustment can leave the position in a worse-off state than the caller expected due to the unprocessed margin. To note, more PoCs can be observed in #37 and #22 that demonstrate the flaw and the possibility that funds will indeed be locked in the router.

A subset of submissions has been marked as no-reward due to lacking sufficient justification as to why the low-level interaction must be validated and how it can be exploited. Simply pointing out that the low-level call remains unchecked is insufficient for this particular vulnerability as it is identical to re-hashing static analysis output without properly understanding its ramifications.

To note, the primary submission of this duplicate set might change after PJQA as this submission details the vulnerability nicely but fails to demonstrate a valid exploitation path in its PoC.

[c4-judge]

alex-ppg changed the severity to 3 (High Risk)

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked the issue as selected for report

[Slavchew]

@alex-ppg

I believe that the above PoC sufficiently justifies a high severity risk rating as the funds transmitted alongside the call are lost and a CDP adjustment can leave the position in a worse-off state than the caller expected due to the unprocessed margin.

It's not valid, as like @GalloDaSballo stated and it's just visible, if you adjust the position by 0 nothing worse can happen. In addition, there is a slippage ensuring that the position is as the user wants it, and a worse condition as you stated will be impossible.

What should the provided test show? This maybe: demonstrate the flaw and the possibility that funds will indeed be locked in the router.

This is also an invalid assumption because the test will fail if the balance is not 0, since the assertion is for equality. assertEq(address(leverageZapRouter).balance, 0);

Also, the probability of a low-level stETH call revert due to a pause or deposit limit is very unlikely. The current staking limit is set at 150,000 ETH, which makes it even less likely to happen. - https://docs.lido.fi/guides/lido-tokens-integration-guide#staking-rate-limits

[alex-ppg]

Hey @Slavchew, thank you for your feedback! The screenshot shared does not properly apply the PoC described as you did not perform the necessary adjustments in the CollateralTokenTester (i.e. stETH) contract for it to revert. As it is misleading, I strongly advise revising your latest feedback.

The PoC shared alongside the CollateralTokenTester adjustment demonstrates that the funds transmitted alongside the call were lost. As such, the vulnerability can lead to direct fund loss.

The likelihood of a stETH call reverting was deemed as medium-risk given that it has historically been breached (example) and is set in place for a reason.

While I am more than happy to hear opinions on and debate whether this vulnerability should be deemed medium-risk or high-risk, it is clear that the vulnerability exists.

[Slavchew]

Hey @alex-ppg, I rechecked the whole flow and yes, the problem is there indeed.

The likelihood of a stETH call reverting was deemed as medium-risk given that it has historically been breached (example) and is set in place for a reason.

This article only announces that the stETH depositLimit is enabled at 150,000 ETH per day, which I already said. The article also clarifies that this 150 000 ETH has been set, because in the past one address deposited over 150,000 in a day and it is highly unlikely to happen again. So it only remains to happen if stETH is paused. With the user also providing a slippage for what they want to deposit, the severity of this should be Medium at maximum imo.

[alex-ppg]

Hey @Slavchew, thank you for your follow-up contribution. The slippage the user provides when it comes to leverage is not sufficient in protecting against this attack as evidenced by the PoCs.

I do understand the concerns around the likelihood of the vulnerability manifesting, however, the result of its manifestation is a direct and complete loss of native funds. Additionally, stETH is upgradeable and we cannot be sure of what the future holds with deposit limitations as it is an external project and not related to Badger.

Nevertheless, after consulting with a fellow judge I have decided to indeed reduce the severity of this submission to medium-risk. The low likelihood of the vulnerability manifesting would necessitate a critical impact (i.e. total loss of funds) which is not demonstrated here as we have a high impact (i.e. loss of funds for the caller at that particular time).

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)

[GalloDaSballo]

We agree with the decision above, going through various scenarios, not all scenarios would cause a loss of funds, however some would and all are conditional on stETH being paused