Kelp DAO | rsETH - adriro's results

A collective DAO designed to unlock liquidity, DeFi and higher rewards for restaked assets through liquid restaking.

General Information

Platform: Code4rena

Start Date: 10/11/2023

Pot Size: $28,000 USDC

Total HM: 5

Participants: 185

Period: 5 days

Judge: 0xDjango

Id: 305

League: ETH

Kelp DAO

Findings Distribution

Researcher Performance

Rank: 3/185

Findings: 5

Award: $1,086.27

QA:
grade-b

🌟 Selected for report: 0

🚀 Solo Findings: 0

Findings Information

Labels

bug
3 (High Risk)
satisfactory
sufficient quality report
upgraded by judge
duplicate-584

Awards

902.5718 USDC - $902.57

External Links

Lines of code

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

Vulnerability details

Summary

Prices returned from Chainlink oracles have different conditions to update the reported values, which can be abused by

Impact

Prices for the different LST assets supported in the Kelp protocol are obtained from a Chainlink oracle. The data feeds for each one can be found here:

Chainlink price feeds follow different rules in order to update their prices. There are essentially two conditions that could trigger a price change, a heartbeat and a price deviation. A heartbeat updates the feed if the configured time since last update has elapsed, and the price deviation triggers the update when the price has moved above a configured percentage.

In all three cases, the heartbeat is the same (24 hours), however the price deviation target is not. stETH has a percentage of 0.5%, cbETH of 1% and rETH of 2%. This means that there are big differences between how fast an oracle is updated between the different assets.

As deposits in the protocol can be done using any assets, this means that a user could choose to deposit a particular asset knowing that the current on-chain price is deviated from the actual price, which can go as far as 2% for the rETH case. Note also that this can be used in a front-run fashion before the price of an oracle is updated on-chain, reducing the uncertainty to almost none in order to take advantage of a big price movement.

Similarly, since feeds have different triggers, a user could use this to their advantage to deposit the asset that is currently most deviated between all three. As participation in the protocol is measured by a share of RSETH, this means that the user's share will also include the other two assets, gaining an advantage in value due to the mispriced deposit asset.

Currently, there is no way to withdraw assets from the protocol, but this could eventually lead to a situation where a user deposits one asset before the price movement, and then withdraws after the price has impacted, or simply withdraws any other asset from the pool which has a more recent price update.

Recommendation

As the protocol is currently relying on Chainlink oracles, it is difficult to provide a recommendation using solely these price feeds. A potential solution would be to factor different oracles, such as a TWAP or other on-chain price sources.

Assessed type

Oracle

#0 - c4-pre-sort

2023-11-16T23:36:28Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2023-11-16T23:36:55Z

raymondfam marked the issue as duplicate of #609

#2 - c4-judge

2023-12-01T17:43:14Z

fatherGoose1 marked the issue as unsatisfactory: Invalid

#3 - romeroadrian

2023-12-03T23:59:43Z

@fatherGoose1 I really don't think this issue is correctly marked as a duplicate of #609. I think this is more on the line of #584.

#4 - c4-judge

2023-12-08T17:32:46Z

fatherGoose1 marked the issue as duplicate of #584

#5 - c4-judge

2023-12-08T17:50:31Z

fatherGoose1 marked the issue as satisfactory

#6 - c4-judge

2023-12-08T18:55:04Z

fatherGoose1 changed the severity to 3 (High Risk)

#7 - d3e4

2023-12-08T21:45:10Z

@fatherGoose1 This report states that the price oracle might be inaccurate. This is not sufficient for #584. The only thing even related to #584 is "As participation in the protocol is measured by a share of RSETH, this means that the user's share will also include the other two assets, gaining an advantage in value due to the mispriced deposit asset." There is not enough information here to arrive at the issue in #584.

"Currently, there is no way to withdraw assets from the protocol, but this could eventually lead to a situation where a user deposits one asset before the price movement, and then withdraws after the price has impacted, or simply withdraws any other asset from the pool which has a more recent price update." seems to suggest the issue described in #443, which is about the fact that you can predict the market at the moment of a price update, but unrelated to #584

#8 - romeroadrian

2023-12-08T21:51:15Z

@fatherGoose1 This report states that the price oracle might be inaccurate. This is not sufficient for #584. The only thing even related to #584 is "As participation in the protocol is measured by a share of RSETH, this means that the user's share will also include the other two assets, gaining an advantage in value due to the mispriced deposit asset." There is not enough information here to arrive at the issue in #584.

