Kelp DAO | rsETH - bronze_pickaxe's results

Lines of code

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

Vulnerability details

Impact

A user can use LRTDepositPool.depositAsset to deposit any of the supported assets into the LRTDepositPool contract:

LRTDepositPool.sol#L119-L144

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

The first user to deposit the funds(rsETH supply will be 0 because no one has minted rsETH before) will get rsETH minted in a 1:1 rate:

LRTOracle.sol#L56-L58

if (rsEthSupply == 0) {
	return 1 ether;
}

The problem lays in LRTDepositPool.getAssetDistributionData(). This function calculates the asset amount distribution data among depositPool, NDCs and eigenLayer by using balanceOf(). How can this impact a depositor? Consider the following.

Proof of Concept

• Alice is the first depositor and deposits 1 wei of stETH(this can be any arbitrary amount depending on how many decimals the getAssetPrice() function returns).
• After Alice has minted her rsETH, Alice decides to directly transfer 100 wei of stETH(or any other supported coin) into the LRTDepositPool contract.
• Bob is the second depositor and decides to deposit 10 stETH.
• The amount of rsETH that Bob should receive is calculated here:

LRTDepositPool.sol#L109

rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

(amount * lrtOracle.getAssetPrice(asset)) will work as it is supposed to, but this time, since Alice has sent extra funds to the LRTDepositPool contract, (amount * lrtOracle.getAssetPrice(asset)) will be divided by a much bigger number, leading to having way less rsETH minted:

LRTOracle.sol#L70-L71

uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
totalETHInPool += totalAssetAmt * assetER;

LRTOracle.sol#L78

return totalETHInPool / rsEthSupply;

What happens here it the following:

• totalAssetAmt will be 101 wei because of the 1 wei + 100 wei of Alice.
• totalAssetAmt will be multiplied by the price of the asset, this value will be assigned to totalETHInPool
• However, the return value is totalETHInPool will be divided by rsEthSupply, which is 1 wei not 101 wei! This means this number lrtOracle.getRSETHPrice(); that gets used to divide, will be disproportionally bigger than the previous and next mints, leading to less rsETH minted to Bob.

Tools used

Manual review

Recommended Mitigation Steps

Do not use address(this).balanceOf(), use another way of accounting the deposits.

Assessed type

Invalid Validation

1. maxNodeDelegatorCount does not get enforced

Impact

There are a few problems with the usage of maxNodeDelegatorCount in LRTDepositPool.sol.

First problem, consider the following:

• maxNodeDelegatorCount = 10
• 10 NodeDelegators have been added using LRTDepositPool.addNodeDelegatorContractToQueue
• maxNodeDelegatorCount gets updated to 8
• However, LRTDepositPool.getAssetDistributionData will still use all 10 NodeDelegators, despite the max being set to 8:

LRTDepositPool.sol#L81-L86

 uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;

Second problem, consider the following: First problem, consider the following:

• maxNodeDelegatorCount = 10
• 10 NodeDelegators have been added using LRTDepositPool.addNodeDelegatorContractToQueue
• maxNodeDelegatorCount gets updated to 8
• However, it is still possible to send funds from LRTDepositPool to any NodeDelegator that has ever been added using LRTDepositPool.transferAssetToNodeDelegator

LRTDepositPool.sol#L193-195

address nodeDelegator = nodeDelegatorQueue[ndcIndex];
        if (!IERC20(asset).transfer(nodeDelegator, amount)) {
            revert TokenTransferFailed();

These two scenarios should not be possible.

Tools used

Manual Review

Recommended Mitigation Steps

Either remove NodeDelegators from the nodeDelegatorQueue when setting a maxNodeDelegatorCount < currentmaxNodeDelegatorCount, or, remove the functionality of being able to set a maxNodeDelegatorCount that is less than the current maxNodeDelegatorCount.

2. Incorrect access control in 'addNodeDelegatorToQueue'

Impact

    /// @notice add new node delegator contract addresses
    /// @dev only callable by LRT manager
    /// @param nodeDelegatorContracts Array of NodeDelegator contract addresses
    // @audit should be only callable by LRT Manager but is onlyLRTAdmin
    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;
            }
        }
    }

As you can see from the comments of the devs comment it should be the LRT manager that should be able to call this, but currently its the LRTAdmin that can call it only because of the onlyLRTAdmin modifier.

Recommended Mitigation Steps

Use onlyLRTManager instead of onlyLRTAdmin

3. A person can always mint rsETH for cheaper by minting just before stETH rebases

Impact

stETH from Lido is a rebase token. The Lido docs state:

The mechanism which updates the stETH balances every day is called a “rebase”. Every day at 12PM UTC the amount of stETH in your address will increase with the current APR.

This means that any user can just wait until stETH almost rebases and mint rsETH to receive more rsETH than he would have after the rebase