Kelp DAO | rsETH - Matin's results

Lines of code

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

Vulnerability details

Impact

Duplicate nodeDelegator contract addresses can lead to wrong distribution data in the contract LRTDepositPool

Proof of Concept

The function addNodeDelegatorContractToQueue() is responsible for adding the nodeDelegator contract addresses to the nodeDelegatorQueue array. This will then be used for distribution data in this contract. As we can see in the function addNodeDelegatorContractToQueue():

    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]);
            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
            unchecked {
                ++i;
            }
        }
    }

This function doesn't check the duplicate nodeDelegatorContracts[i] and just checks the mentioned address not be a zero address. In the case of providing duplicate and repetitive node delegator contract addresses mistakenly by the admin, these addresses would be added to the mentioned array. Now, If we look at the function getAssetDistributionData(), we can notice that the return amount would be non-accurate:

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

As it is clearly seen, the three return amounts of this function would be wrong due to recalculation of the balance of a nodeDelegatorQueue[i] if there would be duplicates of this address.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider checking the nodeDelegatorContracts[i] in the function addNodeDelegatorContractToQueue() before passing the address to the array nodeDelegatorQueue.

Assessed type

Invalid Validation