Renzo - zzykxx's results

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Delegation/OperatorDelegator.sol#L501

Vulnerability details

It's impossible to withdraw ETH from Eigenlayer because OperatorDelegator::completeQueuedWithdrawal() will always revert due to nonReentrant modifiers.

When completing a withdrawal for ETH the function OperatorDelegator::completeQueuedWithdrawal() needs to be called, which will internally perform the following calls:

1. DelegationManager::completeQueuedWithdrawal()
2. EigenPodManager::withdrawSharesAsTokens()
3. EigenPod::withdrawRestakedBeaconChainETH()
4. OperatorDelegator::receive()

The call flow starts and ends in the same contract: OperatorDelegator. Both the functions OperatorDelegator::completeQueuedWithdrawal() and OperatorDelegator::receive() have a nonReentrant modifier, which will make the call revert.

Impact

ETH can't be withdrawn from Eigenlayer to Renzo, making it impossible for stakers to exchange their ezETH for ETH.

Proof of Concept

It's possible to verify this by following the flow outlined above. Note that the function EigenPod::withdrawRestakedBeaconChainETH()(4) transfers ETH to the EigenPod owner, which is the OperatorDelegator of which the receive() function will be triggered.

Recommended Mitigation Steps

Remove the nonReentrant modifer from the OperatorDelegator::receive() function.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L299 https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L303

Vulnerability details

Executing WithdrawQueue::claim() to complete the withdrawal process will almost always lead to instant changes in ezETH valuation.

Redeeming ezETH for underlying assets involves two steps:

1. User calls WithdrawQueue::withdraw(), which:
   1. Transfers ezETH from the caller to the contract
   2. Caches the amount of assets to redeem based on the current ezETH valuation
2. After a delay, the user can call WithdrawQueue::claim(), which:
   1. Burns the amount of ezETH previously transferred
   2. Transfers to the caller the amount of assets to redeem cached during step 1

Because there is an uncapped delay in between step 1 and step 2, when step 2 is executed the amount of ezETH burned might not be equivalent to the assets transferred out anymore, which will cause an instant change in the ezETH valuation.

Impact

The valuation of ezETH should not increase (or decrease) instantly because this is not a situation where the protocol is receiving rewards (or penalties).

The instant increase of ezETH value can be abused by an attacker by depositing ETH in the protocol via RestakeManager::depositETH() before a call to WithdrawQueue::claim() and withdrawing after the call.

Important to note that the delay between a call to withdraw() and claim() is uncapped and can be very long.

Proof of Concept

Let's assume the protocol TVL is 2 ETH and there are a total of 2 ezETH in circulation, which implies 1 ezETH is valued at 1 ETH.

1. Alice calls WithdrawQueue::withdraw() to redeem 1 ezETH, the protocol caches the current equivalent amount to be redeemed, 1 ETH.
2. In between the calls the TVL of the protocol increases while the amount of ezETH in circulation stays constant, due to rewards. Let's assume the TVL doubled and is now 4 ETH (unrealistic, but simple math).
3. Alice calls WithdrawQueue::claim(), which burns 1 ezETH and transfers out 1 ETH.

Before step 3 was executed the protocol state was:

• 4 ETH TVL
• 2 ezETH in circulation
• 1 ezETH = 2 ETH

After step 3 is executed the protocol state is:

• 3 ETH TVL
• 1 ezETH in circulation
• 1 ezETH = 3 ETH

The valuation of ezETH instantly increased by 1ETH after claim() was executed.

Recommended Mitigation Steps

When WithdrawQueue::withdraw() is executed the protocol should:

1. Burn the transferred ezETH tokens
2. Don't account for the amount of assets that will be redeemed as part of the TVL. They can be sent to an external contract that is responsible to keep the funds until they are claimed via WithdrawQueue::claim().

If this is implemented calls to WithdrawQueue::claim() will have no impact on the ezETH valuation, because both the amount of ezETH and the TVL won't change after the call.

Assessed type

Other

Lines of code

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

Vulnerability details

The way the protocol is designed allows an attacker to take advantage of a sudden TVL increase. In Renzo a sudden TVL increase might happen for multiple reasons:

