Kelp DAO | rsETH - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. LRTDepositPool.depositAsset() allows one to deposit depositAmount of asset in exchange of rsethAmountMinted of rsEth. However, it allows the first depositor to inflate the price of RsETH and then rip off from future depositors.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

LRTDepositPool.depositAsset() allows the first depositor to inflate the price of RsETH and then rip off from future depositors as follows:

1. Alice can deposit one wei of asset and get one wei of RsETH first. This is because lrtOracle.getRSETHPrice() will return the price of 1 ether when rsEthSupply == 0.

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

2. Then Alice, can "donate" a sizable amount of asset, say 1000e18 to the LRTDepositPool. As a result, now one wei of RsETH is worthy of 1000e18 + 1 of the asset.

3. Now another user, Jack deposits 2000e18 to LRTDepositPool, he will only get 1 wei of RsETH also due to division loss in the following line:

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

4. Now the price of RsETH becomes: (3000e18+1) / 2 = 1500e18. So effectively Alice steals 500 eth asset from Jack. If she can withdraw the asset now, she will get 1500e18 asset, gaining 500 eth asset.

5. If Jack withdraw the asset now, he will only get 1500et18, and lost 500 et18.

6. Alice might rip off from future depositors in a similar manner due to the highly inflated price of RsEth and the division precision loss.

Tools Used

VSCode

Recommended Mitigation Steps

There are many ways to prevent such first depositor attack:

1. Ensure a minimum deposit value, say at least 10000, this increase the price for the attacker.
2. The deployer of contract can call the LRTDepositPool.depositAsset() with 100000 depositamount and then lock those asset forever and prevent future first deposit attack. Since it is now hard to inflate the price.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. LRTConfig.updateAssetStrategy() fails to withdraw funds from the old strategy before setting a new strategy for the asset. There are two impacts for this: 1) The funds in the old strategy might be lost; 2) the total value for the asset returned by getTotalAssetDeposits() will be wrong since it does not include the portion in the old strategy; 3) Then the LRTOracle.getRSETHPrice() will return a smaller value too. Such price drop might lead trading that take advantage of the situation.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

LRTConfig.updateAssetStrategy() allows the admin to change the strategy for an asset to a new one.

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

However, the function fails to withdraw the funding from the old strategy first and as a result those funding might be lost. In addition, the following impacts will ensure:

1. the total value for the asset returned by getTotalAssetDeposits() will be wrong since it does not include the portion in the old strategy; and 2) Then the LRTOracle.getRSETHPrice() will return a smaller value too. Such price drop might lead trading that take advantage of the situation.

Tools Used

VSCode

Recommended Mitigation Steps

Make sure the balance for the strategy is zero before changing an old strategy to a new one.

Assessed type

Invalid Validation

QA1. It might overwrite an existing key:

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

Mitigation:

 function _setToken(bytes32 key, address val) private {
        UtilLib.checkNonZeroAddress(val);
-        if (tokenMap[key] == val) {
+        if (tokenMap[key] != address(0)) {

            revert ValueAlreadyInUse();
        }
        tokenMap[key] = val;
        emit SetToken(key, val);
    }

QA2. LRTConfigRoleChecker.lrtConfigAddr() has the danger to lose the admin for the newly set lrtConfigAddr.

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

The function is only callable by the current LRTAdmin, which might not be the LRTAdmin for the newly set lrtConfigAddr. If an invalid lrtConfigAddr is set, then there is no way to correct it.

Mitigation: ensure that the current LRTadmin will also be the LRTAdmin for the new lrtConfigAddr. After that, one can also transfer the LRTadmin to another use using a two-step procedure.

QA3. There is no mechanism to check whether duplicate nodeDelegatorContract is added to the array or not.

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

Impact: getTotalAssetDeposits() might return the wrong total value since the balance for the duplicate nodeDelegatorContract will be doubly accounted. A user might not be able to deposit asset due to falsely inflated total deposit amount.

Mitigation: Introduce a mapping so that one can check whether we are adding a new nodeDelegatorContract or not to avoid adding duplicate. Otherwise, the limit maxNodeDelegatorCount might be wasted due to duplicates.

QA4. LRTDepositPool.updateMaxNodeDelegatorCount() might decrease maxNodeDelegatorCount, but fails to check that the new maxNodeDelegatorCount_ might result that the number of nodeDelegators already exceeds the new limit.

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

Mitigation: Check and ensure the new maxNodeDelegatorCount_ is not small than the number of current nodeDelegators. Otherwise, the function should revert.

QA5. getAssetPrice() fails to check that assetPriceOracle[asset] == address(0). It is possible that the Oracle for the asset has not been set yet.

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

Mitigation: check and ensure assetPriceOracle[asset] != address(0).

QA6. NodeDelegator.transferBackToLRTDepositPool() fails to check that lrtDepositPool != address(0). When lrtDepositPool == address(0), the function will send tokens to the zero address.

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

Mitigation: check and make sure lrtDepositPool != address(0)

QA7. The NodeDelegator contract has not function to withdraw assets from a strategy.

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

Impact: the funds deposited into a strategy is lost since there is no function to withdraw them from the strategy.

Mitigation: add a function to allow a user to withdraw funds from a strategy.