Protocol supports stETH but doesn't consider its unique transfer logic which would lead to not only a DOS of the depositing/withdrawal channel for this collateral token but also a flaw in multiple other core protocol logic

[c4-bot-3]

Lines of code

ttps://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L491-L576
https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L664-L665
https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Deposits/DepositQueue.sol#L134-L145
https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/xERC20/contracts/XERC20Lockbox.sol#L125-L152

Vulnerability details

Proof of Concept

NB: This bug report contains two sides of the coin on how the 1-2 wei corner case problem could affect renzo, 50% builds on the OOS safeApprove() from the bug report, however the second part of the report is in scope.

First, would be key to note that stETH is a special token when it comes to it's transfer logic, navigating to lido's official docs we can see that there is a special section that talks about it's unique concept, i.e the "1-2 wei corner case", see https://docs.lido.fi/guides/lido-tokens-integration-guide/#1-2-wei-corner-case, quoting them:

stETH balance calculation includes integer division, and there is a common case when the whole stETH balance can't be transferred from the account while leaving the last 1-2 wei on the sender's account. The same thing can actually happen at any transfer or deposit transaction. In the future, when the stETH/share rate will be greater, the error can become a bit bigger. To avoid it, one can use transferShares to be precise.

That's to say at any transfer tx there is a possibility that the amount that actually gets sent is up to 2 wei different, a minute value you might think, however when we couple this with the fact that protocol heavily uses safeApprove() to pass on approvals for collateral tokens before depositing or withdrawing, this corner case could then brick the protocol.

Now see OpenZeppelin's implementation of safeApprove() and how it will revert if the current allowance is non-zero and the approval attempt is also passing a non-zero value.

    function safeApprove(
        IERC20 token,
        address spender,
        uint256 value
    ) internal {
        // safeApprove should only be called when setting an initial allowance,
        // or when resetting it to zero. To increase and decrease it, use
        // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
        require(
            //@audit
            (value == 0) || (token.allowance(address(this), spender) == 0),
            "SafeERC20: approve from non-zero to non-zero allowance"
        );
        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
    }

Consider a minimalistic generic scenario:

• Allowance is set by user A for user B to 1e18 "wei" stETH tokens.

• User B attempts to transfer these tokens, however due to the corner case, stETH balance gets converted to shares, integer division happens and rounding down applies, the amount of tokens that are actually transferred would be 1e18 - 2 "wei" tokens.

• Now user A assumes that user A has expended their allowance and attempts granting them a new allowance of a fresh 1e18 "wei" stETH tokens, doing this with the normal ERC20.approve() is going to go through, however user A attempts to do this with SafeERC20.safeApprove() which would fail cause SafeERC20.safeApprove() reverts on non-zero to non-zero approvals and user B is currently being approved of 2 wei tokens which they've not spent yet.

  More can be read on the "1-2 wei corner case" issue from here: Account's stETH balance getting lower on 1 or 2 wei due to rounding down integer math lidofinance/core#442