• A validator receives rewards and a partial withdrawal is triggered. Renzo is notified of the profit only when DelayedWithdrawalRouter::claimDelayedWithdrawals() gets called, which will instantly increase the TVL.
• A call to DepositQueue::sweepERC20(). The function transfers tokens from the DepositQueue, whose tokens are not accouted for in the protocol TVL, and deposits them in the protocol via the RestakeManager without minting extra ezETH tokens.
• The protocol receives execution layer / MEV rewards.
• Oracle updates. Chainlink price feeds are updated only when a specified "deviation threshold" is reached, or an amount of time defined as "heartbeat" is passed. Deviation thresholds can be up to ~2%, meaning the protocol could see a 2% instant valuation increase of an asset (and TVL as a consequence) as soon as the price for an asset is updated. Depending on the amount of assets held in the protocol, a 2% could be a meaningful amount.
• RestakeManager::addOperatorDelegator() is called to add an operator delegator that already holds some supported asset shares.

An user can exploit this for profit by minting ezETH tokens before the TVL increases, by doing the following:

1. Deposit an accepted asset to mint ezETH tokens via RestakeManager::deposit()
2. The TVL of the protocol gets updated and instantly increases, making the attacker ezETH tokens worth more.
3. Withdraw the ezETH tokens via WithdrawQueue::withdraw().
4. Wait a delay and complete the withdrawal via WithdrawQueue::claim().

Impact

An attacker can take advantage of a suddent TVL increase to capture extra profit, which leads to fair users earning less than they should.

Proof of Concept

Alice notices there are validator rewards in an EigenPod, and the rewards can be transferred to Renzo:

1. Alice mints ezETH via RestakeManager::deposit().
2. Alice calls DelayedWithdrawalRouter::claimDelayedWithdrawals(), which instantly increases the TVL.
3. Alice ezETH are now valued more, she calls WithdrawQueue::withdraw() to schedule a future withdrawal of her ezETH.
4. After a delay Alice calls WithdrawQueue::claim() to transfer the ETH/tokens to herself.

1, 2 and 3 can be performed atomically. 4 enforces a delay.

Recommended Mitigation Steps

Sudden TVL increases are unavoidable because at some point rewards have to be added to the TVL, and oracles updates are discrete. Similar systems generally use a deposit queue. Users should deposit their assets (ex. ETH) first, and be able to claim their ezETH after a delay. The amount of ezETH to mint should be calculated at claim time.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Withdraw/WithdrawQueue.sol#L220-L224

Vulnerability details

The way the protocol is designed allows ezETH holders to avoid incurring into losses when the protocol TVL has a instant decrease. In Renzo a sudden TVL decrease might happen for multiple (mostly unavoidable) reasons:

• A validator gets slashed. Renzo will be notified of the loss when EigenPod::verifyAndProcessWithdrawals() is executed, which will instantly lower the TVL.
• A validator receives penalties. Renzo will be notified of the loss when EigenPod::verifyBalanceUpdates() is called, which will instantly lower the TVL.
• One of the supported LSDs (wbETH, mETH, etc.) has a sudden value drop, this can happen for penalties, slashings or exploits on the LSD side.
• Oracle updates. Chainlink price feeds are updated only when a specified "deviation threshold" is reached, or an amount of time defined as "heartbeat" is passed. Deviation thresholds can be up to ~2%, meaning the protocol could see a 2% instant valuation loss of an asset (and TVL as a consequence) as soon as the price for that asset is updated. Depending on the amount of assets held in the protocol, a 2% could be a meaningful amount in absolute terms.
• RestakeManager::removeOperatorDelegator() is called to remove an operator delegator that still holds some shares of a supported asset.

To avoid the loss, a staker can withdraw his ezETH via WithdrawQueue::withdraw() before the loss is reflected in the protocol TVL, this is possible because WithdrawQueue::withdraw() calculates and caches the amount of ETH/Tokens to redeem based on the current value of the ezETH, which doesn't account for losses yet:

...SNIP...
@> uint256 amountToRedeem = renzoOracle.calculateRedeemAmount(
   _amount,
   ezETH.totalSupply(),
   totalTVL
);
...SNIP...
// add withdraw request for msg.sender
withdrawRequests[msg.sender].push(
    WithdrawRequest(
        _assetOut,
        withdrawRequestNonce,
@>      amountToRedeem,
        _amount,
        block.timestamp
    )
);
...SNIP...

After a time delay, the staker can call WithdrawQueue::claim() to claim the amount of assets calculated during the execution of WithdrawQueue::withdraw(), which is unaffected by the losses.

Important to note that, in case of penalties/slashings of validators controlled by Renzo both functions used to notify Renzo of the loss (EigenPod::verifyAndProcessWithdrawals() and/or EigenPod::verifyBalanceUpdates()) are permissionless, which allows a staker to avoid the loss by executing the calls atomically.

Impact

Stakers can avoid being affected by instant TVL drops, this will also make fair users incur into more losses than they should.

Proof of Concept

Alice, a staker in the Renzo protocol, is monitoring the beacon chain and notices a validator is being slashed:

