Kelp DAO | rsETH - lsaudit's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L121-L124 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109-L122

Vulnerability details

Impact

Function getAssetBalance() is responsible for fetching the balance of an asset that was deposited into strategy.

The issue with this function is that it will return incorrect result, when the strategy is changed.

Proof of Concept

Function updateAssetStrategy() updates only assetStrategy mapping. Whenever we update the strategy, there's no check if asset is already deposited to the previous strategy.

File: LRTConfig.sol

function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)
    {
        UtilLib.checkNonZeroAddress(strategy);
        if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;
    }

Let's consider an asset X which is using strategy STRATEGY_1. We call depositAssetIntoStrategy() to deposit that asset into that strategy. Now, let's consider a scenario when updateAssetStrategy() is being called and asset X points to STRATEGY_2 now. We call depositAssetInfoStrategy() to deposit that asset into another strategy this time.

This constitutes the problem, when we call getAssetBalance():

File: NodeDelegator.sol

function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

As demonstrates above, function getAssetBalance() returns userUnderlyingView() only for STRATEGY_2 (updated one), even though we deposited asset X into two strategies.

Function getAssetBalance() does not take into consideration old strategies for the asset. Whenever we deposit asset into strategy, then change that strategy, function getAssetBalance() won't count deposit related to the previous strategy.

Tools Used

Manual code review

Recommended Mitigation Steps

Whenever the strategy for the asset is being updated, the previous deposited amount for that strategy should be stored and taken into account when getAssetBalance() returns the balance for that asset.

Assessed type

Other

[QA-01] updatePriceFeedFor() emits an event even when the priceFeed has not been changed

File: ChainlinkPriceOracle.sol

    function updatePriceFeedFor(address asset, address priceFeed) external onlyLRTManager onlySupportedAsset(asset) {
        UtilLib.checkNonZeroAddress(priceFeed);
        assetPriceFeed[asset] = priceFeed;
        emit AssetPriceFeedUpdate(asset, priceFeed);
    }

Function updatePriceFeedFor() does not verify if provided priceFeed is not already set in assetPriceFeed[asset]. If onlyLTRManager provides priceFeed which was set before (assetPriceFeed[asset] == priceFeed), function will still emit an event. Emitting this event may be very misleading to the end user - thus he/she may think that the price was updated, even though, it wasn't.

The same issue occurs in LRTOracle.sol:

File: LRTOracle.sol

 function updatePriceOracleFor(
        address asset,
        address priceOracle
    )
        external
        onlyLRTManager
        onlySupportedAsset(asset)
    {
        UtilLib.checkNonZeroAddress(priceOracle);
        assetPriceOracle[asset] = priceOracle;
        emit AssetPriceOracleUpdate(asset, priceOracle);
    }

It's a good practice to perform additional check which verifies if the value is really being updated:

if (assetPriceFeed[asset] == priceFeed) revert ValueAlreadyInUse();

You can verify how it's done in LRConfig.sol. For example here:

File: LRTConfig.sol

 if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
    }

[QA-02] Override renounceRole() in RSETH.sol

RSETH inherits from AccessControlUpgradeable, which implies, that it implements renounceRole() function. renounceRole() allows accounts to renounce granted roles belonging to them.

The DEFAULT_ADMIN_ROLE role is granted to admin address when function initialize() is called. This role is needed for proper contract functioning. E.g. role DEFAULT_ADMIN_ROLE allows to grant others with BURNER_ROLE and MINTER_ROLE - roles needed to mint and burn rsETH. Moreover - DEFAULT_ADMIN_ROLE is the only role which allows to unpause a contract (which can be paused by onlyLRTManager).

This implies, that whenever admin mistakenly calls renounceRole(), protocol will be left with no DEFAULT_ADMIN_ROLE and may not be functioning properly.

It's a good security practice to override renounceRole(), so it won't be called by mistake.

[QA-03] Emit AssetDepositIntoStrategy() event after performing depositIntoStrategy()

File: NodeDelegator.sol

        emit AssetDepositIntoStrategy(asset, strategy, balance);

        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);

The whole code-base follows the coding-pattern: action -> event. In every implemented function, we have noticed, that the action is being performed first, then event is being emitted. The only exception for that rule was noticed in depositAssetIntoStrategy() - when event AssetDepositIntoStrategy() is emitted before calling depositIntoStrategy().

Sometimes moving event up allows to save gas (we don't need to declare additional variables used in events) - however, this is not the case in the following example.

Our recommendation is to follow previously defined pattern and emit AssetDepositIntoStrategy() after performing depositIntoStrategy().

[QA-04] Improve code readability by defining a constant for default max node delagator count in LRTDepositPool.sol

File: LRTDepositPool.sol

maxNodeDelegatorCount = 10;

Function initialize() sets maxNodeDelegatorCount to 10, which should be considered as default number of max node delegator count (which might be updated later by calling updateMaxNodeDelegatorCount()).

However, instead of using numbers, the code readability would be much clearer, when this value would be defined as constant:

uint256 private constant DEFAULT_MAX_NODE_DELEGATOR_COUNT = 10;

function initialize(address lrtConfigAddr) external initializer {
        UtilLib.checkNonZeroAddress(lrtConfigAddr);
        __Pausable_init();
        __ReentrancyGuard_init();
        maxNodeDelegatorCount = DEFAULT_MAX_NODE_DELEGATOR_COUNT;
        lrtConfig = ILRTConfig(lrtConfigAddr);
        emit UpdatedLRTConfig(lrtConfigAddr);
    }

[QA-05] Use more descriptive variable naming in LRTDepositPool.sol

File: LRTDepositPool.sol

        uint256 length = nodeDelegatorContracts.length;
        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
            revert MaximumNodeDelegatorCountReached();
        }

        for (uint256 i; i < length;) {

When calling addNodeDelegatorContractToQueue(), we provide nodeDelegatorContracts - an array of NodeDelegator contract addresses. Function verifies if current number of NodeDeleagor contract addresses (nodeDelegatorContracts) and provided NodeDelegator contract addresses in parameter nodeDelegatorContracts, does not exceed maxNodeDelegatorCount.

It assigns nodeDelegatorContracts.length to variable length. However, since the code uses two different arrays: (nodeDelegatorContracts and nodeDelegatorContracts), this variable naming may be confusing in later part of the codebase. It will be much better to change the name of length to straightforwardly state that it's the length of nodeDelegatorContracts:

uint256 nodeDelegatorContractsLength = nodeDelegatorContracts.length;

[QA-06] updateMaxNodeDelegatorCount() in LRTDepositPool.sol emits an event, even when maxNodeDelegatorCount has not been changed.

File: LRTDepositPool.sol

    function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
        maxNodeDelegatorCount = maxNodeDelegatorCount_;
        emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
    }

Function updateMaxNodeDelegatorCount() does not verify if provided maxNodeDelegatorCount_ is different than the one previously set. If onlyLRTAdmin provides maxNodeDelegatorCount_ with the value which was set before (maxNodeDelegatorCount_ == maxNodeDelegatorCount), function will still emit an event. Emitting this event may be very misleading to the end user - thus he/she may think that the maxNodeDelegatorCount was updated, even though, it wasn't.

It's a good practice to perform additional check which verifies if the value is really being updated:

if (maxNodeDelegatorCount == maxNodeDelegatorCount_) revert ValueAlreadyInUse();

You can verify how it's done in LRConfig.sol. For example here:

File: LRTConfig.sol

 if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
    }

[QA-07] Whenever updating some state, event should emit both old and new values

Functions which update some values should emit an event which contains not only the new value, but also the old one.

It's a good practice to emit event which informs the end-user not only about the new value, but also the previous (updated) one.

E.g.:

File: LRTDepositPool.sol

    function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
        maxNodeDelegatorCount = maxNodeDelegatorCount_;
        emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
    }

should be changed to:

    function updateMaxNodeDelegatorCount(uint256 maxNodeDelegatorCount_) external onlyLRTAdmin {
        uint256 oldMaxNodeDelegatorCount = maxNodeDelegatorCount;
        maxNodeDelegatorCount = maxNodeDelegatorCount_;    
        emit MaxNodeDelegatorCountUpdated(oldMaxNodeDelegatorCount, maxNodeDelegatorCount);
    }

This issue occurs in multiple of instances:

LRTDepositPool.sol#L204  ->  emit MaxNodeDelegatorCountUpdated(maxNodeDelegatorCount);
LRTOracle.sol#L98 -> emit AssetPriceOracleUpdate(asset, priceOracle);
RSETH.sol#L76 -> emit UpdatedLRTConfig(_lrtConfig);
ChainlinkPriceOracle.sol#L48 -> emit AssetPriceFeedUpdate(asset, priceFeed);
LRTConfigRoleChecker.sol#L50 -> emit UpdatedLRTConfig(lrtConfigAddr);