"Currently, there is no way to withdraw assets from the protocol, but this could eventually lead to a situation where a user deposits one asset before the price movement, and then withdraws after the price has impacted, or simply withdraws any other asset from the pool which has a more recent price update." seems to suggest the issue described in #443, which is about the fact that you can predict the market at the moment of a price update, but unrelated to #584

Nice to see you here d, I'm clearly not stating that the price oracle might be inaccurate, I'd recommend reading the issue again.

#9 - d3e4

2023-12-08T22:35:12Z

@romeroadrian What is your point? Do you mean that what you stated is contrary to "the price oracle might be inaccurate" (in which case you are wrong) or that you stated something else (what?)?

#10 - romeroadrian

2023-12-08T23:13:24Z

@romeroadrian What is your point? Do you mean that what you stated is contrary to "the price oracle might be inaccurate" (in which case you are wrong) or that you stated something else (what?)?

I mean that you summarized my report using those words which is not even close to the reported issue. I encourage you to read it again.

#11 - d3e4

2023-12-08T23:37:38Z

@romeroadrian Please be specific about how you think I have misread your report. A dodging encouragement to read it again will not help anyone.

I did not summarise your report by those words. This was the first part of your report. I then mentioned the two other things in your report. If you think I missed something important, be explicit about what it is.

#12 - romeroadrian

2023-12-09T14:00:50Z

I did not summarise your report by those words. This was the first part of your report.

I think you are really confused here. Those are your words, I literally quoted them. That's not part of my report.

#13 - d3e4

2023-12-09T16:43:02Z

You are abusing a literal parsing of what I wrote. I am sure you understand what I meant.

Are you unable to be explicit and specific about what I missed?

Awards

36.0335 USDC - $36.03

Labels

bug
3 (High Risk)
satisfactory
sufficient quality report
upgraded by judge
edited-by-warden
duplicate-62

External Links

Lines of code

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

Vulnerability details

Summary

The calculation in the deposit function of the DepositPool contract is flawed as it factors the deposited amount into the RSETH price to calculate the amount to mint.

Impact

When a user deposits in the DepositPool contract, the amount of RSETH to mint is determined by the price returned by the LRTOracle:

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

52:     function getRSETHPrice() external view returns (uint256 rsETHPrice) {
53:         address rsETHTokenAddress = lrtConfig.rsETH();
54:         uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();
55: 
56:         if (rsEthSupply == 0) {
57:             return 1 ether;
58:         }
59: 
60:         uint256 totalETHInPool;
61:         address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
62: 
63:         address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
64:         uint256 supportedAssetCount = supportedAssets.length;
65: 
66:         for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
67:             address asset = supportedAssets[asset_idx];
68:             uint256 assetER = getAssetPrice(asset);
69: 
70:             uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
71:             totalETHInPool += totalAssetAmt * assetER;
72: 
73:             unchecked {
74:                 ++asset_idx;
75:             }
76:         }
77: 
78:         return totalETHInPool / rsEthSupply;
79:     }

In summary, the implementation takes all assets held by the protocol and normalizes them into ETH. This value is then divided by the current supply of RSETH to get the price per token. For each asset, deposits are given by the getTotalAssetDeposits() function:

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

47:     function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
48:         (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
49:             getAssetDistributionData(asset);
50:         return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
51:     }

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

71:     function getAssetDistributionData(address asset)
72:         public
73:         view
74:         override
75:         onlySupportedAsset(asset)
76:         returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
77:     {
78:         // Question: is here the right place to have this? Could it be in LRTConfig?
79:         assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
80: 
81:         uint256 ndcsCount = nodeDelegatorQueue.length;
82:         for (uint256 i; i < ndcsCount;) {
83:             assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
84:             assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
85:             unchecked {
86:                 ++i;
87:             }
88:         }
89:     }

The implementation sums the assets owned by the DepositPool, as well as the balance and staked amount in Eigenlayer for each node delegator.

The RSETH price is then used to calculate the minting amount in depositAsset():

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

