Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Kelp DAO | rsETH smart contract system written in Solidity. The audit took place between November 10—November 15 2023.

Wardens

194 Wardens contributed reports to the Kelp DAO | rsETH audit:

1. m_Rassska
2. CatsSecurity (nirlin and cats)
3. adriro
4. Bauchibred
5. PENGUN
6. SBSecurity (Slavcheww and Blckhv)
7. deth
8. 0xDING99YA
9. jayfromthe13th
10. Aamir
11. HChang26
12. almurhasan
13. d3e4
14. anarcheuz
15. circlelooper
16. hals
17. Bauer
18. ck
19. Pechenite (Bozho and radev_sw)
20. Aymen0909
21. 0xbrett8571
22. crack-the-kelp (Parth and wildanvin)
23. max10afternoon
24. hunter_w3b
25. lsaudit
26. 0xmystery
27. glcanvas
28. btk
29. ge6a
30. turvy_fuzz
31. Daniel526
32. Shaheen
33. 0xhacksmithh
34. 0xMilenov
35. 0xVolcano
36. 0xepley
37. T1MOH
38. osmanozdemir1
39. zhaojie
40. unique
41. ast3ros
42. DanielArmstrong
43. sakshamguruji
44. ZanyBonzy
45. albahaca
46. foxb868
47. invitedtea
48. chaduke
49. jasonxiale
50. tallo
51. 0xffchain
52. deepkin
53. Stormy
54. bLnk
55. roleengineer
56. nmirchev8
57. rouhsamad
58. GREY-HAWK-REACH (Kose, dimulski, and aslanbek,Habib0x)
59. ayden
60. rvierdiiev
61. AlexCzm
62. adam-idarrha
63. QiuhaoLi
64. Ruhum
65. mahdirostami
66. xAriextz
67. 0x1337
68. Juntao
69. 0xluckhu
70. cryptothemex
71. trachev
72. deepplus
73. Weed0607
74. Jiamin
75. crunch
76. Varun_05
77. 7siech
78. 0xNaN
79. Sathish9098
80. ABAIKUNANBAEV
81. 0xSmartContract
82. fouzantanveer
83. SAAJ
84. clara
85. KeyKiril
86. Myd
87. digitizeworx
88. tala7985
89. Raihan
90. 0xAnah
91. JCK
92. 0xhex
93. 0xta
94. shamsulhaq123
95. Rickard
96. rumen
97. spark
98. Madalad
99. Phantasmagoria
100. SandNallani
101. bronze_pickaxe
102. zach
103. pep7siup
104. twcctop
105. joaovwfreire
106. TheSchnilch
107. gumgumzum
108. 0xrugpull_detector
109. wisdomn_
110. ke1caM
111. critical-or-high
112. ubl4nk
113. Krace
114. peanuts
115. mahyar
116. qpzm
117. peter
118. SpicyMeatball
119. ptsanev
120. Banditx0x
121. shealtielanz
122. _thanos1
123. 0xAadi
124. phoenixV110
125. poneta
126. 0xvj
127. Udsen
128. gesha17
129. Topmark
130. dipp
131. adeolu
132. leegh
133. cartlex_
134. RaoulSchaffranek
135. Matin
136. cheatc0d3
137. Tumelo_Crypto
138. Yanchuan
139. 0xHelium
140. Amithuddar
141. Inspecktor
142. 0xblackskull
143. 0xLeveler
144. baice
145. niser93
146. ro1sharkm
147. hihen
148. McToady
149. bareli
150. eeshenggoh
151. seerether
152. Soul22
153. AerialRaider
154. Cryptor
155. boredpukar
156. TeamSS (0xSimeon and 0xsagetony)
157. alexfilippov314
158. Draiakoo
159. amaechieth
160. squeaky_cactus
161. King_
162. MatricksDeCoder
163. Noro
164. paritomarrr
165. soliditytaker
166. ziyou-
167. codynhat
168. passion
169. catellatech
170. supersizer0x
171. stackachu
172. evmboi32
173. taner2344
174. marchev
175. Stormreckson
176. Eigenvectors (Cosine and timo)
177. ElCid
178. desaperh
179. debo
180. merlinboii
181. pipidu83
182. LinKenji
183. zhaojohnson
184. MaslarovK
185. Tadev

This audit was judged by 0xDjango.

Final report assembled by liveactionllama.

Summary

The C4 analysis yielded an aggregated total of 5 unique vulnerabilities. Of these vulnerabilities, 3 received a risk rating in the category of HIGH severity and 2 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 125 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 16 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Kelp DAO | rsETH repository, and is composed of 10 smart contracts written in the Solidity programming language and includes 498 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, Hound from warden DadeKuma, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (3)

[H-01] Possible arbitrage from Chainlink price discrepancy

Submitted by m_Rassska, also found by SBSecurity, Aamir, d3e4, adriro, CatsSecurity, jayfromthe13th, Bauchibred, deth, 0xDING99YA, HChang26, almurhasan, PENGUN, anarcheuz, and circlelooper

Some theory needed

• Currently KelpDAO relies on the following chainlink price feeds in order to calculate rsETH/ETH exchange rate:

• As we can see, an acceptable deviation for rETH/ETH price feed is about [-2% 2%], meaning that the nodes will not update an on-chain price, in case the boundaries are not reached within the 24h period. These deviations are significant enough to open an arbitrage opportunities which will impact an overall rsETH/ETH exchange rate badly.
• For a further analysis we have to look at the current LSD market distribution, which is represented here:

• Where LRTDepositPool ratio is an approximate ratio of deposited lsts based on the overall LSD market.

An example of profitable arbitrage

See original submission for full details.

Final words

