Kelp DAO | rsETH - ZanyBonzy's results

Lines of code

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

Vulnerability details

Impact

Previous strategy is overwritten and can cause tokens to be lost forever.

Proof of Concept

When the asset strategy is updated, it doesn't transfer the current strategy asset balance to the new one.

   function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)
    {
        UtilLib.checkNonZeroAddress(strategy);
        if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;
    }

This futher translates to the getAssetBalance and getAssetBalances functions which inturn affects the returned value of the getTotalAssetDeposits and affects the rsETH pricing. See my other issues for more info.

 function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this)); //@note checks the for the exact strategy.
    }

Tools Used

Manual code review

Recommended Mitigation Steps

Use safeTransferFrom to send the asset balance to the new strategy.

Assessed type

Other

1. Unnecessary cast in getRSETHPrice should be avoided

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
       ...

        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;
    }

The asset_idx counter is a uint16 value and is being compared to the uint256 supportedAssetCount variable. In making this comparison, the asset_idx is repeatedly casted into uint256, which is unnecessary. Also, in the rare case that the supportedAssetCount is greater than type(uint16).max, the constant unchecked increment in the loop might cause the asset_idx value to wrap around and start over from 0 when it reaches its max value. This results in an infinite loop which can dos system.
If the uint16 type is desired, then consider implementing a safeCast library to protect against overflows.

2. updatePriceOracleFor function doesn't check if oracle already exists.

   function updatePriceOracleFor(
       address asset,
       address priceOracle
   )
       external
       onlyLRTManager
       onlySupportedAsset(asset)
   {
       UtilLib.checkNonZeroAddress(priceOracle);
       assetPriceOracle[asset] = priceOracle;
       emit AssetPriceOracleUpdate(asset, priceOracle);
   }
   

3. updatePriceFeedFor function doesn't check if priceFeed already exists.

      function updatePriceFeedFor(address asset, address priceFeed) external onlyLRTManager onlySupportedAsset(asset) {
       UtilLib.checkNonZeroAddress(priceFeed);
       assetPriceFeed[asset] = priceFeed;
       emit AssetPriceFeedUpdate(asset, priceFeed);
   }
   
   

4. The RSETH contract implements a burnFrom function.

    function burnFrom(address account, uint256 amount) external onlyRole(BURNER_ROLE) whenNotPaused {
        _burn(account, amount);
    }

However, its interface IRETH implements a burn function.

    function burn(address account, uint256 amount) external;

Consequently, any indirect calls made from outside the RETH contract to burn tokens will fail and tokens will not be burnt.

5. .approve() in maxApproveToEigenStrategyManager(https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L45) is vulnerable to frontrunning which can lead to loss of assets. Use safeApprove or safeIncreaseAllowance instead. Also, approve first to 0 and check for return values.

        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);
        IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);
    }