119:     function depositAsset(
120:         address asset,
121:         uint256 depositAmount
122:     )
123:         external
124:         whenNotPaused
125:         nonReentrant
126:         onlySupportedAsset(asset)
127:     {
128:         // checks
129:         if (depositAmount == 0) {
130:             revert InvalidAmount();
131:         }
132:         if (depositAmount > getAssetCurrentLimit(asset)) {
133:             revert MaximumDepositLimitReached();
134:         }
135: 
136:         if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
137:             revert TokenTransferFailed();
138:         }
139: 
140:         // interactions
141:         uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
142: 
143:         emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
144:     }
151:     function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
152:         (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);
153: 
154:         address rsethToken = lrtConfig.rsETH();
155:         // mint rseth for user
156:         IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
157:     }
095:     function getRsETHAmountToMint(
096:         address asset,
097:         uint256 amount
098:     )
099:         public
100:         view
101:         override
102:         returns (uint256 rsethAmountToMint)
103:     {
104:         // setup oracle contract
105:         address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
106:         ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);
107: 
108:         // calculate rseth amount to mint based on asset amount and asset exchange rate
109:         rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
110:     }

getRsETHAmountToMint() takes the deposit amount, multiplies it by the asset price (to normalize it to ETH) and divides it by the previously mentioned RSETH price calculation. However, the implementation of depositAsset() transfers first the deposit into the contract before fetching the current price. Line 136 pulls the tokens from the user into the contract and the call in line 141 does the calculation. This means the RSETH price calculation, which is based on the amount of assets held by the protocol (including the ones in the DepositPool), will be affected by the same transfer that happens in the function.

As balances are increased by the user's tokens, the RSETH price will be increased too (since the supply is still the same), which means that the user will be minted less RSETH tokens than expected.

Proof of Concept

In the following test, Alice first deposits 1e18 stETH tokens. As this is the first deposit in the pool, she is minted 1e18 shares of RSETH. Now, Bob deposits the same amount of stETH tokens but he just gets 0.5e18 RSETH tokens, half of what Alice received for depositing the same amount.

The difference comes from the fact that Bob's tokens are pulled into the contract before calculating the price, effectively duplicating the price of RSETH. Once the price is doubled, Bob is minted half of what it should be.

Note: the snippet shows only the relevant code for the test. Full test file can be found here.

function test_DepositPool_WrongMintCalculation() public {
    uint256 depositAmount = 1e18;

    // Alice deposits 1e18 stETH
    vm.startPrank(alice);

    stETH.approve(address(lrtDepositPool), type(uint256).max);

    lrtDepositPool.depositAsset(address(stETH), depositAmount);

    vm.stopPrank();

    // Alice is minted 1e18 RSETH
    assertEq(rseth.balanceOf(alice), 1e18);

    // Now, Bob deposits the same amount
    vm.startPrank(bob);

    stETH.approve(address(lrtDepositPool), type(uint256).max);

    lrtDepositPool.depositAsset(address(stETH), depositAmount);

    vm.stopPrank();

    // Bob is minted just 0.5e18 RSETH when he deposited the same amount as Alice
    assertEq(rseth.balanceOf(bob), 0.5e18);
}

Recommendation

The RSETH price calculation should not factor the tokens that are part of the deposit. Either, snapshot the price before pulling the tokens, or simply switch the minting order so that tokens are pulled after the mint:

+   // interactions
+   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

Other

#0 - c4-pre-sort

2023-11-16T23:31:58Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2023-11-16T23:32:08Z

raymondfam marked the issue as duplicate of #62

#2 - c4-judge

2023-11-29T21:26:12Z

fatherGoose1 marked the issue as satisfactory

#3 - c4-judge

2023-12-01T19:00:05Z

fatherGoose1 changed the severity to 2 (Med Risk)

#4 - c4-judge

2023-12-04T15:31:41Z

fatherGoose1 changed the severity to 3 (High Risk)

Lines of code

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

Vulnerability details

Summary

The DepositPool contract is susceptible to the Inflation Attack, in which the first depositor can be front-runned by an attacker to steal their deposit.

Impact

The DepositPool pool contract acts mainly as a vault: accounts deposit LST assets and get back RSETH tokens (a share) that represents their participation in the vault.

These types of contracts are potentially vulnerable to the inflation attack: an attacker can front-run the initial deposit to the vault to inflate the value of a share and render the front-runned deposit worthless.

The mint amount in the pool is given by the getRsETHAmountToMint() function:

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

