KupiaSec - Queueing withdrawals from EigenLayer does not guarantee successful withdrawals.

[sherlock-admin]

KupiaSec

high

Queueing withdrawals from EigenLayer does not guarantee successful withdrawals.

Summary

When withdrawing from EigenLayer, the withdrawal address is either withdrawal queue or deposit pool, while the staker address is the operator delegator.
Since EigenLayer has a feature flag to enable/disable third party transfers, withdrawals fail.

Vulnerability Detail

EigenLayer's StrategyManager contracts stores a mapping named thirdPartyTransfersForbidden which maps from strategy to boolean flag that represents if third party transfer is forbidden or not.

// EigenLayer's DelegationManager contract, _removeSharesAndQueueWithdrawal function
// Rio's queueWithdrawal function calls queueWithdrawal function of EigenLayer's DelegationManager contract, and it calls this _removeSharesAndQueueWithdrawal function in it
require(
    staker == withdrawer || !strategyManager.thirdPartyTransfersForbidden(strategies[i]),
    "DelegationManager._removeSharesAndQueueWithdrawal: withdrawer must be same address as staker if thirdPartyTransfersForbidden are set"
);

When the flag is set to true, withdrawal address must be same as staker address, unless it reverts.

However, in Rio protocol, the staker address is always operator delegator and the withdrawal address is always either deposit pool or withdrawal queue. This means Rio's withdrawing features do not work for EigenLayer strategies with above flag set true.

Impact

Rio's withdrawal features do not work at all thus do not allow users to withdraw their deposits.

Code Snippet

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

Tool used

Manual Review

Recommendation

Withdrawal logic needs to be modified so that it uses operator delegator address as withdrawal address, once withdrawal is complete, it should transfer withdrawn tokens to either deposit pool or withdrawal queue.

Duplicate of #237

[KupiaSecAdmin]

Escalate

@nevillehuang - The issue is not about malicious behavior of Eigen's strategy manager, enabling/disabling third party withdrawals is a feature of EigenLayer.

@solimander - For your information, EigenLayer does not support third party withdrawals in their upgrade, which means you guys need to write the withdrawal logic again.

[sherlock-admin2]

Escalate

@nevillehuang - The issue is not about malicious behavior of Eigen's strategy manager, enabling/disabling third party withdrawals is a feature of EigenLayer.

@solimander - For your information, EigenLayer does not support third party withdrawals in their upgrade, which means you guys need to write the withdrawal logic again.

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.

[nevillehuang]

@KupiaSecAdmin I don't think this feature is permisionless, it is protected by a modifier as seen here, which involves an trusted external admin of eigenlayer

[KupiaSecAdmin]

@nevillehuang - Yeah that's correct, EigenLayer's strategy manager can set flags for each strategy, and when the flat is set to false, Rio's withdrawal feature does not work.

[solimander]

Technically valid - current version of Rio assumes the flag will never be flipped.

