mstpr-brainbot - AssetRegistry owner can be frontrunned when removing asset

[sherlock-admin4]

mstpr-brainbot

medium

AssetRegistry owner can be frontrunned when removing asset

Summary

The AssetRegistry owner can remove an asset at any time, provided that certain checks are satisfied. One of these checks is that the TVL for the asset must be "0". This implies that the asset should not exist in the system at all, neither in the deposit pool nor in the EigenLayer strategy. However, anyone can donate 1 wei of the asset to the deposit pool address to grief the owner, and the owner cannot do anything to prevent it.

Vulnerability Detail

This is the validation checks in the removeAsset function implemented:

function removeAsset(address asset) external onlyOwner {
        if (!isSupportedAsset(asset)) revert ASSET_NOT_SUPPORTED(asset);
        // @review someone can donate 1 wei to grief here
        -> if (getTVLForAsset(asset) > 0) revert ASSET_HAS_BALANCE();
        .
    }

now let's also check how getTVLForAsset function is implemented:

function getTVLForAsset(address asset) public view returns (uint256) {
        uint256 balance = getTotalBalanceForAsset(asset);
        if (asset == ETH_ADDRESS) {
            return balance;
        }
        return convertToUnitOfAccountFromAsset(asset, balance);
    }

function getTotalBalanceForAsset(address asset) public view returns (uint256) {
        .
        .
        -> uint256 tokensInRio = IERC20(asset).balanceOf(depositPool_);
        uint256 tokensInEigenLayer = convertFromSharesToAsset(getAssetStrategy(asset), sharesHeld);

        return tokensInRio + tokensInEigenLayer;
    }

as we can observe, 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.

Another scenario from same root cause: Since every LRT gets a sacrificial deposit in the deployment phrase, there will be always some excess tokens that are not possible to be burnt because the coordinator can't burn the LRT tokens received in deployment.

Impact

Very cheap to execute the attack (1 wei of token) and can be called simply even every block to grief owner if really wanted.

Code Snippet

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

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

Tool used

Manual Review

Recommendation

[nevillehuang]

Borderline medium/low, need @Czar102 opinion, see similar finding here

[solimander]

Technically valid. May just drop asset removals in this version.

[nevillehuang]

I think this falls under the following category so leaving as medium severity, given users can easily prevent removal of assets

1. The issue causes locking of funds for users for more than a week.

[KupiaSecAdmin]

Escalate

Severity has to be considered as Low, because:

1. The attacker does not have incentive to do the attack, only for grief purpose, but attackers consume gas. Nearly makes no sense.
2. 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.
3. No user funds are locked.
4. Lock period can't be a week.

[sherlock-admin2]

Escalate

Severity has to be considered as Low, because:

1. The attacker does not have incentive to do the attack, only for grief purpose, but attackers consume gas. Nearly makes no sense.
2. 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.
3. No user funds are locked.
4. Lock period can't be a week.

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.

[shaka0x]

I do agree with the above escalation. And would add that the owner can just set the deposit cap to 0, which also prevents new deposits for the asset, that is the final purpose.

Also, would stress that there are no funds lock, so this dos not apply:

1. The issue causes locking of funds for users for more than a week.

[nevillehuang]

Agree this should be low severity.

[0xcats]

Currently the only way to change the strategy for an asset is to completely remove it and re-add it. I think due to that it's definitely not low severity.

Pointing which eigen layer strategy an asset is using is core functionality.

[pronobis4]

I disagree, it's core functionality that can easily be rendered useless. https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue

[0xdanial]

I disagree with escalation because executing this attack is very simple, rendering one of the core functions in the protocol useless. According to Sherlock rules, this is a completely valid Medium issue.

Setting the deposit cap to zero doesn't prevent anything because one doesn't even need to call the deposit() function. simply transferring the target asset directly to the deposit pool is enough to revert removeAsset().

function getTotalBalanceForAsset(address asset) public view returns (uint256) {
        if (!isSupportedAsset(asset)) revert ASSET_NOT_SUPPORTED(asset);

        address depositPool_ = address(depositPool());
        if (asset == ETH_ADDRESS) {
            return depositPool_.balance + getETHBalanceInEigenLayer();
        }

        uint256 sharesHeld = getAssetSharesHeld(asset);
        uint256 tokensInRio = IERC20(asset).balanceOf(depositPool_);
        uint256 tokensInEigenLayer = convertFromSharesToAsset(getAssetStrategy(asset), sharesHeld);

        return tokensInRio + tokensInEigenLayer;
    }

We can observe that tokensInRio represents the balance of the asset in depositPool_, making the attack very simple.

[nevillehuang]

Currently the only way to change the strategy for an asset is to completely remove it and re-add it. I think due to that it's definitely not low severity.

Pointing which eigen layer strategy an asset is using is core functionality.

If this is true, then it definitely warrants medium severity, will loop in sponsor for discussion

[KupiaSecAdmin]

Currently the only way to change the strategy for an asset is to completely remove it and re-add it. I think due to that it's definitely not low severity. Pointing which eigen layer strategy an asset is using is core functionality.

If this is true, then it definitely warrants medium severity, will loop in sponsor for discussion

Talking about changing strategy address, once set, it never needs to be changed because EigenLayer's strategy address for each asset is fixed, any upgrades on EigenLayer's strategy is implemented through proxy.

