Renzo - Bauchibred's results

A protocol that abstracts all staking complexity from the end-user and enables easy collaboration with EigenLayer node operators and a Validated Services (AVSs).

General Information

Platform: Code4rena

Start Date: 30/04/2024

Pot Size: $112,500 USDC

Total HM: 22

Participants: 122

Period: 8 days

Judge: alcueca

Total Solo HM: 1

Id: 372

League: ETH

Renzo

Findings Distribution

Researcher Performance

Rank: 3/122

Findings: 7

Award: $4,459.97

🌟 Selected for report: 0

🚀 Solo Findings: 0

Findings Information

🌟 Selected for report: LessDupes

Also found by: Bauchibred, grearlake

Labels

3 (High Risk)
satisfactory
duplicate-612

Awards

2675.4886 USDC - $2,675.49

External Links

Judge has assessed an item in Issue #543 as 3 risk. The relevant finding follows:

QA-09 Funds could be locked for some users if collateral to withdraw is native ETH token

#0 - c4-judge

2024-05-28T15:15:32Z

alcueca marked the issue as duplicate of #52

#1 - c4-judge

2024-05-28T17:21:14Z

alcueca marked the issue as duplicate of #612

#2 - c4-judge

2024-05-30T05:20:16Z

alcueca marked the issue as satisfactory

Awards

0.4071 USDC - $0.41

Labels

bug
3 (High Risk)
satisfactory
sufficient quality report
upgraded by judge
:robot:_224_group
duplicate-326

External Links

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L491-L576

Vulnerability details

Proof of Concept

Take a look 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();

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

        // (...snip)

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

        // (...snip)

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

This function is used to deposit any supported collateral token can be to the protocol so as to get the equivalent amount of ezETH minted to user calling this function. (keep in mind that protocol currently supports two collateral tokens, but this number could as well increase in the future).

Considering the heavy sentiments around the crypto space and how reactive it is to news, let's assume a heavy fud is going on with Binance and now the wBETH is now losing trust in the market as such it's TVL , price and market value are all plummeting, which could be due to the fud + users are selling their holdings off or trying to move their assets as such the token's value is dropping fast, note that this is not uncommon in the crypto world. Users that then get hold of this news would start withdrawing their assets from Renzo since in this case users know that Renzo has a heavy backlog of assets depending on the stability of wBETH.

Using the two (wBETH & stETH) out of the three supported assets for a minimalistic analysis (doing away with native ETH to also keep the logic as minimal as possible) :

  • Assume each of the supported collateral token cover 50% of the protocol's overall TVL.
  • Considering protocol includes restaking logic we'd assume users are somewhat tech savvy.
  • Protocol have a 100 users who have deposited different collateral tokens, i.e both wBETH &stETH
  • Assume all users deposit their assets via the same collateral value, i.e 50 users deposited wBETH and the other 50 deposited stETH.
  • Now a few users notice the heavy fud around wBETH, knowing that protocol has an equal share of the two collateral tokens, and that the value of wBETH is plummeting they start placing in withdrawal requests to withdraw their assets, but while doing this, they then specify stETH as the collateral token they'd like to withdraw.

    Note that the above is allowed, since during withdrawals, users are requested to pass in which collateral token they'd like to receive on claim for their formerly minted ezETH which is going to be unlocked & burnt so as to finalize the withdrawal request.

  • Since users are considered tech savvy, users that are online would then try to front run each other in placing withdrawal requests so as to make the most out of their already deposited assets.
  • This then leaves users who notice this fud a bit later to be left with ~unworthy tokens available if they want to place withdrawal requests, and this also causes a plummet in the collateral backing of the ezETH itself

    Note that the above is highly possible as this is web3 and users could be from anywhere in the world, so depending on the time frame the heavy news/fud drops some locations would have the upper hand over the other, considering the usual flux in the market and news depending on day/night.

Impact

As explained in the last section of the ## Proof of Concept, this causes an unfair advantage generally on users who were not available during the time the market turbulence started.

The above is not only the impact of this issue unfortunately, this is because this then causes an unfair advantage on the users who purely deposited stETH but weren't on time to claim their withdrawals in their deposited token, explaining this a bit more with our 50 users purely deposited stETH scenario explained in the ## Proof of Concept, assume only 20 users got to withdraw on time, this essentially means that the remaining 30 users have been unfairly stripped off of their assets.

NB: Whereas this is possible considering how unpredictable the crypto/web3 space is right now, the complexity and likelihood of this bug increases as more LSTs are integrated into protocol in the future.

Here are two suggestions:

  • Either consider having a mapping of which user deposited what collateral token and hardcode that they can only place withdrawal requests on that token (this at least makes the approach fair on users, since everyone makes a choice on which asset to deposit and has it in the back of their mind that they receive this token back when withdrawing)... or
  • Introduce a mapping to store the last price of all supported collateral token and then include a differential price difference check in the logic of the withdrawal requests to see if the difference between the last price of any of the collateral token supported is higher than a set margin, if it is then execute the withdrawal with a similar logic as to the concept of bad debt socialisation this could be achieved by then breaking down the requested withdrawal equally in all supported collateral token, this way, "market percentage wise" no user gets the upper hand over another and every one is treated fairly as the plummet in price has been socialised.

Assessed type

Context

#0 - c4-judge

2024-05-16T14:02:57Z

alcueca changed the severity to 3 (High Risk)

#1 - c4-judge

2024-05-16T14:03:17Z

alcueca marked the issue as duplicate of #326

#2 - c4-judge

2024-05-17T12:48:33Z

alcueca marked the issue as satisfactory

Awards

0.4071 USDC - $0.41

Labels

bug
3 (High Risk)
satisfactory
sufficient quality report
upgraded by judge
:robot:_68_group
duplicate-326

External Links

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L206-L263 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L206-L263

Vulnerability details

Proof of Concept

It's a known fact that validators can indeed lose part of their deposit via penalties or slashing events:

  • In case of penalties Eigenlayer can be notified of the balance drop via the permissionless EigenPod#verifyBalanceUpdates() function.
  • In case of slashing the validator is forced to exit and Eigenlayer can also be notified via the permissionless EigenPod#verifyAndProcessWithdrawals() function because the slashing event is effectively a full withdrawal.