Basically, price feeds don’t have to reach those extreme boundaries in order to profit from it. In theory presented above we were able to get +2.3% profit, which is significant in case there is a huge liquidity supplied. The combination of deviations might be absolutely random, since it operates in set of rational numbers. But it will constantly open a small [+1%; +1.5%] arbitrage opportunities to be exploited.

Proof on Concept

Recommended Mitigation Steps

Short term:

• N/A

Long term:

• I was thinking about utilizing multiple price oracles, which could potentially close any profitable opportunities, but the gas overhead and overall complexity grows rapidly. Unfortunately, I don’t have anything robust to offer by now, but open to discuss about it.

Assessed type

Math

gus (Kelp) disputed and commented:

We appreciate the explanation at length you have here. We see the arbitrage as a feature rather than a bug in the system. The past 2 year data on the price discrepancy assures us that this is a minor vector that will not impact deposits or withdraws significantly. Moreover, the fact that minters earn nominally more and withdrawals earn nominally less balances the system. We also want to call out that price arbitrage is not a unique problem to our design. It is virtually present across every staking protocol and we encourage healthy arbitrage opportunity here.

0xDjango (judge) commented:

This is valid. Depositors will be able to deposit the minimally-priced asset and steal value from the deposit pool. The deviation % and heartbeat are too large and this will open up arbitrage opportunities to the detriment of Kelp’s users. When Kelp implements the withdrawal mechanism, we will have a better understanding of the profitability of such an attack. For example, if Kelp implements withdrawals without a staking delay, given the large amount of on-chain liquidity of the underlying assets, the pool may be able to be exploited for large amounts given even a 1% price discrepancy between the different LSTs.

Note: see original submission for full discussion.

[H-02] Protocol mints less rsETH on deposit than intended

Submitted by T1MOH, also found by 0xepley, SBSecurity, cryptothemex, adriro, AlexCzm, trachev, adam-idarrha, Aymen0909, deepplus, xAriextz, ast3ros, Weed0607, DanielArmstrong, rouhsamad, osmanozdemir1, GREY-HAWK-REACH, 0x1337, zhaojie, Jiamin, crunch, Varun_05, 7siech, QiuhaoLi, circlelooper, HChang26, Juntao, ayden, Aamir, rvierdiiev, max10afternoon, crack-the-kelp, Ruhum, 0xluckhu, 0xNaN, mahdirostami, and 0xmystery

Price of rsETH is calculated as totalLockedETH / rsETHSupply. rsETH price is used to calculate rsETH amount to mint when user deposits. Formulas are following:

• rsethAmountToMint = amount * assetPrice / rsEthPrice
• rsEthPrice = totalEthLocked / rsETHSupply

Problem is that it transfers deposit amount before calculation of rsethAmountToMint. It increases totalEthLocked. As a result rsethAmountToMint is less than intended because rsEthPrice is higher.

For example:

1. Suppose totalEthLocked = 10e18, assetPrice = 1e18, rsETHSupply = 10e18
2. User deposits 30e18. He expects to receive 30e18 rsETH
3. However actual received amount will be 30e18 * 1e18 / ((30e18 * 1e18 + 10e18 * 1e18) / 10e18) = 7.5e18

Proof of Concept

Here you can see that it firstly transfers asset to address(this), then calculates amount to mint:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        ...

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

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

There is long chain of calls:

_mintRsETH()
    getRsETHAmountToMint()
        LRTOracle().getRSETHPrice()
            getTotalAssetDeposits()
                getTotalAssetDeposits()

Finally getTotalAssetDeposits() uses current balanceOf(), which was increased before by transferring deposit amount:

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

Recommended Mitigation Steps

Transfer tokens in the end:

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        ...

+       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

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

-       // interactions
-       uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
-
        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Oracle

RaymondFam (lookout) commented:

This vulnerability is worse than a donation attack.

gus (Kelp) confirmed and commented:

This is a legitimate issue and has been fixed in commit 3b4e36c740013b32b78e93b00438b25f848e5f76 to separately have rsETH price calculators and read value from state variables, which also helped reduce gas cost to a great extent as well. We thank the warden for alerting us to this issue.

0xDjango (judge) commented:

This is a High severity issue. The miscalculation causes direct fund loss to users.

[H-03] The price of rsETH could be manipulated by the first staker

Submitted by Krace, also found by SBSecurity, spark, adriro, Madalad, peanuts, adam-idarrha, Phantasmagoria, Aymen0909, CatsSecurity, SandNallani, chaduke, AlexCzm, deth, bronze_pickaxe, osmanozdemir1, ast3ros, zach, m_Rassska, jasonxiale, mahyar, pep7siup, twcctop, ck, joaovwfreire, GREY-HAWK-REACH, 0xDING99YA, zhaojie, qpzm, TheSchnilch, gumgumzum, QiuhaoLi, 0xrugpull_detector, Aamir, almurhasan, ayden, wisdomn_, peter, max10afternoon, rouhsamad, SpicyMeatball, rvierdiiev, crack-the-kelp, Ruhum, ke1caM, ptsanev, mahdirostami, Banditx0x, critical-or-high, T1MOH, ubl4nk, Bauer, and btk

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

The first staker can potentially manipulate the price of rsETH through a donation attack, causing subsequent stakers to receive no rsETH after depositing. The first staker can exploit this method to siphon funds from other users.

Proof of Concept

The mining amount of rsETH is calculated in function getRsETHAmountToMint which directly utilizes the total value of the asset divided by the price of a single rsETH.

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

Subsequently, the price of rsETH is related to its totalSupply and the total value of deposited assets.

    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 the price of rsETH is calculated based on the asset and totalSupply
        return totalETHInPool / rsEthSupply;
    }

The total value of deposited assets comprises three parts: the assets in LRTDepositPool, the assets in NodeDelagator, and the assets in the eigenlayer. Anyone can directly contribute asset tokens to LRTDepositPool or NodeDelegator to augment the total value of deposited assets.

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

Case

