Kelp DAO | rsETH - CatsSecurity's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L45-L79

Vulnerability details

Impact

stETH/ETH, cbETH/ETH, rETH/ETH chainlink oracle has too long of heartbeat and deviation threshold which can wrong calculations and loss of funds

Proof of Concept

Kelp is using the chainlink oracle to get the exchange rate for three LST tokens which is evident as there are no two step conversions in the chainlink oracle:

• stETH
• cbEtH
• rETH

But if we look at the chainlink price feeds all of them have the heartbeat of 24 hours and deviation threshold of 2% for cbEth, 1% for rEth, and 0.5% for stETH which in our case is actually too much and will lead to wrong calculations and mintings due to stale price/exchange rate and will not be updated unless price deviation is 2% or more :

stETH Price Feed

cbETH Price Feed

rETH Price Feed

Now imagine the scenario for the sake of example for one asset cbETH for which the deviation threshold is 2% and heartbeat is 24 hours and within that 24 hours deviation of 1.8% occurs in positive and someone tries to deposit via the kelp.

1. Alice has 4 cbETH which at the current price of cbEth against ETH (1.0538) are worth $8460
2. But the price fetched from the Chainlink oracle is off by 1.8% which means the price returned will actually be 1.8% less than the actual price.
3. Alice will be minted fewer tokens than she is accounted for and the total loss for her will account to be 1.8% of $8460 which is equal to $152 which is a substantial amount of loss and will increase as the deposited amount increases.

Tools Used

Chainlink Price Feed Data and some common sense plus validation from solodit.

Recommended Mitigation Steps

There are two options:

1. Use custom-configured TWAP oracles instead.
2. Use chainlink ETH/USD pair which has the 1-hour heartbeat and 0.5% deviation along with stETH/USD price feed which also has 1 1-hour heat beat and 1% deviation to do two-step conversions and prevent stale prices. The only problem is I was not able to find cbETH/USD and rETH/USD pairs on the chainlink price feed.

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L109 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L79

Vulnerability details

Impact

The vulnerability is similar to how the well-known first deposit attack works, but in this case since there is no withdraw function, the result is huge loss of minted rsETH for the 2nd depositor.

Proof of Concept

The entry-point for depositing into the protocol is LRTDepositPool::depositAsset which invokes _mintRsETH, which in turn invokes getRsETHAmountToMint to calculate how much rsETH to mint for the deposit amount. Taking a closer look at the function we can see that it is calculated by:

// calculate rseth amount to mint based on asset amount and asset exchange rate
rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

Inspecting the LRTOracle::getRSETHPrice function we can see that if the rsETHSupply == 0 it will return 1:

function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

Otherwise, the function loops through the suportedAssetCount storing their combined total value in the totalETHInPool variable, then divides it by the rsEthSupply:

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

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

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;

Attack order:

1. Bob monitors the mempool for the first coming deposit in the contract
2. Alice decides to deposit 10 stETH and mint the corresponding rsETH
3. Bob front-runs her and deposits 0.001 stETH, since rsEthSupply == 0, Bob receives 0.001 rsETH
4. Bob then sends another 0.1 stETH directly to the contract without depositing
5. Since getTotalAssetDeposits(asset) doesn't use internal accounting but balanceOf the totalETHInPool variable is now 0.101, whilst the rsEthSupply is still 0.001
6. Alice's transaction goes through, getRSETHPrice will return 101 since 0.101 / 0.001 and rsethAmountToMint will mint 0.099 instead of 10 rsETH to Alice resulting in a significant loss of value

                                                    // (10 * 1) / 101
rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

The following example was given with whole numbers to maintain readability and simplicity. In solidity it would be notated under 1eN value.

Tools Used

Manual review

Recommended Mitigation Steps

Using internal accounting for storing deposits is a simple mitigation.

Assessed type

Math

[NC - 1] - Add onlySupportedAsset(asset) modifier to getAssetBalance() in NodeDelegator.sol

The modifier can be added to the function to catch the mistake earlier instead of reverting letter

  function getAssetBalance(address asset) 
external 
view 
override 
+ onlySupportedAsset(asset)
returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121-L124

