Kelp DAO | rsETH - pep7siup's results

Lines of code

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

Vulnerability details

Impact

LRTDepositPool.depositAsset relies on getRsETHAmountToMint to determine the amount of rsETH to mint. The first deposit can mint a minimal number of rsETH then donate LST tokens to the pool to grossly manipulate the rsETH price. When later depositor stakes into the pool they will lose their fund due to precision loss.

Proof of Concept

The root cause of the vulnerability is that the depositAsset function allows users to mint a realy small amount of rsETH at the initial stage (when rsETH supply is zero), exposing the pool to the exchange-rate inflation attack. Let us walk through the issue with the following scenario:

1. At initial pool condition, getRSETHPrice returns 1 ether. Alice initiates a LRTDepositPool.depositAsset call with a minimal amount of 1 stETH (price of approx. 1e18 ETH/stETH in base units) to mint roughly (1 * 1e18 / 1e18 = 1) rsETH.

// File: src/LRTDepositPool.sol
    function getRsETHAmountToMint(
        ...
@>1        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

2. Alice proceeds to transfer 1e18 stETH directly to the LRTDepositPool contract so that getTotalAssetDeposits now reads as 1 + 1e18. As a result, the getRSETHPrice call is manipulated to return an extremely high value since totalETHInPool was pumped up while rsEthSupply remains small. Given assetER stays unchanged at 1e18, getRSETHPrice now reads as (1 + 1e18) * 1e18 / 1 ~= 1e36.

// File: src/LRTOracle.sol
    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;
        ...
@>2        return totalETHInPool / rsEthSupply;
    }

3. Bob joins the pool after Alice with 0.99e18 stETH. Due to the inflated rsETH price, he receives ZERO rsETH in return due to precision loss.

@>1 becomes: rsethAmountToMint = 0.99e18 (stETH) * 1e18 (ETH/stETH) / 1e36 ~= 0 rsETH

4. As a result, Alice's 1 rsETH token now represents the entire stETH asset in the pool including the 0.99e18 stETH that belongs to Bob. Alice has effectively stolen Bob's fund. Furthermore, the rsETH price is now even pumped 2x higher since rsEthSupply stays the same while totalETHInPool has doubled, ensuring innevitable loss for future depositors.

Notice: Bob's deposit amount being higher than 0.99e18 will not save him from the loss since Alice can always sniff Bob transaction and frontruns him with the right donation amount described at step 2.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimal amount of rsETH tokens to be minted for the first minter, so that the rsETH price can be more resistant to manipulation.

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-11-kelp/tree/main/src/LRTDepositPool.sol#L170

Vulnerability details

Impact

The smart contract fails to check for duplicate entries in the nodeDelegatorQueue, leading to a potentially serious issue. The duplicated entries cause inflated balance readings in getAssetDistributionData, impacting the protocol's accouting functionality. This issue could cause potential losses of funds for users interacting with the protocol when the assets prices are maliciously inflated.

Proof of Concept

The proof of concept involves an admin adding the same nodeDelegatorContract to the queue multiple times, resulting in inflated balance readings during functions like getAssetDistributionData, getTotalAssetDeposits and getRSETHPrice.

1. The admin add the same nodeDelegatorContract to the queue multiple times without properly checking.
2. This leads to reading inflated value when sequences of functions getAssetDistributionData, getTotalAssetDeposits and getRSETHPrice are referenced. As can be seen from Line 83-84 below, assetLyingInNDCs & assetStakedInEigenLayer can be incorrectly acrued multiple times if nodeDelegatorQueue[i] is not unique, resulting in nth-time larger read value compared with the actual figures.

// File: src/LRTDepositPool.sol
71:    function getAssetDistributionData(address asset)
        ...
82:        for (uint256 i; i < ndcsCount;) {
83:            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]); // <= FOUND
84:            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset); // <= FOUND
        ...
89:    }

3. For example, if Alice joins the pool when rsETH is nth times over-valued, she would minting nth times less rsETH compared with normal circumstances, constituting a loss of fund.

// File: src/LRTOracle.sol
52:    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        ...
70:            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset); // <= FOUND: getTotalAssetDeposits reads values from the affected getAssetDistributionData() described above, resulting in inflated totalAssetAmt value, like-wise getRSETHPrice also jumps.
71:            totalETHInPool += totalAssetAmt * assetER;
            ...
79:    }

Tools Used

Manual Review

Recommended Mitigation Steps

Utilizing OpenZeppelin's EnumerableSet or a similar data structure for better uniqueness management.

Assessed type

Invalid Validation