The scenario above is quite generic but this exact idea can be applied to current protocol's logic of passing on allowances around contracts in scope, for example see RestakeManager.deposit() at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L491-L576

    function deposit(
        IERC20 _collateralToken,
        uint256 _amount,
        uint256 _referralId
    ) public nonReentrant notPaused {
        // Verify collateral token is in the list - call will revert if not found
        uint256 tokenIndex = getCollateralTokenIndex(_collateralToken);

        // Get the TVLs for each operator delegator and the total TVL
        (
            uint256[][] memory operatorDelegatorTokenTVLs,
            uint256[] memory operatorDelegatorTVLs,
            uint256 totalTVL
        ) = calculateTVLs();

        // Get the value of the collateral token being deposited
        uint256 collateralTokenValue = renzoOracle.lookupTokenValue(_collateralToken, _amount);

        // Enforce TVL limit if set, 0 means the check is not enabled
        if (maxDepositTVL != 0 && totalTVL + collateralTokenValue > maxDepositTVL) {
            revert MaxTVLReached();
        }

        // Enforce individual token TVL limit if set, 0 means the check is not enabled
        if (collateralTokenTvlLimits[_collateralToken] != 0) {
            // Track the current token's TVL
            uint256 currentTokenTVL = 0;

            // For each OD, add up the token TVLs
            uint256 odLength = operatorDelegatorTokenTVLs.length;
            for (uint256 i = 0; i < odLength; ) {
                currentTokenTVL += operatorDelegatorTokenTVLs[i][tokenIndex];
                unchecked {
                    ++i;
                }
            }

            // Check if it is over the limit
            if (currentTokenTVL + collateralTokenValue > collateralTokenTvlLimits[_collateralToken])
                revert MaxTokenTVLReached();
        }

        // Determine which operator delegator to use
        IOperatorDelegator operatorDelegator = chooseOperatorDelegatorForDeposit(
            operatorDelegatorTVLs,
            totalTVL
        );

        // Transfer the collateral token to this address
        _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);

        // Check the withdraw buffer and fill if below buffer target
        uint256 bufferToFill = depositQueue.withdrawQueue().getBufferDeficit(
            address(_collateralToken)
        );
        if (bufferToFill > 0) {
            bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
            // update amount to send to the operator Delegator
            _amount -= bufferToFill;

            // safe Approve for depositQueue @audit
            _collateralToken.safeApprove(address(depositQueue), bufferToFill);

            // fill Withdraw Buffer via depositQueue
            depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
        }
        //@audit
        // Approve the tokens to the operator delegator
        _collateralToken.safeApprove(address(operatorDelegator), _amount);

        // Call deposit on the operator delegator
        operatorDelegator.deposit(_collateralToken, _amount);

        // Calculate how much ezETH to mint
        uint256 ezETHToMint = renzoOracle.calculateMintAmount(
            totalTVL,
            collateralTokenValue,
            ezETH.totalSupply()
        );

        // Mint the ezETH
        ezETH.mint(msg.sender, ezETHToMint);

        // Emit the deposit event
        emit Deposit(msg.sender, _collateralToken, _amount, ezETHToMint, _referralId);
    }

As hinted by the two @Audit tags, protocol uses safeApprove() to grant approval in this case to both the depositQueue and the operator delegator, note that the implementations of both operatorDelegator.deposit() and depositQueue.fillERC20withdrawBuffer() include transfers of the allowances they've already been given from the execution of RestakeManager.deposit() considering the transfers is going to get rounded down, then minute part of the allowance is going to be left untransferred and in consequent calls to safeApprove() when depositing stETH the call to safeApprove is going revert (as shown in the attached openZeppelin's safeApprove() snippet above) and throw an error since an attempt is being made to approve from a non-zero to a non-zero value, effectively bricking/DOS'ing the depositing logic for the supported stETH collateral token.

---

Now, parallel to the already explained issue with safeApprovals, this 1-2 wei corner case is going to also cause protocol to make a wrong assumption on the amount of tokens that were really transferred, would be key to note that the amount of ezETH that get minted to the msg.sender is directly proportional to the collateral value of the amount of tokens that were considered to be "transferred" in to the RestakeManager during the deposit attempt.

Evidently, since during deposits, protocol assumes the amount of tokens that was specified in the safeTransferFrom() is actually the amount of tokens that end up getting transferred in, the amount of ezETH that gets minted for users is going to be inflated as more than what was transferred in is going to be considered as the amount transferred in when calculating the collateral token being deposited https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L506-L508

        // Get the value of the collateral token being deposited
        uint256 collateralTokenValue = renzoOracle.lookupTokenValue(_collateralToken, _amount);

The logic from the last two paragraphs hint that the stETH token does somewhat behave like the popular Fee-On-Transfertokens , albeit in this case the discrepancy in the amount of tokens being recieved is due to rounding down and not fees, also this could lead to the depositing/withdrawing logic of the XERC20Lockbox to also work with flawed data.