[NC-2] updateMaxNodeDelegatorCount can increase or decrease the maxDelegatorCount but there is no mechanism to remove the extra delegators in case the maxNodeDelegatorCount is decreased from current value

The following function can either increase or decrease the max number of node delegators, lets say currently there are 10 node delegators, all handling the funds. There is no mechanism to remove extras from nodeDelegatorQueue in case the max is decreased from current value.

    function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
      maxNodeDelegatorCount = maxNodeDelegatorCount_;
      emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
  }

[NC-3] Emit the depositers address too in depositAsset()

This instance was not reported in the bot so adding it here.

depositAsset function should emit the address of the depositer too along with the other parameters for easy off-chain tracking.

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        // if it returns true anyways, what will happen?  it will not revert and transfer will happen
        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        emit AssetDeposit(asset, 
                                  depositAmount, 
                                  rsethAmountMinted,
+                               msg.sender);
    }

[NC-4] There is no mechanism to handle in case the deposit limit is decreased below the current deposits in the contracts.

Deposit limit can be increased or decreased anytime but the contracts implement no mechanism to handle in case it is decreased and current deposits in the system are greater than the deposit limit. There should be a mechanism whenever the limit is decreased below current funds the extra should be immediately taken out of eigen layer

    function updateAssetDepositLimit(
        address asset,
        uint256 depositLimit
    )
        external
        onlyRole(LRTConstants.MANAGER)
        onlySupportedAsset(asset)
    {
        depositLimitByAsset[asset] = depositLimit;
        emit AssetDepositLimitUpdate(asset, depositLimit);
    }

[NC-5] Opezeppelin enumerable set can be used in LRTDepositPool.so to better handle the node delegator queue using its build in functions

Enumerable sets have a lot of built-in functions to easily add and remove items from such structures and are also tested and secure. Use them to add and remove items from the nodeDelegator queue, will be more readable too.

    address[] public nodeDelegatorQueue;

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L22

[NC-6] Use Address instead of Addr for more readability

Replace lrtConfigAddr with lrtConfigAddress

        UtilLib.checkNonZeroAddress(lrtConfigAddr);
        __Pausable_init();
        __ReentrancyGuard_init();
        maxNodeDelegatorCount = 10;
        lrtConfig = ILRTConfig(lrtConfigAddr);
        emit UpdatedLRTConfig(lrtConfigAddr);
    }

[NC-6] getLstToken is unnecessary, being not used anywhere, if it is for frontend simply use the supportedAssetList to get the desired token

-    function getLSTToken(bytes32 tokenKey) external view override returns (address) {
-       return tokenMap[tokenKey];
-    }

[NC-7] Keep track of the token each user deposited and the amount deposited by an address, may help when implementing the withdraw flow.

In the deposit function we can keep track of each token an address deposited and the amount deposited, while implementing the withdraw there can be multiple nuances and such structure may help in doing right calculations.

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        // if it returns true anyways, what will happen?  it will not revert and transfer will happen
        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

+     amountDeposited[msg.sender][asset] = depositAmount;

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Kelp Protocol Analysis

Description of Kelp

Kelp represents a collaborative DAO with the purpose of enhancing liquidity, DeFi, and maximizing rewards for assets that are restaked. The core of this initiative lies in the integration of a single liquid restaked token made for recognized LSTs, ensuring seamless accessibility. The protocol will allow users to employ their rsETH within EigenLayer strategies, enabling them to generate returns. The ongoing development of rsETH will be carried out within the framework of the Kelp brand.

1. System Overview

1.1 Scoping