[N-01] Typos/comments-coding-style consistency

• LRTConstants.sol

18:     //Roles

Every comment describing set of variables in LRTConstants.sol is lowercased, e.g.: //contracts, //tokens. Keep your comment-coding-style consistent by changing //Roles to //roles

• LRTConstants.sol

5:    //tokens
6:    //rETH token
8:    //stETH token
10:   //cbETH token
13:   //contracts
18:   //Roles

Write comment after white-space character: // Comment, instead of //Comment.

• LRTConfigRoleChecker.sol

46:     /// @param lrtConfigAddr the new LRT config contract Address

Typo: Address should be lowercased - address

• ChainlinkPriceOracle.sol

16: /// @notice contract that fetches the exchange rate of assets from chainlink price feeds

Typo: chainlink should be changed to Chainlink

• RSETH.sol

59:    /// @dev Only callable by LRT config manager. Contract must NOT be paused.

None of the previous comment contains the dot at the end of line. Keep your comment-coding-style consistent, by removing dot at the end.

• LRTOracle.sol.sol

50:     /// @dev calculates based on stakedAsset value received from eigen layer

Typo: eigen layer should be changed to EigenLayer.

[N-02] Typo in event name in LRTDepositPool.sol

File: LRTDepositPool.sol

emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);

According to Solidity naming conventions, NodeDelegatorAddedinQueue() should be renamed to NodeDelegatorAddedInQueue(). Moreover, more grammaticaly is: NodeDelegatorAddedToQueue()

[G-01] Redundant variable in LRTConfigRoleChecker.sol

File: LRTConfigRoleChecker.sol

    modifier onlyLRTAdmin() {
        bytes32 DEFAULT_ADMIN_ROLE = 0x00;
        if (!IAccessControl(address(lrtConfig)).hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {
            revert ILRTConfig.CallerNotLRTConfigAdmin();
        }
        _;
    }

Variable DEFAULT_ADMIN_ROLE is used only once, thus it does not need to be declared at all:

    modifier onlyLRTAdmin() {
        if (!IAccessControl(address(lrtConfig)).hasRole(0x00, msg.sender)) {
            revert ILRTConfig.CallerNotLRTConfigAdmin();
        }
        _;
    }

[G-02] Redundant variables is LRTOracle.sol

File: LRTOracle.sol

 function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

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

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);

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

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

Variables rsETHTokenAddress, assetER, totalAssetAmt are used only once, thus they don't need to be declared at all:

 function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        
        uint256 rsEthSupply = IRSETH(lrtConfig.rsETH()).totalSupply();

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

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        uint256 supportedAssetCount = supportedAssets.length;

        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            totalETHInPool += ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset) * getAssetPrice(asset);

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

[G-03] Do not perform depositIntoStrategy() when deposited amount is 0

File: NodeDelegator.sol

 uint256 balance = token.balanceOf(address(this));

        emit AssetDepositIntoStrategy(asset, strategy, balance);

        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);

IEigenStrategyManager(eigenlayerStrategyManagerAddress.depositIntoStrategy() deposits balance of token into the specified strategy.

However, whenever balance is 0, performing that function basically does nothing.

Checking non-zero transfer values can avoid an expensive external call and save gas.

Moreover, we can change the order of operations in function, to perform balance-check quicker.

Function can be rewritten from:

        address strategy = lrtConfig.assetStrategy(asset);
        IERC20 token = IERC20(asset);
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

        uint256 balance = token.balanceOf(address(this));

        emit AssetDepositIntoStrategy(asset, strategy, balance);

        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);

to:

        IERC20 token = IERC20(asset);
        uint256 balance = token.balanceOf(address(this));

        if (balance == 0) revert DepositingZeroAmount();

        address strategy = lrtConfig.assetStrategy(asset);
        
        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

        emit AssetDepositIntoStrategy(asset, strategy, balance);

        IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);

That way, when balance == 0, we don't spend gas not only on useless depositIntoStrategy(), but we also don't spend gas on assetStrategy() and lrtConfig.getContract() calls.

[G-04] Redundant variables in NodeDelegator.sol

• Function transferBackToLRTDepositPool():

Variable lrtDepositPool is used only once, which means it doesn't need to be declared at all:

File: NodeDelegatotor.sol

        address lrtDepositPool = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        if (!IERC20(asset).transfer(lrtDepositPool, amount)) {
            revert TokenTransferFailed();
        }

can be changed to:

        if (!IERC20(asset).transfer(lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL), amount)) {
            revert TokenTransferFailed();
        }

• Function getAssetBalances():

Variable eigenlayerStrategyManagerAddress is used only once, which means it doesn't need to be declared at all.

File: NodeDelegatotor.sol

        address eigenlayerStrategyManagerAddress = lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER);

        (IStrategy[] memory strategies,) =
            IEigenStrategyManager(eigenlayerStrategyManagerAddress).getDeposits(address(this));

can be changed to:

 (IStrategy[] memory strategies,) =
            IEigenStrategyManager(lrtConfig.getContract(LRTConstants.EIGEN_STRATEGY_MANAGER)).getDeposits(address(this));

• Function getAssetBalance():

Variable strategy is used only once, which means it doesn't need to be declared at all:

File: NodeDelegatotor.sol

        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));

can be changed to:

        return IStrategy(lrtConfig.assetStrategy(asset)).userUnderlyingView(address(this));

[G-05] In transferBackToLRTDepositPool() amount should be checked for 0 before calling transfer()

Checking non-zero transfer values can avoid an expensive external call and save gas.

File: NodeDelegatotor.sol

        address lrtDepositPool = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        if (!IERC20(asset).transfer(lrtDepositPool, amount)) {
            revert TokenTransferFailed();
        }

[G-06] Use do-while loops instead of for-loops

Solidity do-while loops are more gas efficient than for loops.

File: NodeDelegator.sol

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

File: LRTDepositPool.sol

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

Moreover, since above loops do not perform any advanced operations (assigning data to an array and calculating the sum of array)) - we can easily re-write them, so they will count down.

Counting down is more gas efficient than counting up because neither we are making zero variable to non-zero variable and also we will get gas refund in the last transaction when making non-zero to zero variable - reference

[G-07] Redundant variables in LRTDepositPool.sol

• Function getRsETHAmountToMint():

Variable lrtOracleAddress is used only once, which means it doesn't need to be declared at all.

File: LRTDepositPool.sol

        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

can be changed to:

        ILRTOracle lrtOracle = ILRTOracle(lrtConfig.getContract(LRTConstants.LRT_ORACLE));

• Function _mintRsETH():

Variable rsethToken is used only once, which means it doesn't need to be declared at all.

File: LRTDepositPool.sol

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

can be changed to:

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

• Function transferAssetToNodeDelegator():

Variable nodeDelegator is used only once, which means it doesn't need to be declared at all.

File: LRTDepositPool.sol

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

can be changed to:

if (!IERC20(asset).transfer(nodeDelegatorQueue[ndcIndex], amount)) {

[G-08] initialize() in LRTConfig.sol does not need to call UtilLib.checkNonZeroAddress on stEth, rETH, cbETH:

File: LRTConfig.sol

        UtilLib.checkNonZeroAddress(stETH);
        UtilLib.checkNonZeroAddress(rETH);
        UtilLib.checkNonZeroAddress(cbETH);

Above calls are redundant. initialize() calls _setToken() on stEth, rETH, cbETH:

File: LRTConfig.sol

        _setToken(LRTConstants.ST_ETH_TOKEN, stETH);
        _setToken(LRTConstants.R_ETH_TOKEN, rETH);
        _setToken(LRTConstants.CB_ETH_TOKEN, cbETH);

As demonstrated above, initialize() firstly checks for address(0) by performing UtilLib.checkNonZeroAddress() on stEth, rETH, cbETH, and then, calls _setToken() on that addresses.

However, function _setToken() already implements a check for address(0)

File: LRTConfig.sol

    function _setToken(bytes32 key, address val) private {
        UtilLib.checkNonZeroAddress(val);

thus doing it one more time in initialize() is redundant.

        UtilLib.checkNonZeroAddress(stETH);
        UtilLib.checkNonZeroAddress(rETH);
        UtilLib.checkNonZeroAddress(cbETH);

can be removed from initialize().

[G-09] Use constants instead of type(uintX).max

When you use type(uintX).max - it may result in higher gas costs because it involves a runtime operation to calculate the type(uintX).max at runtime. This calculation is performed every time the expression is evaluated.

To save gas, it is recommended to use constants to represent the maximum value. Declaration of a constant with the maximum value means that the value is known at compile-time and does not require any runtime calculations.

File: NodeDelegator.sol

 IERC20(asset).approve(eigenlayerStrategyManagerAddress, type(uint256).max);