1. Alice calls WithdrawQueue::withdraw() to withdraw all of her ezETH
2. Alice (or a third party) calls EigenPod::verifyAndProcessWithdrawals(), Renzo TVL drops instantly.
3. Alice calls WithdrawQueue::claim() and redeems her ezETH, being unaffected by the slashing

Recommended Mitigation Steps

The amount of ETH/Tokens to be claimed should be calculated in WithdrawQueue::claim() instead of WithdrawQueue::withdraw(). This way the funds that are currently queued for withdrawals will be still subject to losses.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/Bridge/L1/xRenzoBridge.sol#L139

Vulnerability details

The protocol allows to deposit ETH/WETH (or the specific chain native currency) on a supported L2 in order to mint ezETH tokens, this is the process:

1. User mints xezETH on L2s via xRenzoDeposit::deposit() in exchange for either ETH or WETH. The xezETH are minted based on the current ezETH valuation.
2. After some time the bridge sweepers transfer the ETH/WETH collected to the L1, via xRenzoDeposit::sweep(). The funds are transferred to the xRenzoBridge contract on L1 via Connext.
3. Connext calls xRenzoBridge::xReceive() on L1 which will receive the ETH/WETH and deposit them in the protocol via RestakeManager::depositETH(). This will mint ezETH tokens based on the current ezETH valuation, the ezETH tokens will then be locked in the lockbox in exchange for xezETH, which are then immediately burned because an equivalent amount should have already been minted on the L2 during step 1.

Theoretically the amount of xezETH tokens minted during step 1 should be the same as the amount of ezETH tokens minted during step 3, but because on both steps the tokens are minted at the current valuation, and the valuation changes over time, there will be a discrepancy between the amount of xezETH and ezETH tokens in circulation.

This is an issue because XERC20Lockbox::withdraw() always exchanges xezETH for ezETH 1:1.

Impact

The price of ezETH is expected to increase over time. This will create a situation where there will be more xezETH in circulation than ezETH, rendering some xezETH worthless and impossible to redeem for ezETH.

A situation in which the ezETH valuation decreases instead of increasing is also problematic, because the protocol will mint less xezETH than it should.

The discrepancy will become bigger and bigger with time.

Proof of Concept

As an example, let's assume ezETH valuation is currently 1 ETH:

1. Alice deposits 1 ETH on L2 and receives 1 xezETH
2. Some time passes and the valuation of ezETH increases to 2ETH (unrealistic, but allows simple math)
3. The bridge sweeper calls sweep(), transferring the 1 ETH to the xRenzoBridge contract on L1
4. The xRenzoBridge deposits the received 1 ETH in the RestakeManager, which mints 0.5 ezETH because the ezETH valuation doubled
5. The 0.5 ezETH is locked in the lockbox, 0.5 xezETH are minted in return and immediately burned

The situation now is the following:

• Alice has 1 xezETH on L2
• The lockbox contains 0.5 ezETH on L1

Alice can at best redeem 0.5 xezETH in exchange for 0.5 ezETH, and the remaning 0.5 xezETH is worthless. The amount of value she can redeem is the correct one, (1 ETH), but now there are xezETH in circulation that are not backed by ezETH.

Recommended Mitigation Steps

The protocol should track the amount of xezETH tokens minted via xRenzoDeposit::deposit() on the L2 and mint the equivalent amount of ezETH tokens on L1 when xRenzoBridge::xReceive() is executed by passing the necessary data on the xcall() to Connext.

If this is implemented it's possible for the valuation of ezETH tokens to change instantly after xRenzoBridge::xReceive() is executed, this is because the amount of ezETH tokens is not minted based on the current valuation anymore. As explained in other reports, the protocol will be subject to instant ezETH valuation changes (increase/decrease) no matter what (rewards, slashing, penalties), and should gracefully handle these situations via appropriate deposit and withdrawals queues.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L318

Vulnerability details

RestakeManager::calculateTVLs() calculates the value of the tokens held in the protocol by, among other things, calculating the value of all tokens held in the WithdrawQueue contract and summing them.

This calculation is performed in a nested loop, but the indices used to calculate the values are incorrect. The function uses the index i instead of j:

if (!withdrawQueueTokenBalanceRecorded) {
    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
@>      collateralTokens[i],
        collateralTokens[j].balanceOf(withdrawQueue)
    );
}

It calculates the value of the tokens in the WithdrawQueue based on the value of collateralTokens[i] instead of collateralTokens[j].

