Kelp DAO | rsETH - Stormreckson's results

Lines of code

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

Vulnerability details

The node delegator is added by the LRTAdmin by calling addNodeDelegator the new nodeDelegatorContract is pushed into the nodeDelegatorQueue array which can later be used by the LRTManager to deposit assets on EigenLayer.

However there are a few potential problems with the addNodeDelegator function..

Impact

1- nodeDelegatorContracts can be pushed into the array and they can't be popped this cause an issue where a nodeDelegatorContract gets taken over by a malicious user and it can't be removed. However unlikely It can be more significant because of the different access roles to the functions if the LRTAdmin which is in charge of calling the addNodeDelegatorContract function adds a malicious contract due to human error or the nodeDelegatorContract gets taken over the LRTManager which is in charge of calling the transferAssetToNodeDelegator might still procced by sending assets to such contract which will lead to loss of users asset.

2-No existence check on the nodeDelegatorContract being added.

The same nodeDelegatorContract can represent multiple array indexes, when LRTManager calls transferAssetToNodeDelegator to transfer assets the nodeDelegatorContract is represented by it's index in nodeDelegatorQueue[ndcIndex].

If a nodeDelegatorContract is represented multiple times in the array it can potentially lead to confusion for the users and a possible misplace of asset into a different strategy.

Proof of Concept

For example

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

        for (uint256 i; i < length;) {
            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);

addNodeDelegator adds a new nodeDelegatorContract without checking if it already exists in the array, adding the same nodeDelegatorContract of the same address in different indexes.

Also due to the fact that there's no way to remove a nodeDelegatorContract the array indexes will not be in order, a contract address might be represented twices or more in the index.

LRTManager might call transferAssetToNodeDelegator with the wrong index.

function transferAssetToNodeDelegator(
        uint256 ndcIndex,
        address asset,
        uint256 amount
    )
        external
        nonReentrant
        onlyLRTManager
        onlySupportedAsset(asset)
    {
        address nodeDelegator = nodeDelegatorQueue[ndcIndex];
        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
            revert TokenTransferFailed();
        }

Tools Used

Manual Review

Recommended Mitigation Steps

Add a remove function for the nodeDelegatorContract and an existence check for the newly added nodeDelegatorContract

Assessed type

Invalid Validation