We support in a newer version as EigenLayer began enforcing that the withdrawer is the staker (see: #160) after the code was locked for this audit.

[nevillehuang]

Doesn't this issue involve external admins that are trusted? If so I believe it is still invalid

[KupiaSecAdmin]

@nevillehuang - The issue happens regardless of trustness of external admins.
EigenLayer's admins are trusted, but they can still flip the flag because it's EigenLayer's one of features whether to allow or not third-party withdrawal.

[nevillehuang]

@KupiaSecAdmin For rio network integrating eigenlayer, I believe it is fair to consider that this flag will never be flipped, and if so the risks is acceptable as noted below. I believe this is synonymous to say pausing withdrawals

In case of external protocol integrations, are the risks of external contracts pausing or executing an emergency withdrawal acceptable? If not, Watsons will submit issues related to these situations that can harm your protocol's functionality.

Yes, the risks associated with EigenLayer and EigenLayer's governance are considered acceptable.

[KupiaSecAdmin]

@nevillehuang - Well, to be honest, I think it should not be considered as pausing withdrawals.
Instead it can be thought of flags applied to strategies. For example, some strategies have that flag set to True, while some strategies have the flag set to False. The flag value per strategy is defined by EigenLayer strategy manager, also whether flip the flag or not also depends on EigenLayer strategy manager.

This means that even though the flag isn't flipped once it's set, Rio can only interact with strategies with flags set to True.

Another point to consider is that there is no statement about the flag will never be flipped. In fact, for now in current version of EigenLayer strategies, they don't allow third party withdrawals at all, which means Rio is not able to interact with any strategy.

[Czar102]

I believe EigenLayer should be aware that Rio requires this flag set to false, so, since they are trusted, they are assumed to never flip it.

Hence, I am planning to reject the escalation and leave the issue as is.

[KupiaSecAdmin]

@Czar102 - EigenLayer's strategies don't serve for specific protocol, meaning there exists each strategy for stEth, rETH, etc. And as of fact, they disabled third party withdrawal in their upgrades.

[Czar102]

In that case, I think it may be a valid Medium severity issue. (contracts are upgradeable, right?)

[KupiaSecAdmin]

In that case, I think it may be a valid Medium severity issue. (contracts are upgradeable, right?)

@Czar102 - Yes it is upgradeable.
The reason I submitted the report as High was because with current codebase(also with locked assumed version of EigenLayer protocol), it only can interact with a part of EigenLayer protocols, and with current version of EigenLayer protocol, it can not interact with EigenLayer at all. Also the mitigation for the issue is huge which is to rewrite the whole logic for withdrawals.

[Czar102]

I see. I'm planning to accept the escalation and make this a Medium severity issue.

@nevillehuang @KupiaSecAdmin are there any duplicates of this issue?

[nevillehuang]

@Czar102 @KupiaSecAdmin Could you point me to resources indicating they disabled withdrawals in upgrades? Did it happened after or before the audit contest? @Czar102 See duplicates under issue #237

[Czar102]

@KupiaSecAdmin can you answer @nevillehuang's question above? I will validate #237 and duplicates shortly after.

Why hasn't the escalation been on the main issue?

[KupiaSecAdmin]

@nevillehuang - https://github.com/Layr-Labs/eigenlayer-contracts/blob/dev/docs/core/DelegationManager.md#queuewithdrawals

It is described in the docs above, you might look into the docs of queueWithdrawals function, and it says the withdrawer address must be staker address, that represents third party withdrawal is completely forbidden.(I also confirmed with EigenLayer protocol team when I participated in their contest)

@Czar102 - It was my mistake that I didn't escalate on the main issue.

[KupiaSecAdmin]

@nevillehuang - When the contest started, it was already disabled on EigenLayer contracts but on the branch that Rio team referenced, it was not totally disabled but controllable by flags, that's why it could be medium severity.
If it was already disabled on the branch Rio team referenced, it deserves high I guess.

[nevillehuang]

@KupiaSecAdmin Doesn't that mean during the time of the audit, it is externally controlled by admin as, and also corresponds to the code logic I verified as noted by my comments here? I believe my judgement was correct for the referenced branch, and any upgrades or code changes should be out of scope of this contest

@solimander could you confirm tour following comments

invalid - assumption in this release is that thirdPartyTransfersForbidden would always be false. side note: eigenlayer introduced a breaking change to M2 following the start of this audit. we've added support, which will subsequently fix this issue

[KupiaSecAdmin]

@nevillehuang - It seems like this issue would be medium at most with 6 dups, and nobody else is raising opinions here. Thus I can also accept any judgement as well. 👍

[solimander]

@solimander could you confirm tour following comments
invalid - assumption in this release is that thirdPartyTransfersForbidden would always be false. side note: eigenlayer introduced a breaking change to M2 following the start of this audit. we've added support, which will subsequently fix this issue

@nevillehuang Yes, his was the assumption at the time of the audit. This has since changed due to EigenLayer's need to combat an ongoing phishing attack and support has been added post-audit. Related to #160 (comment)

[Czar102]

I believe this is a valid concern – it is not clear that this state change would be malicious for the EigenLayer trusted admin to execute, while it would have a high impact on the Rio codebase.

Hence, I still think Medium severity is appropriate.

[nevillehuang]

@Czar102 What do you mean by not clear? See my comment here for additional details. The flag can only be triggered by eigen layer admins at the time of the audit. Anything else is future changes not valid based on sherlock rules. I think if you validate this it will directly contradict that rule.

Future issues: Issues that result out of a future integration/implementation that was not mentioned in the docs/README or because of a future change in the code (as a fix to another issue) are not valid issues.

[Czar102]

@nevillehuang that rule concerns code changes, not normal usage of the existing implementation.

By "not clear", I meant that I don't think we can objectively state that changing a configuration of the product to a valid state as designed in the contracts is malicious. We can't assume that the admin won't execute non-malicious actions by the trusted external admin rule. We only assume they won't perform malicious actions.

[nevillehuang]

@Czar102 It is assumed that this flag is always false at the time of the contest. There is nothing indicating the flag will be switched to true at the time of the contest based on “normal actions” by eigen layers strategy managers. The breaking change happened during the time of the contest as indicated by sponsors comments and kupia comments. Am I missing something here?

Don’t think it was my responsibility to look into new changes that permanently sets the flag to true after upgrades that happened during the contest period given audit scope is fixed.

Also afaik, the check is working as intended. If third party withdrawals are restricted, then it should revert, given Rio is a third party here

[Czar102]

From the README:

the risks associated with EigenLayer and EigenLayer's governance are considered acceptable

Hence, I think this issue is informational only.

Planning to reject the escalation and leave the issue as is.

[Czar102]

Result:
Low
Duplicate of #237

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: rejected