Handling of stETH Transfers Can Lead to Rounding Errors and Incorrect Balances due to "1-2 wei corner case"

[c4-bot-5]

Lines of code

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

Vulnerability details

Title

Handling of stETH Transfers Can Lead to Rounding Errors and Incorrect Balances due to "1-2 wei corner case"

Impact

The protocol wrongly assumes the amount specified in a stETH transfer is what gets sent. This can lead to overvaluing the amount of assets deposited and incorrect handling of withdrawals.

Proof of Concept

The 1-2 wei corner case issue primarily affects transfers involving stETH due to its share-based system. This issue can manifest both during deposits into Lido and when transferring stETH directly.

The _depositRawEthIntoLido function already handles the balance check correctly as seen here:

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

The _transferStEthToCaller function however does not handle the balance check correctly,
In the ZapRouterBase, there are several functions where the protocol assumes the amount specified in a stETH transfer is the exact amount that gets transferred. However, due to the share-based system used by stETH, the actual amount transferred can be slightly less than the specified amount, leading to various issues.

Let's take a look at _transferInitialStETHFromCaller Function:
This function also correctly handles the balance check before and after the transfer to determine the actual amount of stETH received:

function _transferInitialStETHFromCaller(uint256 _initialStETH) internal returns (uint256) {
    // check before-after balances for 1-wei corner case
    uint256 _balBefore = stEth.balanceOf(address(this));
    stEth.transferFrom(msg.sender, address(this), _initialStETH);
    uint256 _deposit = stEth.balanceOf(address(this)) - _balBefore;
    return _deposit;
}

Now let's navigate to _transferStEthToCaller Function:
This function does not handle the balance check correctly when transferring stETH directly to the caller

function _transferStEthToCaller(
    bytes32 _cdpId,
    EthVariantZapOperationType _operationType,
    bool _useWstETH,
    uint256 _stEthVal
) internal {
    if (_useWstETH) {
        // return wrapped version(WstETH)
        uint256 _wstETHVal = IWstETH(address(wstEth)).wrap(_stEthVal);
        emit ZapOperationEthVariant(
            _cdpId,
            _operationType,
            false,
            address(wstEth),
            _wstETHVal,
            _stEthVal,
            msg.sender
        );

        wstEth.transfer(msg.sender, _wstETHVal);
    } else {
        // return original collateral(stETH)
        emit ZapOperationEthVariant(
            _cdpId,
            _operationType,
            false,
            address(stEth),
            _stEthVal,
            _stEthVal,
            msg.sender
        );

        stEth.transfer(msg.sender, _stEthVal);
    }
}

In this case, the contract uses the _stEthVal amount directly without checking the actual transferred amount. Protocol assumes that _stEthVal is the exact amount transferred, which can lead to incorrect protocol state due to rounding errors.
Worthy to note that stETH is a special token when it comes to it's transfer logic, navigating to lido's official docs, where during transfers the amount that actually gets sent is actually a bit less than what has been specified in the transaction. More can be read on the "1-2 wei corner case" issue from lidofinance/core#442.

stETH is using shares for tracking balances and it is a known issue that due to rounding error, transferred shares may be 1-2 wei(as stated above) less than amount passed.

Recommended Mitigation Steps

Consider using the balance check before and after the transfer to determine the actual amount of stETH received. This ensures that the protocol works with the correct amount of assets and avoids the issues caused by rounding errors. Something in this light:

function _transferStEthToCaller(
    bytes32 _cdpId,
    EthVariantZapOperationType _operationType,
    bool _useWstETH,
    uint256 _stEthVal
) internal {
    if (_useWstETH) {
        // return wrapped version(WstETH)
        uint256 _wstETHVal = IWstETH(address(wstEth)).wrap(_stEthVal);
        emit ZapOperationEthVariant(
            _cdpId,
            _operationType,
            false,
            address(wstEth),
            _wstETHVal,
            _stEthVal,
            msg.sender
        );

        wstEth.transfer(msg.sender, _wstETHVal);
    } else {
        // return original collateral(stETH)
+        uint256 _stETHBalBefore = stEth.balanceOf(address(this));
        stEth.transfer(msg.sender, _stEthVal);
+        uint256 _stETHTransferred = stEth.balanceOf(address(this)) - _stETHBalBefore;

        emit ZapOperationEthVariant(
            _cdpId,
            _operationType,
            false,
+            address(stEth),
+            _stETHTransferred,
+            _stETHTransferred,
            msg.sender
        );

-        stEth.transfer(msg.sender, _stEthVal);
+        stEth.transfer(msg.sender, _stETHTransferred);
    }
}

Assessed type

ETH-Transfer

[c4-bot-8]

Withdrawn by Rhaydden