Some functionalities would often revert, due to protocol's assumption that the amount specified in a transfer is directly what's been received

[c4-bot-10]

Lines of code

https://github.com/Renzo-Protocol/Contracts/blob/74a6c9963d4e9359c542901fdd2dd3f5a6864f16/contracts/Delegation/OperatorDelegator.sol#L139

Vulnerability details

Proof of Concept

In multiple instances, protocol directly transfers out tokens that were received or in some instances the "exact difference", however this would be problematic for tokens that have features similar to fee on transfer, protocol does not support any fee on transfer tokens, however protocol does support the stETH token. And the stETH token has a special feature when being transferred, quotig lido's offiicial docs from here: 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.

Now before diving deep to this issue, I'd like to indicate that this is not the same as the controversial issue from the previous contest around the DOS of the depositing/withdrawing channel due to the usage of safeApprove() on the stETH asset. After some research and with the help of other wardens that bug case is indeed invalid, cause whenever a transfer is being made on the stETH asset, the allowance specified in the function call first gets engulfed even if the transferred amount end up being 1-2 wei less, this is because in a transferFrom() call the entire allowance gets used up via _spendAllowance before attempting the transfer, see https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code:

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

However the stETH token's transfer mechanism is still special as documented by Lido and also we can confirm this from the contract code on Etherscan: https://etherscan.io/address/0x17144556fd3424edc8fc8a4c940b2d04936d17eb#code: Whenever a transfer is to be executed the function below is called:

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

This function, rounds down the amount to be returned by 1.

And when the final transfer is done this - 1 would be reflected in the amount that's received, i.e:

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

Asides using the code snippets from Etherscan, the official Lido team have also indicated that after their protocol-wide regression test coverage was introduced. There is now a possibility to even face a 2 wei corner case, which happens due to the fact that stETH balance calculation depends on two integer divisions (each one has 1 wei "loss" at most), this claim can be confirmed here: https://github.com/lidofinance/lido-dao/issues/442#issue-1298099918

Now since sufficient prove has been given on how the amount specified in a transfer for stETH could actually be more than is specified in the transaction we can now go deeper on how this affects Renzo, fortunately this does not mean that the depositing or the withdrawal channel would be permanently DOS'd when this integration is being done with safeApprove(), however not taking this into account would lead to often reverts in the case where the tokens being transferred in is stETH.

There are multiple instances of this in scope, with some even having more than one attempt of transferring out the tokens, now for example let's consider the new implementation of the RestakeManager#deposit() after this pull request was finalized to mitigate H-08 & M-09 from the previous contest

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

-        // Approve the tokens to the operator delegator
-        _collateralToken.safeApprove(address(operatorDelegator), _amount);

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

+        //  check if amount needs to be sent to operatorDelegator
+         if(_amount > 0) {
+             // 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);
    }

Now, to use only this instance to showcase how not taking the 1-2 wei corner case would cause a revert, let's only look at instances when transfers occur within the execution of RestakeManager#deposit().

• After all the validations have been done the collateral token is being transferred to the RestakeManager address using this line _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);
  • We can assume the 1 wei rounding down issue has occurred here, so the real amount transferred in is _amount - 1.
• Now, after the collateral toke has been transferred, protocol checks if there is a need to send tokens to the operatorDelegator, and if yes, how much tokens.
• Now. operatorDelegator.deposit() also queries a safeTransferFrom() on the amount of token specified as shown below.

    function deposit(
        IERC20 token,
        uint256 tokenAmount
    ) external nonReentrant onlyRestakeManager returns (uint256 shares) {
        if (address(tokenStrategyMapping[token]) == address(0x0) || tokenAmount == 0)
            revert InvalidZeroInput();

        // Move the tokens into this contract
        token.safeTransferFrom(msg.sender, address(this), tokenAmount);

        return _deposit(token, tokenAmount);
    }

• So in our case here, considering the _amount has been truncated by one, the attempt to transfer in operatorDelegator.deposit() reverts, since from RestakeManager#deposit() we pass on the non-truncated amount value when querying the operatorDelegator.deposit(_collateralToken, _amount);

Impact

Frequent reverts in core functionality when integrating with stETH, would be key to also note that the instance hinted in the report is just one of many, other call route in scope even include more transfers in their executions which causes this bug case to occur.

Submitting this as medium as I assume it satisfies the criteria of the function of the protocol or its availability could be impacted.

Recommended Mitigation Steps

A very popular approach when directly integrating stETH and having to call transfer/transferFrom() on it, is to also include a balance difference check, i.e before the transfer check to see how much of stETH is in the contract and after then after transfer, the accounting should be done on the real amount of tokens received, so a quick pseudo fix for the instance explained in this report would be to integrate the snippet below into RestakeManager#deposit():

uint256 balanceBefore;
uint256 realAmountReceived;

