c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

During the LRTDepositPool configuration admin should put Node Delegator contracts with addNodeDelegatorContractToQueue(address[]). If there will be a duplication of the contract, whole Pool pricing and minting logic will become invalid. Even more, because there are no options to delete items from the "nodeDelegatorQueue", it will require new contract version update.

Severity justification

While this function has an admin access level, it still can be considered as a Medium because of how crucial is impact can be. https://docs.code4rena.com/awarding/judging-criteria/severity-categorization 2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Proof of Concept

1) NodeDelegators is a core part of LRTDepositPool with an idea to split total pool assets into multiple addresses to have some flexibility during interactions with Eigenlayer contracts(StrategyManager and Strategy). With this concept, it is essential to maintain the number of delegators dynamically.

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

    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;
            }
        }
    }

2) Because physically all assets-based interactions with Eigenlayer contracts will be done with Delegator addresses, bigger part of total pool assets will be split and sent to Delegators addresses(with later deposits into Eigenlayer strategies). https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L183-L197

    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();
        }
    }

3) Finally, LRTOracle during pricing and LRTDepositPool during mint calculation will use "getAssetDistributionData" where all Delegators from "nodeDelegatorQueue" will count up their assets together. https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L71-L89

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

4) There is also no options to fix such mistake, so contract Pause and new Version deploy will be required.

Tools Used

Manual audit

Recommended Mitigation Steps

Replace array with Enumerable set https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/structs/EnumerableSet.sol

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)