Kelp DAO | rsETH - glcanvas's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L119-L145

Vulnerability details

Impact

User may receive less amount of rsETH during token minting (When calling LRTDepositPool#depositAsset. Because, amount of rsETH to be minted depends on total supply and underlying asset prices.

This might be happens due to:

• Transaction stuck;
• Oracle's underlying price changes significantly.

Proof of Concept

Let's consider next case:

1. User decided to deposit 1ether of stETH asset, so he at the first calls getRsETHAmountToMint(stETH, 1 ether), to determine how many rsETH he will get. He expected to receive 1 ether of rsETH;
2. After that, when he understand how many tokens he get, user calls depositAsset(stETH, 1 ether), but due to high network loads his transaction might stuck in pool for some time, for example two hours;
3. After this two hours -- prices can be changed significantly, because to determine RsETH amount to mint the protocol is used Chainlink service. And rsETH amount to mint = asset_amount * asset_price * rsETH_supply / (∑ asset_amount_i. * asset_price_i). As you can notice from formula above, the amount of rsETH depends on multiple parameters, which can be changed independently (stETH, rETh and cbETH don't depends on each other, so their prices may change);
4. When a lot of time passed, and user's transaction applied due to underlying price changing he can receive instead of 1 ether just 0.75 ether of rsETH.

This behavior is incorrect, and should be prohibited due to prevent occasionally user's lost.

Tools Used

Manual review

Recommended Mitigation Steps

To function depositAsset add parameter minOutputAmount, which corresponds minimum amount of rsETH to be minted to user, or revert transaction otherwise. See how it's implemented in uniswap: https://github.com/Uniswap/v2-periphery/blob/master/contracts/UniswapV2Router01.sol

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

Current implementation of EIGEN_STRATEGY_MANAGER is proxy, which means that Eigen Strategy manager can be changed in future. NodeDelegator handles passed asset, and delegates them to Eigen Strategy manager, which delegates them to one of strategy. If something happens with EigenLayer, and attacker can operate EigenLayer protocol, he can withdraw all funds from NodeDelegator, because this contract gives infinity approve to strategy manager address.

So, to sum up: In cases of EigenLayer will be hacked, Kelp may lost all money from NodeDelegator protocol.

Of course, manager can transfer money back to DepositPool, but old approve to StrategyManger not going anywhere, and any further transfers to NodeDelegator will carry the risks of losing money.

Proof of Concept

Let's consider next case:

1. Funds storages in DepositPool;
2. Admin transfer funds to NodeDelegator;
3. Admin gives infinite approve to EigenLayer's strategy manager for all assets uses: maxApproveToEigenStrategyManager;
4. EigenLayer hacked, and Kelp's protocol team moved funds from NodeDelegator to DepositPool ASAP;
5. EigenLayer decided to move strategy manager to new address;
6. If now Kelp protocol moves funds back to NodeDelegator even if Kelp's team update EIGEN_STRATEGY_MANAGER address, then previously given approve is still active, and compromised address can execute transferFrom for all approved funds.

So, correct way to resolve such issue is add function to remove allowance for EIGEN_STRATEGY_MANAGER contract.

Tools Used

Manual review

Recommended Mitigation Steps

Add function to emergency revoke approve for contract EIGEN_STRATEGY_MANAGER.

Assessed type

Other

Approach taken in evaluating the codebase

To understand protocol was used EigenLayer documentation.

Flow:

1. Start with understanding linked protocol. This protocol is EigenLayer. Read whitepaper and linked documentation first.
2. Then determine which part of EigenLayer protocol used by Kelp DAO. The audited protocol uses only Strategy Manager and Strategy. Current implementation of Kelp DAO doesn't support withdrawn.
3. Then proceed to manual code review.

Architecture recommendations

Current architecture is well designed.

It's absolutely correct to separate DepositPool and NodeDelagator into two different contracts. Because, if one of these contract will be hacked, then protocol lost only one part.

But here is one detail, which is important to highlight. Withdraws. Current implementation doesn't support withdraws. Devs told that it will be added later. But, when we deposit some asset (like stETH and rETH) to Kelp -- we erase the boundaries of these tokens and after that moment it becomes unclear who contributed which tokens. For example:

• Alice deposits 1 stETH, he got 1 rsETH;
• Bob deposits 1 rETH, he got 1 rsETH;
• Now we have 2 rsETH and 1 stETH and 1 rETH, but don't know who initial depositor.

Moreover, if you go to prod right now, it will be impossible to determine later who deposit which tokens.

Need to support additional mapping of: user => asset => initial asset amount.

Codebase quality analysis

Codebase is really good, all cases are handled correctly. Dev team writes good and clear comments and uses meaningful named-attributes in mapping.

However, devs uses transfer, transferFrom and approve instead of safe version of these functions. Better to use safe implementation by OppenZeppelin, because tokens, which will be added in future may return nothing in transfer.

Additional notice: LRTConfig has tokenMap which is used for clarity description of what address responsible for. But current implementation gives possibility to update one of isSupportedAsset or depositLimitByAsset or assetStrategy keys without tokenMap modification. These should be fixed and work synchronously.

Centralization risks

Current protocol's version is absolute centralized. Core contracts, and core functions: DepositPool, NodeDelegator is under modifiers. Only admin can move funds to strategy. After you enter to protocol, only admins can control your money. This behavior is not desired.

Better develop or think about more freedom for users in terms of transfer funds.

Mechanism review

The system consist of three main parts:

• First part is DepositPool with Oracle. This is main entry point. User deposits funds use DepositPool contract, and Oracle contract uses to calculate price;

• Second part is NodeDelegator part. This part responsible to communication with EigenLayer. Funds passed to this contract are passed to EigenLayer's Strategies, which bring profit;

• Last one part is RsETH token, when user enter to protocol via DepositPool he receives RsETH, which is proof of ownership.

So, whole protocol in a general sense, can be represented as the following scheme:

  User
   |
   | User's funds
   ∨
--------------- transfer assets  -----------------  enter into strategy --------------
| DepositPool | <============>   | NodeDelegator |  =====>              | EigenLayer |
---------------                  -----------------                      --------------
       ^
       | 
       | Request Price
       ∨
   ---------- 
   | Oracle |
   ----------

Systemic risks

A systemic risk in this system is the dependence on ChaniLink.

In future, when protocol will have bigger liquidity, better to additionally fetch price from on chain dex, like it's implemented in GMX.

Because, Oracle can be stopped, can be hacked, so instead of relay only on one system, better has fallback option. Or even better to use two price providers (Oracle and DEX) and use mean price.

Time spent:

12 hours