Therefore, the price of rsETH is susceptible to manipulation by the first staker, considering the following scenario:

• Alice is the first staker and she deposits 1 USDC (the price of USDC is set to $1), she will get 1 wei rsETH, and the totalSupply of rsETH is 1 wei.

Here is the test, add it to test/LRTDepositPoolTest.t.sol and run with forge test --match-test test_ControlPrice -vv.

diff --git a/test/LRTDepositPoolTest.t.sol b/test/LRTDepositPoolTest.t.sol
index 40abc93..63349c2 100644
--- a/test/LRTDepositPoolTest.t.sol
+++ b/test/LRTDepositPoolTest.t.sol
@@ -9,10 +9,11 @@ import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";

 import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
 import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
+import "forge-std/console.sol";

 contract LRTOracleMock {
     function getAssetPrice(address) external pure returns (uint256) {
-        return 1e18;
+        return 1;
     }

     function getRSETHPrice() external pure returns (uint256) {
@@ -109,6 +110,23 @@ contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
         lrtDepositPool.depositAsset(rETHAddress, 2 ether);
     }

+    function test_ControlPrice() external {
+        vm.startPrank(alice);
+
+        // alice balance of rsETH before deposit
+        uint256 aliceBalanceBefore = rseth.balanceOf(address(alice));
+
+        rETH.approve(address(lrtDepositPool), 1 ether);
+        lrtDepositPool.depositAsset(rETHAddress, 1 ether);
+
+        // alice balance of rsETH after deposit
+        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
+        vm.stopPrank();
+
+        console.log(" rsETH of Alice: ", aliceBalanceAfter - aliceBalanceBefore);
+
+    }
+
     function test_DepositAsset() external {
         vm.startPrank(alice);

• Alice donates 10000 USDC to the LRTDepositPool to inflate the price of rsETH. Now the price of rsETH is: (10000 + 1)ether / 1 wei = 10001 ether
• Any subsequent staker who deposits assets worth less than 10001 USDC will not receive any rsETH, and they won’t be able to withdraw the deposited assets either. Alice can directly siphon off these funds.

For example, if Bob deposit 10000 USDC, then the rsethAmountToMint is (10000 ether * 1) / (10001)ether = 0

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

Moreover, there is no check on the actual amount of rsETH received by the user, and the execution continues even if this amount is zero.

    function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        //@audit sender could receive 0 token
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

Recommended Mitigation Steps

It is recommended to pre-mint some rsETH tokens to prevent price manipulation or ensure that the rsethAmountToMint is greater than zero.

gus (Kelp) disagreed with severity and commented:

We agree this is an issue. We also agree that it should be of a MEDIUM severity as it is an edge case that happens on the first protocol interaction.

manoj9april (Kelp) confirmed

0xDjango (judge) commented:

Judging as HIGH. While it is an edge case, the potential loss of funds is present. Vault donation attacks have been judged as high in the majority of C4 audits where no safeguards are implemented.

manoj9april (Kelp) commented:

Initial minting is a way of mitigating this issue. And this mitigation could be done after deployment. Hence no safeguard were added in contract. Hence request to decrease to medium.

0xDjango (judge) commented:

Initial minting is a way of mitigating this issue. And this mitigation could be done after deployment. Hence no safeguard were added in contract. Hence request to decrease to medium.

Based on the implementation, this issue will remain HIGH. Funds are at risk until Kelp takes subsequent action to mitigate.

gus (Kelp) confirmed and commented:

We disagree with the severity of this issue. Every protocol has to setup the contracts first before publicizing that contracts are ready for public usage. We take measures to ensure the exchange rate is closer to 1 before users interact with contracts.

Medium Risk Findings (2)

[M-01] Update in strategy will cause wrong issuance of shares

Submitted by crack-the-kelp, also found by crack-the-kelp, osmanozdemir1, T1MOH, tallo, 0xffchain, Stormy, jayfromthe13th, Aymen0909, bLnk, Pechenite, chaduke, roleengineer, ast3ros, jasonxiale, deth, ZanyBonzy, zhaojie, 0xDING99YA, Bauchibred, lsaudit, deepkin, DanielArmstrong, and nmirchev8

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L70
https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L49
https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L84
https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L122-L123

If there is update in strategy, getTotalAssetDeposits(asset) will reduce for asset. Because it checks for the assets deposited in NodeDelegator. NodeDelegator.getAssetBalance() will check for asset balance for the strategy of the user. If strategy is updated, then the balance of old strategy won’t be taken into account which will reduce the totalDeposits. This will reduce the rsETH share price and cause more rsETH shares to be minted for the depositors. Thus, depositors can immediately deposit and their shares are worth more than their deposit.

Proof of Concept

contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle public lrtOracle;
    NodeDelegator public nodeDelegator;
    ILRTConfig public ilrtConfig;
    MockEigenStrategyManager public mockEigenStraregyManager;
    MockStrategy public mockStrategy1;
    MockStrategy public mockStrategy2;

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

        //deploy rsETH
        proxyAdmin = new ProxyAdmin();
        RSETH tokenImpl = new RSETH();
        TransparentUpgradeableProxy tokenProxy = new TransparentUpgradeableProxy(
                address(tokenImpl),
                address(proxyAdmin),
                ""
            );
        rseth = RSETH(address(tokenProxy));

        //deploy lrtConfig
        proxyAdmin = new ProxyAdmin();
        LRTConfig lrtConfigImpl = new LRTConfig();
        TransparentUpgradeableProxy lrtConfigProxy = new TransparentUpgradeableProxy(
                address(lrtConfigImpl),
                address(proxyAdmin),
                ""
            );

        lrtConfig = LRTConfig(address(lrtConfigProxy));

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

        // deploy LRTOracle
        ProxyAdmin proxyAdmin1 = new ProxyAdmin();
        lrtOracle = new LRTOracle();
        TransparentUpgradeableProxy contractProxy1 = new TransparentUpgradeableProxy(
                address(lrtOracle),
                address(proxyAdmin1),
                ""
            );
        lrtOracle = LRTOracle(address(contractProxy1));

        // deploy NodeDelegator
        proxyAdmin1 = new ProxyAdmin();
        nodeDelegator = new NodeDelegator();
        contractProxy1 = new TransparentUpgradeableProxy(
            address(nodeDelegator),
            address(proxyAdmin1),
            ""
        );
        nodeDelegator = NodeDelegator(address(contractProxy1));

        lrtConfig.initialize(
            admin,
            address(stETH),
            address(rETH),
            address(cbETH),
            address(rseth)
        );
        rseth.initialize(admin, address(lrtConfig));
        lrtOracle.initialize(address(lrtConfig));
        nodeDelegator.initialize(address(lrtConfig));
        lrtDepositPool.initialize(address(lrtConfig));

        mockEigenStraregyManager = new MockEigenStrategyManager();
        mockStrategy1 = new MockStrategy(address(stETH), 0);
        mockStrategy2 = new MockStrategy(address(stETH), 0);

        vm.startPrank(admin);
        lrtConfig.setContract(
            keccak256("LRT_DEPOSIT_POOL"),
            address(lrtDepositPool)
        );
        lrtConfig.setContract(
            keccak256("EIGEN_STRATEGY_MANAGER"),
            address(mockEigenStraregyManager)
        );
        lrtConfig.grantRole(keccak256("MANAGER"), manager);
        lrtConfig.setRSETH(address(rseth));
        vm.stopPrank();

        vm.startPrank(manager);
        lrtOracle.updatePriceOracleFor(
            address(stETH),
            address(new LRTOracleMock())
        );
        lrtOracle.updatePriceOracleFor(
            address(rETH),
            address(new LRTOracleMock())
        );
        lrtOracle.updatePriceOracleFor(
            address(cbETH),
            address(new LRTOracleMock())
        );
        // lrtConfig.addNewSupportedAsset(address(randomToken), 100_000 ether);
        // lrtOracle.updatePriceOracleFor(
        //     address(randomToken),
        //     address(new LRTOracleMock())
        // );
        vm.stopPrank();

        vm.startPrank(admin);
        lrtConfig.updateAssetStrategy(address(stETH), address(mockStrategy1));

        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        address[] memory nodeDelegatorContracts = new address[](1);
        nodeDelegatorContracts[0] = address(nodeDelegator);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorContracts);
        vm.stopPrank();
    }