And still, the main point here is that the attacker has no incentive at all to monitor all of Rio transactions and front-run every removeAsset transaction by paying extremely higher gas.

[0xcats]

Currently the only way to change the strategy for an asset is to completely remove it and re-add it. I think due to that it's definitely not low severity. Pointing which eigen layer strategy an asset is using is core functionality.

If this is true, then it definitely warrants medium severity, will loop in sponsor for discussion

Talking about changing strategy address, once set, it never needs to be changed because EigenLayer's strategy address for each asset is fixed, any upgrades on EigenLayer's strategy is implemented through proxy.

And still, the main point here is that the attacker has no incentive at all to monitor all of Rio transactions and front-run every removeAsset transaction by paying extremely higher gas.

The devs stated in chat that currently the only way to change an asset's strategy is to remove and re-add an asset. To me that means that they have the intention to change an asset's strategy if needed.

Your reasoning for incentive makes no sense. This is a griefing attack where the incentive is to cause harm, not to walk away with profit. Nowhere does anyone say the attacker needs to front-run every removeAsset, doing it to only 1 is enough damage done.

Additionally, when griefing attacks happen, they are often carried out by competitor protocols. The cost for carrying out such an attack to Rio, even for multiple of it's assets, would be negligible for a well-funded competitor.

[shaka0x]

The devs stated in chat that currently the only way to change an asset's strategy is to remove and re-add an asset. To me that means that they have the intention to change an asset's strategy if needed.

If there was such intention there would be a setter like there is for the deposit cap. Can you please share a link to that comment to get more context?

[solimander]

The devs stated in chat that currently the only way to change an asset's strategy is to remove and re-add an asset. To me that means that they have the intention to change an asset's strategy if needed.

This does not mean there is an intention to change an asset's strategy. They are as good as fixed given that they are upgradeable proxies on the EigenLayer side and it would be incredibly complex for them to change strategy address. This was a response when someone asked how a strategy address would be changed (even though it won't happen).

[nevillehuang]

@0xcats Could you share the comment (if public) where the sponsor indicated the following, if not based on this comment here I believe it is low severity

The devs stated in chat that currently the only way to change an asset's strategy is to remove and re-add an asset. To me that means that they have the intention to change an asset's strategy if needed.

[0xcats]

@0xcats Could you share the comment (if public) where the sponsor indicated the following, if not based on this comment here I believe it is low severity

The devs stated in chat that currently the only way to change an asset's strategy is to remove and re-add an asset. To me that means that they have the intention to change an asset's strategy if needed.

In private thread.

[Czar102]

I think this falls under the following category so leaving as medium severity, given users can easily prevent removal of assets

1. The issue causes locking of funds for users for more than a week.

Please see the following clarification from the same rule:

Griefing for gas (frontrunning a transaction to fail, even if can be done perpetually) is considered a DoS of a single block, hence only if the function is clearly time-sensitive, it can be a Medium severity issue.

Planning to accept the escalation and consider this issue a Low severity one.

[nevillehuang]

@Czar102 I agree with your logic, this issue can be low severity.

[Czar102]

Result: Low Has duplicates

[sherlock-admin3]

Escalations have been resolved successfully!

Escalation status:

• KupiaSecAdmin: accepted

[pronobis4]

So removing the LRT support asset is not a key feature, interesting. I don't understand at all why it was about removing the strategy and not simply about deleting the asset. After all, these are two completely different functions. Maybe I don't notice, but in my opinion there is no option to block the deposit of a given asset other than by deleting it, which may not work, so it cannot be done.

[0xdanial]

@solimander

 function test_poc() public {
        deal(address(RETH_ADDRESS), address(reLST.depositPool), 1 wei);
        deal(address(CBETH_ADDRESS), address(reLST.depositPool), 1 wei);

        vm.expectRevert(abi.encodeWithSelector(IRioLRTAssetRegistry.ASSET_HAS_BALANCE.selector));
        reLST.assetRegistry.removeAsset(CBETH_ADDRESS);

        vm.expectRevert(abi.encodeWithSelector(IRioLRTAssetRegistry.ASSET_HAS_BALANCE.selector));
        reLST.assetRegistry.removeAsset(RETH_ADDRESS);
    }

just add this function to test/RioLRTAssetRegistry.t.sol and run with this: forge test --match-contract RioLRTAssetRegistryTest --match-test test_poc

this poc simply shows if even 1 wei asset exists in depositPool the removeAsset() will revert. if attacker sends 1 wei of asset to depositPool directly, there is no way to remove the asset anymore so this core function will blocked forever.

this happens because tokensInRio in the getTotalBalanceForAsset() function represents the balance of asset in depositPool, which means transferring asset directly to depositPool blocks removeAsset() forever. is it not enough to consider this issue as medium?

function getTotalBalanceForAsset(address asset) public view returns (uint256) {
        if (!isSupportedAsset(asset)) revert ASSET_NOT_SUPPORTED(asset);

        address depositPool_ = address(depositPool());
        if (asset == ETH_ADDRESS) {
            return depositPool_.balance + getETHBalanceInEigenLayer();
        }

        uint256 sharesHeld = getAssetSharesHeld(asset);
        uint256 tokensInRio = IERC20(asset).balanceOf(depositPool_);
        uint256 tokensInEigenLayer = convertFromSharesToAsset(getAssetStrategy(asset), sharesHeld);

        return tokensInRio + tokensInEigenLayer;
    }