Kelp DAO | rsETH - leegh's results

Lines of code

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

Vulnerability details

Impact

Node delegator is pushed to nodeDelegatorQueue without checking if the node delegator already exists in the queue. Thus it is possible that one node delegator might be pushed to nodeDelegatorQueue multiple times. If this happens, getAssetDistributionData will produce wrong distribution for the specified asset, and affect the deposit of that asset.

Proof of Concept

LRTAdmin can add node delegator contract by function addNodeDelegatorContractToQueue. The function pushes the provided node delegator(s) to the queue directly, without checking if the node delegator already exists in the queue. Therefore it is possible that one node delegator is pushed to the queue multiple times if LRTAdmin calls addNodeDelegatorContractToQueue with an existing node delegator.

162:    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
163:        uint256 length = nodeDelegatorContracts.length;
164:        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
165:            revert MaximumNodeDelegatorCountReached();
166:        }
167:
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:        }
176:    }

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

If one node delegator is pushed into nodeDelegatorQueue multiple times, one asset's distribution of that delegator will be calculated multiple times as well in getAssetDistributionData, resulting a greater distribution than it should be.

Function: getAssetDistributionData
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:        }

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

The reuslt of getAssetDistributionData is used in depositAsset to check if the amount being deposited will exceed the asset's limit. If the result is incorrect (greater than it should be), a normal deposit which should succeed may be reverted.

Function: depositAsset
132:        if (depositAmount > getAssetCurrentLimit(asset)) {
133:            revert MaximumDepositLimitReached();
134:        }

Function: getAssetCurrentLimit
56:    function getAssetCurrentLimit(address asset) public view override returns (uint256) {
57:        return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
58:    }

Function: getTotalAssetDeposits
47:    function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
48:        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
49:            getAssetDistributionData(asset);
50:        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
51:    }

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L132-L134 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L56-L58 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L47-L51

Tools Used

Manual audit

Recommended Mitigation Steps

1. Define a mapping to record the added node delegators: mapping(address nodeDelegator => bool isExist) addedNodeDelegators.
2. When adding a new node delegator, check if the delegator exists already and only push it to queue if not existing.

Assessed type

Other