Kelp DAO | rsETH - Aamir's results

Lines of code

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

Vulnerability details

ChainlinkPriceOracle fetches prices from the Chainlink contracts. But the price feeds in the consideration has a very long price heartbeat and deviation rate which might lead to wrong price calculation and loss of token to the user.

Impact

According to the chainlink docs, following are the Price feeds info for the assets that will be used initially:

For Info, Heartbeat and Deviation are variables on which the update of the prices for price feeds depends. That mean the current price feed price will be updated for the token only if one of them is met. That means if we take the example of RETH here, the price feed will be update only when the market price for the RETH fluctuates by 2% or time equal to 86400s(1 day) has passed since last price update. This could be a very problematic thing. Let's assume a following scenario:

-> Let's say price feeds was updated at time T1 and price for RETH / ETH is [1]. And since then 43200s(half day) has passed and price has become [1.018] (up by 1.8 %). -> Now Bob comes and decides to deposit 50 RETH by calling LSTDepositPool::depositAsset(...). -> But the amount of RSETH token he will get is equal to:

depositAmount = 500 RETH

// balances
bob = 500 RETH
LRTDepositPool = 0
EigenStrategy = 0
NodeDelegator = 0

// RETH Price
RETHPrice = 1e18 // hasn't updated because heartbeat and deviation hasn't reached


// total ETH (assuming zero for the sake of simplicity)
totalETHInPool = 0
RSETHSupply = 0


///////////////////////////////////////////////////////////////
// functions calculations according to chainlink price feeds //
///////////////////////////////////////////////////////////////

rsETHPrice(...) = if -> 
                      (supply = 0) 1 ETH // this condition will met
                  else -> 
                      totalETHInPool / rsETHTotalSupply

rsEthAmountToMint(...) = depositAmount * RETHPrice / rsETHPrice()
                  = 500 * 1e18 / 1e18 = 500 rsETH


/////////////////////////////////////////////////////////
/// functions calculations according to market price ////
/////////////////////////////////////////////////////////

rsETHPrice(...) = if -> 
                      (supply = 0) 1 ETH // this condition will met
                  else -> 
                      totalETHInPool / rsETHTotalSupply

rsEthAmountToMint(...) = depositAmount * RETHPrice / rsETHPrice()
                  = 500 * (1.018 * 1e18) / 1e18 = ~509 rsETH

As you can see according to the market price, 9 rsETH should be minted extra to the user. But it will not happen as the price feed deviation and heartbeat threshold hasn't passed. And that is why the prices will not be updated. Even if the checks are added in the future for staleness of price, this mechanism will not let anybody deposit token for a whole day because of staleness checks like maxTimestamp etc.

This is one of the valid issue founded in a sherlock audit. Here is a link: Link

Proof of Concept

Calculation and Link given in the above section.

Tools Used

• Solodit
• Manual Review

Recommended Mitigation Steps

It is recommended to use price feeds with less heartbeat and deviation rate. Although the deviation can be handled the heartbeat is still a thing that needs to be considered. Since the time of writing this, Chainlink only support few price feeds for the above tokens with high heartbeat and deviation rate. It is recommended to go for other price oracles or combine more than one together to get the average price.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Users will receive less RSETH tokens when depositing into the contract using LRTDepositPool::deposit(...). The function transfers the deposit tokens to itself and then makes calculation for the amount of RSETH to be minted for the user. Because of this token transfer to self before the calculation for the mint, the users will get less tokens then actual amount.

Impact

The LRTDepositPool::deposit(...) function makes call to different functions from the same contract that again calls other contracts to calculate the amount of token to mint for the user. Here is a high level flow:

LRTDepositPool::deposit()
           |_ LRTDepositPool::_mintRsETH() 
                      |_ LRTDepositPool::getRsETHAmountToMint() 
                                |_ LRTOracle::getAssetPrice()
                                |_ LRTOracle::getRSETHPrice()
                                            |_ LRTOracle::getSupportedAssetList()
                                            |_ LRTOracle::getAssetPrice()
                                            |_ LRTDepositPool::getTotalAssetDeposits()