As soon as either EigenPod#verifyBalanceUpdates() or EigenPod#verifyAndProcessWithdrawals() is called the TVL of the Renzo protocol drops instantly. This is because both of the functions update the variable podOwnerShares[podOwner]:

This makes it possible for stakers to:

  1. Request a withdrawal via WithdrawQueue#withdraw() for all their ezETH held.
  2. Then call either EigenPod#verifyBalanceUpdates() or EigenPod#verifyAndProcessWithdrawals().

So at the point when WithdrawQueue#withdraw() will be called and the withdrawal will be successfully queued, the TVL is not going to include penalties or slashing.

Impact

Stakers can frontrun validators penalties and slashing events with a withdrawal request in order to avoid the loss when the deposit pool has enough liquidity effectively allowing them to game the system.

This essentially shows that it's possible to withdraw already minted ezETH while avoiding penalties or slashing up to the amount of liquidity available in the deposit pool.

After executing this, all the staker needs to do is to wait the 7 day period and they can then claim their withdrawal request via WithdrawQueue#claim() completely sidestepping the slashing/penalties

Consider making the n WithdrawQueue#withdraw() execution more effective, this can be achieved by introducing a check whenever the function is called to see if penalties or slashing events happened during the epoch the request is being passed, if yes then distribute the correct amount of penalties to all the ezETH withdrawn in the current epoch, including the ones that requested the withdrawal before the drop.

Assessed type

Context

#0 - c4-judge

2024-05-16T08:17:58Z

alcueca marked the issue as satisfactory

#1 - c4-judge

2024-05-23T14:04:55Z

alcueca changed the severity to 3 (High Risk)

#2 - c4-judge

2024-05-23T14:05:22Z

alcueca marked the issue as duplicate of #326

Findings Information

🌟 Selected for report: guhu95

Also found by: Bauchibred, LessDupes, ilchovski, zzykxx

Labels

bug
2 (Med Risk)
grade-a
satisfactory
sponsor acknowledged
:robot:_primary
duplicate-514

Awards

347.9473 USDC - $347.95

External Links

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L216-L224 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RateProvider/BalancerRateProvider.sol#L31 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L131-L157

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L131-L157

    function addOperatorDelegator(
        IOperatorDelegator _newOperatorDelegator,
        uint256 _allocationBasisPoints
    ) external onlyRestakeManagerAdmin {
        // Ensure it is not already in the list
        uint256 odLength = operatorDelegators.length;
        for (uint256 i = 0; i < odLength; ) {
            if (address(operatorDelegators[i]) == address(_newOperatorDelegator))
                revert AlreadyAdded();
            unchecked {
                ++i;
            }
        }

        // Verify a valid allocation
        if (_allocationBasisPoints > (100 * BASIS_POINTS)) revert OverMaxBasisPoints();

        // Add it to the list
        operatorDelegators.push(_newOperatorDelegator);

        emit OperatorDelegatorAdded(_newOperatorDelegator);

        // Set the allocation
        operatorDelegatorAllocations[_newOperatorDelegator] = _allocationBasisPoints;

        emit OperatorDelegatorAllocationUpdated(_newOperatorDelegator, _allocationBasisPoints);
    }

This is the function used by restake manager admin to add an OperatorDelegator to the list of ODs, would be key to note that there is no check whatsoever on the amount of already added operators meaning no limit to the amount of operators that could be added and with time a lot more operators would be added.

Now would be key to note that the RestakeManager.calculateTVLs() is a hefty gas consuming function since it loops through all the existing operators and having the list be extensively long would lead this to an OOG if coupled with the gas already burnt on the side by the core functionality calling this in some instance.

Now this function is called whenever withdrawing via the withdrawal queue, i,e https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L216-L224

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
(...snip)
        // calculate totalTVL
        (, , uint256 totalTVL) = restakeManager.calculateTVLs();

        // Calculate amount to Redeem in ETH
        uint256 amountToRedeem = renzoOracle.calculateRedeemAmount(
            _amount,
            ezETH.totalSupply(),
            totalTVL
        );
(...snip)
}

And also when getting the current rate of ezETH in ETH via the BalancerRateProvider.

Impact

  • Withdrawal attempts would be bricked, leaving user funds stuck in the protocol in the case of the WithdrawQueue.withdraw().
  • For BalancerRateProvider.getRate() however this function is heavily used across protocol, with implementations in the xRenzoBridge which would then mean that functionalities that need price to be sent to the CCIP destination via xRenzoBridge.sendPrice() would all fail faulting the cross chain functionality of protocol.

Consider having a max accepted figure for the operator delegators.

Assessed type

DoS

#0 - jatinj615

2024-05-14T12:14:09Z

Similar to #560

#1 - alcueca

2024-05-20T02:53:12Z

Admin error, easily fixable by calling removeOperatorDelegator.

#2 - c4-judge

2024-05-20T02:53:17Z

alcueca changed the severity to QA (Quality Assurance)

#3 - c4-judge

2024-05-20T02:53:22Z

alcueca marked the issue as grade-a

#4 - c4-judge

2024-05-20T03:57:38Z

This previously downgraded issue has been upgraded by alcueca

#5 - c4-judge

2024-05-20T03:57:47Z

alcueca marked the issue as duplicate of #514

#6 - c4-judge

2024-05-20T03:57:57Z

alcueca marked the issue as partial-75

#7 - c4-judge

2024-05-20T03:58:01Z

alcueca marked the issue as satisfactory

#8 - alcueca

2024-05-20T03:58:38Z

Not pointing out that calculateTVLs loops through both operators and tokens is a major flaw in this report.

Awards

1.479 USDC - $1.48

Labels

bug
2 (Med Risk)
downgraded by judge
satisfactory
:robot:_107_group
duplicate-484

External Links

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L206-L263