• LRTConfig: This contract includes the functionality to configure the system such as adding new supported assets, updating assets' deposit limit, updating an asset's strategy, setting the rsETH contract address, setting the token address and numerous getter functions.
• LRTDepositPool: This contract is the main user-facing contract and includes the functionality to deposit assets, mint rsETH, add node delegator's to the queue, transfer assets to node delegators, update the max node delegators count and numerous getter functions.
• NodeDelegator: This contract is the recipient of funds from the LRTDepositPool contract and in turn sends them to the EigenLayer strategies, includes functionality to approve assets to EigenLayer, deposit assets into the strategy, transfer funds back to LRTDepositPool, and numerous getter functions.
• LRTOracle: This contract is utilized as the Oracle to get prices of liquid staking tokens, includes functionality to update the price oracle for supported assets, as well as fetch supported asset and rsETH prices.
• RSETH: This contract is the rsETH ERC20 Token the protocol uses and contains all the standard functionality as per OpenZeppelin's requirements.
• ChainlinkPriceOracle: This contract is the Chainlink Price Oracle wrapper to integrate their oracles in the LRTOracle and contains all standard Chainlink Oracle functionality.
• LRTConfigRoleChecker: This contract is abstract and is responsible for handling role checks such as onlyLRTManager, onlyLRTAdmin, onlySupportedAsset, and can also be used to update the LRTConfig contract.
• UtilLib: This contract is a simple helper library that contains a check for non zero address function.
• LRTConstants: This contract is contains all the shared constant variables used within the system such as R_ETH_TOKEN, ST_ETH_TOKEN, CB_ETH_TOKEN, LRT_ORACLE, LRT_DEPOSIT_POOL, EIGEN_STRATEGY_MANAGER, MANAGER.

1.2 Access Control Roles

The contracts uses the following 4 access control modifiers for different mechanisms within the system:

• DEFAULT_ADMIN_ROLE: Used for core administrative functions within the system that have to do with initial setting of addresses as well as updating asset strategies.
• onlyLRTADMIN: Used for functionality within the system that has to do with node delegaotrs and unpausing the contracts.
• onlyLRTMANAGER: Used for functionality within the system that has to do with node delegators, transfer of funds between contracts and EigenLayer strategies and price oracle configurations.
• LRTConstants.MANAGER: Used for functionality within the system that has to do with supported assets and their deposit limits.

1.3 Access Restricted Functions

• DEFAULT_ADMIN_ROLE: This role has access to administrative functions such as updating asset strategies, setting the rsETH address, setting the token address & setting the contract address.
• onlyLRTADMIN: This role has access to functions related to the Node Delegators such as adding new node delegator contracts to the queue, updating the maximum node delegator count and unpausing the various contracts that allow for that.
• onlyLRTMANAGER: This role has access to other functions related to the Node Delegators such as transferring assets from the deposit pool to the node delegator contracts, as well as approving assets to the EigenLayer strategy manager, depositing assets from node delegators to the strategy, transferring funds from node delegators back to the deposit pool, updating the price oracle of suported assets, adding or updating the oracle price feed for assets, and pausing the various contracts that allow for that.
• LRTConstants.MANAGER: This role has access to functions related to adding new supported assets, and updating the asset deposit limits.

2. Architecture Overview & Diagrams

2.1 Architecture Overview

• LRTConfig: The LRTConfig contract is a critical part of the Kelp protocol. It contains the logic for the system's configuration such as supported assets and their deposit limits and asset strategies.

• LRTDepositPool: The LRTDepositPool contract is the main user-facing contract in the protocol responsible for taking in deposits and minting rsETH for users. Users can also get information related to current asset limits, total asset deposits and the node delegator queue. Restricted roles have access to node delegator and transferring of funds functions inside this contract.

• LRTOracle: The LRTOracle contract is the one responsible for fetching prices of LSTs (liquid staking tokens). It works together with the LRTDepositPool to maintain accurate price validity and helps in calculating how much rsETH to mint for users.

• RSETH: The RSETH Contract is the system's ERC20 token implementation. Not much to say here.

• ChainlinkPriceOracle: The ChainLinkPriceOracle contract is the wrapper contract used to integrate Chainlink's Oracles. It works together with the LRTOracle contract to supply it's fetched data.

• LRTConfigRoleChecker: The LRTConfigRoleChecker contract is the system's implementation for access control modifiers enabling certain functions to only be accessible by privileged roles.

• UtilLib: The UtilLib contract is a helper function lib that contains a check for non zero address function.

• LRTConstants: The LRTConstants contract contains the constant variables shared between the contracts such as R_ETH_TOKEN & other supported LSTs, LRT_ORACLE & other contracts, as well as the MANAGER role.

