Kelp DAO | rsETH - ubl4nk's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTOracle.sol#L70-L71 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTOracle.sol#L78 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L47-L51 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L79

Vulnerability details

Impact

Attacker is able transfer some amount of an underlying asset directly to LRTDepositPool (or a NodeDelegator, the goal of attacker is to increase the balance of protocol for an underlying asset) and inflate the rsETH price.

Proof of Concept

Note: getTotalAssetDeposits returns the total asset (from token X for example) present in protocol: (contract balance of LRTDepositPool for asset X + amount of asset X that are transferred to NodeDelegators + amount of asset X that are deposited into strategies)

This is how the price of rsETH is calculated (see LRTOracle#getRSETHPrice):

    for (uint16 asset_idx; asset_idx < supportedAssetCount;) { // <-- Assume supportedAssetCount is 1 and there is only 1 asset `X`
            address asset = supportedAssets[asset_idx]; // <-- address of asset `X`
            uint256 assetER = getAssetPrice(asset); // <-- price of asset `X`

            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset); // <-- amount of asset `X` owned by Protocol (Attacker can directly transfer some amount of asset `X` to LRTDepositPool, this increases balance of LRTDepositPool contract, so when LRTDepositPool's balance increases, as a result, Protocol's balance also increases and rsETH price will be manipulated)

            totalETHInPool += totalAssetAmt * assetER; // <-- (number of asset `X` owned by Protocol) * (price of asset `X`)

            unchecked {
                ++asset_idx;
            }
    }

    return totalETHInPool / rsEthSupply; // <-- (value of asset `X` (value in $dollar) owned by the Protocol) / (number of rsETH minted) = rsETH_price

(Everything is commented in code) So what will happen if attacker transfers some amount of asset X (for example 1000) directly to LRTDepositPool (without minting any rsETH) ? The answer is: totalSupply of rsETH will remain the same, but the totalETHInPool will be a higher amount (because totalETHInPool depends on getTotalAssetDeposits and getTotalAssetDeposits depends on the contract balance of LRTDepositPool for asset X) -> which means the rsETH price will be higher than before and inflated. Let's imagine an example:

• Assuming there is one supported asset stETH (we know there are more than one, but let's for now assume there is only one to make the example easier to understand) and price of stETH is $2000.
• Assume LRTDepositPool owns 2 tokens of stETH (getTotalAssetDeposits returns 2) and minted 2 rsETH (totalSupply of rsETH is 2), in other words, someone has deposited 2 stETH and minted 2 rsETH.

Now: totalETHInPool = 4000$ (because 2 * 2000$) rsEthSupply = 2 rsETH_price = totalETHInPool / rsEthSupply = $4000 / 2 = $2000

• Attacker transfers 2 stETH directly (without calling depositAsset, just through stETH contract) to LRTDepositPool and increases the balance of LRTDepositPool for stETH.
• Now getTotalAssetDeposits returns 4 (it means the protocol owns 4 stETH)

Now: totalETHInPool = 8000$ (because 4 * 2000$) rsEthSupply = 2 rsETH_price = totalETHInPool / rsEthSupply = $8000 / 2 = $4000

Tools Used

Manual Review

Recommended Mitigation Steps

getTotalAssetDeposits() should not be dependent of contract balance for an underlying asset, consider adding a state variable which tracks how much balance is deposited through protocol.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTConfig.sol#L80-L89 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTConfig.sol#L73-L75

Vulnerability details

Impact

There is no way for MANAGER to remove an asset from supportedAssets.

Proof of Concept

• MANAGER adds a new supported asset to the protocol (let's say asset X).
• If something unexpected happens to the asset X (for example eigenlayer governance decides to remove an asset), MANAGER is not able to remove that asset (asset X) from supportedAssets.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a function that MANAGER is able to remove some asset from supportedAssets.

Assessed type

Context