c4-submissions commented 10 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L162-L176 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L110

Vulnerability details

Proof of Concept

The function addNodeDelegatorContractToQueue() adds a list of node delegators to the nodeDelegatorQueue array. The issue is that it does not check whether these node delegators are already part of the array. If they are already part of it, they will be added again. The function getAssetDistributionData(address asset) iterates through all records in nodeDelegatorQueue and sums the tokens located in the depositPool, NDCs, and eigenLayer. Thus, if a node delegator is duplicated, the tokens managed by it will be added more than once to the total sum. Consequently, this will lead to incorrectly calculated rsETH price in the getRSETHPrice() function, and a user would receive a smaller amount of rsETH in exchange for their deposit. An additional issue is the fact that node delegators cannot be removed from nodeDelegatorQueue; they can only be added. Finally a simple test showing it is possible.

function test_NodeDelegatorQueueDuplicates() external {
        address nodeDelegatorContractOne = address(new MockNodeDelegator());
        address nodeDelegatorContractTwo = address(new MockNodeDelegator());
        address nodeDelegatorContractThree = address(new MockNodeDelegator());

        address[] memory nodeDelegatorQueue = new address[](3);
        nodeDelegatorQueue[0] = nodeDelegatorContractOne;
        nodeDelegatorQueue[1] = nodeDelegatorContractTwo;
        nodeDelegatorQueue[2] = nodeDelegatorContractThree;

        vm.startPrank(admin);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);

        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);

        vm.stopPrank();
    }

Impact

This issue has High impact and Low likelihood, due to the fact that the administrative account is a multisignature wallet. My reasons to assign a Medium severity instead of Low are two. First is the high impact: potentially significant losses for users and a high likelihood of it going unnoticed for an extended period. Second, contract addresses are long, and it's not so difficult to make an oversight mistake, even by multiple people.

Tools Used

Manual review

Recommended Mitigation Steps

Check if a node delagator is already part of nodeDelegatorQueue before add it again.

Assessed type

Invalid Validation

c4-pre-sort commented 10 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 10 months ago

raymondfam marked the issue as duplicate of #36

c4-judge commented 10 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)