function test_update_strategy_cause_wrong_minting_of_shares() public {
        //NOTE: comment line 207-211 in setup for this
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(stETH), 10 ether); //NOTE: 10000000000000000000 rsETH amount minted
        vm.stopPrank();
        vm.startPrank(manager);
        lrtDepositPool.transferAssetToNodeDelegator(
            0,
            address(stETH),
            10 ether
        );
        vm.stopPrank();
        vm.startPrank(admin);
        lrtConfig.updateAssetStrategy(address(stETH), address(mockStrategy2));
        lrtConfig.updateAssetStrategy(address(rETH), address(mockStrategy2));
        lrtConfig.updateAssetStrategy(address(cbETH), address(mockStrategy2));

        vm.stopPrank();
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(stETH), 10 ether); //NOTE:   2500000000000000000 rsETH amount minted even after fixing deposit bug
        vm.stopPrank();
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

While updating the strategy, ensure that balance deposited in previous strategy for same asset is accounted while minting the shares.

Assessed type

ERC4626

gus (Kelp) disputed and commented:

The Eigenlayer has a 1-1 mapping for asset and its strategy. We don’t believe this an issue with KelpDAO or its code but with Eigenlayer if it upgrades its protocol to have multiple strategies for an asset. Then if this occurs, KelpDAO will also have to upgrade its contracts to ensure the compatibility to Eigenlayer.
Also, if Eigenlayer changes strategies, they are responsible to migrate the internal accounting to the new address.

manoj9april (Kelp) commented:

Currently Eigenlayer’s implementation allows them to have multiple startegies for any asset.
-> But we can continue to deposit into one strategy for a asset and it won’t be a problem for us. That is assuming 1:1 mapping won’t harm us.

Above assumptions will only create problem in case, Eigenlayer wants to spread the current funds into multiple strategies for any asset in future upgrades. And in this case, they will provide proper heads up ahead of time and we can upgrade along with them accordingly.

In case they change their strategy for any asset, we can point to new strategy address and it should work fine. And in this case Eigenlayer should should take care of all migration of funds from old to new strategy.

But as currently nothing has been decided at Eigenlayer end. I believe we are good with our current assumption of 1:1. It would be overkill to assume anything else as of now.

Please check the attached conversation with Eigenlayer team.
Discord: https://discord.com/channels/1089434273720832071/1096331191243788379

0xDjango (judge) invalidated and commented:

Kelp has provided adequate reasoning behind the current design. I agree that it is unlikely that a breaking change from the Eigenlayer protocol would not be properly communicated ahead of time. Designing around this potentiality would impose added risk in the current design.

radev_sw (warden) commented:

Hey, @0xDjango.

I disagree with the reasons provided above.

We don’t believe this an issue with KelpDAO or its code but with Eigenlayer if it upgrades its protocol to have multiple strategies for an asset.

The problem lies specifically in how Kelp DAO upgrades the strategy for assets. Nothing related to the functionality of EigenLayer Strategies.

Let’s view at the code of startegy upgrading

    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, when an EigenLayer Strategy for an asset is changed, there is currently no mechanism in place to migrate user deposits from the old strategy to the new one. It is precisely in this scenario that the vulnerability arises.