095:     function getRsETHAmountToMint(
096:         address asset,
097:         uint256 amount
098:     )
099:         public
100:         view
101:         override
102:         returns (uint256 rsethAmountToMint)
103:     {
104:         // setup oracle contract
105:         address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
106:         ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);
107: 
108:         // calculate rseth amount to mint based on asset amount and asset exchange rate
109:         rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
110:     }

While the getRSETHPrice() is determined by the asset balance in the contracts:

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

52:     function getRSETHPrice() external view returns (uint256 rsETHPrice) {
53:         address rsETHTokenAddress = lrtConfig.rsETH();
54:         uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();
55: 
56:         if (rsEthSupply == 0) {
57:             return 1 ether;
58:         }
59: 
60:         uint256 totalETHInPool;
61:         address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
62: 
63:         address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
64:         uint256 supportedAssetCount = supportedAssets.length;
65: 
66:         for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
67:             address asset = supportedAssets[asset_idx];
68:             uint256 assetER = getAssetPrice(asset);
69: 
70:             uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
71:             totalETHInPool += totalAssetAmt * assetER;
72: 
73:             unchecked {
74:                 ++asset_idx;
75:             }
76:         }
77: 
78:         return totalETHInPool / rsEthSupply;
79:     }

An attacker can then mint a minimal amount of shares by depositing just 1 token of any valid asset, and then inflate the price per share by donating assets to the deposit pool. Once the share price has been inflated, the front-runned deposit is rounded down to zero or a very small number of shares, causing a loss of funds to the depositor as any precision loss will be captured by the attacker's shares.

Proof of Concept

For simplicity, let's say that the current Chainlink stETH/ETH price is 1 (1e18).

  1. An user is about to deposit X amount of stETH in the protocol.
  2. The attacker front-runs the transaction and deposits 1 stETH. The attacker is minted (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice() = 1 * 1e18 / 1e18 = 1 RSETH.
  3. The attacker donates X amount of stETH tokens to the DepositPool. The stETH balance of the DepositPool is now X + 1.
  4. The user transaction is processed. At this point lrtOracle.getRSETHPrice() will be calculated using the sum of balances of stETH (there are no other assets in the deposit pool), (X + 1) * 1e18 / 1 = (X + 1) * 1e18. The minted amount is then calculated as X * 1e18 / (X + 1) * 1e18 = X / (X + 1) = 0. The user is minted 0 RSETH and the deposited amount belongs to the attacker's share.

Recommendation

There are multiple ways to prevent the inflation attack:

There are multiple ways of solving the issue:

  1. Introduce a minimum output parameter to control slippage when depositing.
  2. Track balances internally so donations cannot affect the share value.
  3. Mint an initial amount of "dead shares" so the attacker cannot frontrun and inflate the pool.

A very good discussion of these can be found here.

Assessed type

Other

#0 - c4-pre-sort

2023-11-16T23:33:15Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2023-11-16T23:33:22Z

raymondfam marked the issue as duplicate of #42

#2 - c4-judge

2023-12-01T17:02:48Z

fatherGoose1 changed the severity to 3 (High Risk)

#3 - c4-judge

2023-12-01T17:06:41Z

fatherGoose1 marked the issue as satisfactory

Findings Information

Awards

140.2525 USDC - $140.25

Labels

bug
2 (Med Risk)
satisfactory
sufficient quality report
duplicate-148

External Links

Lines of code

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

Vulnerability details

Summary

Users depositing in the protocol have no control over the amount of RSETH minted in return for their deposit.

Impact

The depositAsset() function present in the LRTDepositPool contract allows users to deposit any of the supported assets into the protocol in exchange for RSETH.

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

119:     function depositAsset(
120:         address asset,
121:         uint256 depositAmount
122:     )
123:         external
124:         whenNotPaused
125:         nonReentrant
126:         onlySupportedAsset(asset)
127:     {
128:         // checks
129:         if (depositAmount == 0) {
130:             revert InvalidAmount();
131:         }
132:         if (depositAmount > getAssetCurrentLimit(asset)) {
133:             revert MaximumDepositLimitReached();
134:         }
135: 
136:         if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
137:             revert TokenTransferFailed();
138:         }
139: 
140:         // interactions
141:         uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
142: 
143:         emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
144:     }

The RSETH mint amount depends on different factors, including the current price of the deposit asset, and the RSETH price, which also factors prices and amounts from all of the supported assets:

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

