Kelp DAO | rsETH - Topmark's results

Lines of code

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

Vulnerability details

Impact

In situations whereby Node Delegator Contract is compromised, since there is no way to remove this node Delegator contract from storage it would in extension affect the security of the Protocol. In addition to this in a situation admins due to human error adds same node Delegator contract twice it would cause Inflation in calculation from L83 & excess transfer at L194 of the same contract since node Delegator cannot be removed.

Proof of Concept

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

The code above shows how Node Delegator is added to the contract Node Delegator Queue, there is no point in the contract or other related inherited contract that shows where Node Delegator can be removed if Node Delegator address is compromised.

POC code

No Poc Code since it is a design problem

Tools Used

Manual Review

Recommended Mitigation Steps

An Additional function should be added to the contract to make removal of node delegator is possible after necessary conditions and interaction precautions have been met, it should be callable by only Admin i.e onlyLRTAdmin. A sample removal function could look like this :

 function removeNodeDelegatorContractToQueue(uint index) external onlyLRTAdmin {
        if (index >= nodeDelegatorQueue.length) return;

        for (uint i = index; i < nodeDelegatorQueue.length-1; i++){
            nodeDelegatorQueue[i] = nodeDelegatorQueue[i+1];
        }
        nodeDelegatorQueue.pop();
       emit NodeDelegatorRemovedinQueue(nodeDelegatorQueue[i]);
    }

Note: The Removal function should be more sophisticated that this to ensure node Delegator address is safely removed without escalation to other problems

Assessed type

Access Control