If a strategy update occurs, the function getTotalAssetDeposits(asset) will show a reduced value for that asset. This reduction happens because the function references assets deposited in NodeDelegator. Specifically, NodeDelegator.sol#getAssetBalance() assesses the asset balance based on the user’s current strategy. When a strategy update takes place, the balance associated with the previous strategy is no longer considered, leading to a decrease in totalDeposits. As a result, the share price of rsETH decreases, prompting the system to mint more rsETH shares for depositors. Consequently, depositors can make immediate deposits, gaining shares that are more valuable than the actual amount they deposited.

This vulnerability describes significant flaws in the KelpDAO protocol flow. Nothing related to the functionality of EigenLayer Strategies.

But as currently nothing has been decided at Eigenlayer end. I believe we are good with our current assumption of 1:1. It would be a overkill to assume anything else as of now.

The recommendations in this report do not make any assumptions about the implementation of 1:1 mapping. Instead, they specifically suggest updating the updateAssetStrategy() function. In a way that update would enable the transfer of user deposits from the old strategy to the new strategy once the new strategy is set. (as in my issue #644 writes)

I believe the issue has been misjudged and should be mitigated to High Severity Issue.

manoj9april (Kelp) commented:

@radev_sw - KelpDao will only update the strategy address when EigenLayer updates strategy for any asset (which EigenLayer currently has no plans as such).

Even if above scenario takes place. EigenLayer should provide proper communications ahead of time. And we can go through follow below steps:

• Eigenlayer communicates strategy update
• KelpDao pauses its protocol for a mutually decided duration
• EigenLayer migrates its state from old contracts to new contracts
• KelpDao points to new strategy contract
• KelpDao resumes

If migration of contract state is not possible at EigenLayer and they (Eigenlayer) decomission their old strategy, only in this scenario
KelpDao will have to withdraw its funds from old strategy and redelgate it to new strategy. And these steps will be done in complete visibility and transparency with proper timelocks. But sees no harm or loss to funds or miscalculation.

0xDjango (judge) increased severity to Medium and commented:

This will be upgraded to MEDIUM.

The warden accurately explains how Kelp’s internal accounting will be broken if they decide to use the updateAssetStrategy() function. If Kelp changes the strategy address, rsETH share prices will decrease in a way that should not occur.

While Kelp describes the list of actions that they would take in the case they would need to change the strategy address, the implementation does not protect against the possibility of the broken accounting.

[M-02] Lack of slippage control on LRTDepositPool.depositAsset

Submitted by max10afternoon, also found by 0xhacksmithh, ge6a, btk, anarcheuz, adriro, Aymen0909, Pechenite, 0xMilenov, turvy_fuzz, ck (1, 2), hals (1, 2), Daniel526, Shaheen, PENGUN, glcanvas, 0xbrett8571, 0xmystery, and Bauer (1, 2)

The depositAsset function of the LRTDepositPool contract, enables users to deposit assets into the protocol, getting RSETH tokens in return. The function doesn’t have any type of slippage control; this is relevant in the context of the depositAsset function, since the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.

Also the users will have no defense against price manipulations attacks, if they where to be found after the protocol’s deployment.

Proof of Concept

As can be observed by looking at its parameters and implementation, the depositAsset function of the LRTDepositPool contract, doesn’t have any type of slippage protection:

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Meaning that users have no control over how many RSETH tokens they will get in return for depositing in the contract.

The amount of tokens to be minted, with respect to the deposited amount, is determined by the getRsETHAmountToMint function of the same contract (inside of _mintRsETH):

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

As can be observed, this function uses two oracle interactions to determine how many tokens to mint, getAssetPrice and getRSETHPrice (with getRSETHPrice internally calling getAssetPrice as well).

The getAssetPrice function queries the external oracle for the asset price:

    function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (uint256) {
        return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);
    }

Meaning that the user has no way to predict how many RSETH they will get back at the moment of minting, as the price could be updated while the request is in the mempool.

Here is a very simple foundry script that shows that an oracle price modification, can largely impact the amount of tokens received in return by the user (implying that the depositAsset function has no protection against it).

// 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 { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

import "forge-std/console.sol";

contract LRTOracleMock {

    uint256 assetPrice = 1e18;

    function setAssetPrice(uint256 newPrice) external {
        assetPrice = newPrice;
    }

    function getAssetPrice(address) external view returns (uint256) {
        return assetPrice;
    }

    function getRSETHPrice() external pure returns (uint256) {
        return 1e18;
    }
}

contract MockNodeDelegator {
    function getAssetBalance(address) external pure returns (uint256) {
        return 1e18;
    }
}

contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracleMock public mockOracle;

    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));
        mockOracle = new LRTOracleMock();

        // 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
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(mockOracle));

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

        vm.stopPrank();
    }
}

contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
    address public rETHAddress;

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

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

        rETHAddress = address(rETH);

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


    function test_OracleCanModifyAssetMinted() external {
        
        vm.prank(alice);
        rETH.approve(address(lrtDepositPool), 2 ether);
        vm.prank(bob);
        rETH.approve(address(lrtDepositPool), 2 ether); 

        uint256 aliceDepositTime = block.timestamp;
        vm.prank(alice);
        lrtDepositPool.depositAsset(rETHAddress, 2 ether);

        mockOracle.setAssetPrice(1e17);

        uint256 bobDepositTime = block.timestamp;
        vm.prank(bob);
        lrtDepositPool.depositAsset(rETHAddress, 2 ether);

        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
        
        console.log(aliceBalanceAfter);
        console.log(bobBalanceAfter);    

        console.log(aliceDepositTime);
        console.log(bobDepositTime); 

        assertEq(aliceDepositTime, bobDepositTime);
        assertGt(aliceBalanceAfter, bobBalanceAfter);

    }

}

Recommended Mitigation Steps