095:     function getRsETHAmountToMint(
096:         address asset,
097:         uint256 amount
098:     )
099:         public
100:         view
101:         override
102:         returns (uint256 rsethAmountToMint)
103:     {
104:         // setup oracle contract
105:         address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
106:         ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);
107: 
108:         // calculate rseth amount to mint based on asset amount and asset exchange rate
109:         rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
110:     }

As the user is depositing a single asset, any change over any of the dependent factors may alter the minted amount the user receives for their deposit. Depositors don't have any slippage control over how much RSETH is minted for their deposit.

Recommendation

Add a minOutput parameter to depositAsset() to allow depositors to control the minted amount.

    function depositAsset(
        address asset,
-       uint256 depositAmount
+       uint256 depositAmount,
+       uint256 minOutput
    )
        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);
        
+       if (rsethAmountMinted < minOutput) {
+         revert InsufficientOutputAmount();
+       }

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

Other

#0 - c4-pre-sort

2023-11-16T23:35:29Z

raymondfam marked the issue as sufficient quality report

#1 - c4-pre-sort

2023-11-16T23:35:37Z

raymondfam marked the issue as duplicate of #39

#2 - c4-pre-sort

2023-11-17T06:43:19Z

raymondfam marked the issue as duplicate of #148

#3 - c4-judge

2023-11-29T19:11:08Z

fatherGoose1 marked the issue as satisfactory

Awards

2.7592 USDC - $2.76

Labels

bug
grade-b
QA (Quality Assurance)
sufficient quality report
Q-07

External Links

Report

Summary

Low Issues

Total of 7 issues:

IDIssue
L-1Supported assets cannot be removed
L-2Duplicate access list in RSETH contract
L-3Admin could get locked out when updating LRTConfig
L-4Missing storage gap in LRTConfigRoleChecker
L-5No decimal normalization in price feeds
L-6Potential denial of service due to unbounded gas usage in getTotalAssetDeposits()
L-7Potential overflow in getAssetCurrentLimit()

Non Critical Issues

Total of 2 issues:

IDIssue
NC-1Add an initializer to the LRTConfigRoleChecker contract
NC-2Use UUPS over TransparentProxy

Low Issues

<a name="L-1"></a>[L-1] Supported assets cannot be removed

Configured assets in the LRTConfig contract can be added by calling addNewSupportedAsset(), but there is no current way of removing these assets if it is needed later.

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

73:     function addNewSupportedAsset(address asset, uint256 depositLimit) external onlyRole(LRTConstants.MANAGER) {
74:         _addNewSupportedAsset(asset, depositLimit);
75:     }

Consider adding a function to let admins or managers to eventually remove an asset from the supported list.

<a name="L-2"></a>[L-2] Duplicate access list in RSETH contract

The LRTConfig acts as an access list through the protocol, contracts inherit from LRTConfigRoleChecker that provides a link to the LRTConfig contract to check for access control.

However, the RSETH contract inherits from AccessControlUpgradeable and also uses LRTConfig, meaning this contract has two different access lists. Both of these access lists are mixed within the contract: mint and burn access go through the access list present in the contract itself, while pause is controlled via the onlyLRTManager modifier present in the LRTConfig access list.

This is confusing and error prone. Consider using just the access list defined by LRTConfig.

<a name="L-3"></a>[L-3] Admin could get locked out when updating LRTConfig

Access control within the protocol is provided by an access list implemented in the LRTConfig contract.

This access list is used throughout the protocol by the LRTConfigRoleChecker mixin, which holds the reference to the current LRTConfig contract.

The config instance present in the LRTConfigRoleChecker contract can be updated by an admin using the updateLRTConfig() function:

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

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

Once the new config takes place, the new access list from this instance will be effective. This means that if access control is not properly set up in the new instance, the admin could be locked out of the protocol.

Consider either:

  • Using a two step process, where the change is made effective when the admin accepts the change in the config instance, or
  • Check that the current admin is also an admin in the new config instance before applying the changes in updateLRTConfig().

<a name="L-4"></a>[L-4] Missing storage gap in LRTConfigRoleChecker

