Kelp DAO | rsETH - merlinboii's results

Lines of code

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

Vulnerability details

Risk: High

• Impact: High
• Likelihood: Medium (Even though restricted to LRT admin, the inability to undo mistakenly added duplicates poses a significant risk)

Related Files

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

Impact

The duplicate node delegator addresses in the nodeDelegatorQueue leads to inaccurate calculations of asset amount distribution data of NDCs, and eigenLayer.

This issue is unrecoverable, as the current implementation lacks a mechanism to undo the addition of duplicates.

Vulnerabilities

The addNodeDelegatorContractToQueue function of the LRTDepositPool contract restricts to only the LRT admin to add new node delegator contract addresses by passing the nodeDelegatorContracts array.

File: src/LRTDepositPool.sol

162:     function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {

However, this function does not check check whether the given node delegator contract addresses are already added in the nodeDelegatorQueue.

File: src/LRTDepositPool.sol

168:      for (uint256 i; i < length;) {
169:            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
170:            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
171:            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
172:            unchecked {
173:                ++i;
174:            }
175:        }

As a result, if the LRT admin mistakenly adds node delegator contract addresses that are already included, the calculation of asset amount distribution data of NDCs, and eigenLayer from the getAssetDistributionData function will be incorrect.

File: src/LRTDepositPool.sol

81:        uint256 ndcsCount = nodeDelegatorQueue.length;
82:        for (uint256 i; i < ndcsCount;) {
83:            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
84:            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
85:            unchecked {
86:                ++i;
87:            }
88:        }

Furthermore the result of that calculation will always incorrect since implementation lacks functionality to remove duplicates from the nodeDelegatorQueue.

Proof of Concept

• PoC code: https://gist.github.com/filmptz/c396138c42a64d27702f92955c04c751
• The result of the test:

Tools Used

Foundry

Recommended Mitigation Steps

1. Consider introducing a mapping mechanism to track unique node delegator addresses, ensuring that each node delegator address in the nodeDelegatorQueue array is unique.

For example, introducing the mapping (address => bool) isNodeDelegatorAdded;

2. Applying the validation step to check the given array with this mapping into the addNodeDelegatorContractToQueue function (L169-171, and L174).

File: src/LRTDepositPool.sol

168:      for (uint256 i; i < length;) {
169:           if (isNodeDelegatorAdded[nodeDelegatorContracts[i]]) {
170:              revert AlredayAddedNodeDelegator();
171:            }
172:            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
173:            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
174:            isNodeDelegatorAdded[nodeDelegatorContracts[i]] = true;
175:            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
176:            unchecked {
177:                ++i;
178:            }
179:        }

This improvement will ensure accurate calculations in the getAssetDistributionData function.

The recommendation should be considered in alignment with the project's business requirements, and any updates should be tested to ensure they meet the specific needs of the project.

Assessed type

Invalid Validation