An additional parameter could be added to the depositAsset function, to let users decide the minimum amount of tokens to be received, with a relative check after minting.

gus (Kelp) confirmed

manoj9april (Kelp) disagreed with severity and commented:

Slippage control makes more sense in DEXes. But in staking protocols, where prices are mostly stable and users can reject if it txn takes longer time.
But it is a useful recommendation.
I think we can move it to QA.

0xDjango (judge) commented:

Sticking with medium. There is no protection for the user in the current implementation. minShares checks are commonly implemented in staking contracts.

Low Risk and Non-Critical Issues

For this audit, 125 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by m_Rassska received the top score from the judge.

The following wardens also submitted reports: CatsSecurity, Pechenite, hals, 0xepley, 0xffchain, btk, hunter_w3b, turvy_fuzz, adriro, anarcheuz, Madalad, spark, ge6a, shealtielanz, _thanos1, 0xAadi, phoenixV110, rouhsamad, poneta, 0xvj, Udsen, SandNallani, gesha17, Topmark, dipp, adeolu, leegh, tallo, osmanozdemir1, cartlex_, RaoulSchaffranek, Matin, ZanyBonzy, cheatc0d3, Tumelo_Crypto, GREY-HAWK-REACH, twcctop, bronze_pickaxe, Yanchuan, gumgumzum, 0xHelium, Amithuddar, Inspecktor, sakshamguruji, jasonxiale, joaovwfreire, 0xblackskull, 0xLeveler, baice, SBSecurity, xAriextz, niser93, ro1sharkm, hihen, Phantasmagoria, McToady, bareli, 0x1337, Aamir, almurhasan, eeshenggoh, seerether, zhaojie, Bauchibred, Soul22, ayden, lsaudit, deepkin, chaduke, AerialRaider, Shaheen, Cryptor, boredpukar, TeamSS, alexfilippov314, Draiakoo, Daniel526, amaechieth, zach, squeaky_cactus, crack-the-kelp, King_, MatricksDeCoder, Noro, circlelooper, wisdomn_, 0xrugpull_detector, pep7siup, Juntao, paritomarrr, soliditytaker, ziyou-, codynhat, passion, catellatech, supersizer0x, stackachu, evmboi32, TheSchnilch, taner2344, glcanvas, marchev, Stormreckson, rvierdiiev, Eigenvectors, PENGUN, ElCid, ke1caM, 0xluckhu, desaperh, debo, merlinboii, ABAIKUNANBAEV, pipidu83, LinKenji, 0xbrett8571, 0xmystery, zhaojohnson, critical-or-high, MaslarovK, T1MOH, ubl4nk, Bauer, and Tadev.

Summary

Low Severity

• [L-01] Prevent deposits to the nodes with max amount of assets being staked already
• [L-02] Set the price feeds during an initialization
• [L-03] Dropping a dust to grief the staking process
• [L-04] Approve to StrategyManager during an initialization
• [L-05] Enforce maxnodeDelegatorCount > nodeDelegatorQueue.length
• [L-06] Place an assertion to ensure the system in paused/unpaused state

Non-Critical Severity

• [N-01] Introduce removeNodeDelegatorToQueue

[L-01] Prevent deposits to the nodes with max amount of assets being staked already

If the node has staked already 1e23 of assets, it should not receive any transfers, since there is no more space to stake at EigenLayer.

Example of an occurrence

Could be implied here.

[L-02] Set the price feeds during an initialization

Currently, the price feeds are set by using updatePriceOracleFor() after an LRTOPracle being deployed. However, it’s better to set up everything during an initialization process.

Example of an occurrence

Could be added, for instance, here.

[L-03] Dropping a dust to grief the staking process

After an amount of lst being transferred to a specified NodeDelegator, the stake on EigenLayer is expected to happen. However, if there is a possibility to grief depositAssetIntoStrategy, adversary might drop some dust and front-run the tx submitted by LRTManager. This forces to transfer some assets back to the pool in order to successfully stake at EigenLayer.

Example of an occurrence

It’s better to provide an opportunity for LRTManager to decide, how much lst amount is about to be transferred and not blindly rely on the total balance here.

[L-04] Approve to StrategyManager during an initialization

There is no need to inflate the bytecode for NodeDelegator by introducing a special function for an approval grants. Instead, approve to the eigenlayerStrategyManagerAddress during an initialization.

Example of an occurrence

Could be added, for instance, here.

[L-05] Enforce maxnodeDelegatorCount > nodeDelegatorQueue.length

Put an assertion to ensure that the maxnodeDelegatorCount > nodeDelegatorQueue.length to prevent unexpected behavior from happening.

Example of an occurrence

Could be added, for instance, here.

[L-06] Place an assertion to ensure the system in paused/unpaused state

The functions pause()/unpause() are super important in case something suspicous starts to happen. An LRTManager might accidentally invoke unpause() instead of hitting pause() and convince himself of stopping suspicous activity. However, this might give an attacker the chance to continue his dirty activity.

Example of an occurrence

Could be added, for instance, here.

[N-01] Introduce removeNodeDelegatorFromQueue

It is recommended to introduce removeNodeDelegatorFromQueue in case the NodeDelegator is no longer needed.

Example of an occurrence

Could be introduced, for instance, here.

Gas Optimizations

For this audit, 16 reports were submitted by wardens detailing gas optimizations. The report highlighted below by 0xVolcano received the top score from the judge.

The following wardens also submitted reports: hunter_w3b, unique, lsaudit, tala7985, Raihan, 0xAnah, JCK, 0xhex, 0xta, ZanyBonzy, shamsulhaq123, Sathish9098, ABAIKUNANBAEV, Rickard, and rumen.

[G-01] Avoid zero to non-zero storage writes

When a storage variable goes from zero to non-zero, the user must pay 22,100 gas total (20,000 gas for a zero to non-zero write and 2,100 for a cold storage access).

