Kelp DAO | rsETH - ABAIKUNANBAEV's results

Lines of code

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

Vulnerability details

Impact

In LRTDepositPool.sol contract, there is a function called getAssetCurrentLimit() that calculates the difference between lrtConfig.depositLimitByAsset(asset) and getTotalAssetDeposits(asset). However, due to the possibility of increasing the total asset deposits above the deposit limit level, there is a chance that the function will underflow and, consequently, depositAsset() will revert as well.

Proof of Concept

getTotalAssetDeposits(asset) consists of 3 components: assetLyingInDepositPool, assetLyingInNDCs`:

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

function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

The problem is that only the assetLyingInDepositPool can be controlled in the system due to the depositAsset() check:

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

if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

But assetLyingInNDCs and assetLyingInDepositPool can be increased due to the fact that the tokens can be sent to the node delegator contracts directly and without using transferAssetToNodeDelegator(). This can effectively lead to a situation where the limit may reach the level (even if it's not intentionally by the node delegator) where it's more than the current deposit limit for a specific type of asset. depositAsset() where getAssetCurrentLimit() is used will also underflow. Node delegators can also send tokens directly and put them into the strategy without using deposit pool functionality at all.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider changing the way of how total deposits are calculated.

Assessed type

Other

Gas Optimizations List

Total 5 issues.

[G‑01] Consider not using nonReentrant modifier where the possibility of reentrancy is 0.

Although it's considered as the best practice, it's recommended to use it with the functions with callbacks are possible such as ERC1155 or ERC777 functionality, for example:

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

 function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    }

[G‑02] Use hard-coded address instead of address(this) to save gas.

In terms of gas, it's better to use pre-calculated contract address instead of address(this):

assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

[G‑03] Consider not using pausability in the contract where whenNotPaused modifier is not used.

LRTOracle.sol pause() and unpause()functions are initialized butwhenNotPaused` modifier is never used. Therefore, these 2 functions and pausability feature may not be implemented as it just consumes more gas and not actually represent any logic:

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L101-109

function pause() external onlyLRTManager {
        _pause();
    }

    /// @dev Returns to normal state. Contract must be paused
    function unpause() external onlyLRTAdmin {
        _unpause();
    }
}

[G-04] Use assembly to implement for-loops.

That's much cheaper to implement for-loops using assembly even though the readability suffers a bit.

 for (uint256 i = 0; i < strategiesLength;) {
            assets[i] = address(IStrategy(strategies[i]).underlyingToken());
            assetBalances[i] = IStrategy(strategies[i]).userUnderlyingView(address(this));
            unchecked {
                ++i;
            }

[G-05] Use assembly to check for address(0).

https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L27

 UtilLib.checkNonZeroAddress(lrtConfigAddr);