Kelp DAO | rsETH - jayfromthe13th's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L37-L38

Vulnerability details

Impact

In the getAssetPrice function we need to use a heartbeat check with the lastRoundData function. The absence of a Chainlink heartbeat check could potentially lead to several issues to the overall protocol.

The use of the lastRoundData function in this context is essential to ensure that the data being used is current and accurate. Without this Chainlink heartbeat check, the system is vulnerable to several risks. Firstly, there is the danger of relying on stale or outdated price data, which could lead to incorrect operational decisions. This is particularly critical in dynamic market environments where price accuracy is paramount.

Additionally, the absence of a heartbeat check opens up the possibility for malicious manipulation. An attacker could potentially freeze the price feed, providing data that benefits their operations while disadvantaging others. Furthermore, the system's heavy reliance on external data sources becomes a liability if these sources go offline or stop providing updates. In such scenarios, the contract might continue to operate on the last available price, potentially leading to significant financial losses. Therefore, implementing a heartbeat check is not just a technical enhancement but a fundamental requirement to safeguard the protocol against these issues.

Proof of Concept

-Setup Scenario: In this hypothetical scenario, a user engages with the Kelp product by depositing Ethereum (ETH) and receiving RSETH tokens. The pricing mechanism for this transaction is managed by the LRTOracle contract, which relies on the ChainlinkPriceOracle contract's getAssetPrice function to determine the market price of Liquid Staking Tokens (LST).

-Issue with Chainlink Oracle: A critical situation arises when the Chainlink oracle, responsible for providing LST/ETH price updates, experiences a technical failure and goes offline. Consequently, the price feed halts, freezing the last recorded price at 1 LST = 0.01 ETH.

-Market Price Variation: In the market, the value of LST experiences a significant increase, reaching 1 LST = 0.02 ETH. However, due to the offline status of the Chainlink oracle, this price change is not reflected in the Kelp product's system.

-Redemption Process: A user, unaware of the oracle's failure, initiates a redemption of their RSETH tokens for LST. The LRTOracle contract, in response, calls upon the getAssetPrice function to ascertain the current LST price.

-Impact of Stale Data: The getAssetPrice function, lacking a mechanism to verify the freshness of data (a heartbeat check), erroneously returns the outdated price of 1 LST = 0.01 ETH.

-Resulting Under-Redemption: This incorrect price leads to the user receiving a lower quantity of LST tokens than the market value dictates, potentially causing a financial loss.

-Trust Implications: Should the user realize the discrepancy in token quantity after the Chainlink oracle resumes operation and updates the price, it could severely undermine their trust in the Kelp product and the broader protocol.

Tools Used

Foundry

Recommended Mitigation Steps

Modify the getAssetPrice function to include a heartbeat check. This can be done by calling the latestRoundData function from the Chainlink oracle and checking the timestamp of the latest price data.

function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
    (,int price,,uint timeStamp,) = AggregatorInterface(assetPriceFeed[asset]).latestRoundData();
    require(block.timestamp - timeStamp < acceptableDelay, "Price data is stale");
    return uint(price);
}

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L59 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L122

Vulnerability details

Impact

The NodeDelegator contract tracks the balance of assets deposited into a strategy, which is configured in the LRTConfig contract. However, the strategy associated with a particular asset can be changed using the updateAssetStrategy function in the LRTConfig contract. This could potentially lead to loss of funds if the strategy is changed while there are still assets in the old strategy.

If the strategy for an asset is changed while there are still funds in the old strategy, those funds could be lost or stuck. This is because the NodeDelegator contract might not have a reference to the old strategy anymore, and therefore cannot interact with it.

Proof of Concept

-Initial Setup:

The LRT platform uses the NodeDelegator contract to manage assets and strategies. The platform supports a variety of assets, including rsETH, and uses different strategies to optimize the yield for each asset. The strategies are implemented in separate contracts and can be changed by the LRT manager using the LRTConfig contract.

-Depositing Assets:

The LRT manager decides to deposit a certain amount of rsETH into the platform. They call the depositAssetIntoStrategy function in the NodeDelegator contract, which transfers the rsETH from the NodeDelegator contract to the current strategy for rsETH, let's call it StrategyA.

-Changing the Strategy:

Later, the LRT manager decides to change the strategy for rsETH to a new strategy, StrategyB, because it offers a higher yield. They call the updateAssetStrategy function in the LRTConfig contract, which updates the strategy for rsETH in the NodeDelegator contract.

-The Issue:

However, they forget to withdraw the rsETH from StrategyA before changing the strategy. Now, the NodeDelegator contract thinks that the strategy for rsETH is StrategyB and has no reference to StrategyA anymore. This means it cannot interact with StrategyA to withdraw the rsETH.

The rsETH that was deposited into StrategyA is now stuck. Even though the rsETH is technically still in the platform, it cannot be accessed or managed by the NodeDelegator contract. If the LRT manager tries to withdraw the rsETH, the NodeDelegator contract will try to withdraw it from StrategyB, not from StrategyA, and the withdrawal will fail because there is no rsETH in StrategyB.

This scenario illustrates how changing the strategy for an asset while there are still funds in the old strategy can lead to loss of funds. The NodeDelegator contract loses the reference to the old strategy and cannot interact with it anymore, which means it cannot withdraw the funds from the old strategy.

Tools Used

Foundry

Recommended Mitigation Steps

Withdraw funds before updating strategy: Before updating the strategy for an asset, ensure that all funds are withdrawn from the current strategy. This could be enforced in the updateAssetStrategy function in the LRTConfig contract.

function updateAssetStrategy(address asset, address newStrategy) external onlyLRTManager {
    // Ensure all funds are withdrawn from the current strategy
    address currentStrategy = assetStrategy(asset);
    require(IStrategy(currentStrategy).balanceOf(address(this)) == 0, "Funds still in current strategy");

    // Update the strategy
    _assetStrategies[asset] = newStrategy;
}

Keep track of all used strategies: Modify the NodeDelegator contract to keep track of all strategies that have ever been used for each asset. This would allow it to interact with old strategies even after they have been replaced.

mapping(address => address[]) private _assetStrategies;

function getAssetStrategies(address asset) external view returns (address[] memory) {
    return _assetStrategies[asset];
}

function depositAssetIntoStrategy(address asset) external override {
    // Deposit into the current strategy
    address currentStrategy = lrtConfig.assetStrategy(asset);
    _assetStrategies[asset].push(currentStrategy);
    // ...
}

Restrict who can call updateAssetStrategy: Ensure that only trusted accounts can call the updateAssetStrategy function to prevent unauthorized changes to the strategies.

function updateAssetStrategy(address asset, address newStrategy) external onlyLRTManager {
    // Update the strategy
    _assetStrategies[asset] = newStrategy;
}

Assessed type

Error