This is why the Openzeppelin reentrancy guard registers functions as active or not with 1 and 2 rather than 0 and 1. It only costs 5,000 gas to alter a storage variable from non-zero to non-zero.

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

File: /src/LRTDepositPool.sol
19:contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgradeable, ReentrancyGuardUpgradeable {
20:    uint256 public maxNodeDelegatorCount;

29:    /// @dev Initializes the contract
30:    /// @param lrtConfigAddr LRT config address
31:    function initialize(address lrtConfigAddr) external initializer {
32:        UtilLib.checkNonZeroAddress(lrtConfigAddr);
33:        __Pausable_init();
34:        __ReentrancyGuard_init();
35:        maxNodeDelegatorCount = 10;
36:        lrtConfig = ILRTConfig(lrtConfigAddr);
37:        emit UpdatedLRTConfig(lrtConfigAddr);
38:    }

When declaring the variable maxNodDelegatorCount the default value is applied since we do not assign anything, the default value of uint256 is 0.

We then call the function initialize() and here we assign the value 10 maxNodeDelegatorCount = 10;. This means our value is updated from 0 to 10.

Since the initialize() function will always be called first, we can initialize our variable maxNodDelegatorCount with 1 during declaration which should avoid the zero to non zero writes.

 /// @title LRTDepositPool - Deposit Pool Contract for LSTs
 /// @notice Handles LST asset deposits
 contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgradeable, ReentrancyGuardUpgradeable {
-    uint256 public maxNodeDelegatorCount;
+    uint256 public maxNodeDelegatorCount = 1;

     address[] public nodeDelegatorQueue;

[G-02] We can optimize the function updateLRTConfig()

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/utils/LRTConfigRoleChecker.sol#L47-L51

File: /src/utils/LRTConfigRoleChecker.sol
47:    function updateLRTConfig(address lrtConfigAddr) external virtual onlyLRTAdmin {
48:        UtilLib.checkNonZeroAddress(lrtConfigAddr);
49:        lrtConfig = ILRTConfig(lrtConfigAddr);
50:        emit UpdatedLRTConfig(lrtConfigAddr);
51:    }

The above function uses the modifier onlyLRTAdmin which has the following implementation:

    modifier onlyLRTAdmin() {
        bytes32 DEFAULT_ADMIN_ROLE = 0x00;
        if (!IAccessControl(address(lrtConfig)).hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
            revert ILRTConfig.CallerNotLRTConfigAdmin();
        }
        _;
    }

The modifier reads a state variable lrtConfig and also makes an external call to hasRole().

If the check does not pass we have a revert. Using this modifier in our function means we run the check first and revert if the check does not pass.

If the check passes, our function has one more check UtilLib.checkNonZeroAddress which basically checks that our address is not zero. This is way cheaper compared to the check inside the modifier

If this check does not pass, it means we would also revert, now suppose the first check(modifier) passes but the non zero fails, the gas consumed inside the modifier doing the state read and external call would all be wasted.

We can do this check more efficiently by prioritizing cheaper checks first but for this to work, we would need to inline our modifier as shown below:

diff --git a/src/utils/LRTConfigRoleChecker.sol b/src/utils/LRTConfigRoleChecker.sol
index 991d0e9..1b9917f 100644
--- a/src/utils/LRTConfigRoleChecker.sol
+++ b/src/utils/LRTConfigRoleChecker.sol
@@ -44,8 +44,12 @@ abstract contract LRTConfigRoleChecker {
     /// @notice Updates the LRT config contract
     /// @dev only callable by LRT admin
     /// @param lrtConfigAddr the new LRT config contract Address
-    function updateLRTConfig(address lrtConfigAddr) external virtual onlyLRTAdmin {
+    function updateLRTConfig(address lrtConfigAddr) external virtual  {
         UtilLib.checkNonZeroAddress(lrtConfigAddr);
+        bytes32 DEFAULT_ADMIN_ROLE = 0x00;
+        if (!IAccessControl(address(lrtConfig)).hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
+            revert ILRTConfig.CallerNotLRTConfigAdmin();
+        }
         lrtConfig = ILRTConfig(lrtConfigAddr);
         emit UpdatedLRTConfig(lrtConfigAddr);
     }
     

[G-03] Emit local variables instead of state variables(not found by bot)

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

File: /src/LRTDepositPool.sol
202:    function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
203:        maxNodeDelegatorCount = maxNodeDelegatorCount_;
204:        emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
205:    }

@@ -201,7 +201,7 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
     /// @param maxNodeDelegatorCount_ Maximum count of node delegator
     function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
         maxNodeDelegatorCount = maxNodeDelegatorCount_;
-        emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
+        emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount_);//@audit emit the local var
     }

[G-04] Don’t cache calls/variables if using them once

No need to cache lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER)

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

File: /src/NodeDelegator.sol
38:    function maxApproveToEigenStrategyManager(address asset)
39:        external
40:        override
41:        onlySupportedAsset(asset)
42:        onlyLRTManager
43:    {
44:        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);
45:        IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);
46:    }

The variable eigenlayerStrategyManagerAddress is only used once, therefore no need to cache the call.

diff --git a/src/NodeDelegator.sol b/src/NodeDelegator.sol
index bba20ca..989218c 100644
--- a/src/NodeDelegator.sol
+++ b/src/NodeDelegator.sol
@@ -41,8 +41,7 @@ contract NodeDelegator is INodeDelegator, LRTConfigRoleChecker, PausableUpgradea
         onlySupportedAsset(asset)
         onlyLRTManager
     {
-        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);
-        IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);
+        IERC20(asset).approve(lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY

No need to cache lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER)

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

File: /src/NodeDelegator.sol
51:    function depositAssetIntoStrategy(address asset)

