howlbot-integration[bot] commented 4 months ago

Lines of code

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

Vulnerability details

Impact

// File: src/ZapRouterBase.sol
34:    function _depositRawEthIntoLido(uint256 _initialETH) internal returns (uint256) {
        ...
36:        uint256 _balBefore = stEth.balanceOf(address(this));
37:        // TODO call submit() with a referral?
38:        payable(address(stEth)).call{value: _initialETH}(""); // <= FOUND: return value of the low-level call to Lido not checked
39:        uint256 _deposit = stEth.balanceOf(address(this)) - _balBefore;
40:        return _deposit;
41:    }

The Lido _submit's implementation contains various validations that can potentially revert transactions, such as STAKING_PAUSED and STAKE_LIMIT exceeded. However, the ZapRouterBase::_depositRawEthIntoLido function above does not verify if the success return value is actually true to proceed further. If a reversion did occur in the Lido's stEth contract, the Zap router will not revert and keep executing as normal. As a result, the _deposit will return 0 at Line 40.

// File: src/EbtcZapRouter.sol
267:    function adjustCdpWithWrappedEth(
        ...
278:        uint256 _collBalanceIncrease = _wethBalanceIncrease;
279:        if (_wethBalanceIncrease > 0) {
280:            _collBalanceIncrease = _convertWrappedEthToStETH(_wethBalanceIncrease); // <= FOUND: _collBalanceIncrease might be overriden to 0 if deposit failed
                ...
303:    }

The return zero _deposit would propagate through _convertWrappedEthToStETH and then override _collBalanceIncrease at line 280 to 0. Since the adjust operation allows 0 changes for either collateral or debt positions, an overridden deposit in the case of an adjust containing both collateral and debt changes might go silent. Therefore, the equivalent _initialETH amount of ETH will be kept inside the contract without refunding the caller, causing a loss of funds.

This vulnerability is reported as HIGH severity as it directly results in the loss of user funds, Zap router cannot sweep native ETH to refund the user and the likelihood of reversion in Lido deposit operation could be MED (STAKING_PAUSED and STAKE_LIMIT conditions could be configured occasionally).

Proof of Concept

Apply this patch:

For test patch:

cd 2024-06-badger/ebtc-zap-router/ && git apply test_ZapAdjustCDPWithWrappedEthDepositAndRepay_EthGotStuck_NoCollateralIncrease.patch

For lib patch:

cd 2024-06-badger/ebtc-zap-router/lib/ebtc && git apply ebtcLib.patch

Result

forge test -vv --mt test_ZapAdjustCDPWithWrappedEthDepositAndRepay_EthGotStuck_NoCollateralIncrease

[⠊] Compiling... No files changed, compilation skipped Ran 1 test for test/NoLeverageZaps.t.sol:NoLeverageZaps [FAIL. Reason: Zap should have no raw ETH: 2000000000000000000 != 0] test_ZapAdjustCDPWithWrappedEthDepositAndRepay_EthGotStuck_NoCollateralIncrease() (gas: 1352712) Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 9.84ms (2.58ms CPU time)

The test results show that a zero deposit was recorded while the user lost 2 ETH, which is now stuck inside the Zap router.

Tools Used

Foundry Test

Recommended Mitigation Steps

Ensure that the call to stEth contract is successful before continuing the operation.

Assessed type

call/delegatecall

c4-judge commented 4 months ago

alex-ppg marked the issue as satisfactory