code-423n4 / 2023-11-kelp-findings

13 stars 11 forks source link

```LRTConfig``` has no ability to remove an asset making it especially vulnerable if an underlying asset has security issues #685

Open c4-submissions opened 11 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L80 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52

Vulnerability details

Impact

LRTConfig only has the ability to add supported assets and not remove them. This means that the Kelp protocol will be forever vulnerable if there is an issue with any of the underlying assets.

Proof of Concept

There are several main issues that can be problematic:

  1. Admin accidentally adds an unintended asset to the supported list
  2. Existing underlying has a security incident with an attacker or malicious admin
  3. Eigenlayer removes support for an asset

While the administrator can stop deposits by just setting the deposit limit of the compromised/unintended asset to 0, the assets price is still taken into account inside the LRTOracle#getRSETHPrice function to determine the price of RSETH.

     address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
     uint256 supportedAssetCount = supportedAssets.length;
     for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];

            //@audit here get the asset price
@>            uint256 assetER = getAssetPrice(asset);

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;

This means that the price of RSETH will forever be reliant on the compromised/unintended asset which can greatly affect the integrity of the protocol should something go wrong.

Tools Used

VIM

Recommended Mitigation Steps

Add the ability for the admin to remove support for an asset inside LRTConfig

Assessed type

Other

c4-pre-sort commented 11 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #36

c4-judge commented 10 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)

c4-judge commented 10 months ago

fatherGoose1 marked the issue as grade-b