@>                                                          |_ LRTDepositPool::getAssetDistributionData()
                                                                   |_ NodeDelegator::getAssetBalance()

This function makes a call to LRTDepositPool::getAssetDistributionData()(indirectly) to fetch the total amount of tokens deposited in LRTDepositPool, NodeDelegator and Eigen Layer strategies. To fetch the assets deposited in LRTDepositPool this LRTDepositPool::getAssetDistributionData() function perform the following operation:

File: Breadcrumbs2023-11-kelp/src/LRTDepositPool.sol

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

Github: 79

This just reads the balance of the asset that LRTDepositPool has. Now the problem is, when user calls LRTDepositPool::deposit(), this function first transfer the tokens to itself and then calls _mintRsETH() to calculate the amount of RSETH to mint to the user.

2023-11-kelp/src/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();
        }

        // this will increase the balance before any calculation
@>        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Github: 136-138,141

But when a call reaches to LRTDepositPool::getAssetDistributionData(), the function calculate the assetLyingInDepositPool using the above operation that just reads the balance of token that is stored in LRTDepositPool using ERC20::balanceOf() function. But this balance now also stores the newly added tokens as they were transferred to this contract in the beginning. That means the data in assetLyingInDepositPool will be more than it should be. This amount will be returned back to LRTOracle::getRSETHPrice() after going through some other functions that will add this amount to balance of token stored in NodeDelegator and Eigen Layer strategies.

File: 2023-11-kelp/src/LRTOracle.sol

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

        // expensive?
        return totalETHInPool / rsEthSupply;
    }

Github: 70-71

This totalAssetAmt contains all amount of a token that is stored in different contracts that also includes the newly deposited amount. Then this amount will be multiplied with corresponding ETH price feed and will be stored in totalETHInPool. Now when the RSETH price is returned from the function it will be in ratio of total eth deposited in pools to the total supply of RSETH. And since the RSETH supply will be less than the total deposits of eth stored in totalETHInPool because the corresponding RSETH has not been minted yet. the ratio returned will be more than expected. This ratio will increase if the amount provided to LRTDepositPool::deposit(...) is increased.

Now this ratio will be returned to the LRTDepositPool::getRsETHAmountToMint() and it will perform the following calculation:

File: 2023-11-kelp/src/LRTDepositPool.sol
    
    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        // @audit what if the tokens has different decimals?

@>        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

Github: 95-110

This ratio returned is in the denominator of the calculation that means the bigger this ratio is and the lesser will be rsethAmountToMint. And this rsethAmountToMint is the actual amount of RSETH token that will be minted to the user. So bigger the deposit by the user, the lesser the tokens he will receive.

Proof of Concept

Test for Poc

    function test_depositToPoolMultipleTimes() public {
        uint256 amount = 500 ether;

        for (uint256 i; i < 5; i++) {
            // minting tokens to user to make sure he has enough tokens
            cbETH.mint(alice, amount);

            uint256 userBalanceBefore = IERC20(cbETH).balanceOf(alice);
            (uint256 rsEthMinted) = depositAssetToPool(address(cbETH), amount, alice, i + 1);
            uint256 userBalanceAfter = IERC20(cbETH).balanceOf(alice);

            assertEq(userBalanceBefore - userBalanceAfter, amount, "amount is not same");
        }
    }

Link to the original test: Test

Output

 ------------------------ Run 1:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 0
  LRTDepositPool Balance After Deposit: 500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 500000000000000000000
  TotalSupply of RSETH: 500000000000000000000

  ------------------------------------------------------------------------



  ------------------------ Run 2:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 500000000000000000000
  LRTDepositPool Balance After Deposit: 1000000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 750000000000000000000

  ------------------------------------------------------------------------



  ------------------------ Run 3:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 1000000000000000000000
  LRTDepositPool Balance After Deposit: 1500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 1000000000000000000000

  ------------------------------------------------------------------------



  ------------------------ Run 4:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 1500000000000000000000
  LRTDepositPool Balance After Deposit: 2000000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 1250000000000000000000

  ------------------------------------------------------------------------



  ------------------------ Run 5:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 2000000000000000000000
  LRTDepositPool Balance After Deposit: 2500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 250000000000000000000
  TotalSupply of RSETH: 1500000000000000000000

  ------------------------------------------------------------------------