Vulnerability details

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L206-L263

    function withdraw(uint256 _amount, address _assetOut) external nonReentrant {
        // check for 0 values
        if (_amount == 0 || _assetOut == address(0)) revert InvalidZeroInput();

        // check if provided assetOut is supported
        if (withdrawalBufferTarget[_assetOut] == 0) revert UnsupportedWithdrawAsset();

        // transfer ezETH tokens to this address
        IERC20(address(ezETH)).safeTransferFrom(msg.sender, address(this), _amount);

        // calculate totalTVL
        (, , uint256 totalTVL) = restakeManager.calculateTVLs();

        // Calculate amount to Redeem in ETH
        uint256 amountToRedeem = renzoOracle.calculateRedeemAmount(
            _amount,
            ezETH.totalSupply(),
            totalTVL
        );

        // update amount in claim asset, if claim asset is not ETH
        if (_assetOut != IS_NATIVE) {
            // Get ERC20 asset equivalent amount
            amountToRedeem = renzoOracle.lookupTokenAmountFromValue(
                IERC20(_assetOut),
                amountToRedeem
            );
        }
        //@audit
        // revert if amount to redeem is greater than withdrawBufferTarget
        if (amountToRedeem > getAvailableToWithdraw(_assetOut)) revert NotEnoughWithdrawBuffer();

        // increment the withdrawRequestNonce
        withdrawRequestNonce++;

        // add withdraw request for msg.sender
        withdrawRequests[msg.sender].push(
            WithdrawRequest(
                _assetOut,
                withdrawRequestNonce,
                amountToRedeem,
                _amount,
                block.timestamp
            )
        );

        // add redeem amount to claimReserve of claim asset
        claimReserve[_assetOut] += amountToRedeem;

        emit WithdrawRequestCreated(
            withdrawRequestNonce,
            msg.sender,
            _assetOut,
            amountToRedeem,
            _amount,
            withdrawRequests[msg.sender].length - 1
        );
    }

This function is used to create withdrawals however there is no availability for a user to provide their own slippage, i.e if (amountToRedeem > getAvailableToWithdraw(_assetOut)) revert NotEnoughWithdrawBuffer()... keep in mind that the amountToRedeem is calculated on-chain from the RenzoOracle and not in control of the user, i.e protocol automatically calculates this value and then hardcodes a 0% slippage on the attempt to withdraw, this leads to two issues as explained in the Impact section.

Impact

Since the calculation of the amountToRedeem is not in the user's control (i.e no slippage provided) this could lead to multiple scenarios.

  • In the case the amountToRedeem value returned from RenzoOracle is very minute or unfair to the user there is no way for them to dispute the transaction, i.e a normal withdrawal flaw that consists of the ideas of exchange rates or conversions always have a user slippage attached so they can clarify if the final amount to be gotten is accepted by them or they would like to try the withdrawal later on a better term to them, also note that this could actually happen even in normal market condition, but then consider a situation where the oracle is stale and the amountToRedeem deviates too much from the real market price and the user would not like to go on with the withdrawal request tx.

  • Another case is when the amountToRedeem returned value is not up to the getAvailableToWithdraw(_assetOut) but there is a very minute/negligible difference, which users might have accepted but this transaction instead fails, and this could lead to users losing out on monetary value of their assets as they might be attempting to withdraw to get their token out cause they feel the crypto market is taking a dump, but they can't do that on time since they can't really place their withdrawal requests on time

Considering attaching a slippage parameter to the WithdrawQueue's withdrawal attempt and then ensure that the value returned from renzoOracle.lookupTokenAmountFromValue() is not less than this value and revert if otherwise, additionally the check here should be made against the slippage provided value to ensure a user's withdrawal request is not DOS'd if they can accept a value <= the current available max withdrawal.

Assessed type

Context

#0 - c4-judge

2024-05-17T13:44:07Z

alcueca marked the issue as duplicate of #345

#1 - c4-judge

2024-05-17T13:45:12Z

alcueca marked the issue as satisfactory

#2 - c4-judge

2024-05-24T10:28:57Z

alcueca changed the severity to 2 (Med Risk)

Findings Information

🌟 Selected for report: LessDupes

Also found by: Bauchibred

Labels

2 (Med Risk)
satisfactory
duplicate-113

Awards

1416.4352 USDC - $1,416.44

External Links

Judge has assessed an item in Issue #543 as 2 risk. The relevant finding follows:

QA-02 Balancer pools are going to use the wrong rates

#0 - c4-judge

2024-05-24T09:45:38Z

alcueca marked the issue as duplicate of #113

#1 - c4-judge

2024-05-24T09:45:41Z

alcueca marked the issue as satisfactory

Findings Information

Awards

18.1958 USDC - $18.20

Labels

bug
2 (Med Risk)
downgraded by judge
satisfactory
sponsor acknowledged
edited-by-warden
:robot:_28_group
duplicate-103