---

NB: This report hints other instances where protocol's logic could be flawed due to not considering the transfer logic attached to stETH, however the report mainly focuses on only RestakeManager#deposit() &RestakeManager#depositTokenRewardsFromProtocol() as these instances are enough to prove the bug case (keep in mind that this function is always queried whenever there is a need to sweep any accumulated ERC20 tokens in the DepositQueue to the RestakeManager, other instances still exist in scope however, like depositQueue.fillERC20withdrawBuffer() & operatorDelegator.deposit(), but in short this bug case can be applied to all instances where protocol attempts to query safeApprove() on the collateral token as can be pinpointed using this search command, i.e the DepositQueue#fillERC20withdrawBuffer() could now encounter a failure making it impossible fill the up the stETHbuffer in the withdraw queue, extensively this subtly affects all the collateral token transfer logic too.

Impact

This bug cases leads to multiple issues and the root cause is the fact that protocol does not take the 1-2 wei corner case of stETH into mind, a few noteworthy impacts would be:

• When the allowance of the supported stETH token is non-zero in instances where protocol thinks it's already zero, all attempts to safeApprove() on this collateral token (stETH) is going to fail DOSing the depositing attempts, making protocol's core functionality unavailable to some users.

• Additionally, the accounting of the backed collateral for minted ezETH could now be flawed since it's going to assume the wrong amount of collaterals are backing already minted assets which covers the main window under the requested bug windows/attack ideas since the integrity on the TVL calculations (ezETH Pricing) is now going to be slightly flawed, i.e users are now going to mint & withdraw at slightly invalid prices considering the stETH is a core integrated token and with multiple transfers this minute differences could amount to quite a reasonable sum.

• So, this means that depositing into the strategy manager for the stETH collateral token would also be broken.

• This bug case also makes it impossible to complete queued withdrawals for stETH from OperatorDelegator.sol, since the channel is going to be DOS'd when the residual amount of approval already made to deposit queue causes this attempt at a new approval to fail, showcasing how the withdrawal channel is also going to be DOS'd.

• Another subtle one, would be the Inability to fill up the withdrawal queue buffer when needed for the stETHtoken, (albeit in this case as hinted by protocol the admins can manually unstake to ensure the buffer is at where it needs to be).

• Finally, there seems to be a subtle edge case, where, if every user is to attempt withdrawing their deposited assets back, the last set of users might not receive their assets it due to protocol not having enough assets backed for ezETH already minted to send to back to users.

And extensively many more ways where/how this bug case could impact protocol, just depends on the context in which safeApprove() is being applied or instances where an assumption is being made that the amount specified in the transfer is actually what's been received.

Recommended Mitigation Steps

First consider scraping the idea of safeApprove() for stETH, since the all assets being supported right now (ezETH, stETH, wBETH) are standard tokens then the normal ERC20.approve() can be used which wouldn't revert on non-zero to non-zero approvals.

For the parallel case in regards to the amount specified in the transferral attempt not being the amount that gets transferred in, then the differences in balance could be used to see the real amount that was transferred in and then use this value to calculate the amount of ezETH to be minted.

Alternatively, protocol can just consider integrating wstETH instead of stETH as suggested by the official Lido docs for more ease in regards to DEFI integration, see how this can be done here.

Assessed type

Token-Transfer

[C4-Staff]

CloudEllie marked the issue as duplicate of #389

[c4-judge]

alcueca marked the issue as selected for report

[alcueca]

@jatinj615, please review

[jatinj615]

@alcueca , I think this report is highlighting somewhat similar issue as #389 around "1-2 corner case". Also I think the safeApprove will fail around these "1-2 corner case" as well before which I think transferFrom will fail. As specified in the #389 -

EigenLayer itself has the same methodology to transfer into strategy. Hence, even if we fix it, it will revert on depositIntoStrategy

Also the wstETH integration does not seems to be the mitigation as EigenLayer Strategy only accepts stETH not wstETH.