In this case i will always be 0, the code it's only executed once because of the if statement. Depending on the valuation of the token collateralTokens[0] and the amount of tokens collateralTokens[j] in the WithdrawQueue contract the TVL calculation could end up either being higher or lower than expected.

Impact

The TVL will be calculated incorrectly. An attacker can move funds in/out from/to the WithdrawQueue in order to manipulate the TVL for profit.

Proof of Concept

Let's suppose:

• collateralTokens[0] is valued at 1000$
• collateralTokens[j] is valued at 990$

If an attacker transfers 5 collateralTokens[j] to the WithdrawQueue, which is worth 5 * $990 = $4950, the TVL will increase by 5 * 1000$ = 5000$, an overvaluation of $50. An attacker can move tokens in such a way to overvalue the TVL before withdrawing and undervalue the TVL before depositing, which can be achieved either by withdrawing tokens and/or by depositing tokens if the withdrawal buffer needs to be filled.

Recommended Mitigation Steps

At RestakeManager.sol#L318 use j index:

    totalWithdrawalQueueValue += renzoOracle.lookupTokenValue(
@>      collateralTokens[j],
        collateralTokens[j].balanceOf(withdrawQueue)
    );

Assessed type

Error

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

[9] Adding too many operator delegators and/or collateral tokens could DOS the protocol

Lines of code

https://github.com/code-423n4/2024-04-renzo/blob/main/contracts/RestakeManager.sol#L558-L562

Vulnerability details

The function RestakeManager::deposit() checks if the WithdrawQueue has a buffer deficit, and sends tokens to it when necessary:

if (bufferToFill > 0) {
    bufferToFill = (_amount <= bufferToFill) ? _amount : bufferToFill;
    // update amount to send to the operator Delegator
    _amount -= bufferToFill;

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

    // fill Withdraw Buffer via depositQueue
    depositQueue.fillERC20withdrawBuffer(address(_collateralToken), bufferToFill);
}

When the WithdrawQueue buffer deficit is bigger (or equal) to the amount of assets being deposited all of the deposit will be transferred to the WithdrawQueue and because there are no assets left, the variable _amount will be set to 0.

After this the function tries to deposit 0 tokens in the OperatorDelegator:

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

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

but the call to OperatorDelegator::deposit() will revert when the amount that is being deposited is 0:

if (address(tokenStrategyMapping[token]) == address(0x0) || tokenAmount == 0)
    revert InvalidZeroInput();

Impact

It's impossible to deposit token amounts that are smaller or equal to the current WithdrawQueue buffer deficit.

Recommended Mitigation Steps

Don't perform the deposit to the operator delegator when the amount to be deposited is 0:

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

RestakeManager.sol#L558-L562

Keep in mind that deposits being 0 is not the only issue here, small amounts of deposit might also revert on Eigenlayer side if the amount of shares minted from the deposit are 0.

Assessed type

DoS

[1] Malicious operator delegator can manipulayte TVL by unstaking from Eigenlayer

Operators can undelegate shares delegated to them via DelegationManager::undelegate() in Eigenlayer. An operator can undelegate all of the shares delegate to him by Renzo OperatorDelegator(s), which will cause an instant drop in the TVL, potentially causing damage.

[2] Oracles safety mechanisms might brick the system if a big legitimate price swing happens

There are two instances of this:

• RenzoOracleL2::getMintRate() always reverts if the reported price ezETH price is less than 1 ether. Altought unlikely, it's possible for the price of ezETH to drop below 1 ether. If this happens, prices on L2s won't be able to be reported anymore, and minting of xezETH tokens will be bricked.

• xRenzoDeposit::_updatePrice() always reverts if the reported price change differs by at 10% from the previous one. Altought unlikely, it's possible for the price of ezETH to swing 10%. A big slashing event might cause a 10% drop, if this happens prices on L2 will become (and stay) stale until the ezETH goes back within a 10% range of the last reported price.

Introduce admin functionality to adjust the oracle contraints instead of using hardcoded values.

[3] Lack of proper functionality to withdraw router fees from xRenzoDeposit

xRenzoDeposit collects router fees and leaves them in the contract in order to be withdrawn later. The system doesn't track the amount of router fees that are being collected and the only way to withdraw them is via xRenzoDeposit::recoverERC20.

Introduce a proper router fees accouting mechanism, and funcionality to withdraw router fees.

[4] EnumerableSetUpgradeable return values are never checked

The openzeppelin EnumerableSetUpgradeable library doesn't revert when trying to add an item that already exists to a set, but returns false instead. The contract XERC20Factory uses the library in two instances, both times without ensuring the returned value is true:

• When deploying a XERC20 token
• When deploying a lockbox