As you can see the RSETH minted was equal to the token deposited in the first deposit only because the RSETH supply was 0 at that time and LRTOracle::getRSETHPrice(...) function has this check that ensures that the ratio will be 1 ether in case the supply of RSETH is zero:

File: 2023-11-kelp/src/LRTOracle.sol

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

Github: 56

So the ratio of token minted by token deposited in second transaction becomes = 250 / 500 = 0.5. Or we can say for each token deposit we got back 0.5 RSETH.

The mint amount get lesser when bigger amount of tokens are deposited. Here is a test that proves that:

    function test_depositToPoolMultipleTimesDifferentAmount() public {
        uint256 amount = 500 ether;
        uint256 amount2 = 20_000 ether;


        // minting tokens to user to make sure he has enough tokens
        cbETH.mint(alice, amount + amount2);


        uint256 userBalanceBefore = IERC20(cbETH).balanceOf(alice);
        (uint256 rsEthMinted) = depositAssetToPool(address(cbETH), amount, alice, 1);
        uint256 userBalanceAfter = IERC20(cbETH).balanceOf(alice);
        assertEq(userBalanceBefore - userBalanceAfter, amount, "amount is not same");


        userBalanceBefore = IERC20(cbETH).balanceOf(alice);
        (rsEthMinted) = depositAssetToPool(address(cbETH), amount2, alice, 2);
        userBalanceAfter = IERC20(cbETH).balanceOf(alice);


        assertEq(userBalanceBefore - userBalanceAfter, amount2, "amount is not same");
    }

Link to original Test: 373

Output:

PS E:\kelp-audit> forge test --mt test_depositToPoolMultipleTimesDifferentAmount -vv  
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for test/Integration.t.sol:BaseIntegrationTest
[PASS] test_depositToPoolMultipleTimesDifferentAmount() (gas: 413133)    
Logs:


  ------------------------ Run 1:  Deposit Info  ------------------------

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 0
  LRTDepositPool Balance After Deposit: 500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 500000000000000000000
  TotalSupply of RSETH: 500000000000000000000

  ------------------------------------------------------------------------



  ------------------------ Run 2:  Deposit Info  ------------------------

  Deposit Amount: 20000000000000000000000
  LRTDepositPool Balance Before Deposit: 500000000000000000000
  LRTDepositPool Balance After Deposit: 20500000000000000000000
  Change in Balance of LRTDepositPool: 20000000000000000000000
  RSETH Minted to User: 487804878048780487804
  TotalSupply of RSETH: 987804878048780487804

  ------------------------------------------------------------------------


Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 6.11ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)
PS E:\kelp-audit> 

As you can can see for 500 tokens we got 500 RSETH initially and for 20000 tokens we got only 487 RSETH approx. So the ratio of token minted by token deposited becomes = 487 / 20000 = 0.02 approx. Or we can say for each token deposit we got 0.02 RSETH.

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

It is recommended to add the following check after the minting function:

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

        // this will increase the balance before any calculation
-        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
-            revert TokenTransferFailed();
-        }

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

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L63 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L141-L141 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L79

Vulnerability details

The calculation for amount of RSETH to be minted to the user for the deposit made depends upon the balances of different tokens deposited in the LRTDepositPool, NodeDelegator and EigenLayer strategies. But for the calculation of these deposit it uses IERC20(asset).balanceOf(address(this)). This simply means the amount of tokens that these contracts has not the actual amount deposited by the user. Now anyone can transfer the asset directly to the contracts and completely mess up the calculation which will result in less tokens to be minted to the users.

Impact

The LRTDepositPool::depositAsset(...)) function uses LRTDepositPool::_mintRsETH(...) function to calculate the amount of RSETH to be minted to the user. Here is a high level flow of the calls for the function:

LRTDepositPool::deposit()
           |_ LRTDepositPool::_mintRsETH() 
                      |_ LRTDepositPool::getRsETHAmountToMint() 
                                |_ LRTOracle::getAssetPrice()
                                |_ LRTOracle::getRSETHPrice()
                                            |_ LRTOracle::getSupportedAssetList()
                                            |_ LRTOracle::getAssetPrice()
                                            |_ LRTDepositPool::getTotalAssetDeposits()
@>                                                          |_ LRTDepositPool::getAssetDistributionData()
                                                                   |_ NodeDelegator::getAssetBalance()

When the call reaches to LRTDepositPool::getAssetDistributionData(), this function does the following calculation to check the total deposits made by the user:

File: 2023-11-kelp/src/LRTDepositPool.sol

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
@>        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
@>            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

Github: 71

It just uses IERC20(asset).balanceOf(address(this)) to get the balances stored in the contracts. But this balance doesn't mean that the balance returned is the actual deposit made. It is just the amount of tokens the contracts hold. Now anybody can come and mess this calculation up by transferring LRTDepositPool and NodeDelegator some tokens directly without going through the LRTDepositPool::deposit(...). Because of this transfer there will be no corresponding RSETH minted and the amount of tokens stored in the contract will be increased. So assetLyingInDepositPool and assetLyingInNDCs will return incorrect deposit data because of the amount transferred. Now when this amount is returned back to the caller function that is LRTDepositPool::getTotalAssetDeposits(...) it will be combined together and returned back to LRTOracle::getRSETHPrice() function:

File: 2023-11-kelp/src/LRTDepositPool.sol

    function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
@>        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

Github: 47-51

When this amount is received by LRTOracle::getRSETHPrice(), it will multiply the amount will it's corresponding ETH conversion price. And will store it in totalETHInPool. Since the wrong info was returned from the previous function, the amount stored in totalETHInPool will be wrong(will be more if we say more precisely) as well.

File: 2023-11-kelp/src/LRTOracle.sol

    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;
            }
        }
        // @audit can we manipulate the price and get more rsEth? or can we manipulate the price and make the rsEth more
        // expensive?
        return totalETHInPool / rsEthSupply;
    }

Github: 52

And in the end this totalETHInPool is used to return the ratio of total Eth deposited to total RSETH supply. Or we can say for a single RSETH minted, how much ETH(or token in terms of eth) is deposited. since totalETHInPool holds more amount than the actual, the ratio returned will be more than the actual ratio. Now this ratio will be returned back to LRTDepositPool::getRsETHAmountToMint() and it will perform the following calculation:

File: 2023-11-kelp/src/LRTDepositPool.sol

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        // @audit what if the tokens has different decimals?

@>        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

Github: 95

As you can see lrtOracle::getRSETHPrice(...) is in the denominator, that means bigger the value returned from the function is the lesser will be rsethAmountToMint which is the actual amount of RSETH tokens that user will get. And that is what happening actually in this case. That means the amount of tokens minted to the user will be less than it should be. Hence user will be in loss.

Now this is not it, this problem will become even bigger if the amount transferred to NodeDelegator or LRTDepositPool is transferred to EigenLayer strategy. Because then we will not have any control of it in our hand. So that means the function that does the deposit to EigenLayer should never deposit any extra amount rather than the deposit made through LRTDepositPool::depositAsset(...). But that is not what's happening in the contracts. Here is the function that deposit assets to EigenLayer strategy:

File: 2023-11-kelp/src/NodeDelegator.sol

    function depositAssetIntoStrategy(address asset)
        external
        override
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
        onlyLRTManager
    {
        address strategy = lrtConfig.assetStrategy(asset);
        IERC20 token = IERC20(asset);
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

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

        emit AssetDepositIntoStrategy(asset, strategy, balance);

@>        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
    }

Github: 51

As you can see, this also has the same issue that we talked earlier. This will also read the amount of token to be transferred to the strategy based on the same calculation token.balanceOf(address(this)). That means this will deposit all amount of tokens that is stored in the contracts into the strategy and the following line of code will also fetch the wrong balance in LRTDepositPool::getAssetDistributionData(...) that we cannot even change(because now we loose the control of assets) as this line only reads the amount of tokens deposited in the strategy from the strategy itself by using NodeDelegator::getAssetBalance(...) function:

File: 
    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
@>            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

Github: 71

Now the amount of assets returned will always be more than the corresponding RSETH minted and the ratio will go higher and the calculation will calculate less tokens to be minted for the user.

Proof of Concept

Here is a test for PoC

File: 2023-11-kelp/src/LRTDepositPool.sol

   function test_TheRSETHAmountToBeMintedCanBeChangedByAnyoneEasily() public {
        uint256 amount = 500 ether;
        uint256 biggerAmount = 10_000 ether;

        // minting tokens to user to make sure he has enough tokens
        cbETH.mint(alice, 20_000 ether);

        uint256 userBalanceBefore = IERC20(cbETH).balanceOf(alice);
        (uint256 rsEthMinted) = depositAssetToPool(address(cbETH), amount, alice, 1);
        uint256 userBalanceAfter = IERC20(cbETH).balanceOf(alice);

        assertEq(userBalanceBefore - userBalanceAfter, amount, "amount is not same");

        // depositing tokens to node delegator. This is neccessary to make this work.
        // Also it will be very normal that some amount of tokens will remain deposited in the node delegator
        vm.startPrank(manager);
        lrtDepositPoolP.transferAssetToNodeDelegator(0, address(cbETH), amount);
        vm.stopPrank();

        // getting the RSETH amount to be minted for `amount` of tokens
        // the actual amount minted will be less than `rsEthMintedForAmount` because there is
        // another bug in the code that will make the amount less than `rsEthMintedForAmount`
        uint256 rsEthMintedForAmount = lrtDepositPoolP.getRsETHAmountToMint(address(cbETH), amount);

        console2.log("> Getting RSETH Amount to be Minted After the token deposit through `depositAsset(...)`:");
        console2.log("\tAmount To Deposit: %s", amount);
        console2.log("\tAmount of RSETH Tokens to Recieve: %s\n", rsEthMintedForAmount);

        // alice transfer tokens directly to node delegator without going through the deposit pool
        vm.startPrank(alice);
        IERC20(cbETH).transfer(address(nodeDelP), amount);
        vm.stopPrank();

        // again fetching the RSETH amount to be minted for `amount` of tokens
        rsEthMintedForAmount = lrtDepositPoolP.getRsETHAmountToMint(address(cbETH), amount);

        console2.log("> Getting RSETH Amount to be Minted After the direct token deposit of %s amount:", amount);
        console2.log("\tAmount To Deposit: %s", amount);
        console2.log("\tAmount of RSETH Tokens to Recieve: %s", rsEthMintedForAmount);
    }

Link to Original Test: Test

Output:

PS E:\kelp-audit> forge test --mt test_TheRSETHAmountToBeMintedCanBeChangedByAnyoneEasily -vv
[⠰] Compiling...
No files changed, compilation skipped

Running 1 test for test/Integration.t.sol:BaseIntegrationTest
[PASS] test_TheRSETHAmountToBeMintedCanBeChangedByAnyoneEasily() (gas: 460785)
Logs:


  ------------------------ Run 1:  Deposit Info  ------------------------     

  Deposit Amount: 500000000000000000000
  LRTDepositPool Balance Before Deposit: 0
  LRTDepositPool Balance After Deposit: 500000000000000000000
  Change in Balance of LRTDepositPool: 500000000000000000000
  RSETH Minted to User: 500000000000000000000
  TotalSupply of RSETH: 500000000000000000000

  ------------------------------------------------------------------------    

  > Getting RSETH Amount to be Minted After the token deposit through `depositAsset(...)`:
        Amount To Deposit: 500000000000000000000
        Amount of RSETH Tokens to Recieve: 500000000000000000000

  > Getting RSETH Amount to be Minted After the direct token deposit of 500000000000000000000 amount:
        Amount To Deposit: 500000000000000000000
        Amount of RSETH Tokens to Recieve: 250000000000000000000