External Links

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L274-L358 WithdrawQueue.withdraw()`](https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L217-L224 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L1/xRenzoBridge.sol#L214-L215 https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L594-L600

Vulnerability details

Proof of Concept

First going to the contest's outlined documentation we can see that the first invariant stated is :

ezETH should be minted or redeemed based on current supply and TVL.

Additionally the requested bug windows/attack ideas to focus on clearly hinted we check on the integrity on the TVL calculations (ezETH Pricing), i.e a user should not mint or withdraw at invalid prices.

Now Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L244-L264

    function removeCollateralToken(
        IERC20 _collateralTokenToRemove
    ) external onlyRestakeManagerAdmin {
        // Remove it from the list
        uint256 tokenLength = collateralTokens.length;
        for (uint256 i = 0; i < tokenLength; ) {
            if (address(collateralTokens[i]) == address(_collateralTokenToRemove)) {
                collateralTokens[i] = collateralTokens[collateralTokens.length - 1];
                collateralTokens.pop();
                emit CollateralTokenRemoved(_collateralTokenToRemove);
                return;
            }
            unchecked {
                ++i;
            }
        }

        // If the item was not found, throw an error
        revert NotFound();
    }

The restake manager is allowed to remove a collateral whenever, which pops it off the collateralTokens array, note that this function exists and is to be used, so we can consider this normal integration, however there are no checks that no operator or even the withdrawalQueue have a balance of this collateral token that's to be removed, and also this collateral gets completely sidestepped when calculating the OD and protocol's TVL, see how the TVL's are being calculated at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L274-L358

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
            ///(...snip)
            uint256[] memory operatorValues = new uint256[](collateralTokens.length + 1);
            operatorDelegatorTokenTVLs[i] = operatorValues;

            // Iterate through the tokens and get the value of each
            uint256 tokenLength = collateralTokens.length;
            for (uint256 j = 0; j < tokenLength; ) {
                // Get the value of this token

                uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
                    collateralTokens[j]
                );

                // Set the value in the array for this OD
                operatorValues[j] = renzoOracle.lookupTokenValue(
                    collateralTokens[j],
                    operatorBalance
                );

                // Add it to the total TVL for this OD
                operatorTVL += operatorValues[j];

                // record token value of withdraw queue
                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                        collateralTokens[i],
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }

                unchecked {
                    ++j;
                }
            }

            // Get the value of native ETH staked for the OD
            uint256 operatorEthBalance = operatorDelegators[i].getStakedETHBalance();

            // Save it to the array for the OD
            operatorValues[operatorValues.length - 1] = operatorEthBalance;

            // Add it to the total TVL for this OD
            operatorTVL += operatorEthBalance;

            // Add it to the total TVL for the protocol
            totalTVL += operatorTVL;

            // Save the TVL for this OD
            operatorDelegatorTVLs[i] = operatorTVL;

            // Set withdrawQueueTokenBalanceRecorded flag to true
            withdrawQueueTokenBalanceRecorded = true;

            unchecked {
                ++i;
            }
        }

        // Get the value of native ETH held in the deposit queue and add it to the total TVL
        totalTVL += address(depositQueue).balance;

        // Add native ETH help in withdraw Queue and totalWithdrawalQueueValue to totalTVL
        totalTVL += (address(withdrawQueue).balance + totalWithdrawalQueueValue);

        return (operatorDelegatorTokenTVLs, operatorDelegatorTVLs, totalTVL);
    }

We can see that there is a need to route through all operators and their delegators to iterate to get the amount of each collateral token they own which is then added to their total TVL, also the amount present in the withdrawal queue for each collateral token is counted and added to the protocol's totalTVL being returned from the calculation , however since the way this collateralTokens are queried is dependent on the stored array, the collateral token that's been popped essentially wouldn't get iterated on, that's to say all the amount of asset protocol holds via the OD of the WithdrawalQueue is not going to be taken in this calculation which would mean that the amount of TVL returned by this calculation is going to be heavily deflated considering these balances just get sidestepped and are not added to the calculation

Impact

This easily faults any logic that directly/indirectly queries or needs the either the OD's or the protocol's totalTVL, to list out some noteworthy impacts:

Keep in mind that this function inherently gets called when there is a need to price the assets to be withdrawn or to get the rate from the BalancerRateProvider among other instances i.e:

        (, , uint256 totalTVL) = calculateTVLs();

        // Enforce TVL limit if set
        if (maxDepositTVL != 0 && totalTVL + msg.value > maxDepositTVL) {
            revert MaxTVLReached();
        }
  • One last subtle thing to note is that about how the operatorTVL is finalized, which is by accumalating it into the operatorDelegatorTVLs that gets returned, so now whenever there is a need to deposit, either via depositTokenRewardsFromProtocol() or deposit() the chooseOperatorDelegatorForDeposit()function is being called, and all this function does is to pick the OperatorDelegator with the TVL below the threshold, but this would return the wrong data since the operator delegator TVL has been deflated as it would assume the operator to be below the threshold whereas they should be above... lastly an operator that in real sense would be able to process a withdrawal would also not be chosen for the withdrawal in some cases via the checks in chooseOperatorDelegatorForWithdraw() leaving users assets to be stuck in protocol if say there are little number of operators and their deflated TVL is making this check revert before withdrawals are processed.

TLDR: Asides the already stated impacts, the invariant about using the current TVL/ correct ezETH pricing to process withdrawals or mint requests is also going to be broken.

Consider reimplementing this logic and ensure that after removals of collateral tokens this don't immediately affect the accounting logic of protocol, i.e they shouldn't be dropped/sidestepped immediately from the TVL calculations and instead they should only be dropped after all parties no longer hold this previously supported collateral token.

Assessed type

Context

#0 - c4-judge

2024-05-17T14:05:47Z

alcueca marked the issue as unsatisfactory: Invalid

#1 - c4-judge

2024-05-20T04:34:14Z

alcueca changed the severity to 2 (Med Risk)

#2 - c4-judge

2024-05-20T04:38:45Z

alcueca marked the issue as satisfactory

#3 - c4-judge

2024-05-20T04:38:51Z

alcueca marked the issue as selected for report

#4 - alcueca

2024-05-20T04:43:31Z

I assume the sponsor team to be competent and test their governance actions before executing them. Therefore, the impact from this finding would likely be caught in time and not lead to TVL loss or users funds lost, but just to the inability to remove tokens. Hence the Medium rating.

#5 - Bauchibred

2024-05-26T20:37:44Z

Hi @alcueca, thanks for judging! A fellow warden left the comment below under a different report to suggest this issue should be downgraded, replying here to keep the reply on topic.

I disagree with the warden's assertion that the report assumes admins would carelessly remove a collateral token. The existence of the removeCollateralToken() function indicates there are valid scenarios where removing a collateral token is necessary.

The core argument of the report is not about a "reckless mistake" but rather the need for a more robust implementation of the removal functionality. Specifically, it suggests that even when a token is removed, it should not be excluded from the TVL calculations. Instead, the token's support should be removed without affecting the TVL calculations.

Facts to consider:

  • The protocol has a removeCollateralToken() function, suggesting that there are valid cases where a collateral token may need to be removed.
  • The report does not assume carelessness; it highlights the potential flaw in the current implementation and offers mitigation steps.
  • The recommendation is to consider maintaining the removed token in the TVL calculations while ceasing its support. This approach ensures the protocol's TVL logic remains accurate and consistent.
  • Suggesting that tokens should never be removed contradicts the intention behind the existing functionality, as evidenced by the function's inclusion in the codebase.

Given that the removal of collateral tokens is an intended feature, the report rightly points out the need to handle this process correctly to avoid breaking invariants. Therefore, I see no reason why the issue should be categorized as QA, considering the current implementation would automatically lead to a core invariant of the protocol being broken.

#6 - c4-judge

2024-05-27T05:15:38Z

alcueca marked issue #103 as primary and marked this issue as a duplicate of 103

Findings Information

Awards

18.1958 USDC - $18.20

Labels

2 (Med Risk)
satisfactory
duplicate-103

External Links

Judge has assessed an item in Issue #543 as 2 risk. The relevant finding follows:

QA-11 Flawed TVL calculation on collateral token removal

#0 - c4-judge

2024-05-24T09:46:58Z

alcueca marked the issue as duplicate of #5

#1 - c4-judge

2024-05-24T09:47:01Z

alcueca marked the issue as satisfactory

QA Report for Renzo

Table of Contents

Issue IDDescription
QA-01Renzo currently misses out on Blast yields and redirected rewards from the sequencer
QA-02Balancer pools are going to use the wrong rates
QA-03The owner is allowed to set any price
QA-04Push method for withdrawals is generally frowned upon
QA-05Collateral tokens could be over/undervalued due to their not so safe dependance on external integration with Chainlink
QA-06Consider implementing a better documented max fees applied to deposits
QA-07DepositQueue#sweepERC20() in some cases would be non-functional
QA-08Depositing protocol rewards in some instances might revert due to an underflow
QA-09Funds could be locked for some users if collateral to withdraw is native ETH token
QA-10Consider wrapping all latestRoundData() calls pertaining collateral tokens in a try catch
QA-11Flawed TVL calculation on collateral token removal
QA-12Protocol should consider that stETH can be paused
QA-13Transfers currently might fail silently
QA-14First check if withdrawRequestIndex == withdrawRequests[msg.sender].length - 1 before setting the storage
QA-15Fix documentation
QA-16TVL logic might be faulted when considering the integrational context around it and the withdrawalQueue

QA-01 Renzo currently misses out on Blast yields and redirected rewards from the sequencer

Proof of Concept

The Renzo protocol overlooks a potential revenue stream on the Blast network, this is because Blast unlike other optimistic L2s redirects sequencer fees and also USDB/WETH yields to deployed contracts on the network.

Also, it's bee hinted in the readMe and during the contest that both the xERC20 and OptimismMintableXERC20 would be deployed on Blast, however these contracts lack any configuration whatsoever to claim the rewards.

Impact

Loss of revenue for the protocol (redirected fees and USDB/WETH yields). (could be considered leak in value).

This has been submitted as low since it's not been documented that the protocol intends to have access to this, but leaving to the discretion of the judge to upgrade as this submission can be considered as medium.

Recommendation

Reimplement the current logic of contracts to be deployed on Blast, ensure that the rewards have been set to the claimable mode to access these yield/rewards, more can be read from blast's official docs

QA-02 Balancer pools are going to use the wrong rates

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L2/xRenzoDeposit.sol#L451-L459

    /**
     * @notice  Exposes the price via getRate()
     * @dev     This is required for a balancer pool to get the price of ezETH
     * @return  uint256  .
     */@audit stale rates
    function getRate() external view override returns (uint256) {
        return lastPrice;
    }

This function exposes the prices, note that this functionality is required to get the price of ezETH, issue with this is that the function in it's sense actually returns stale rates, this is cause it uses the last returned lastPrice instead of querying to get the current price.

Impact

Note that the rate gotten from xRenzoDeposit.getRate() is essentially used in the ezETH pricing from the balancer pool, however this would be wrong, which means that the ezETH token would be wrongly priced.

I'd argue this is borderline medium/low and considering this is one of the hinted attack bugs and idea listed in the readMe, judge should upgrade if they see the upside of this bug as a medium.

Introduce a view function that queries the contracts necessary state and returns an updated price, something similar to _updatePrice(), but this new function shouldn't include any state changes since it's being view modified.

QA-03 The owner is allowed to set any price

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Bridge/L2/xRenzoDeposit.sol#L315-L360


    function updatePriceByOwner(uint256 price) external onlyOwner {
        return _updatePrice(price, block.timestamp);
    }

        function _updatePrice(uint256 _price, uint256 _timestamp) internal {
        // Check for 0
        if (_price == 0) {
            revert InvalidZeroInput();
        }

        // Check for price divergence - more than 10%
        if (
            (_price > lastPrice && (_price - lastPrice) > (lastPrice / 10)) ||
            (_price < lastPrice && (lastPrice - _price) > (lastPrice / 10))
        ) {
            revert InvalidOraclePrice();
        }

        // Do not allow older price timestamps
        if (_timestamp <= lastPriceTimestamp) {
            revert InvalidTimestamp(_timestamp);
        }

        // Do not allow future timestamps
        if (_timestamp > block.timestamp) {
            revert InvalidTimestamp(_timestamp);
        }

        // Update values and emit event
        lastPrice = _price;
        lastPriceTimestamp = _timestamp;

        emit PriceUpdated(_price, _timestamp);
    }

This function indicates that the owner is allowed to set any price whatsoever for the token.

Following COde4rena new QA rules this is very important to mention, as the owner is now allowed to set any price which would affect the amount of tokens users could get minted for their assets

Impact

Malicious admin could break the minting logic

Consider clearly indicating his is possible in the docs and to users

QA-04 Push method for withdrawals is generally frowned upon

Proof of Concept

Take a look at how withdrawal requests are being claimed https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L279-L312

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
        // check if provided withdrawRequest Index is valid
        if (withdrawRequestIndex >= withdrawRequests[msg.sender].length)
            revert InvalidWithdrawIndex();

        WithdrawRequest memory _withdrawRequest = withdrawRequests[msg.sender][
            withdrawRequestIndex
        ];
        if (block.timestamp - _withdrawRequest.createdAt < coolDownPeriod) revert EarlyClaim();

        // subtract value from claim reserve for claim asset
        claimReserve[_withdrawRequest.collateralToken] -= _withdrawRequest.amountToRedeem;

        // delete the withdraw request
        withdrawRequests[msg.sender][withdrawRequestIndex] = withdrawRequests[msg.sender][
            withdrawRequests[msg.sender].length - 1
        ];
        withdrawRequests[msg.sender].pop();

        // burn ezETH locked for withdraw request
        ezETH.burn(address(this), _withdrawRequest.ezETHLocked);

        // send selected redeem asset to user
        if (_withdrawRequest.collateralToken == IS_NATIVE) {
            payable(msg.sender).transfer(_withdrawRequest.amountToRedeem);
        } else {
            IERC20(_withdrawRequest.collateralToken).transfer(
                msg.sender, //@audit
                _withdrawRequest.amountToRedeem
            );
        }
        // emit the event
        emit WithdrawRequestClaimed(_withdrawRequest);
    }

In the claim function of the WithdrawQueue contract, we can see that withdrawal assets are transferred to msg.sender without allowing users to specify a different recipient. However, if a user gets blacklisted, they cannot claim their withdrawals, effectively locking their funds.

NB: After finding out that the wBETH token is supported by protocol a more deeper analysis on this issue has been submitted under the H/M category, considering the wBETH token is infact blacklistable.

Impact

This method of pushing assets to msg.sender and the potential for blacklisting can lead to user funds being indefinitely stuck in the protocol. Users may lose access to their funds if they are unable to claim their withdrawals due to being blacklisted.

Consider implementing a pull method for withdrawals, allowing users to specify a recipient address when claiming withdrawals. This would prevent funds from being stuck in the protocol if users are blacklisted.

QA-05 Collateral tokens could be over/undervalued due to their not so safe dependance on external integration with Chainlink

Proof of Concept

Using this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-renzo+latestRoundData+NOT+language%3AMarkdown+NOT+language%3ATypeScript&type=code, we can see that in multiple instances, protocol queries Chainlink's latestRoundData() to get the current price, whereas this report is not attempting to show a flaw in the integration for custom oracles like the one to be set for the L2, there is a problem when this is used to price the current LSTs and any other LST that might be added in the future, considering this query is used to get the price of an asset, and this ends up in some cases deciding the amount of ezETH to be minted.

That is to say that the pricing logic requires us to query chainlink at the end of the calls, but evidently, we can see that these calls lack any check on the in-aggregator built min/max circuits, which would make protocol either overvalue or undervalue the collateral token depending on which boundary is crossed.

A little bit more on the min/max circuits is that, Chainlink price feeds aggregators have an in-built minimum & maximum prices they will return; if during a flash crash, bridge compromise, or depegging event, an asset’s value falls below the price feed’s minimum price, the oracle price feed will continue to report the (now incorrect) minimum price, so an An attacker could:

  • Have their some holding of collateral token
  • Real price of collateral token dropped very low
  • Attacker deposits these tokens to protocol since they would value it at a higher price.
  • Protocol then mints ezETH to attacker overvaluing the value of the tokens deposited.

Impact

Borderline medium/low, as this essentially breaks core functionalities, note that one of the attack bug ideas is to consider when the TVL logic could be broken and how users could mint ezETH on wrong prices.

Store the collateral token's min/max checks, reimplement the way the latestRoundData() is being queried and have direct access to the price data being returned and check if its at these boundaries and revert or alternatively integrate a fallback oracle and then use the price from this instead.

QA-06 Consider implementing a better documented max fees applied to deposits

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Deposits/DepositQueue.sol#L93-L109

   function setFeeConfig(
        address _feeAddress,
        uint256 _feeBasisPoints
    ) external onlyRestakeManagerAdmin {
        // Verify address is set if basis points are non-zero
        if (_feeBasisPoints > 0) {
            if (_feeAddress == address(0x0)) revert InvalidZeroInput();
        }

        // Verify basis points are not over 100%
        if (_feeBasisPoints > 10000) revert OverMaxBasisPoints();

        feeAddress = _feeAddress;
        feeBasisPoints = _feeBasisPoints;

        emit FeeConfigUpdated(_feeAddress, _feeBasisPoints);
    }

We can see that max allowed fee is actually 100% where as we can agree that in no case should this fee level be set this high, the possibility still exists and might have end users being a bit suspicious of protocol since there is a possibility to set the fee to 100%.

Impact

Untrustful front for users.

Considering, the new C4 rules on Admin actions, would be key to note this is also a centralization risk front as the fee can just be set to 100% by the admin.

Most renowned protocols in DEFI actually have a hardcoded max stored in their codes and this value ranges around the 20 %, this way users are sure that in no case would the fee ever be more that 20%.

QA-07 DepositQueue#sweepERC20() in some cases would be non-functional

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Deposits/DepositQueue.sol#L254-L277

    function sweepERC20(IERC20 token) external onlyERC20RewardsAdmin {
        uint256 balance = IERC20(token).balanceOf(address(this));
        if (balance > 0) {
            uint256 feeAmount = 0;

            // Sweep fees if configured
            if (feeAddress != address(0x0) && feeBasisPoints > 0) {
                feeAmount = (balance * feeBasisPoints) / 10000;
                IERC20(token).safeTransfer(feeAddress, feeAmount);

                emit ProtocolFeesPaid(token, feeAmount, feeAddress);
            }

            // Approve and deposit the rewards
            token.approve(address(restakeManager), balance - feeAmount);
            restakeManager.depositTokenRewardsFromProtocol(token, balance - feeAmount);

            // Add to the total earned
            totalEarned[address(token)] = totalEarned[address(token)] + balance - feeAmount;

            // Emit the rewards event
            emit RewardsDeposited(IERC20(address(token)), balance - feeAmount);
        }
    }

This function in short is used for the logic of sending/rescuing any token left in the DepositQueue to the RestakeManager, however the whole logic of the contract is wrapped under an if check, i.e if (balance > 0), however the way the balance is gotten is uint256 balance = IERC20(token).balanceOf(address(this));.

Now it'w somewhat a popular logic that not all renowned ERC20 tokens support the IERC20.balanceOf() and as such if a token gets sent to the DepositQueue and doesn't support the IERC20.balanceOf() any attempt at sweeping this ERC20 would revert in this line.

Impact

Protocol's core functionality is somewhat broken, since rescuing these set of tokens would now be impossible, showcasing the exact opposite of the intention behind sweepERC20() is being upheld.

Consider calling balanceOf() on a low level instead

QA-08 Depositing protocol rewards in some instances might revert due to an underflow

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Delegation/OperatorDelegator.sol#L501-L524

    receive() external payable nonReentrant {
        // check if sender contract is EigenPod. forward full withdrawal eth received
        if (msg.sender == address(eigenPod)) {
            restakeManager.depositQueue().forwardFullWithdrawalETH{ value: msg.value }();
        } else {
            // considered as protocol reward
            uint256 gasRefunded = 0;
            uint256 remainingAmount = msg.value;
            if (adminGasSpentInWei[tx.origin] > 0) {
                gasRefunded = _refundGas();
                // update the remaining amount
                remainingAmount -= gasRefunded;
                // If no funds left, return
                if (remainingAmount == 0) {
                    return;
                }
            }
            // Forward remaining balance to the deposit queue
            address destination = address(restakeManager.depositQueue());
            (bool success, ) = destination.call{ value: remainingAmount }("");
            if (!success) revert TransferFailed();

            emit RewardsForwarded(destination, remainingAmount);
        }

We can see that this is used to receive ETH in the OperatorDelegator.sol contract, now in the case where the deposit is not coming from eigenPod it's been considered as protocol rewards and an attempt is being made to pay of the adminGasSpentInWei if the value is positive, issue is that this attempt is not done in a safe way, i.e there is no check whatsoever that the remainingAmount is indeed > than the gasRefunded which would then cause the subtraction to underflow and then prompt a revert.

Impact

Protocol rewards would not be deposited.

Consider checking if gasRefunded > remainingAmount and if yes, then refund only the remainingAmount instead.

QA-09 Funds could be locked for some users if collateral to withdraw is native ETH token

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L279-L312

    function claim(uint256 withdrawRequestIndex) external nonReentrant {
        // check if provided withdrawRequest Index is valid
        if (withdrawRequestIndex >= withdrawRequests[msg.sender].length)
            revert InvalidWithdrawIndex();

        WithdrawRequest memory _withdrawRequest = withdrawRequests[msg.sender][
            withdrawRequestIndex
        ];
        if (block.timestamp - _withdrawRequest.createdAt < coolDownPeriod) revert EarlyClaim();

        // subtract value from claim reserve for claim asset
        claimReserve[_withdrawRequest.collateralToken] -= _withdrawRequest.amountToRedeem;

        // delete the withdraw request
        withdrawRequests[msg.sender][withdrawRequestIndex] = withdrawRequests[msg.sender][
            withdrawRequests[msg.sender].length - 1
        ];
        withdrawRequests[msg.sender].pop();

        // burn ezETH locked for withdraw request
        ezETH.burn(address(this), _withdrawRequest.ezETHLocked);

        // send selected redeem asset to user
        if (_withdrawRequest.collateralToken == IS_NATIVE) {
            payable(msg.sender).transfer(_withdrawRequest.amountToRedeem);//@audit using payable.transfer to send eth?
        } else {
            IERC20(_withdrawRequest.collateralToken).transfer(
                msg.sender,
                _withdrawRequest.amountToRedeem
            );
        }
        // emit the event
        emit WithdrawRequestClaimed(_withdrawRequest);
    }

As hinted by the @audit tag we can see that there is an implementation of using payable.transfer() to send ETH when claiming the withdrawals however this is heavily frowned upon cause it can lead to the locking of funds. The transfer() call requires that the recipient is either an EOA account, or is a contract that has a payable callback. For the contract case, the transfer() call only provides 2300 gas for the contract to complete its operations. This means the following cases can cause the transfer to fail:

  • The contract does not have a payable callback
  • The contract’s payable callback spends more than 2300 gas (which is only enough to emit something)
  • The contract is called through a proxy which itself uses up the 2300 gas Use OpenZeppelin’s Address.sendValue() instead Reference

Impact

Fund's lock up in some cases as the withdrawals can no longer be finalized.

Consider not using the payable.transfer() and instead send the ether via the .call() method.

QA-10 Consider wrapping all latestRoundData() calls pertaining collateral tokens in a try catch

Proof of Concept

Using this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-renzo+latestRoundData+NOT+language%3AMarkdown+NOT+language%3ATypeScript&type=code, we can see that in multiple instances, protocol queries Chainlink's latestRoundData() to get the current price, whereas this report is not attempting to show a flaw in the integration for custom oracles like the one to be set for the L2, there is a problem when this is used to price the current LSTs and any other LST that might be added in the future, considering this query is used to get the price of an asset, and this ends up in some cases deciding the amount of ezETH to be minted.

That is to say that the pricing logic requires us to query chainlink at the end of the calls, but evidently, we can see that these calls lack any error handling for the potential failure of oracle.latestRoundData(), note that Chainlink pricefeeds could revert due to whatever reason, i.e say maintenance or maybe the Chainlink team decide to change the underlying address. Now this omission of not considering this call failing would lead to systemic issues, since calls to this would now revert halting any action that requires this call to succeed.

Impact

Borderline medium/low, as this essentially breaks core functionalities like withdrawing/depositing, as during mints or redemption there is an evaluation of the worth of the collateral assets to be deposited which could lead to a complete revert

Wrap the oracle.latestRoundData() call in a try-catch block, then handle the error (e.g., revert with a specific message or use an alternative pricing method, the latter is a better fix as it ensures the protocol still functions as expected on the fallback oracle.

QA-11 Flawed TVL calculation on collateral token removal

Proof of Concept

In the removeCollateralToken function of the RestakeManager contract, collateral tokens are removed from the collateralTokens array without considering balances held by operators or the withdrawal queue. This flawed removal process affects the TVL calculation, leading to inaccurate TVL figures.

NB: After understanding that this is indeed going to be called during the lifetime of the protocol a more detailed impactful finding has been submitted on how this could lead to loss of funds for users.

Impact

The flawed TVL calculation can cause various issues across the protocol, such as inaccurate asset pricing, deposit limit enforcement failures, and incorrect operator selection for deposits.

Reimagine the collateral token removal logic to ensure that balances held by operators and the withdrawal queue are properly considered before removing a collateral token from the collateralTokens array. This will ensure that TVL calculations accurately reflect the protocol's asset holdings.

QA-12 Protocol should consider that stETH can be paused

Proof of Concept

Protocol integrates stETH as one of the core supported collateral tokens, however it's not been documented i any instance to users/developers that the stETH token can indeed be paused.

Since stETH can be paused, so all transfers and any interactions would revert. Even withdrawal requests will revert.

Impact

This could lead to sneaky bug windows, but it generally is a bug rooted with an external admin, however protocol should indicate this to integrators as this could as well mean that withdrawal requests can get processed after the documented 7 days, as the attemot is going to meet a revret when the token to be withdrawn is stETH

Consider heavily documenting this.

QA-13 Transfers currently might fail silently

Proof of Concept

Protocol in multiple instances uses the .call() method to transfer native assets to their target addresses as can be confirmed by this search command: https://github.com/search?q=repo%3Acode-423n4%2F2024-04-renzo+.call+NOT+language%3AMarkdown&type=code, however none of these attemopts at transfer include a logic to verify that the addresses are valid, i.e non-zero or atleast have code in them or check the success return value of this call, which would then mean that this call could silently fail, below is a minimalistic POC to prove this bug case

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.21;
import "hardhat/console.sol";
contract test {
constructor() {
bytes memory txData*; (bool success,) = payable(address(0)).call{ value: 0 }(txData*); console.log("success",success);
}
}

Impact

Failed transfers would be assume as not failed.

Consider introducing a valid address checker before the transfers are executed.

QA-14 First check if withdrawRequestIndex == withdrawRequests[msg.sender].length - 1 before setting the storage

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/Withdraw/WithdrawQueue.sol#L292-L297

        // delete the withdraw request
        withdrawRequests[msg.sender][withdrawRequestIndex] = withdrawRequests[msg.sender][
            withdrawRequests[msg.sender].length - 1
        ];
        withdrawRequests[msg.sender].pop();

This could be made more efficient by first checking if withdrawRequestIndex == withdrawRequests[msg.sender].length - 1 before setting the storage since it's going to get popped anyways.

Impact

Lack of efficiency in code.

Consider applying the check.

QA-15 Fix documentation

Proof of Concept

Take a look at this section of the readMe: https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/README.md#L161-L166

To submit a withdraw request:

- Calculate how much ezETH you want to burn in base units (e.g. 5000000000000000 for 0.005 ezETH)
- Call approve() on the ezETH contract to approve the RestakeManager to move your ezETH
- Call startWithdraw() on the RestakeManager with the amount of ezETH you want to burn and which collateral token you would like to get back

We can see the steps outlined for a withdrawal, however navigating to the RestakeManager.sol contract at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol we can see that there is no definition whatsoever of a startWithdraw() function which then leaves all three parties of users/developers & security researchers confused about this description.

Impact

Bad code documentation, confused integration.

Consider correctly documenting how & what the withdrawal process should entail.

QA-16 TVL logic might be faulted when considering the integrational context around it and the withdrawalQueue

Proof of Concept

Take a look at https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L274-L358

    function calculateTVLs() public view returns (uint256[][] memory, uint256[] memory, uint256) {
        uint256[][] memory operatorDelegatorTokenTVLs = new uint256[][](operatorDelegators.length);
        uint256[] memory operatorDelegatorTVLs = new uint256[](operatorDelegators.length);
        uint256 totalTVL = 0;

        // Iterate through the ODs
        uint256 odLength = operatorDelegators.length;

        // flag for withdrawal queue balance set
        bool withdrawQueueTokenBalanceRecorded = false;
        address withdrawQueue = address(depositQueue.withdrawQueue());

        // withdrawalQueue total value
        uint256 totalWithdrawalQueueValue = 0;

        for (uint256 i = 0; i < odLength; ) {
            // Track the TVL for this OD
            uint256 operatorTVL = 0;

            // Track the individual token TVLs for this OD - native ETH will be last item in the array
            uint256[] memory operatorValues = new uint256[](collateralTokens.length + 1);
            operatorDelegatorTokenTVLs[i] = operatorValues;

            // Iterate through the tokens and get the value of each
            uint256 tokenLength = collateralTokens.length;
            for (uint256 j = 0; j < tokenLength; ) {
                // Get the value of this token

                uint256 operatorBalance = operatorDelegators[i].getTokenBalanceFromStrategy(
                    collateralTokens[j]
                );

                // Set the value in the array for this OD
                operatorValues[j] = renzoOracle.lookupTokenValue(
                    collateralTokens[j],
                    operatorBalance
                );

                // Add it to the total TVL for this OD
                operatorTVL += operatorValues[j];

                // record token value of withdraw queue
                if (!withdrawQueueTokenBalanceRecorded) {
                    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
                        collateralTokens[i],
                        collateralTokens[j].balanceOf(withdrawQueue)
                    );
                }

                unchecked {
                    ++j;
                }
            }

            // Get the value of native ETH staked for the OD
            uint256 operatorEthBalance = operatorDelegators[i].getStakedETHBalance();

            // Save it to the array for the OD
            operatorValues[operatorValues.length - 1] = operatorEthBalance;

            // Add it to the total TVL for this OD
            operatorTVL += operatorEthBalance;

            // Add it to the total TVL for the protocol
            totalTVL += operatorTVL;

            // Save the TVL for this OD
            operatorDelegatorTVLs[i] = operatorTVL;

            // Set withdrawQueueTokenBalanceRecorded flag to true
            withdrawQueueTokenBalanceRecorded = true;

            unchecked {
                ++i;
            }
        }

        // Get the value of native ETH held in the deposit queue and add it to the total TVL
        totalTVL += address(depositQueue).balance;

        // Add native ETH help in withdraw Queue and totalWithdrawalQueueValue to totalTVL
        totalTVL += (address(withdrawQueue).balance + totalWithdrawalQueueValue);

        return (operatorDelegatorTokenTVLs, operatorDelegatorTVLs, totalTVL);
    }

We can see that while calculating the current TVL the ETH present in the withdrawal queue is also taken into account, however this logic seems to be flawed, cause any asset that's currently present in the withdrawal queue is due to a withdrawal request that has been passed on and as such the balance should not count in protocol as within the cooldown period these locked ETH would be claimed by the users who passed the request.

Impact

A somewhat flawed calculation on the current TVL in protocol, submitting as QA, as one can argue this is a feature not a bug, however the attempt at querying the TVL could return inflated amount when heavy withdrawal requests have been queued.

Keep in mind that this function inherently gets called when there is a need to price the assets to be withdrawn or to get the rate from the BalancerRateProvider, i.e:

  • Withdrawal attempts could now returned flawed data for the amount of collateral to send the user due to having an inflated data returned as the TVL from WithdrawQueue.withdraw().
  • The BalancerRateProvider.getRate() is also heavily used across protocol, with implementations in the xRenzoBridge which would then mean that functionalities that need price to be sent to the CCIP destination via xRenzoBridge.sendPrice() would all be concluding o potentially faulty price due to using an inflated TVL in the calculation.

Consider reimplementing the logic to have the right amount of TVL used in the calculations.

#0 - CloudEllie

2024-05-13T14:05:12Z

#1 - c4-judge

2024-05-24T09:47:16Z

alcueca marked the issue as grade-a

#2 - Bauchibred

2024-05-28T11:22:27Z

Hi @alcueca, apologies for commenting late, however there is a new group of finding that would be validated as a high in regards to claiming of funds not being finalized based on your discussion from here: https://github.com/code-423n4/2024-04-renzo-validation/issues/980#issuecomment-2134731647, i.e due to the usage of payable.transfer().

I'd like to indicate that QA-09 from this report is a duplicate of the issue and would appreciate if it could get upgraded.

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter