search

sherlock-audit / 2024-02-rio-network-core-protocol-judging

4 stars 4 forks source link

monrel - ETH withdrawers do not earn yield while waiting for a withdrawal #370

Open sherlock-admin2 opened 8 months ago

sherlock-admin2 commented 8 months ago

monrel

high

ETH withdrawers do not earn yield while waiting for a withdrawal

Summary

In the Rio doc we can read the following

"Users will continue to earn yield as they wait for their withdrawal request to be processed."

This is not true for withdrawals in ETH since they will simply receive an equivalent to the sharesOWed calculated when requesting a withdrawal.

Vulnerability Detail

When requestWithdrawal() is called to withdraw ETH sharesOwed is calculated

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTCoordinator.sol#L101

sharesOwed = convertToSharesFromRestakingTokens(asset, amountIn);

The total sharesOwed in ETH is added to epcohWithdrawals.assetsReceived if we settle with settleCurrentEpoch() or settleEpochFromEigenlayer()

Below are the places where assetsReceived is is set and accumulated

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L194

epochWithdrawals.assetsReceived = SafeCast.toUint120(assetsReceived);

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L162

epochWithdrawals.assetsReceived = SafeCast.toUint120(assetsReceived);

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L268

epochWithdrawals.assetsReceived += SafeCast.toUint120(assetsReceived);

when claiming rewards this is used to calculate users share

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L104

amountOut = userSummary.sharesOwed.mulDiv(epochWithdrawals.assetsReceived, epochWithdrawals.sharesOwed);

The portion of staking rewards accumulated during withdrawal that belongs to LRT holders is never accounted for so withdrawing users do not earn any rewards when waiting for a withdrawal to be completed.

Impact

Since a portion of the staking reward belongs to the LRT holders and since the docs mentions that yield is accumulated while in the queue It is fair to assume that withdrawing users have a proportional claim to the yield.

As shown above this is not true, users withdrawing in ETH do no earn any rewards when withdrawing.

Code Snippet

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L268

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L194

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L162

https://github.com/sherlock-audit/2024-02-rio-network-core-protocol/blob/4f01e065c1ed346875cf5b05d2b43e0bcdb4c849/rio-sherlock-audit/contracts/restaking/RioLRTWithdrawalQueue.sol#L104

Tool used

Manual Review

Recommendation

Account for the accumulate rewards during the withdrawal period that belongs to the deposit pool. This can be calculated based on data in DelayedWithdrawalRouter on Eigenlayer.

0xmonrel commented 8 months ago

Escalate

I will argue that this should be a separate High issue

This is not a duplicate of #109. This is an entirely different issue. What I show here is that ETH is the only asset that does not earn yield during withdrawal. The documentation clearly states that users earn yield when withdrawing:

From Rio doc "Users will continue to earn yield as they wait for their withdrawal request to be processed."

We should also consider that the time to withdrawal ETH can be longer than the EigenLayer withdrawal period depending on how many other validators are exiting.

Here is the time depending on how many validators are exiting

SCR-20240327-qzov source

If there is a large outflow of exiting validators it could take weeks to even receive the withdrawal status.

Every single user that withdrawals in ETH lose the yield that they are promised and would have received if they had withdrawn in another asset.

I believe this fulfills the following criteria for a High "Definite loss of funds without (extensive) limitations of external conditions."

sherlock-admin2 commented 8 months ago

Escalate

I will argue that this should be a separate High issue

This is not a duplicate of #109. This is an entirely different issue. What I show here is that ETH is the only asset that does not earn yield during withdrawal. The documentation clearly states that users earn yield when withdrawing:

From Rio doc "Users will continue to earn yield as they wait for their withdrawal request to be processed."

We should also consider that the time to withdrawal ETH can be longer than the EigenLayer withdrawal period depending on how many other validators are exiting.

Here is the time depending on how many validators are exiting

SCR-20240327-qzov source

If there is a large outflow of exiting validators it could take weeks to even receive the withdrawal status.

Every single user that withdrawals in ETH lose the yield that they are promised and would have received if they had withdrawn in another asset.

I believe this fulfills the following criteria for a High "Definite loss of funds without (extensive) limitations of external conditions."

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

solimander commented 8 months ago

Technically valid, but would argue the loss is highly constrained in that the only loss is yield while withdrawing. I'll update the docs for this issue.

nevillehuang commented 8 months ago

I believe #177 and #367 to be duplicates of this issue

0xmonrel commented 8 months ago

177 is not a duplicate it is about rebasing tokens. #367 is definitely a duplicate.

nevillehuang commented 8 months ago

@solimander I am still unsure if this is not a duplicate of #109 and others. Could you elaborate more and is a separate fix required? The following impact highlighted by the watson seems to indicate otherwise. To me it seems like the same accrual inconsistency due to exchange rate used to compute sharesOwed regardless of type of asset.

The portion of staking rewards accumulated during withdrawal that belongs to LRT holders is never accounted for so withdrawing users do not earn any rewards when waiting for a withdrawal to be completed.

solimander commented 8 months ago

@nevillehuang The issues do seem slightly different, though the fix being considered will fully fix #109 and partially fix this issue.

The core issue in #109 is that there's a period between the time that the shares owed is locked and the rebalance occurs in which yield to the EigenLayer strategy can cause a rebalance revert.

Locking shares owed at the time of withdrawal request also affects this issue in that it prevents withdrawals from earning yield at the time the withdrawal request is received. Once a rebalance occurs, this issue has a secondary cause that prevents yield from being earned - unlike other strategies, "shares" in the beacon chain strategy are just ETH, so no additional yield can be earned once the withdrawal from EigenLayer is queued.

I plan to address both with the above fix and update the docs to inform users that yield will only be earned between the time of withdrawal request and rebalance for ETH withdrawals.

0xmonrel commented 8 months ago

@solimander I am still unsure if this is not a duplicate of #109 and others. Could you elaborate more and is a separate fix required? The following impact highlighted by the watson seems to indicate otherwise. To me it seems like the same accrual inconsistency due to exchange rate used to compute sharesOwed regardless of type of asset.

The portion of staking rewards accumulated during withdrawal that belongs to LRT holders is never accounted for so withdrawing users do not earn any rewards when waiting for a withdrawal to be completed.

No, that issue does not fix this one. This issue can be fixed in protocol but it would require quite a bit of added complexity to account for the correct share of yield. Solimander will instead settle on not giving out yield during the withdrawal process, which of course users needs to be aware of.

@solimander have you considered that the withdrawal of ETH could take weeks or months if the POS withdrawal queue is congested? I just want to confirm that we have covered everything here.

Maybe we can add functionality to compensate users that have their assets locked for a long period of time without earning yield. E.g. if POS withdrawal takes 2 week more than the EigenLayer withdrawal the admin can issue 0-5% APY equivalent amount of yield to the cohort from the deposit pool.

Just brainstorming on a fix that does not add a lot of complexity but defends against the worst case scenario..

Actually, I think there is reasonable fix that solves all this in protocol. It requires that an Oracle is used to update the cumulative yield in a single storage slot when rebalance is called. We also need to add a mapping epoch->time and then let ETH withdrawers take their yield directly from the deposit pool at the time of withdrawal.

Solimander, I can provide an MVP for the above if you are interested.

nevillehuang commented 8 months ago

Locking shares owed at the time of withdrawal request also affects this issue in that it prevents withdrawals from earning yield at the time the withdrawal request is received. Once a rebalance occurs, this issue has a secondary cause that prevents yield from being earned - unlike other strategies, "shares" in the beacon chain strategy are just ETH, so no additional yield can be earned once the withdrawal from EigenLayer is queued.

The secondary cause seems to not be highlighted in the original submission. I believe they are duplicates because both issues point to locking of assets (albeit different assets) within deposit pools and locking the exchange rate during deposits. I believe if exchange rate accounts for yield accrued that is supposed to be delegated to the user, both issues would be solved.

0xmonrel commented 8 months ago

Locking shares owed at the time of withdrawal request also affects this issue in that it prevents withdrawals from earning yield at the time the withdrawal request is received. Once a rebalance occurs, this issue has a secondary cause that prevents yield from being earned - unlike other strategies, "shares" in the beacon chain strategy are just ETH, so no additional yield can be earned once the withdrawal from EigenLayer is queued.

The secondary cause seems to not be highlighted in the original submission. I believe they are duplicates because both issues point to locking of assets (albeit different assets) within deposit pools and locking the exchange rate during deposits. I believe if exchange rate accounts for yield accrued that is supposed to be delegated to the user, both issues would be solved.

I believe if exchange rate accounts for yield accrued that is supposed to be delegated to users, both issues would be solved

LST and ETH earn yield differently. This entire issue is on the topic of how ETH yield is not accounted for at all since it is distributed through a separate system that has nothing to do with the exchange rate. The fix to #109 does not lead to users earning yield during the withdrawal period since the yield from rebalance -> completed withdrawal is not accounted for.

I am clearly referring to the secondary issue:

The portion of staking rewards accumulated during withdrawal that belongs to LRT holders is never accounted for so withdrawing users do not earn any rewards when waiting for a withdrawal to be completed.

Czar102 commented 8 months ago

@0xmonrel from my understanding, #109 is an issue that makes the LST not earn yield during withdrawals, while this and #367 are documentation issues about the fact that the documentation mentions that the yield is being earned during withdrawal for ETH.

Such a mechanic for ETH (described in the docs) would be fundamentally flawed, so I'm not sure how to consider this finding, given that the implementation works as it should, though against the specification.

@solimander @0xmonrel @nevillehuang do you agree?