// Check if the collateral token is stETH
if (_collateralToken == stETH) {
    balanceBefore = IERC20(stETH).balanceOf(address(this));
}

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

// Calculate the real amount received if the collateral token is stETH
if (_collateralToken == stETH) {
    realAmountReceived = IERC20(stETH).balanceOf(address(this)) - balanceBefore;
}

// Check if amount needs to be sent to operatorDelegator
if (_amount > 0) {
    // Approve the tokens to the operator delegator
    _collateralToken.safeApprove(address(operatorDelegator), _amount);

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

    // Additional deposit for stETH
    if (_collateralToken == stETH) {
        operatorDelegator.deposit(_collateralToken, realAmountReceived);
    }
}

And extensively all other instances where steth could be transferred in/out.

Assessed type

Token-Transfer

[0xEVom]

I explained here why this is not an issue and no reverts will occur.

[jatinj615]

It’s about stETH 1-2 wei scenario which was downgraded to QA or disputed if I remeber correctly. So I think this argument on stETH is invalid as protocol acknowledged this issue.

Also protocol have updated the safeApprove to safeIncreaseAllowance in the PR - https://github.com/Renzo-Protocol/Contracts/pull/100 . Which would forward the remaining shares/tokens in the next deposit with it's increase allowance.

[Bauchibred]

I explained here why this is not an issue and no reverts will occur.

I acknowledged your explanation in this submission, which is why I indicated that the issue on safeApprove() reverting is indeed invalid, but your explanation only considers when the call is being routed in the same instance from contract to contract with the same amount, which means that the rounded down amount of shares would be the same in that tx.

However, in Renzo as far as I can remember these amounts are sometimes broken into different parts.

Attaching an example POC below were I still see how a revert could occur, @0xEVom, I'd appreciate if you could clarified if I've missed anything.

- If we use an example were say, two `amounts = 100` & `amount = 110` are transferred in and were rounded down by `2`. - The real balance in the contract would be 206 - If we transfer out "70" in three instances and final share in every instance is rounded down by `1` - This would lead to a revert in the third transaction, considering - - After the first transaction, real balance in the contract would be `206 - 69` = `137` - - After the second transaction, real balance in the contract would be `137 - 69` = `68` - - As at the time of the third transaction, real balance in the contract would be `68` now since `68` < `69`, the final attempt at transferring reverts. _I assume this same logic could be applicable in this protocol's case and considering the reason for reverts would be the 1-2 wei differences wouldn't it be better if protocol just uses the exact amount transferred in to ensure operations always move smoothly_

---

It’s about stETH 1-2 wei scenario which was downgraded to QA or disputed if I remeber correctly. So I think this argument on stETH is invalid as protocol acknowledged this issue.

Also protocol have updated the safeApprove to safeIncreaseAllowance in the PR - https://github.com/Renzo-Protocol/Contracts/pull/100 . Which would forward the remaining shares/tokens in the next deposit with it's increase allowance.

Hi @jatinj615 I can't access the PR, though would be key to note that in this case, the report is not saying the approval reverts, but rather that the transfers would revert since the balance would be less than is needed to be transferred.

---

Also to save everyone time, I'd like to indicate that the judge hinted this part of the Guidelines on mitigation review from his comment on another issue which I wasn't previously aware of:

Vulnerabilities submitted during the preceding audit should not be submitted as new HM issues during the mitigation review. They will be considered out of scope and ineligible for awards. If a warden feels an issue from the preceding audit was overlooked or undervalued, it is recommended to look into submitting it to the sponsor via other channels (e.g., a bug bounty program).

I assume depending on if we're looking at the root cause of this issue as the 1-2 wei scenario attached to stETH or if we are breaking that and having the previous submission being based on the reverts during approvals and this being on reverts during transfers this submission could be in-scope or OOS.

So leaving it to the judge to decide if it's OOS, cause if we perceive the root cause as the former which is the 1-2 wei scenario attached to stETH, then imo per Code4rena mitigation reviews guidelines this report should be OOS.

If otherwise, then pending @0xEVom addition in the case if I've missed anything then this submission could be valid/invalid and in-scope.

[s1n1st3r01]

Agree with @0xEVom and the sponsor's assessment. This is the same as https://github.com/code-423n4/2024-04-renzo-findings/issues/10 therefore should be invalid.

Such issue never appeared in the live instance despite their being 20k stETH deposited in the live protocol at this point.

[alcueca]

let's consider the new implementation of the RestakeManager#deposit() Which for effects of this finding is the same as the old implementation.

Such issue never appeared in the live instance despite their being 20k stETH deposited in the live protocol at this point. The issue should have materialized by now.

I do not think this is OOS, because the original finding specifically mentioned safeApprove, but I do think that there isn't enough proof that the vulnerability exists in the face of the protocol executing deposits without problems.

[c4-judge]

alcueca marked the issue as unsatisfactory: Insufficient proof