Kelp DAO | rsETH - ge6a'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#L95-L110

Vulnerability details

Proof of Concept

The function depositAsset() allows a user to deposit a specific amount of any supported token and, in return, receive a specific amount of rsETH. This amount of rsETH is calculated by dividing the total value of deposited assets in ETH by the price of rsETH in ETH. The price of rsETH in ETH is obtained by summing all protocol assets denominated in ETH and dividing by the total supply of rsETH. The price of the supported assets in ETH is sourced from Chainlink or another oracle. Due to the volatility of the supported assets, the price of rsETH can change, affecting the amount of rsETH a user would receive. The tokens used represent staked ethers and typically have a price close to that of ether. I examined historical charts of the supported Asset/ETH pairs, showing changes of a few percents. For instance, on 8/14/2023, the price of stETH dropped from 1.002 ETH to 0.9245 ETH. For users depositing a significant capital, slippage of a few percents can lead to substantial losses. The same function lacks a deadline. When a deposit transaction is sent, it may take a considerable amount of time before it is executed due to various circumstances or may be intentionally delayed by a malicious operator. When executed, it can result in financial losses for the user, or there may no longer be a need for its execution from the user's perspective.

Impact

Substantial financial losses for users which deposit significant capital.

Tools Used

Manual review

Recommended Mitigation Steps

Implement slippage and deadline protection so that a user can choose the minimum amount of rsETH they want to receive against their deposit and the time until which the transaction is valid.

Assessed type

Timing

Lines of code

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

Vulnerability details

Proof of Concept

The function addNodeDelegatorContractToQueue() adds a list of node delegators to the nodeDelegatorQueue array. The issue is that it does not check whether these node delegators are already part of the array. If they are already part of it, they will be added again. The function getAssetDistributionData(address asset) iterates through all records in nodeDelegatorQueue and sums the tokens located in the depositPool, NDCs, and eigenLayer. Thus, if a node delegator is duplicated, the tokens managed by it will be added more than once to the total sum. Consequently, this will lead to incorrectly calculated rsETH price in the getRSETHPrice() function, and a user would receive a smaller amount of rsETH in exchange for their deposit. An additional issue is the fact that node delegators cannot be removed from nodeDelegatorQueue; they can only be added. Finally a simple test showing it is possible.

function test_NodeDelegatorQueueDuplicates() external {
        address nodeDelegatorContractOne = address(new MockNodeDelegator());
        address nodeDelegatorContractTwo = address(new MockNodeDelegator());
        address nodeDelegatorContractThree = address(new MockNodeDelegator());

        address[] memory nodeDelegatorQueue = new address[](3);
        nodeDelegatorQueue[0] = nodeDelegatorContractOne;
        nodeDelegatorQueue[1] = nodeDelegatorContractTwo;
        nodeDelegatorQueue[2] = nodeDelegatorContractThree;

        vm.startPrank(admin);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);

        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);

        vm.stopPrank();
    }

Impact

This issue has High impact and Low likelihood, due to the fact that the administrative account is a multisignature wallet. My reasons to assign a Medium severity instead of Low are two. First is the high impact: potentially significant losses for users and a high likelihood of it going unnoticed for an extended period. Second, contract addresses are long, and it's not so difficult to make an oversight mistake, even by multiple people.

Tools Used

Manual review

Recommended Mitigation Steps

Check if a node delagator is already part of nodeDelegatorQueue before add it again.

Assessed type

Invalid Validation