0xmonrel commented 8 months ago

@Czar102 I agree with your statement other than "such a mechanic for ETH would be fundamentally flawed". Why would it not be possible to account for the ETH yield earned? It might add complexity but it is possible.

And yes, fundamentally the issue is that it is stated that users earn yield during ETH withdrawals but they do not. I am assuming here that users expecting yield based on provided information but not earning any as loss off funds/yield. But its obviously your call to decide if that is a fair assumption.

nevillehuang commented 7 months ago

@Czar102 yea agree with your point for the de-deduplication. Seems like a documentation error if sponsor didn’t fix it, but would be fair to validate it based on information presented at time of audit. I will leave it up to you to decide.

367 and #177 seems to be talking about LST though not ETH? How is the yield earned from native ETH different from LST? Also don’t users accept the exchange rate when requesting withdrawals?

Czar102 commented 7 months ago

I am referencing a fundamental flaw because ETH does not earn rewards when a validator is in the withdrawal queue. This means that Rio is fundamentally unable to provide staking rewards from that period of being locked if the withdrawal is not to impact other users' rewards.

@nevillehuang @0xmonrel does that make sense?

Or is it the same for ETH and other LSTs?

0xmonrel commented 7 months ago

I am referencing a fundamental flaw because ETH does not earn rewards when a validator is in the withdrawal queue. This means that Rio is fundamentally unable to provide staking rewards from that period of being locked if the withdrawal is not to impact other users' rewards.

@nevillehuang @0xmonrel does that make sense?

Or is it the same for ETH and other LSTs?

That is partially correct. Until the validator gets "withdrawable" status it will still be earning yield, i posted a picture of the expected time in a picture above.

As it stand the users withdrawing are actually paying all other users the yield that the validator is generating. A user solo staking or using EigenLayer would earn yield up until their validator reaches "withdrawable status".

0xmonrel commented 7 months ago

@Czar102 yea agree with your point for the de-deduplication. Seems like a documentation error if sponsor didn’t fix it, but would be fair to validate it based on information presented at time of audit. I will leave it up to you to decide.

367 and #177 seems to be talking about LST though not ETH? How is the yield earned from native ETH different from LST? Also don’t users accept the exchange rate when requesting withdrawals?

367 Is talking about an LRT (reETH), which is the token received when depositing ETH or an LST into Rio. So the issue is about ETH withdrawals which require users to deposit reETH. It is a little confusing since the name is similar to rETH which is an LST.

Each reETH can have multiple underlying assets that are supported on EigenLayer. EigenLayer distinguishes between LSTs and ETH since ETH has to be staked with an actual validator - the rewards that are generated here is the yield for ETH. These rewards are distributed through the DelayedWithdrawalRouter contract on EigenLayer. Eventually 90% reaches the deposit pool at which point the yield belongs to reETH holders. None of this is accounted for during withdrawals of ETH, that is why ETH withdrawals earn 0 yield.

For LSTs (if they are not rebasing) the yield is earned in the increased value of the token based on how much ETH it can be redeemed for which increases as yield is added.

On users accept an exchange rate: Users accept the current value of their reETH when requesting a withdrawal but they are also expected to earn yield on top of that during the withdrawal. For non-rebasing LSTs this happens naturally since the yield is baked into the token. Locking in 10 LST today and receiving 10 LST in 1 week will include the yield. This is not true for ETH which is why no yield is earned.

107 is a separate issue that does not talk about ETH withdrawals, it is not a duplicate.

Czar102 commented 7 months ago

I agree. Will make this a separate issue with #367 and #177 as duplicates shortly.

0xmonrel commented 7 months ago

177 should not be a duplicate? It is only on rebasing tokens not earning yield

Explicitly only talking about non-eth rebasing tokens:

Impact section:

Not all depositors will be able to withdraw their assets/principal for non-ETH assets.

Czar102 commented 7 months ago

I see. I will leave it a duplicate of #109, as it was, which will not change the reward distribution from the scenario where it was invalidated. cc @nevillehuang

Thanks for reminding me to remove #177 from the duplicates @0xmonrel.

Czar102 commented 7 months ago

Result: Medium Has Duplicates

Considering this a Medium since the loss is constrained to the interest during some of the withdrawal time, which is a small part of the deposits.

sherlock-admin3 commented 7 months ago

Escalations have been resolved successfully!

Escalation status:

sherlock-admin4 commented 7 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/rio-org/rio-sherlock-audit/pull/13

10xhash commented 7 months ago

Fixed Share value appreciated/yield till the rebalance call will now be considered. Since the queued rio lrt tokens are only burnt after the withdrawal completion, the yield occuring during this time will be temporarily distributed to even the queued tokens causing a temporary decrease in rio lrt value. This is considered accepteable.

sherlock-admin4 commented 7 months ago

The Lead Senior Watson signed off on the fix.