Therefore, can be acknowledged for the time being. lmk if I am missing something here.

[alcueca]

That's right, I did mark #389 as a duplicate of this one previously.

I think that your assessment is right. I do not know what is the right mitigation, but I would suggest that you test this issue thoroughly and talk with the folks at EigenLayer to get their view on it.

[c4-judge]

alcueca marked the issue as satisfactory

[jatinj615]

Yeah what I am wondering about is that we have had stETH deposits enabled and have around 20k stETH deposited in the protocol and never faced this. But will cross check as well.

[alcueca]

High severity warranted on permanent disabling of core functionality. However, I'm quite suspicious on why this hasn't manifested in the live instance.

[rbserver]

Hi @alcueca, thanks for judging!

According to the StETH contract in https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code, when calling the transferFrom function shown below, the _spendAllowance function is called first. Because the _spendAllowance function executes _approve(_owner, _spender, currentAllowance - _amount), the allowance set by the safeApprove function call is all spent, which causes the allowance to become 0.

    function transferFrom(address _sender, address _recipient, uint256 _amount) external returns (bool) {
        _spendAllowance(_sender, msg.sender, _amount);
        _transfer(_sender, _recipient, _amount);
        return true;
    }

    function _spendAllowance(address _owner, address _spender, uint256 _amount) internal {
        uint256 currentAllowance = allowances[_owner][_spender];
        if (currentAllowance != INFINITE_ALLOWANCE) {
            require(currentAllowance >= _amount, "ALLOWANCE_EXCEEDED");
            _approve(_owner, _spender, currentAllowance - _amount);
        }
    }

Then, the _transfer function below is called, which further calls the getSharesByPooledEth and _transferShares functions, where the 1-2 wei corner case might occur.

    function _transfer(address _sender, address _recipient, uint256 _amount) internal {
        uint256 _sharesToTransfer = getSharesByPooledEth(_amount);
        _transferShares(_sender, _recipient, _sharesToTransfer);
        _emitTransferEvents(_sender, _recipient, _amount, _sharesToTransfer);
    }

    function getSharesByPooledEth(uint256 _ethAmount) public view returns (uint256) {
        return _ethAmount
            .mul(_getTotalShares())
            .div(_getTotalPooledEther());
    }

    function _transferShares(address _sender, address _recipient, uint256 _sharesAmount) internal {
        require(_sender != address(0), "TRANSFER_FROM_ZERO_ADDR");
        require(_recipient != address(0), "TRANSFER_TO_ZERO_ADDR");
        require(_recipient != address(this), "TRANSFER_TO_STETH_CONTRACT");
        _whenNotStopped();

        uint256 currentSenderShares = shares[_sender];
        require(_sharesAmount <= currentSenderShares, "BALANCE_EXCEEDED");

        shares[_sender] = currentSenderShares.sub(_sharesAmount);
        shares[_recipient] = shares[_recipient].add(_sharesAmount);
    }

Regarding the issue described by this report, after a StETH's transferFrom function call, calling the safeApprove function will not revert since the allowance has been reduced to 0. Moreover, when the StETH's transferFrom function is called for multiple times sequentially in one transaction, the values returned by _getTotalShares() and _getTotalPooledEther() would stay the same for each of these transferFrom function calls so calling the getSharesByPooledEth function would return the same _sharesToTransfer for the same _amount; because the same number of StETH shares is transferred for each transferFrom function call, the sequential transferFrom function calls in one transaction would not revert given that the first of these function calls does not revert. These reasons can be why the protocol's core functionality was never broken in the live instance. Indeed, the transferred StETH shares can be equivalent to a StETH amount that is a little less than the intended amount to be transferred due to the 1-2 wei corner case. The issue described by this report can be considered as a medium risk issue if the protocol needs to enforce the transfer of all of the intended amount; otherwise, the issue described by this report can be invalid.

[TradMod]