58:    {
59:        address strategy = lrtConfig.assetStrategy(asset);
60:        IERC20 token = IERC20(asset);
61:        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

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

65:        emit AssetDepositIntoStrategy(asset, strategy, balance);

67:IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
68:    }

     /// @notice Deposits an asset lying in this NDC into its strategy
@@ -58,13 +58,12 @@ contract NodeDelegator is INodeDelegator, LRTConfigRoleChecker, PausableUpgradea
     {
         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);
+        IEigenStrategyManager(lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER)).depositIntoStrategy(IStrategy(strategy), token, balance);
     }

No need to cache lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL)

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

File: /src/NodeDelegator.sol
74:    function transferBackToLRTDepositPool(

84:        address lrtDepositPool = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

86:        if (!IERC20(asset).transfer(lrtDepositPool, amount)) {
87:            revert TokenTransferFailed();
88:        }
89:    }

     /// @notice Deposits an asset lying in this NDC into its strategy
@@ -81,9 +81,8 @@ contract NodeDelegator is INodeDelegator, LRTConfigRoleChecker, PausableUpgradea
         onlySupportedAsset(asset)
         onlyLRTManager
     {
-        address lrtDepositPool = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

-        if (!IERC20(asset).transfer(lrtDepositPool, amount)) {
+        if (!IERC20(asset).transfer(lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL), amount)) {
             revert TokenTransferFailed();
         }
     }

No need to cache lrtConfig.assetStrategy(asset)

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

File: /src/NodeDelegator.sol
121:    function getAssetBalance(address asset) external view override returns (uint256) {
122:        address strategy = lrtConfig.assetStrategy(asset);
123:        return IStrategy(strategy).userUnderlyingView(address(this));
124:    }

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

No need to cache nodeDelegatorQueue[ndcIndex]

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

File: /src/LRTDepositPool.sol
183:    function transferAssetToNodeDelegator(

192:    {
193:        address nodeDelegator = nodeDelegatorQueue[ndcIndex];
194:        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
195:            revert TokenTransferFailed();
196:        }
197:    }

@@ -190,8 +190,7 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
         onlyLRTManager
         onlySupportedAsset(asset)
     {
-        address nodeDelegator = nodeDelegatorQueue[ndcIndex];
-        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
+        if (!IERC20(asset).transfer(nodeDelegatorQueue[ndcIndex], amount)) {
             revert TokenTransferFailed();
         }
     }

[G-05] Caching the length of calldata increases gas cost instead of reducing it

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

File: /src/LRTDepositPool.sol
162:    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
163:        uint256 length = nodeDelegatorContracts.length;
164:        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
165:            revert MaximumNodeDelegatorCountReached();
166:        }

168:        for (uint256 i; i < length;) {
169:            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
170:            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
171:            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
172:            unchecked {
173:                ++i;
174:            }
175:        }
176:    }

     function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
         uint256 length = nodeDelegatorContracts.length;
-        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
+        if (nodeDelegatorQueue.length + nodeDelegatorContracts.length > maxNodeDelegatorCount) {
             revert MaximumNodeDelegatorCountReached();
         }

-        for (uint256 i; i < length;) {
+        for (uint256 i; i < nodeDelegatorContracts.length;) {

Audit Analysis

For this audit, 21 analysis reports were submitted by wardens. An analysis report examines the codebase as a whole, providing observations and advice on such topics as architecture, mechanism, or approach. The report highlighted below by CatsSecurity received the top score from the judge.

The following wardens also submitted reports: SBSecurity, hunter_w3b, albahaca, sakshamguruji, foxb868, 0xbrett8571, Bauchibred, invitedtea, 0xepley, 0xSmartContract, fouzantanveer, SAAJ, clara, Sathish9098, unique, KeyKiril, Myd, digitizeworx, ZanyBonzy, and glcanvas.

Description of Kelp

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

1. System Overview

1.1 Scoping

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

1.2 Access Control Roles

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

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

1.3 Access Restricted Functions

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

2. Architecture Overview & Diagrams

2.1 Architecture Overview

• LRTConfig: The LRTConfig contract is a critical part of the Kelp protocol. It contains the logic for the system’s configuration such as supported assets and their deposit limits and asset strategies.
• LRTDepositPool: The LRTDepositPool contract is the main user-facing contract in the protocol responsible for taking in deposits and minting rsETH for users. Users can also get information related to current asset limits, total asset deposits and the node delegator queue. Restricted roles have access to node delegator and transferring of funds functions inside this contract.
• LRTOracle: The LRTOracle contract is the one responsible for fetching prices of LSTs (liquid staking tokens). It works together with the LRTDepositPool to maintain accurate price validity and helps in calculating how much rsETH to mint for users.
• RSETH: The RSETH Contract is the system’s ERC20 token implementation. Not much to say here.
• ChainlinkPriceOracle: The ChainLinkPriceOracle contract is the wrapper contract used to integrate Chainlink’s Oracles. It works together with the LRTOracle contract to supply it’s fetched data.
• LRTConfigRoleChecker: The LRTConfigRoleChecker contract is the system’s implementation for access control modifiers enabling certain functions to only be accessible by privileged roles.
• UtilLib: The UtilLib contract is a helper function lib that contains a check for non zero address function.
• LRTConstants: The LRTConstants contract contains the constant variables shared between the contracts such as R_ETH_TOKEN & other supported LSTs, LRT_ORACLE & other contracts, as well as the MANAGER role.

2.2 Architecture Diagram

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

2.3 Flow of Funds Diagram

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

2.4 Core Contracts Parameters Diagram

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

3. Codebase Assessment

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

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

4. Level of Documentation

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

5. Systemic & Centralization Risks

5.1 Systemic Risks

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

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

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

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

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

5.2 Centralization Risks

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

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

Disclosures

C4 is an open organization governed by participants in the community.

C4 Audits incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Audit submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.

C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.