Require the returned value to be true and revert if this is not the case.

[5] stakeEthFromQueue() allows to deposit twice in the same validator

DepositQueue::stakeEthFromQueue() is admin callable function used to take ETH from the DepositQueue and deposit them into an OperatorDelegator's validator. The OperatorDelegator to deposit to is passed by an admin as an input parameter, but the function never checks if the selected OperatorDelegator validator has already been deposited into.

If an OperatorDelegator validator gets two deposits, the extra 32 ETH will be queued as a partial withdrawal by the beacon chain, causing the Renzo protocol to collect the "rewards" fee over it, causing a loss to users.

In DepositQueue::stakeEthFromQueue() ensure the selected validator has never been deposited into before.

[6] approve used instead of safeApprove in sweepERC20()

In DepositQueue::sweepERC20() approvals are given via approve(). This might lead to the function reverting in case the asset being transferred is not fully ERC20 compliant.

Use safeApprove() instead.

[7] RenzoOracle always uses ~24 hours to ensure a price feed is not expired

The functions:

• RenzoOracle::lookupTokenValue()
• RenzoOracle::lookupTokenAmountFromValue()

Both reverts if the last price update for the given token was performed more than MAX_TIME_WINDOW (24 hours + 60 seconds) seconds before the function execution. Different chainlink price feeds can have different heartbeat updates. For instance a chainlink price feed with an heartbeat of 1 hour should be considered stale after 1 hour passed, instead of 24 hours.

Introduce a mapping that uses the correct heartbeat for each supported chainlink price feed to ensure the price is not stale.

[8] Adding/removing operator delegators might cause instant TVL increase/decrease

The functions RestakeManager::addOperatorDelegator() and RestakeManager::removeOperatorDelegator() allow to add and remove operator delegators to Renzo.

Both functions don't ensure that operators added and removed are currently holding no shares. If an operator delegator that is currently holding shares is added, the TVL will instantly increase. If an operator delegator that is currently holding shares is removed, the TVL will instantly decrease.

Both functions should ensure that the operator delegators that are being added/removed should not hold shares. This is especially true for operator delegators being added. Operator delegator being removed might need to be removed no-matter-what for being deemed malicious.

[9] Adding too many operator delegators and/or collateral tokens could DOS the protocol

A critical component of the protocol is the TVL calculation, which is perfomed in multiple instances:

• RestakeManager::depositETH()
• RestakeManager::deposit()
• RestakeManager::depositTokenRewardsFromProtocol()
• WithdrawQueue::withdraw()
• BalancerRateProvider::getRate()

To perform the calculations, the protocol loops over all of the operator delegators, and for each operator delegator it loops over all of the collateral tokens. Both the amount of collateral tokens and operator delegators that can be added to the protocol is unbounded. This could cause the TVL calculation to run out of gas, DOSing all of the protocol operations.

• In RestakeManager::addOperatorDelegator() revert if too many operator delegators are already in the protocol.
• In RestakeManager::addCollateralToken() revert if too many operator delegators are already being used the protocol.

To understand what the maximum caps should be it's necessary to perform some tests to understand the gas usage of the protocol.

[10] The sum of operator delegators TVL can be more/less than 100%

RestakeManager::setOperatorDelegatorAllocation() allows to set the percentage of TVL that an operator delegator is expected to manage. The function never ensures the sum of the TVL allocations is 100%:

If the sum of TVLs allocation is not 100% (either higher or lower), the extra TVL will always be allocated to the first operator in the operatorDelegators array, which is unfavourable to other operator delegators.

Change RestakeManager::setOperatorDelegatorAllocation() into a function that accepts as input an array of uint256, which should be the same lenght as the current amount of operator delegators in the protocol. Each value in the array represents the TVL percentage to be allocated to the respective operator delegator. The function should ensure the sum of these values to always be 100%.

[11] The ReentrancyGuardUpgradeable contract is not initialized in WithdrawQueue

The function WithdrawQueue::initialize() doesn't initialize the ReentrancyGuardUpgradeable.

Add the following to the function:

__ReentrancyGuard_init();

[12] Submitting two claim requests might lead to unexpected results

Withdrawals requests are tracked via an array, withdrawRequests. The function WithdrawQueue::claim() accepts as input the position in the array of the withdrawal request to be claimed. When the claim is perfomed the corresponding element in the array is deleted by swapping it with the last element, and then popping the array.

This might lead to unexpected results when two claims are perfomed at about the same time. It's impossible to predict which transaction will be executed first, and once a transaction is executed the elements in the array are swapped around, leading to the second transaction potentially claiming a different withdrawal than the intended one.