Kelp DAO | rsETH - dipp'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 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L47-L58 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L70

Vulnerability details

Impact

To add new node delegators, the LRTAdmin calls the addNodeDelegatorContractToQueue which takes an array of node delegators and pushes them to the nodeDelegatorQueue. The added node delegators aren't checked to be unique so nodeDelegatorQueue may contain duplicate node delegator addresses.

If nodeDelegatorQueue contains duplicates then LRTDepositPool:getAssetDistributionData returns an incorrect, higher value for assetLyingInNDCs and assetStakedInEigenLayer. This would cause LRTDepositPool:getTotalAssetDeposits to also return a higher value which affects both the LRTOracle:getRSETHPrice and LRTDepositPool:getAssetCurrentLimit functions.

A higher return value for LRTDepositPool:getAssetCurrentLimit could prevent deposits before the real limit for the asset is reached.

LRTOracle:getRSETHPrice could cause users to receive less rSETH tokens when depositing.

Proof of Concept

The test code below shows how duplicate node delegator addresses may be added and that LRTDepositPool:getAssetDistributionData returns inflated values.

contract LRTDepositPoolGetAssetDistributionData is LRTDepositPoolTest {
    
    [code...]

        function test_DuplicateNodeDelegatorsIncorrectData() 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);
            vm.stopPrank();

            // deposit 3 ether rETH
            vm.startPrank(alice);
            rETH.approve(address(lrtDepositPool), 3 ether);
            lrtDepositPool.depositAsset(rETHAddress, 3 ether);
            vm.stopPrank();

            (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
                lrtDepositPool.getAssetDistributionData(rETHAddress);

            assertEq(assetLyingInDepositPool, 3 ether, "Asset lying in deposit pool is not set");
            assertEq(assetLyingInNDCs, 0, "Asset lying in NDCs is not set");
            assertEq(assetStakedInEigenLayer, 3 ether, "Asset staked in eigen layer is not set");

            // Add duplicates
            vm.startPrank(admin);
            lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorQueue);
            vm.stopPrank();

            (assetLyingInDepositPool, assetLyingInNDCs, assetStakedInEigenLayer) =
                lrtDepositPool.getAssetDistributionData(rETHAddress);

            assertEq(assetLyingInDepositPool, 3 ether, "Asset lying in deposit pool is not set");
            assertEq(assetLyingInNDCs, 0, "Asset lying in NDCs is not set");
            assertEq(assetStakedInEigenLayer, 6 ether, "Asset staked in eigen layer is not set");
        }
}

With inflated values returned, LRTDepositPool:getTotalAssetDeposits also returns a higher value which causes LRTDepositPool:getAssetCurrentLimit to either return a lower value than it should or revert due to underflow preventing further deposits.

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

The incorrect returned value from LRTDepositPool:getTotalAssetDeposits would also cause LRTOracle:getRESTHPrice to have a higher totalETHInPool value that it should, leading to a higher returned value.

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
    [code...]
        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            [code...]
            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

This causes LRTDepositPool:getRsETHAmountToMint to be lower, leading to less shares for the user when depositing assets.

function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

Tools Used

Manual

Foundry

Recommended Mitigation Steps

Consider using a mapping to set check if a node delegator has been added.

Assessed type

Invalid Validation