aslanbek - Permanent DoS of `removeAsset` via one-time 1 gwei deposit

[sherlock-admin2]

aslanbek

medium

Permanent DoS of removeAsset via one-time 1 gwei deposit

Summary

Vulnerability Detail

In case an underlying token becomes unreliable, RioLRTAssetRegistry allows the owner (RioDAO) to remove it from the set of underlying assets, so the LRT will not have to be completely redeployed every time any of its underlying tokens needs to be removed.

By design, the asset can only be removed if the TVL for that asset is zero (balance in EigenLayer + balance in depositPool). While it does guarantee that every genuine user will be able to redeem their underlying token before it becomes unsupported, it opens an attack path that would prevent the token from ever being removed: by depositing just 1 gwei of an underlying token (or withdrawing 1 gwei less than the whole amount), any user would forever (until withdrawn) prevent the removal of that token.

Impact

An asset can not be removed as long as at least one depositor leaves 1 gwei in the protocol.

Code Snippet

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

Tool used

Manual Review

Recommendation

    function removeAsset(address asset) external onlyOwner {
        if (!isSupportedAsset(asset)) revert ASSET_NOT_SUPPORTED(asset);
-       if (getTVLForAsset(asset) > 0) revert ASSET_HAS_BALANCE();

Duplicate of #15

[aslanbekaibimov]

Escalate

In my opinion, this report should be considered separately, as it is the only one that describes a sufficient one-time attack, and addresses most of the points of the escalation.

The attacker does not have incentive to do the attack, only for grief purpose, but attackers consume gas. Nearly makes no sense.

The purpose of the attack is to preserve the token in the set of underlying tokens to be able take advantage of the asset's instability if it ever becomes such; the attacker spends gas only once instead of every time the admin (or anyone) fixes the issue.

Even 1wei is deposited, if admin decides to remove the asset, they can deposit some assets and then withdraw again, and then call removeAsset again.

In this scenario, the admin can not do anything to the deposited 1 gwei.

No user funds are locked.

True, the attack never implies locking of user funds in the first place.

Lock period can't be a week.

The DoS is permanent in this case, as the owner can not unstake underlying tokens staked by users into EigenLayer.

[sherlock-admin2]

Escalate

In my opinion, this report should be considered separately, as it is the only one that describes a sufficient one-time attack, and addresses most of the points of the escalation.

The attacker does not have incentive to do the attack, only for grief purpose, but attackers consume gas. Nearly makes no sense.

The purpose of the attack is to preserve the token in the set of underlying tokens to be able take advantage of the asset's instability if it ever becomes such; the attacker spends gas only once instead of every time the admin (or anyone) fixes the issue.

Even 1wei is deposited, if admin decides to remove the asset, they can deposit some assets and then withdraw again, and then call removeAsset again.

In this scenario, the admin can not do anything to the deposited 1 gwei.

No user funds are locked.

True, the attack never implies locking of user funds in the first place.

Lock period can't be a week.

The DoS is permanent in this case, as the owner can not unstake underlying tokens staked by users into EigenLayer.

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]

Disagree with escalation, the issues should remain as duplicates.

[Czar102]

@aslanbekaibimov I don't see how is this issue different than other duplicates of #15. Please pinpoint the exact differences with quotes that make this issue valid, while #15 and other duplicates – invalid.

[aslanbekaibimov]

@Czar102

1. The original issue describes a donation to depositPool:

tokensInRio variable is the IERC20.balanceOf call result which means that if anyone donates 1 wei of the asset to deposit pool just before the owners removeAsset tx, then the tx will revert.

On the other hand, this issue describes a "normal" deposit of 1 gwei into EigenLayer through Rio:

by depositing just 1 gwei of an underlying token (or withdrawing 1 gwei less than the whole amount), any user would forever (until withdrawn) prevent the removal of that token.

2. The original issue can be mitigated by admin depositing and withdrawing, as described in the original escalation:

Even 1wei is deposited, if admin decides to remove the asset, they can deposit some assets and then withdraw again, and then call removeAsset again.

while this attack is executed once per token and blocks removeAsset permanently, and can not be fixed by admin.

[nevillehuang]

Like mentioned in #15, not allowing a removal of an asset presents no potential fund loss and time sensitive revert (given the explicit requirement of 0 funds required to remove an asset). At worse case the asset remains supported, and users can simply stop depositing.

[aslanbekaibimov]

@nevillehuang isn't the current reason for downgrading the severity exclusively in the fact that the DoS can be lifted? If not, I would like to dispute your point as well.

[Czar102]

@solimander @nevillehuang do you agree that the DoS may be impossible to be lifted?

What are the consequences of the removeAsset functionality being unavailable?

[nevillehuang]

@Czar102 I don’t see a difference in impact between this issue and #15 based on my comment here

[solimander]

do you agree that the DoS may be impossible to be lifted?

@Czar102 I don't believe so. A tiny donation would require a withdrawal before the asset could be removed. To prevent a repeat of this attack, the withdrawal and asset removal would need to be processed in the same transaction.

[Czar102]

@nevillehuang I believe the link in your message is wrong.

@aslanbekaibimov do you agree with the sponsor that this can be mitigated as well?

[nevillehuang]

@Czar102 Updated the link, and agree with sponsors comment

[aslanbekaibimov]

@Czar102 yes, @solimander is right. In comparison to #15, the owner will just need to get LRT to redeem it for the asset they intend to remove. This will also take a week longer, due to EigenLayer escrow period.

[Czar102]

In that case, I'm planning to leave the issue as is – a Low severity issue.

[Czar102]

Result: Low Duplicate of #15

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• aslanbekaibimov: rejected