6. Consider comparing to-be-transfered amount to NodeDelegator's balance before transfer to prevent reduce chances of failure. It protects from sending more than the contract's balance or 0 amount.

    function transferBackToLRTDepositPool(
    ...
    {
        address lrtDepositPool = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
        //@note check for balance
        if (!IERC20(asset).transfer(lrtDepositPool, amount)) {
            revert TokenTransferFailed();
        }
    }

7. The updateMaxNodeDelegatorCount function should implement a check to make sure the new maxNodeDelegatorCount_ is not less than current maxNodeDelegatorCount. Unless this is intended, this can lead to unexpected behaviours.

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

8. The protocol uses stETH as one of its base assets. stETH is a token subject to variable balances, it rebases both positively and negatively. This token type is a source of accounting issue, which consequently leads inflates/deflates the amount of rsETH tokens that the user gets. The protocol also doesn't implement balance checks before the transfer and transferFrom calls are made. Recommend introducing a balance before and after check before calling the safe transfer options to ensure accurate accounting. Note that this might leave the contract vulnerable to reentrancy, the reentrancy guard should be implemented.

9. Consider adding a timelock to critical functions to give time for users to react and adjust to critical changes. Functions that involve making critical updates like updatePriceOracleFor, updateLRTConfig, updateAssetDepositLimit and so on will benefit from this. It also protects from admin errors.

1. Use uint256(1)/uint256(2) instead of true/false to save gas for changes

Avoids a Gsset (20000 gas) when changing from false to true, after having been true in the past.

mapping(address token => bool isSupported) public isSupportedAsset;

LRTConfig.sol L15

2. Initializers can be marked payable

    function initialize(

LRTConfig.sol L41

 function initialize(address lrtConfigAddr) external initializer {

LRTDepositPool.sol L31

    function initialize(address lrtConfigAddr) external initializer {

LRTOracle.sol L29

    function initialize(address lrtConfigAddr) external initializer {

NodeDelegator.sol L26

    function initialize(address admin, address lrtConfigAddr) external initializer {

RSETH.sol L32

    function initialize(address lrtConfig_) external initializer {

ChainlinkPriceOracle.sol L27

3. Instead of using address(this), it is more gas-efficient to pre-calculate and use the hardcoded address, especially if you need to use the same address multiple times in your contract.

The reason for this, is that using address(this) requires an additional EXTCODESIZE operation to retrieve the contract's address from its bytecode, which can increase the gas cost of your contract. By pre-calculating and using a hardcoded address, you can avoid this additional operation and reduce the overall gas cost of your contract.

        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

LRTDepositPool.sol L79

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {

LRTDeposit.sol L136

        uint256 balance = token.balanceOf(address(this));

NodeDelegator.sol L63

           IEigenStrategyManager(eigenlayerStrategyManagerAddress).getDeposits(address(this));

NodeDelegator.sol L103

            assetBalances[i] = IStrategy(strategies[i]).userUnderlyingView(address(this));

NodeDelegator.sol L111

        return IStrategy(strategy).userUnderlyingView(address(this));

NodeDelegator.sol L123

4. x = x + y is cheaper than x += y;

            totalETHInPool += totalAssetAmt * assetER;

LRTOracle.sol L71

            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).

LRTDepositPool.sol 83-84

5. Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

See this issue for a detail description of the issue

    bytes32 public constant R_ETH_TOKEN = keccak256("R_ETH_TOKEN");
    //stETH token
    bytes32 public constant ST_ETH_TOKEN = keccak256("ST_ETH_TOKEN");
    //cbETH token
    bytes32 public constant CB_ETH_TOKEN = keccak256("CB_ETH_TOKEN");

    //contracts
    bytes32 public constant LRT_ORACLE = keccak256("LRT_ORACLE");
    bytes32 public constant LRT_DEPOSIT_POOL = keccak256("LRT_DEPOSIT_POOL");
    bytes32 public constant EIGEN_STRATEGY_MANAGER = keccak256("EIGEN_STRATEGY_MANAGER");

    //Roles
    bytes32 public constant MANAGER = keccak256("MANAGER");

LRTConstants.sol

    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");
    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

RSETH.sol L21-22

Protocol Overview

Kelp is a collective DAO designed to unlock liquidity, DeFi and higher rewards for restaked assets. It aims to address the potential challenges of restaking, such as high gas fees, liquidity constraints, and node operator discovery by introducing a new Liquid Restaked Token which is issued for restaked ETH. At the heart of the protocol is the rsETH token which is an LRT that provides liquidity to illiquid assets deposited in restaking platforms.

---

Codebase Analysis

As the codebase is still in the development phase, it's not very large. However, it's well-structured, and functions are broken down into small, easy-to-understand bits - not too cyclomatically complex. The codebase (in scope) comprises a total of 17 contracts (8 interfaces and 9 actual contracts) and about 500 SLoC. The codebase mostly uses inheritance, defines no structs, and includes 2 external calls (to Chainlink oracle and to EigenLayer). The overall test line coverage is about 98%. The core contracts are designed to be upgradeable; therefore, the protocol uses OZ upgradeable imports.

• The contracts in scope are
  1. LRTConfig.sol is the protocol's configuration contract that also stores the protocols' contract addresses.
  2. LRTDepositPool.sol is the contract through which users deposit funds to the protocol, and have their funds transferred to the NodeDelegator contracts.
  3. NodeDelegator.sol receives funds from the depositpool and delegates them to the eigenlayer strategy.
  4. LRTOracle.sol fetches prices of LST tokens from oracles.
  5. RSETH.sol is the token a user receives upon depositing in LRTDepositPool.sol.
  6. ChainlinkPriceOracle.sol is integrates chainlink oracles in LRTOracle.
  7. LRTConfigRoleChecker.sol performs role checks.
  8. UtilLib.sol is the helper function library. It handles 0x0 address checks.
  9. LRTConstants is contains the global constant variables.

and the interface contracts

---

Architecture Review

Restakers stake their ETH/liquid tokens by transferring them into the depositPool contract. This mints them the rsETH tokens in return, based on the current exchange rate. The staked ETH/liquid tokens are then transferred to the NodeDelegator contract. From here, they are delegated to the Eigenlayer protocol through predefined strategies. It is important to note that as of now, there is no way to withdraw directly from the protocol, but the team has confirmed that it is in the works.

---

Protocol Risks

• Some of the risks that can affect the protocol include
  1. Risk from centralization, malicious or compromised managers and admins.
  2. Risks from third-party contract dependencies - OZ contracts, ChainLink AggregatorInterface, EigenLayer strategyManager.
  3. Risks from compromised smart contracts in the protocol.
  4. Issues with ChainLink oracle can affect asset prices and consequently, amount of rsETH tokens minted.
  5. Non-standard ERC20 token types, if they get integrated. The protocol's stETH is a token with variable balance and can result in accounting issues.

---

Approach to Audit

• We approached the audit in four steps.
  1. We reviewed the provided documents, blogs, and also noted other explanations provided by the developers on Discord.
  2. We ran the contracts through Slither and compared the generated report to the bot reports. From this, we weeded out the known issues and false positives.
  3. We carefully reviewed the codebase, ran provided tests, and worked out various attack vectors. We also tested the functions' logic to make sure they work as intended.
  4. We noted our findings and created the needed reports.

---

Recommendations

• 1. The protocol uses the AggregatorInterface which is a deprecated Chainlink API. Consider switching to the newer AggregatorV3Interface.
  2. Consider also, an alternative/backup oracle to reduce Chainlink dependency.
  3. Use the safe ERC20 operations and introduce balance checks before and after transfers.
  4. Consider introducing a multiplier constant to deal with possible precision loss when performing division operations.
  5. Consider introducing functions to remove assets/strategies from the suppported lists in case support for any of the currently-in-use ones is stopped.
  6. Consider creating a proper documentation through which users and developers can better understand the contracts and functions.
  7. Consider reformatting the functions to better align with the Solidity style guide and introducing NatSpec comments.

  ---

Conclusion

The KelpDao seems interesting and we're anticipating what the team has in store next. The architecture is solid, the codebase is well written, and most of the easy-to-overlook safety measures are actually implemented. However, the identified risks need to be mitigated, and provided recommendations should be taken into consideration. Constant upgrades and audits should be invested in to ensure the security of the protocol.

Time spent:

20 hours