Note The amount shown in the output is not the actual amount that will be minted to the user when called depositAsset(...) as there is another vulnerability in the contract. The getter function does not account for that vulnerability but it is present in deposit function.

As you can see when alice called LRTDepositPool::getRsETHAmountToMint(...) first time after making a deposit, we were getting 500e18 RSETH for 500e18 cbETH because the deposit made by user before this minted the corresponding RSETH. And that's why the ratio didn't change. But when alice sent token directly, the cbETH amount inside the contracts increased but RSETH supply remained same. which is why the ratio become more than expected. Now we are getting 250e18 RSETH for 500e18 cbETH that make the eth to rsETH ratio 500 / 250 = 2 which was 500 / 500 = 1 in previous case.

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Not able to think of a good scenario because of the complexity involved. This is what i could think of:

It is recommended to create a state inside the NodeDelegator and LRTDepositPool that keeps the track of the token deposited and use that state instead of IERC20(token).balanceOf(address(this)). But there is one more problem with this approach. In order for this to work, the protocol will have to store corresponding shares amount(for tokens that has this mechanism) in that state not the balanceOf() amount. Because if the protocol chooses to use balance directly then this recommendation wouldn't work. Because LSTs are rebasing tokens(mostly) that means the prices of these goes up and down because of rewards distribution and changes in the total supply. Infact stETH token that will be added initially into the pool is a rebasing token. And this token has mechanism to change the balances of the addresses after every few hours [Source: Lido Github]. Lido docs doesn't even recommend to use stETH directly. They recommend to use shares of stETH an address hold instead. More info can be gathered on this from here. So if protocol use stETH balance directly with the current recommendation, it wouldn't work because there are chances that the shares prices of stETH goes down or we can say negative rebase happen (and it has happened before) due to slashing or for some other reason. So this created state (that is totalTokensDeposit in below example) holds the amount that is more than the actual balances of the contracts. And this will cause the transfer calls to revert. And all tokens might get stuck. So instead of storing the stETH balance, use either shares(recommended by Lido) or wstETH which is non-rebasing wrapper around stETH. More info on wstETH is available here.

But using shares can cause some other issues like converting it back to stETH etc. So it is recommended to use wstETH. Now we can do the following changes

File: 2023-11-kelp/src/LRTDepositPool.sol

+   mapping(address token => uint256 amount) public totalTokensDeposit;

    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)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
+       totalTokensDeposit[asset] += depositAmount;
        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

File: 2023-11-kelp/src/LRTDepositPool.sol

+   mapping(address token => uint256 amount) public totalTokensDeposit;
    
    function transferAssetToNodeDelegator(
        uint256 ndcIndex,
        address asset,
        uint256 amount
    )
        external
        nonReentrant
        onlyLRTManager
        onlySupportedAsset(asset)
    {
        address nodeDelegator = nodeDelegatorQueue[ndcIndex];
+       totalTokensDeposit[asset] -= amount;

        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
            revert TokenTransferFailed();
        }
    }

File: 2023-11-kelp/src/LRTDepositPool.sol
+   mapping(address token => uint256 amount) public totalTokensDeposit;

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
-        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
+        assetLyingInDepositPool = totalTokensDeposit[asset];

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
-            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
+            assetLyingInNDCs += INodeDelegator(nodeDelegatorQueue[i]).totalTokensDeposit(asset);

            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

Similar kind of changes needs to be done in all contracts.

It is also recommended to create a withdraw(...) function that can be used to withdraw any access balance by owner or manager that might serve as a profit.

Assessed type

Other

Lines of code

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

Vulnerability details

Chainlink Price Feeds does not maintain a consistent pattern for the number of decimals in returned prices. While some feeds provide token prices with 8 decimals, others may have 18 decimals or more. However, the contracts assume that prices will always be provided with 18 decimals. This could lead to discrepancies in the amount of tokens minted to users, potentially resulting in losses for them. While the price feeds for the initially supported token rETH, cbETH and stETH does have 18 decimals price feeds, but there are chances that in future new tokens are added with different decimals price feeds.

Impact