Firstly, I think this report is a bit incorrect and unnecessarily describes the safeApprove issue (which might be wrong). Also, this report's mitigation of wstETH is absolutely wrong as well.

This is clearly stETH's 1-2 wei corner case issue only. And thats exactly whats enough for this issue to be accepted as the Valid High Severity Issue. I'm not going to describe the vulnerabilities here, as I have explained them in detail in my two reports, especially the first one:

1. All stETH withdrawals from the EigenLayer strategy will fail
2. stETH deposits will fail due to the 1-2 wei corner case

Secondly, we have to understand that the 1-2 wei issue can occur often but not always. The current live instances of the protocol only transfer the tokens twice, but these new contracts will transfer 3-4 times (technically 5). So, the likelihood of this issue happening increases massively. As mentioned by the Lido docs, "In the future, when the stETH/share rate is greater, the error can become a bit bigger." When this happens, the whole protocol's stETH functionality will be broken, resulting in users' funds getting frozen in the EigenLayer, as they will be unable to withdraw.

where the 1-2 wei corner case might occur. Yet, since the allowance has been reduced to 0, further safeApprove function calls will not revert

I am uncertain if the safeApprove call will revert or not, but if the 1-2 wei corner case occurs, then the call will definitely revert due to the wrong amount the contracts try to transfer (more than it holds or excess amounts revert).

I believe this issue is rightly judged as a high-severity issue, and the only good mitigation for this is to transfer the actual token balance instead of the expected amounts. Maybe one of the other reports deserves the "selected for report" tag more.

As it seems the sponsor has acknowledged this issue but is not planning to fix it, I would like to request the sponsor @jatinj615 to review this report and revise the decision, as users' 20k stETH are at risk.

Thanks!

[0xEVom]

@alcueca @jatinj615 there is a good reason this hasn't manifested in the live contracts. This finding is invalid, and there is a misunderstanding of the referenced stETH 1-2 wei corner case in these submissions and the comments above.

In stETH, both transfer() and transferFrom() call _transfer() internally with the _amount passed as parameter. This function converts _amount to shares and transfers the converted amount to the recipient.

What this means is it isn't completely accurate to say that 1-2 wei less than passed as parameter are transferred. Rather, the rounded down amount of shares that best approximate the transfer amount is transferred, and these shares may amount to 1-2 wei less than the transfer amount when converted back into balance.

With that in mind, it is easy to see what will happen when multiple subsequent transfers with the same amount are executed in the same transaction. The amount will always be converted to the same number of shares, which means all recipients may obtain 1-2 wei less than the original amount, but not than the amount transferred in the preceding transfer. The transfers will all succeed, since the amount of shares that is transferred stays the same and is transferred from one contract to the next. It is, in fact, not a problem for an address to call transfer() with an amount that is slightly larger than the return value of balanceOf(address), as long as the amount rounds down to a number of shares that the address owns when converted.

[Bauchibred]

Hi @alcueca, I'd like you to consider the fact that this report includes two parts before making the final decision, information shared by @0xEVom & @rbserver is new to me. However this was even stated in the report:

NB: This bug report contains two sides of the coin on how the 1-2 wei corner case problem could affect renzo, 50% builds on the OOS safeApprove() from the bot report, however the second part of the report is in scope.

Which is because the safeApprove case was in the bot report and should be OOS, the case was just attached in this report to provide the maximum information about the potential impacts of the 1-2 wei corner case, the second part of this report however should still stand, which could cause some of the hinted impacts as to when every user attempts withdrawing etc, thank you for your time!

[TradMod]

lidofinance/core#442 (comment)

lidofinance/core#442 (comment)

[alcueca]

Based on the evidence shown, there isn't sufficient proof that an issue exists.

The report will be invalidated on this basis, but I recommend the sponsor to stay vigilant in case poof materializes in the future.

[c4-judge]

alcueca marked the issue as unsatisfactory:
Insufficient proof

[c4-judge]

alcueca marked the issue as unsatisfactory:
Invalid