c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

The nodeDelegatorQueue list in LRTDepositPool can only grow in size and the function to add addresses to it does not check if any of them would be a duplicate address. If the admins by mistake push a duplicate value to this array it would lead to breaking core functionality of the protocol and leading to potential loss of funds.

Proof of Concept

nodeDelegatorQueue is a regular list of addresses. It is only possible to add new addresses to it. The function addNodeDelegatorContractToQueue() has some input validation, checking if the number of addresses to be added exceeds the current limit and also if any of the addresses is a zero address.

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

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

    address[] public nodeDelegatorQueue;

    .
    .
    .

    /// @notice add new node delegator contract addresses
    /// @dev only callable by LRT manager
    /// @param nodeDelegatorContracts Array of NodeDelegator contract addresses
    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 input validation is not enough, as the array should contain only unique values. There is no input validation to check if any of the values in the input nodeDelegatorContracts are duplicate within the array, or check if any of them is already contained in the array.

This is a serious issue, as if accidentally a duplicate address is pushed this would break core functionality in the protocol. As an example let's take a look at the function getAssetDistributionData():

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

    /// @dev provides asset amount distribution data among depositPool, NDCs and eigenLayer
    /// @param asset the asset to get the total amount of
    /// @return assetLyingInDepositPool asset amount lying in this LRTDepositPool contract
    /// @return assetLyingInNDCs asset amount sum lying in all NDC contract
    /// @return assetStakedInEigenLayer asset amount staked in eigen layer through all NDCs
    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;
            }
        }
    }

This function returns the sum of an asset distributed along the Node Delegators and EigenLayer. If there is a duplicate value in the nodeDelegatorQueue array, the sum would contain that value twice, making it incorrect.

Additionally LRTOracle's getRSETHPrice() calls getTotalAssetDeposits() in LRTDepositPool, which in turn calls getAssetDistributionData(). This means that in case this unwanted action happens, the oracle would return a wrong price, leading to possible loss of funds if abused.

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

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        .
        .
        .
            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
        .
        .
        .
    }

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

    function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Before adding elements to the list add a check to see if any of the elements in the input array nodeDelegatorContracts are duplicate or if any element in the input array nodeDelegatorContracts is a duplicate of an element in the nodeDelegatorQueue.

Assessed type

Invalid Validation

c4-pre-sort commented 11 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #36

c4-judge commented 10 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)