Some Chainlink price feeds return prices in different decimal places, such as 8 or 18, which may not align with the token's decimal representation. For example, USDC follows 6 decimal places, but the price feeds for USDC/USD return prices in 8 decimal places. You can check the information using the following links:

• Chainlink Price Feeds for USDC/USD: https://docs.chain.link/data-feeds/price-feeds/addresses?network=ethereum&page=1&search=usdc%2Fusd
• USDC Contract on Etherscan: https://etherscan.io/token/0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48#code

So there are good chances that the price feed for the newly added LST follows different decimal representation than the decimal representation of the LST itself.

This inconsistency can result in the amount of tokens minted to the user being less than the expected amount. The incorrect result is calculated by the following functions:

File: 2023-11-kelp/src/LRTDepositPool.sol

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

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

Github: 109

File: 2023-11-kelp/src/LRTOracle.sol

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

Github: 71

Proof of Concept

Here is a test for PoC

    function test_ProtocolWillNotWorkWithChainlinkPriceFeedsWithDifferentDecimalValues() public {
        // adding new LSToken
        addNewLST();

        uint256 amount = 500 ether;
        // updating decimals of the mock price aggregator.
        // this is not possible to do in original price aggregator. It's just a mock funciton to
        // simulate the condition. This stil represents 1 ETH : 1 LST
        mockPriceAggregator.updateDecimals(1e8);

        // checking if the decimals are updated
        assertEq(mockPriceAggregator.decimals(), 1e8, "decimals are not same");

        // getting the RSETH amount to be minted for `amount` of tokens
        // the actual amount minted will be less than `rsEthMintedForAmount` because there is
        // another bug in the code that will make the amount less than `rsEthMintedForAmount`
        uint256 rsEthMintedForAmount = lrtDepositPoolP.getRsETHAmountToMint(address(mLst), amount);

        console2.log("> Details for the Amount to be Mint for LST deposit");
        console2.log("\tAmount To Deposit: %s", amount);
        console2.log("\tAmount of RSETH Tokens to Recieve: %s\n", rsEthMintedForAmount);
    }

Link to the Original Test: Test

Outupt:

Running 1 test for test/Integration.t.sol:BaseIntegrationTest
[PASS] test_ProtocolWillNotWorkWithChainlinkPriceFeedsWithDifferentDecimalValues() (gas: 425074)
Logs:
  > Details for the Amount to be Mint for LST deposit
        Amount To Deposit: 500000000000000000000
        Amount of RSETH Tokens to Recieve: 50000000000


Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.15ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Note The actual amount minted will be different as there is another vulnerability in the function

As you can see in the output, for 500e18 LST tokens we are only receiving 0.00000005 RSETH.

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

To address the issue of inconsistent decimals in the RSETH minting calculation, it is recommended to do the following changes in the contracts:

1. Add a mapping in the contracts to represent the price feeds decimals corresponding to each asset.
2. Modify the LRTDepositPool::getRsETHAmountToMint(...) and LRTOracle::getRSETHPrice(...) functions to use the price feed decimals for the asset when calculating the amount of RSETH to be minted.

Here's an example of how the changes could be implemented:

File: 2023-11-kelp/src/LRTDepositPool.sol

+    mapping(address asset => uint256 priceFeedDecimals) public priceFeedDecimals

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

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

File: 2023-11-kelp/src/LRTOracle.sol
    
    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;
+            totalETHInPool += totalAssetAmt * assetER * (18 - ILRTDepositPool(lrtDepositPoolAddr).priceFeedDecimals(asset));

            unchecked {
                ++asset_idx;
            }
        }
        // @audit can we manipulate the price and get more rsEth? or can we manipulate the price and make the rsEth more
        // expensive?
        return totalETHInPool / rsEthSupply;
    }

By adding the priceFeedDecimals mapping and modifying the calculation, we ensure that the RSETH amount is calculated correctly based on the decimals provided by the price feeds. This mitigates the potential issue of inconsistent decimals and ensures that users receive the correct amount of RSETH for their deposits.

The changes also needs to be done in other functions as well that are using this data.

Assessed type

Decimal