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
Rank: 3/122
Findings: 7
Award: $4,459.97
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: LessDupes
Also found by: Bauchibred, grearlake
2675.4886 USDC - $2,675.49
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
🌟 Selected for report: guhu95
Also found by: 0rpse, 0x007, 0x73696d616f, 0xCiphky, 0xabhay, Audinarey, Bauchibred, Fassi_Security, GalloDaSballo, GoatedAudits, KupiaSec, LessDupes, MSaptarshi, OMEN, Ocean_Sky, RamenPeople, SBSecurity, Tendency, WildSniper, aslanbek, bill, blutorque, crypticdefense, cu5t0mpeo, d3e4, gjaldon, grearlake, gumgumzum, honey-k12, ilchovski, jokr, josephdara, kennedy1030, p0wd3r, peanuts, stonejiajia, t0x1c, tapir, underdog, zzykxx
0.4071 USDC - $0.41
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) :
TVL
.wBETH
&stETH
wBETH
and the other 50 deposited stETH
.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.
~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.
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:
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.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
🌟 Selected for report: guhu95
Also found by: 0rpse, 0x007, 0x73696d616f, 0xCiphky, 0xabhay, Audinarey, Bauchibred, Fassi_Security, GalloDaSballo, GoatedAudits, KupiaSec, LessDupes, MSaptarshi, OMEN, Ocean_Sky, RamenPeople, SBSecurity, Tendency, WildSniper, aslanbek, bill, blutorque, crypticdefense, cu5t0mpeo, d3e4, gjaldon, grearlake, gumgumzum, honey-k12, ilchovski, jokr, josephdara, kennedy1030, p0wd3r, peanuts, stonejiajia, t0x1c, tapir, underdog, zzykxx
0.4071 USDC - $0.41
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
It's a known fact that validators can indeed lose part of their deposit via penalties or slashing events:
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:
ezETH
held.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.
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.
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
🌟 Selected for report: guhu95
Also found by: Bauchibred, LessDupes, ilchovski, zzykxx
347.9473 USDC - $347.95
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
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.
WithdrawQueue.withdraw()
.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.
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.
🌟 Selected for report: t0x1c
Also found by: 0xCiphky, 0xDemon, Bauchibred, DanielArmstrong, FastChecker, MSaptarshi, Maroutis, NentoR, Ocean_Sky, PNS, Rhaydden, SBSecurity, Shaheen, Tigerfrake, ZanyBonzy, atoko, btk, carlitox477, crypticdefense, honey-k12, hunter_w3b, ilchovski, jokr, ladboy233, rbserver, twcctop, umarkhatab_465
1.479 USDC - $1.48
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.
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.
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)
🌟 Selected for report: LessDupes
Also found by: Bauchibred
1416.4352 USDC - $1,416.44
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
18.1958 USDC - $18.20
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
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
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:
TVL
from the call to RestakeManger.calculateTVLs()
in WithdrawQueue.withdraw()
... from the calculation done to get the amount to redeem in the RenzoOracle
we can see that the totalTVL
has a direct impact on the amount being sent to the user and as such the RestakeManger.calculateTVLs()
returning a deflated totalTVL
would mean the users would receive less for their assets as the final amount of asset they get sent is also going to be deflated, essentially proving the unfair loss of funds for users.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 with faulty price due to using a deflated TVL in the calculation of getting the rates... (different bug ideas can come up from this, just as is explained above).deposit caps
and when these are set it's considered an invariant for the contract to revert on any deposits if that deposit would push the totalTVL
above the max TVL, however as explained in this report the max TVL
is not going to be properly enforced cause as seen below, the check implemented during deposit attempts for the max TVL
is going to be made with the deflated totalTVL
causing the transaction to be accepted where as it shouldn't be successful since the deposit cap has been reached and users need to first claim up all the removed collateral asset from the OD and withdrawal queue before it should be considered reflected in the check, i.e https://github.com/code-423n4/2024-04-renzo/blob/1c7cc4e632564349b204b4b5e5f494c9b0bc631d/contracts/RestakeManager.sol#L594-L600(, , uint256 totalTVL) = calculateTVLs(); // Enforce TVL limit if set if (maxDepositTVL != 0 && totalTVL + msg.value > maxDepositTVL) { revert MaxTVLReached(); }
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.
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.
- If a collateral token gets removed, protocol's TVL logic automatically gets heavily flawed #5 assumes that the admin would carelessly remove a collateral token which is also a reckless mistake. This protocol holds 3B$ in TVL and admins know very well that mint/redeem calculations rely heavily on collateral tokens.
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:
removeCollateralToken()
function, suggesting that there are valid cases where a collateral token may need to be removed.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
18.1958 USDC - $18.20
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
🌟 Selected for report: Sathish9098
Also found by: 0x73696d616f, 0xCiphky, 0xmystery, ABAIKUNANBAEV, Bauchibred, BiasedMerc, Fassi_Security, FastChecker, GalloDaSballo, GoatedAudits, K42, KupiaSec, LessDupes, Limbooo, ReadyPlayer2, Rhaydden, SBSecurity, Sabit, Sparrow, WildSniper, ZanyBonzy, adam-idarrha, adeolu, araj, aslanbek, atoko, b0g0, carlitox477, crypticdefense, fyamf, gesha17, gjaldon, grearlake, gumgumzum, hihen, honey-k12, hunter_w3b, inzinko, jesjupyter, jokr, kennedy1030, kind0dev, kinda_very_good, ladboy233, lanrebayode77, oakcobalt, oualidpro, pauliax, rbserver, t0x1c, tapir, underdog, xg, zzykxx
0 USDC - $0.00
Issue ID | Description |
---|---|
QA-01 | Renzo currently misses out on Blast yields and redirected rewards from the sequencer |
QA-02 | Balancer pools are going to use the wrong rates |
QA-03 | The owner is allowed to set any price |
QA-04 | Push method for withdrawals is generally frowned upon |
QA-05 | Collateral tokens could be over/undervalued due to their not so safe dependance on external integration with Chainlink |
QA-06 | Consider implementing a better documented max fees applied to deposits |
QA-07 | DepositQueue#sweepERC20() in some cases would be non-functional |
QA-08 | Depositing protocol rewards in some instances might revert due to an underflow |
QA-09 | Funds could be locked for some users if collateral to withdraw is native ETH token |
QA-10 | Consider wrapping all latestRoundData() calls pertaining collateral tokens in a try catch |
QA-11 | Flawed TVL calculation on collateral token removal |
QA-12 | Protocol should consider that stETH can be paused |
QA-13 | Transfers currently might fail silently |
QA-14 | First check if withdrawRequestIndex == withdrawRequests[msg.sender].length - 1 before setting the storage |
QA-15 | Fix documentation |
QA-16 | TVL logic might be faulted when considering the integrational context around it and the withdrawalQueue |
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.
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.
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
/** * @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.
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.
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
Malicious admin could break the minting logic
Consider clearly indicating his is possible in the docs and to users
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 thewBETH
token is infact blacklistable.
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.
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:
ezETH
to attacker overvaluing the value of the tokens deposited.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.
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%
.
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%
.
DepositQueue#sweepERC20()
in some cases would be non-functionalTake 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.
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
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.
Protocol rewards would not be deposited.
Consider checking if gasRefunded > remainingAmount
and if yes, then refund only the remainingAmount
instead.
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:
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.
latestRoundData()
calls pertaining collateral tokens in a try catchUsing 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.
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.
TVL
calculation on collateral token removalIn 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.
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.
stETH
can be pausedProtocol 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.
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.
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); } }
Failed transfers would be assume as not failed.
Consider introducing a valid address checker before the transfers are executed.
withdrawRequestIndex
== withdrawRequests[msg.sender].length - 1
before setting the storage// 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.
Lack of efficiency in code.
Consider applying the check.
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.
Bad code documentation, confused integration.
Consider correctly documenting how & what the withdrawal process should entail.
withdrawalQueue
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.
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:
TVL
from WithdrawQueue.withdraw()
.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.