The LRTConfigRoleChecker contract is used as a base contract for upgradeable contracts but doesn't contain any storage gap to ensure enough space for future revisions.

  abstract contract LRTConfigRoleChecker {
      ILRTConfig public lrtConfig;
+     uint256[50] private __gap;

<a name="L-5"></a>[L-5] No decimal normalization in price feeds

Chainlink feeds simply returns the price without checking for any decimal discrepancy.

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

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

In the case price feeds used by supported assets don't match in their decimals, the error will be carried forward during the calculation of the RSETH price in getRSETHPrice(), as numbers with different precision will be aggregated together.

Currently, feeds for all supported assets (stETH, cbETH and rETH) have 18 decimals, but caution must be taken if other assets are added.

<a name="L-6"></a>[L-6] Potential denial of service due to unbounded gas usage in getTotalAssetDeposits()

The implementation of getTotalAssetDeposits() is composed of the asset balance in the DepositPool, and the aggregation of all balances and deposits in EigenLayer for each of the registered node delegators.

47:     function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
48:         (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
49:             getAssetDistributionData(asset);
50:         return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
51:     }
71:     function getAssetDistributionData(address asset)
72:         public
73:         view
74:         override
75:         onlySupportedAsset(asset)
76:         returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
77:     {
78:         // Question: is here the right place to have this? Could it be in LRTConfig?
79:         assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
80: 
81:         uint256 ndcsCount = nodeDelegatorQueue.length;
82:         for (uint256 i; i < ndcsCount;) {
83:             assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
84:             assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
85:             unchecked {
86:                 ++i;
87:             }
88:         }
89:     }

As node delegators are expected to grow over time, this may eventually cause a denial of service due to the unbounded gas usage required to compute the function.

Note that getTotalAssetDeposits() is a key function in the protocol, as it is used in getRSETHPrice() to calculate the RSETH price and in getAssetCurrentLimit(), which is then used to check for current deposit limits in depositAsset().

A potential solution could be to track balances internally instead of aggregating all volume on the fly for each node delegator.

<a name="L-7"></a>[L-7] Potential overflow in getAssetCurrentLimit()

The implementation of getAssetCurrentLimit() subtracts the configured limit for the asset to the current total deposits in the protocol:

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

56:     function getAssetCurrentLimit(address asset) public view override returns (uint256) {
57:         return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
58:     }

As deposits held in EigenLayer are expected to grow over time, it is technically possible for getTotalAssetDeposits(asset) to be greater than lrtConfig.depositLimitByAsset(asset), causing an overflow.

Consider returning 0 if this is this happens:

    function getAssetCurrentLimit(address asset) public view override returns (uint256) {
        uint256 limit = lrtConfig.depositLimitByAsset(asset);
        uint256 balance = getTotalAssetDeposits(asset);
        
        if (limit > balance) {
          unchecked {
            return limit - balance;
          }
        }
        
        return 0;
    }

Non Critical Issues

<a name="NC-1"></a>[NC-1] Add an initializer to the LRTConfigRoleChecker contract

All the contracts that inherit from LRTConfigRoleChecker have to initialize the lrtConfig variable in its own initializer, having this code duplicated across all contracts.

Add an initializer to LRTConfigRoleChecker to have the derived contract use it during initialization.

    function __LRTConfigRoleChecker_init(address lrtConfigAddr) internal onlyInitializing {
        __LRTConfigRoleChecker_init_unchained(lrtConfigAddr);
    }

    function __LRTConfigRoleChecker_init_unchained(address lrtConfigAddr) internal onlyInitializing {
        lrtConfig = ILRTConfig(lrtConfigAddr);
        emit UpdatedLRTConfig(lrtConfigAddr);
    }

<a name="NC-2"></a>[NC-2] Use UUPS over TransparentProxy

The UUPS pattern is more flexible than the TransparentProxy as the upgrade logic resides in the implementation itself. It is also cheaper to use, since the Proxy doesn't need to check if the caller is the admin to switch between logics.

The general recommendation is to use UUPS over TransparentProxy, see Transparent vs UUPS Proxies.

#0 - c4-pre-sort

2023-11-17T23:16:44Z

raymondfam marked the issue as sufficient quality report

#1 - raymondfam

2023-11-17T23:18:50Z

Possible upgrade:

L-5 --> #479

#2 - c4-judge

2023-12-01T16:47:36Z

fatherGoose1 marked the issue as grade-b

#3 - c4-judge

2023-12-01T18:03:24Z

fatherGoose1 removed the grade

#4 - c4-judge

2023-12-01T18:43:56Z

fatherGoose1 marked the issue as grade-b

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter