c4-submissions commented 11 months ago

Lines of code

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

Vulnerability details

Impact

The current implementation of the LRTDepositPool contract does not include a function to remove a NodeDelegator from the nodeDelegatorQueue. This absence presents a risk if an externally owned account (EOA) or an incompatible contract is mistakenly added to the queue. Such an error would disrupt key functions that rely on getAssetDistributionData, as this function attempts to call getAssetBalance on each NodeDelegator in the queue. Affected functions: LRTDepositPool: getTotalAssetDeposits, getRsETHAmountToMint, depositAsset LRTOracle: getRSETHPrice

Proof of Concept

The issue arises due to the iteration over nodeDelegatorQueue in getAssetDistributionData and the absence of validation in addNodeDelegatorContractToQueue. https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L82-L88 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L162-L176

Tools Used

Forge

Recommended Mitigation Steps

add a check in addNodeDelegatorContractToQueue and make sure that each element is linked to the lrtConfig

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]);
        //check if nodeDelegator is linked to lrtConfig or not
        require(
            INodeDelegator(nodeDelegatorContracts[i]).lrtConfig() == lrtConfig
        );
        nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
        emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
        unchecked {
            ++i;
        }
    }
}

add a function that allows safely removing a NodeDelegator from nodeDelegatorQueue:

function removeNodeDelegator(address _nodeDelegator) external onlyLRTAdmin {
    for (uint256 i; i < ndcsCount; ) {
        if(_nodeDelegator == nodeDelegatorQueue[i]) {
           nodeDelegatorQueue[i] = nodeDelegatorQueue[ndcsCount - 1];
           nodeDelegatorQueue.pop();
           emit NodeDelegatorRemovedFromQueue(_nodeDelegator);
           break;
        }
    }
}

Assessed type

DoS

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)