2.2 Architecture Diagram

The architecture diagram visualizes how the contracts in the system interact with eachother and provides short descriptions of the contracts' intended mechanism.

2.3 Flow of Funds Diagram

The flow of funds diagram visualizes the process through which a user goes in order to deposit their assets into the protocol and how they travel to their final destination of the EigenLayer strategies.

2.4 Core Contracts Parameters Diagram

The core contracts parameters diagram visualizes the main parameters and/or storage variables in the system and what they represent.

3. Codebase Assessment

The total nSLOC of the contracts in scope are quite low but it can be seen that the developers have done an excellent job of integrating all the functionality properly. Config, Deposit Pool, Oracles & Node Delegaotrs communicate seamlessly with access control roles functioning as intended wherever needed. The system includes pause functionality which, in my opinion, is always a good idea.

• General Redability: The codebase exhibits sound coding practices, upholding readability and organization through well-defined variables and function names.
• Code Style: The code adheres to a uniform style and indentation pattern, complemented by in-line comments that provide comprehensive guidance throughout it's mechanisms.
• Gas: Gas efficiency is effectively upheld.
• Test Suite: With a test suite coverage of 98%, the protocol did an almost perfect job. The setting up of environments for tests was easy and straight-forward.
• Errors & Events: This is something that codebase lacked in. I didn't come across any handling neither of errors, nor events.
• Upgradability: All core contracts of the protocol maintained upgradabillity, with plans of future expansion & integration this should be a must.

4. Level of Documentation

The documentation provided in the github repo was not extensive, although it's understandable for such a small codebase. There was a link to their blog with some additional information on more in-general subjects such as "What is restaking", "What is EigenLayer" and such. That's all good but I would have liked more techinical documentation about the project.

5. Systemic & Centralization Risks

5.1 Systemic Risks

Although the codebase is small, since this is the protocol's first audit I wouldn't be surprised if a few bugs arise, it is our goal to clear them. I would usually say there is always room for improvement codebase-wise but I belive the developers have done an excellent job here. The owner's have stated that they intend to expand the codebase and conduct subsequent audits for the expansion which is great. Me and my teammate were able to uncover a few vulnerabilities and I am sure other auditors will come up with more stuff.

• Loss of value upon mint: Our team uncovered a vulnerability which if exploited could set the system up in such a way that the 2nd depositor's minting value is significantly reduced. This is quite has quite a serious impact although potentially only possible for the 2nd depositor, even if the system currently has no withdraw functionality, it is still a major loss of value. The issue arises from the fact that the codebase does not use internal accounting during a crucial division to calculate the rsETH amount to mint, but instead takes the contracts' balances. This opens the door for an attacker to simply donate/directly send value instead of going through the deposit function and sway the results.

• Future integration with LSTs with less than 1e18 decimals: Our team uncovered a vulnerability based on the developer's assumption that all LST tokens used as supported assets for the system will use 1e18 decimals. The developers communicated in a private discord thread that they are planning to add more supported LST assets in the future if EigenLayer expands.

Gus — Yesterday at 6:38 PM
When Eigenlayer adds/accepts more LSTs, we will want to support those as well

If in the future additional assets are supported with a decimal of more/less than 1e18, the calculaction used in the for-loop at LRTOracle::getRSETHPrice#L66 will return heavily skewed results and thus minting a way bigger/smaller amount than it should.

5.2 Centralization Risks

Parts of the functionality of the system is locked behind priviliged-role access control modifiers, functions such as adding new supported assets, setting their deposit limits, increasing the node delegator count, pause/unpause of the protocol, transfer of funds between contracts and EigenLayer strategies and oracle integration.

All of these are expected, except maybe letting users control how many of their funds, and when, are allocated to EigenLayer strategies. The codebase is still in a very early stage (especially seeing a Withdraw function is missing), and it might be in the plans of the developers to allow for such actions to be decided by the users, but as it stands right now, I feel like that part is rather centralized and an improvement to this would be to allow the users to choose.

6. Team Findings

Time Spent ⌛

Time spent:

35 hours