Kelp DAO | rsETH - hunter_w3b's results

[L-01] Deposit limits can be updated by the manager role, but there is no check to prevent reducing limits. This could lock user assets

The addNewSupportedAsset function allows the manager role to update the deposit limit amount for any supported asset. However, there is no check to prevent reducing this limit to a value below what users have already deposited.

File: src/LRTConfig.sol

73    function addNewSupportedAsset(address asset, uint256 depositLimit) external onlyRole(LRTConstants.MANAGER) {
74        _addNewSupportedAsset(asset, depositLimit);
75    }
76
77    /// @dev private function to add a new supported asset
78    /// @param asset Asset address
79    /// @param depositLimit Deposit limit for the asset
80    function _addNewSupportedAsset(address asset, uint256 depositLimit) private {
81        UtilLib.checkNonZeroAddress(asset);
82        if (isSupportedAsset[asset]) {
83            revert AssetAlreadySupported();
84        }
85        isSupportedAsset[asset] = true;
86        supportedAssetList.push(asset);
87        depositLimitByAsset[asset] = depositLimit;
88        emit AddedNewSupportedAsset(asset, depositLimit);
89    }

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L73-L89

This could potentially lock funds if a limit is reduced after a user has deposited more than the new lower limit. An attacker with manager role privileges could maliciously lock funds in this way. The deposit limit should always be increased or maintained, never decreased, to avoid locking user funds.

[L-02] Strategies can be updated by any admin, but there is no logic to handle existing funds. Strategy changes could lose user asset

The updateAssetStrategy function allows any admin to change the strategy address for how an asset is managed. However, there is no logic defined in the contract for how existing deposited funds should be handled during a strategy change. Without proper handling, funds could be lost or inaccessible if sent to the new strategy before it is ready. An attacker with admin privileges could maliciously change the strategy and cause user funds to be lost or inaccessible. Before allowing strategy changes, the contract should include logic to withdraw existing funds from the old strategy and ensure they are securely managed during the transition.

File: src/LRTConfig.sol

109    function updateAssetStrategy(
110        address asset,
111        address strategy
112    )
113        external
114        onlyRole(DEFAULT_ADMIN_ROLE)
115        onlySupportedAsset(asset)
116    {
117        UtilLib.checkNonZeroAddress(strategy);
118       if (assetStrategy[asset] == strategy) {
119            revert ValueAlreadyInUse();
120        }
121        assetStrategy[asset] = strategy;
122    }

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

[L-03] Strategies can be set per asset, but contract may expect global strategy behavior mismatch

The architecture of setting strategies per individual asset through the assetStrategy mapping may cause a misalignment with how the contract is intended to handle strategies.

File: src/LRTConfig.sol

17    mapping(address token => address strategy) public override assetStrategy;

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

If the code expects there to only be a single global strategy for all assets, the per-asset setting could lead to unexpected behavior.

For example, asset handling logic may send all assets to a single strategy address instead of the one configured for that specific asset. This mismatched design between the strategy setting and underlying logic could result in assets being sent to incorrect strategies and assets being mishandled or lost as a result.

[L-04] Total balance calculation iterates queue but doesn't consider new deposits in loop

The getTotalAssetDeposits function calculates the total asset balance across the deposit pool and node delegate contracts. It does this by iterating the node delegate queue and summing the balances of each contract. However, this calculation does not properly consider the possibility of new deposits being made to the deposit pool contract during the iteration of the queue. Since external calls are asynchronous, it is possible for a deposit to be made after the deposit pool balance is read, but before balances of node delegates are checked. This would result in the total balance being reported incorrectly low as it would not account for the new deposit amount. The balance calculation needs to prevent re-entrancy or take a snapshot of relevant balances upfront to avoid this potential inaccuracy.

File: src/LRTDepositPool.sol

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    }



71    function getAssetDistributionData(address asset)
72        public
73        view
74        override
75        onlySupportedAsset(asset)
76        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
77    {
78        // Question: is here the right place to have this? Could it be in LRTConfig?
79        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
80
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        }
89    }

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

[L-05] Unused Asset Strategy Logic

The asset strategy concept is defined but not properly implemented or leveraged. By not passing the asset to strategy contracts, important logic like customized minting calculations cannot be executed. This undermines the extensibility of the protocol and may lead to unexpected behaviors.

The _mintRsETH() call does not pass the asset to allow custom strategy handling.

File: src/LRTDepositPool.sol

    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);
    }

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

[L-06] Insufficient Allowance Check

By not checking the allowance before calling transferFrom, token transfers could fail or revert if the allowance is not high enough. This could lead to asset loss and undermine the integrity of deposit/withdrawal functionality.

The depositAsset function does not check the allowance before calling transferFrom.

File: src/LRTDepositPool.sol

    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);
    }

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

• Check allowance before transferFrom using allowance(owner, contract)
• Revert if allowance < amount to transfer

[L-07] Strategy functions are called without validating success or expected return values

By not checking the return values of calls to strategy functions like depositIntoStrategy(), failures or unexpected return values could be ignored. This can lead to funds being deposited into invalid states without detections.

Function calls in depositAssetIntoStrategy() do not validate the call success or expected return value:

File: src/NodeDelegator.sol

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

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

[L08] Lack of Limits on Minting and Burning Allows Uncontrolled Token Inflation/Deflation

The mint and burn functions in the RSETH token contract have no limitations on the amounts of tokens that can be created or destroyed.

File: src/RSETH.sol

    function mint(address to, uint256 amount) external onlyRole(MINTER_ROLE) whenNotPaused {
        _mint(to, amount);
    }

        function burnFrom(address account, uint256 amount) external onlyRole(BURNER_ROLE) whenNotPaused {
        _burn(account, amount);
    }

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

An attacker who gains the minter or burner role could mint or burn arbitrary quantities of tokens, causing rampant inflation or deflation of the total supply. Without caps on mint/burn amounts, a bad actor could manipulate the supply however they choose without constraint. This could severely compromise the stability and value of the RSETH token. Hard or configurable limits should be imposed to prevent such exploitation and maintain control over the token's emission schedule and long term monetary policy.

Gas-Optmization for Kelp DAO | rsETH Protocol

[G-01] Avoid contract existence checks by using low level calls

Issue Description
Prior to Solidity 0.8.10, the compiler would insert extra code to check for contract existence before external function calls, even if the call had a return value. This wasted gas by performing a check that wasn't necessary.

Proposed Optimization
For functions that make external calls with return values in Solidity <0.8.10, optimize the code to use low-level calls instead of regular calls. Low-level calls skip the unnecessary contract existence check.

Example:

//Before:

contract C {
  function f() external returns(uint) {
    address(otherContract).call(abi.encodeWithSignature("func()"));
  }
}


//After:

contract C {
  function f() external returns(uint) {
    (bool success,) = address(otherContract).call(abi.encodeWithSignature("func()"));
    require(success);
    return decodeReturnValue();
  }
}

Estimated Gas Savings
Each avoided EXTCODESIZE check saves 100 gas. If 10 external calls are made in a common function, this would save 1000 gas total.

Attachments

• Code Snippets

File: src/oracles/ChainlinkPriceOracle.sol

38        return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L38

File:  src/utils/LRTConfigRoleChecker.sol

21        if (!IAccessControl(address(lrtConfig)).hasRole(LRTConstants.MANAGER, msg.sender)) {

29        if (!IAccessControl(address(lrtConfig)).hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {

36        if (!IAccessControl(address(lrtConfig)).hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) {

https://github.com/code-423n4/2023-11-kelp/blob/main/src/utils/LRTConfigRoleChecker.sol#L21

File: src/LRTDepositPool.sol

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

83            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);

84            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);

105        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);

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

136        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {

156        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);

194        if (!IERC20(asset).transfer(nodeDelegator, amount)) {

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

File: src/LRTOracle.sol

46        return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);

54        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

70            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);

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

File: src/NodeDelegator.sol

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

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

59        address strategy = lrtConfig.assetStrategy(asset);

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

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

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

103            IEigenStrategyManager(eigenlayerStrategyManagerAddress).getDeposits(address(this));

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

[G-02] Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

Issue Description
The code is declaring constant values such as role identifiers and contract names using 'constant' rather than 'immutable'.

Proposed Optimization
Update the declarations of constant values from 'constant' to 'immutable'. This avoids unnecessary writes to storage every time these values are accessed.

Estimated Gas Savings
Switching to 'immutable' for constant value declarations will save gas by avoiding storage writes on every access. The specific gas savings will depend on how many times each constant value is accessed but is expected to be small for each individual reference. accumlated across many transactions and many references, it could result in non-trivial total savings.

Attachments

• Code Snippets

File: src/RSETH.sol

21    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

22    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

https://github.com/code-423n4/2023-11-kelp/blob/main/src/RSETH.sol#L21-L22

File: src/utils/LRTConstants.sol

7    bytes32 public constant R_ETH_TOKEN = keccak256("R_ETH_TOKEN");

9    bytes32 public constant ST_ETH_TOKEN = keccak256("ST_ETH_TOKEN");

11    bytes32 public constant CB_ETH_TOKEN = keccak256("CB_ETH_TOKEN");

14    bytes32 public constant LRT_ORACLE = keccak256("LRT_ORACLE");

15    bytes32 public constant LRT_DEPOSIT_POOL = keccak256("LRT_DEPOSIT_POOL");

16    bytes32 public constant EIGEN_STRATEGY_MANAGER = keccak256("EIGEN_STRATEGY_MANAGER");

19    bytes32 public constant MANAGER = keccak256("MANAGER");

https://github.com/code-423n4/2023-11-kelp/blob/main/src/utils/LRTConstants.sol#L7

[G-03] Using Storage instead of memory for structs/arrays saves gas

Issue Description
When fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array.

If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read.

Instead of declaring the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incurring the Gcoldsload for the fields actually read

Proposed Optimization
Replace memory keywords with storage when fetching structs/arrays from storage that are not returned from the function or passed to a function requiring memory. Cache any re-read fields in stack variables.

Estimated Gas Savings
By replacing memory with storage, the number of SLOAD operations can be reduced from O(n) to O(1) where n is the number of fields in the struct/array. For large structs/arrays this can result in gas savings of thousands per transaction.

Attachments

• Code Snippets

File: src/LRTOracle.sol

63        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();

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

File: src/NodeDelegator.sol

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

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

[G-04] Use constants instead of type(uintx).max

Issue Description
Using type(uint256).max to represent the maximum approvable amount for ERC20 tokens uses more gas in the distribution process and also for each transaction than using a constant.

Proposed Optimization
Define a constant such as MAX_UINT256 = type(uint256).max and use that constant instead.

Estimated Gas Savings
Using a constant avoids the overhead of calling the type(uint256) method each time. This could save ~100 gas per transaction. For contracts with many transactions, this can add up to significant savings over time.

Attachments

• Code Snippets

File: src/NodeDelegator.sol

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

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

[G-05] Using mappings instead of arrays to avoid length checks

Issue Description
When storing a list or group of items that you wish to organize in a specific order and fetch with a fixed key/index, it’s common practice to use an array data structure. This works well, but did you know that a trick can be implemented to save 2,000+ gas on each read using a mapping?

Proposed Optimization\

See the example below

/// get(0) gas cost: 4860
contract Array {
    uint256[] a;

    constructor() {
        a.push() = 1;
        a.push() = 2;
        a.push() = 3;
    }

    function get(uint256 index) external view returns(uint256) {
        return a[index];
    }
}

/// get(0) gas cost: 2758
contract Mapping {
    mapping(uint256 => uint256) a;

    constructor() {
        a[0] = 1;
        a[1] = 2;
        a[2] = 3;
    }

    function get(uint256 index) external view returns(uint256) {
        return a[index];
    }
}

Just by using a mapping, we get a gas saving of 2102 gas. Why? Under the hood when you read the value of an index of an array, solidity adds bytecode that checks that you are reading from a valid index (i.e an index strictly less than the length of the array) else it reverts with a panic error (Panic(0x32) to be precise). This prevents from reading unallocated or worse, allocated storage/memory locations.

Due to the way mappings are (simply a key => value pair), no check like that exists and we are able to read from the a storage slot directly. It’s important to note that when using mappings in this manner, your code should ensure that you are not reading an out of bound index of your canonical array.

Attachments

• Code Snippets

File: src/LRTConfig.sol

19    address[] public supportedAssetList;

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

File: src/LRTDepositPool.sol

22    address[] public nodeDelegatorQueue;

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

[G-06] Understand the trade-offs when choosing between internal functions and modifiers

Issue Description
Modifiers inject its implementation bytecode where it is used while internal functions jump to the location in the runtime code where the its implementation is. This brings certain trade-offs to both options.

Proposed Optimization
Using modifiers more than once means repetitiveness and increase in size of the runtime code but reduces gas cost because of the absence of jumping to the internal function execution offset and jumping back to continue. This means that if runtime gas cost matter most to you, then modifiers should be your choice but if deployment gas cost and/or reducing the size of the creation code is most important to you then using internal functions will be best.

However, modifiers have the tradeoff that they can only be executed at the start or end of a functon. This means executing it at the middle of a function wouldn’t be directly possible, at least not without internal functions which kill the original purpose. This affects it’s flexibility. Internal functions however can be called at any point in a function.

Estimated Gas Savings
Example showing difference in gas cost using modifiers and an internal function

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

/** deployment gas cost: 195435
    gas per call:
              restrictedAction1: 28367
              restrictedAction2: 28377
              restrictedAction3: 28411
 */
 contract Modifier {
    address owner;
    uint256 val;

    constructor() {
        owner = msg.sender;
    }

    modifier onlyOwner() {
        require(msg.sender == owner);
        _;
    }

    function restrictedAction1() external onlyOwner {
        val = 1;
    }

    function restrictedAction2() external onlyOwner {
        val = 2;
    }

    function restrictedAction3() external onlyOwner {
        val = 3;
    }
}



/** deployment gas cost: 159309
    gas per call:
              restrictedAction1: 28391
              restrictedAction2: 28401
              restrictedAction3: 28435
 */
 contract InternalFunction {
    address owner;
    uint256 val;

    constructor() {
        owner = msg.sender;
    }

    function onlyOwner() internal view {
        require(msg.sender == owner);
    }

    function restrictedAction1() external {
        onlyOwner();
        val = 1;
    }

    function restrictedAction2() external {
        onlyOwner();
        val = 2;
    }

    function restrictedAction3() external {
        onlyOwner();
        val = 3;
    }
}

From the table above, we can see that the contract that uses modifiers cost more than 35k gas more than the contract using internal functions when deploying it due to repetition of the onlyOwner functionality in 3 functions.

During runtime, we can see that each function using modifiers cost a fixed 24 gas less than the functions using internal functions.

Attachments

• Code Snippets

File: src/LRTConfig.sol

28    modifier onlySupportedAsset(address asset) {

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

File: src/utils/LRTConfigRoleChecker.sol

20    modifier onlyLRTManager() {

27    modifier onlyLRTAdmin() {

35    modifier onlySupportedAsset(address asset) {

https://github.com/code-423n4/2023-11-kelp/blob/main/src/utils/LRTConfigRoleChecker.sol#L20

[G-07] Consider using alternatives to OpenZeppelin

OpenZeppelin is a great and popular smart contract library, but there are other alternatives that are worth considering. These alternatives offer better gas efficiency and have been tested and recommended by developers.

Two examples of such alternatives are Solmate and Solady.

Solmate is a library that provides a number of gas-efficient implementations of common smart contract patterns. Solady is another gas-efficient library that places a strong emphasis on using assembly.

[G-08] Using assembly to revert with an error message

Issue Description
When reverting in solidity code, it is common practice to use a require or revert statement to revert execution with an error message. This can in most cases be further optimized by using assembly to revert with the error message.

Estimated Gas Savings
Here’s an example;

/// calling restrictedAction(2) with a non-owner address: 24042
contract SolidityRevert {
    address owner;
    uint256 specialNumber = 1;

    constructor() {
        owner = msg.sender;
    }

    function restrictedAction(uint256 num)  external {
        require(owner == msg.sender, "caller is not owner");
        specialNumber = num;
    }
}

/// calling restrictedAction(2) with a non-owner address: 23734
contract AssemblyRevert {
    address owner;
    uint256 specialNumber = 1;

    constructor() {
        owner = msg.sender;
    }

    function restrictedAction(uint256 num)  external {
        assembly {
            if sub(caller(), sload(owner.slot)) {
                mstore(0x00, 0x20) // store offset to where length of revert message is stored
                mstore(0x20, 0x13) // store length (19)
                mstore(0x40, 0x63616c6c6572206973206e6f74206f776e657200000000000000000000000000) // store hex representation of message
                revert(0x00, 0x60) // revert with data
            }
        }
        specialNumber = num;
    }
}

From the example above we can see that we get a gas saving of over 300 gas when reverting wth the same error message with assembly against doing so in solidity. This gas savings come from the memory expansion costs and extra type checks the solidity compiler does under the hood.

Attachments

• Code Snippets

File: src/LRTConfig.sol

30            revert AssetNotSupported();

83            revert AssetAlreadySupported();

119            revert ValueAlreadyInUse();

159            revert ValueAlreadyInUse();

175            revert ValueAlreadyInUse();

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

File: src/LRTDepositPool.sol

130            revert InvalidAmount();

133            revert MaximumDepositLimitReached();

137            revert TokenTransferFailed();

165            revert MaximumNodeDelegatorCountReached();

195            revert TokenTransferFailed();

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

File: src/utils/LRTConfigRoleChecker.sol

22            revert ILRTConfig.CallerNotLRTConfigManager();

30            revert ILRTConfig.CallerNotLRTConfigAdmin();

37            revert ILRTConfig.AssetNotSupported();

https://github.com/code-423n4/2023-11-kelp/blob/main/src/utils/LRTConfigRoleChecker.sol#L22

[G-09] Use assembly to reuse memory space when making more than one external call

Issue Description
When making external calls, the solidity compiler has to encode the function signature and arguments in memory. It does not clear or reuse memory, so it expands memory each time.

Proposed Optimization
Use inline assembly to reuse the same memory space for multiple external calls. Store the function selector and arguments without expanding memory further.

Estimated Gas Savings
Reusing memory can save thousands of gas compared to expanding on each call. The baseline memory expansion per call is 3,000 gas. With larger arguments or return data, the savings would be even greater.

See the example below;

contract Called {
    function add(uint256 a, uint256 b) external pure returns(uint256) {
        return a + b;
    }
}


contract Solidity {
    // cost: 7262
    function call(address calledAddress) external pure returns(uint256) {
        Called called = Called(calledAddress);
        uint256 res1 = called.add(1, 2);
        uint256 res2 = called.add(3, 4);

        uint256 res = res1 + res2;
        return res;
    }
}


contract Assembly {
    // cost: 5281
    function call(address calledAddress) external view returns(uint256) {
        assembly {
            // check that calledAddress has code deployed to it
            if iszero(extcodesize(calledAddress)) {
                revert(0x00, 0x00)
            }

            // first call
            mstore(0x00, hex"771602f7")
            mstore(0x04, 0x01)
            mstore(0x24, 0x02)
            let success := staticcall(gas(), calledAddress, 0x00, 0x44, 0x60, 0x20)
            if iszero(success) {
                revert(0x00, 0x00)
            }
            let res1 := mload(0x60)

            // second call
            mstore(0x04, 0x03)
            mstore(0x24, 0x4)
            success := staticcall(gas(), calledAddress, 0x00, 0x44, 0x60, 0x20)
            if iszero(success) {
                revert(0x00, 0x00)
            }
            let res2 := mload(0x60)

            // add results
            let res := add(res1, res2)

            // return data
            mstore(0x60, res)
            return(0x60, 0x20)
        }
    }
}

We save approximately 2,000 gas by using the scratch space to store the function selector and it’s arguments and also reusing the same memory space for the second call while storing the returned data in the zero slot thus not expanding memory.

If the arguments of the external function you wish to call is above 64 bytes and if you are making one external call, it wouldn’t save any significant gas writing it in assembly. However, if making more than one call. You can still save gas by reusing the same memory slot for the 2 calls using inline assembly.

Note: Always remember to update the free memory pointer if the offset it points to is already used, to avoid solidity overriding the data stored there or using the value stored there in an unexpected way.

Also note to avoid overwriting the zero slot (0x60 memory offset) if you have undefined dynamic memory values within that call stack. An alternative is to explicitly define dynamic memory values or if used, to set the slot back to 0x00 before exiting the assembly block.

Attachments

• Code Snippets

File: src/LRTDepositPool.sol

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

83            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
84            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);

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

File: src/LRTOracle.sol

53        address rsETHTokenAddress = lrtConfig.rsETH();
61        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
63        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();

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

File: src/NodeDelegator.sol

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

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

This report discusses how using inline assembly can optimize gas costs when making multiple external calls by reusing memory space, rather than expanding memory separately for each call. This can save thousands of gas compared to the solidity compiler's default behavior.

[G-10] Do-While loops are cheaper than for loops

Issue Description
If you want to push optimization at the expense of creating slightly unconventional code, Solidity do-while loops are more gas efficient than for loops, even if you add an if-condition check for the case where the loop doesn’t execute at all.

Proposed Optimization
Replace for loops with do-while loops when possible to reduce gas costs. Add an if check before the do-while to handle cases where the loop body may not need to execute.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.20;

// times == 10 in both tests
contract Loop1 {
    function loop(uint256 times) public pure {
        for (uint256 i; i < times;) {
            unchecked {
                ++i;
            }
        }
    }
}

contract Loop2 {
    function loop(uint256 times) public pure {
        if (times == 0) {
            return;
        }

        uint256 i;

        do {
            unchecked {
                ++i;
            }
        } while (i < times);
    }
}

Estimated Gas Savings
Based on benchmarks, do-while loops can save 15-30 gas per iteration compared to for loops. With loops executing hundreds or thousands of iterations, the savings can add up significantly.

Attachments

• Code Snippets

File: src/LRTDepositPool.sol

82        for (uint256 i; i < ndcsCount;) {

168        for (uint256 i; i < length;) {

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

File: src/NodeDelegator.sol

109        for (uint256 i = 0; i < strategiesLength;) {

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

File: src/LRTOracle.sol

66        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {

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

[G-11] Don't make variables public unless necessary

Issue Description
Making variables public comes with some overhead costs that can be avoided in many cases. A public variable implicitly creates a public getter function of the same name, increasing the contract size.

Proposed Optimization
Only mark variables as public if their values truly need to be readable by external contracts/users. Otherwise, use private or internal visibility.

Estimated Gas Savings
The savings from avoiding unnecessary public variables are small per transaction, around 5-10 gas. However, this adds up over many transactions targeting a contract with public state variables that don't need to be public.

Attachments

• Code Snippets

File: src/LRTConfig.sol

19    address[] public supportedAssetList;

21    address public rsETH;

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

File: src/LRTDepositPool.sol

20    uint256 public maxNodeDelegatorCount;

22    address[] public nodeDelegatorQueue;

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

File: src/RSETH.sol

21    bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE");

22    bytes32 public constant BURNER_ROLE = keccak256("BURNER_ROLE");

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

File: src/utils/LRTConfigRoleChecker.sol

14    ILRTConfig public lrtConfig;

https://github.com/code-423n4/2023-11-kelp/blob/main/src/utils/LRTConfigRoleChecker.sol#L14

File: src/utils/LRTConstants.sol

7    bytes32 public constant R_ETH_TOKEN = keccak256("R_ETH_TOKEN");

9    bytes32 public constant ST_ETH_TOKEN = keccak256("ST_ETH_TOKEN");

11    bytes32 public constant CB_ETH_TOKEN = keccak256("CB_ETH_TOKEN");

14    bytes32 public constant LRT_ORACLE = keccak256("LRT_ORACLE");

15    bytes32 public constant LRT_DEPOSIT_POOL = keccak256("LRT_DEPOSIT_POOL");

16    bytes32 public constant EIGEN_STRATEGY_MANAGER = keccak256("EIGEN_STRATEGY_MANAGER");

19    bytes32 public constant MANAGER = keccak256("MANAGER");

https://github.com/code-423n4/2023-11-kelp/blob/main/src/utils/LRTConstants.sol#L7

[G-12] It is sometimes cheaper to cache calldata

Issue Description
While the calldataload opcode is relatively cheap, directly using it in a loop or multiple times can still result in unnecessary bytecode. Caching the loaded calldata first may allow for optimization opportunities.

Proposed Optimization
Cache calldata values in a local variable after first load, then reference the local variable instead of repeatedly using calldataload.

Estimated Gas Savings
Exact savings vary depending on contract, but caching calldata parameters can save 5-20 gas per usage by avoiding extra calldataload opcodes. Larger functions with many parameter uses see more benefit.

Attachments

• Code Snippets

File: src/LRTDepositPool.sol

162    function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {

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

[G-13] Use hardcoded addresses instead of address(this)

Issue Description
The address(this) construct pushes the contract's address onto the stack at runtime. This has a small gas cost compared to hardcoding the address directly.

Proposed Optimization
For contract methods/functions that are internal and will never be called from outside the contract, hardcode the contract address instead of using address(this).

Estimated Gas Savings
Using a hardcoded address saves ~2 gas per use compared to address(this). For functions that reference the contract address multiple times, the savings can add up.

Attachments

• Code Snippets

File: src/LRTDepositPool.sol

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

136        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {

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

File: src/NodeDelegator.sol

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

103            IEigenStrategyManager(eigenlayerStrategyManagerAddress).getDeposits(address(this));

111            assetBalances[i] = IStrategy(strategies[i]).userUnderlyingView(address(this));

123        return IStrategy(strategy).userUnderlyingView(address(this));

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

[G-14] Shorten arrays with inline assembly

Issue Description
When shortening an array in Solidity, it creates a new shorter array and copies the elements over. This wastes gas by duplicating storage.

Proposed Optimization
Use inline assembly to shorten the array in place by changing its length slot, avoiding the need to copy elements to a new array.

Estimated Gas Savings
Shortening a length-n array avoids ~n SSTORE operations to copy elements. Benchmarking shows savings of 5000-15000 gas depending on original length.

function shorten(uint[] storage array, uint newLen) internal {

  assembly {
    sstore(array_slot, newLen)
  }

}

// Rather than:
function shorten(uint[] storage array, uint newLen) internal {

  uint[] memory newArray = new uint[](newLen);

  for(uint i = 0; i < newLen; i++) {
    newArray[i] = array[i];
  }

  delete array;
  array = newArray;

}

Using inline assembly allows shortening arrays without copying elements to a new storage slot, providing significant gas savings.

Attachments

• Code Snippets

File: src/NodeDelegator.sol

106        assets = new address[](strategiesLength);

107        assetBalances = new uint256[](strategiesLength);

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

[G-15] Mark initializer functions as payable to save deployment gas

Attachments

• Code Snippets

File: src/LRTConfig.sol

41    function initialize(

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L41-L68

File: src/LRTDepositPool.sol

31    function initialize(address lrtConfigAddr) external initializer {

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

File: src/NodeDelegator.sol

26    function initialize(address lrtConfigAddr) external initializer {

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

File: src/LRTOracle.sol

29    function initialize(address lrtConfigAddr) external initializer {

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

File: src/RSETH.sol

32    function initialize(address admin, address lrtConfigAddr) external initializer {

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

File: src/oracles/ChainlinkPriceOracle.sol

27    function initialize(address lrtConfig_) external initializer {

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

[G-16] Access mappings directly instead of through accessor functions

Issue Description
Using accessor functions to access mappings incurs some additional gas costs due to an extra layer of function call indirection.

Proposed Optimization
For internal mappings where gas optimization is important, access the mapping values directly rather than going through getter functions.

Estimated Gas Savings
Benchmarking shows direct mapping access can save around 5 gas per lookup compared to an accessor function. With many lookups this savings adds up significantly.

Attachments

• Code Snippets

File: src/LRTConfig.sol

128        return tokenMap[tokenKey];

132        return contractMap[contractKey];

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

[G-17] Cache external calls outside of loops

Issue Description
Making external function calls like STATICCALL inside a loop incurs a gas cost per iteration that can be optimized.

Proposed Optimization
Cache the results of static external calls before any loops, rather than re-calling the function on each iteration.

Estimated Gas Savings
Avoids 100 gas cost per STATICCALL by caching results. With loops of 100+ iterations, savings can exceed 10,000 gas.

Attachments

• Code Snippets

File: src/LRTDepositPool.sol

82        for (uint256 i; i < ndcsCount;) {
83:            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
84:            assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
85            unchecked {
86                ++i;
88            }

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

File: src/LRTOracle.sol

66        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
67            address asset = supportedAssets[asset_idx];
68            uint256 assetER = getAssetPrice(asset);
69
70:            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
71            totalETHInPool += totalAssetAmt * assetER;

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

File: src/NodeDelegator.sol

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

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

Analysis - Kelp DAO | rsETH Contest

Description overview of Kelp DAO Contest

Kelp DAO aims to unlock liquidity, DeFi opportunities, and higher rewards for restaked assets. As a DAO, Kelp operates in a decentralized manner driven by its community. The core focus of Kelp is the development of rsETH (Liquid Restaked ETH), its flagship project. RsETH provides liquid staking for ETH deposited in the EigenLayer proof-of-stake platform. It acts as a liquid token that represents the staked ETH. This innovative approach allows holders to retain ongoing staking yields while gaining liquidity and usability of their assets in decentralized finance applications.

The key contracts of Kelp DAO | rsETH protocol for this Audit are:

• LRTDepositPool.sol: It is the main contract that users interact with to deposit assets into the Kelp DAO protocol, allowing users to easily deposit assets and receive rsETH tokens, while properly distributing those assets across contracts, the LRTDepositPool contract forms the core user interface and asset management layer for the protocol.

• LRTConfig.sol: The LRTConfig contract centralizes all the important configuration details of the Kelp DAO protocol.

  It stores information like:

  • Supported assets and their deposit limits
  • Contract addresses for key components
  • Token and strategy mappings

• NodeDelegator.sol: This contract handles the actual deposit of assets into the strategies on EigenLayer.By acting as an intermediary that directly interacts with the EigenLayer strategies, the NodeDelegator contract enables assets to flow from the deposit pool into yield/liquidity generating strategies. This is a core part of realizing the liquid staking functionality that Kelp DAO aims to provide through integration with EigenLayer.

• LRTOracle.sol: LRTOracle is responsible for fetching and calculating crucial exchange rates between different assets in the protocol.

  Specifically:

  • It provides the asset/ETH price by querying configured price feed contracts for each supported asset.
  • It calculates the RSETH/ETH price based on total underlying asset value deposited across the protocol and current RSETH supply.

• RSETH.sol: This contract represents the receipt token that users receive in exchange for assets they deposit into the protocol. By acting as the receipt token for the protocol, RSETH enables users to easily participate, while still retaining benefits of the underlying staked assets like yield/liquidity.

System Overview

Scope

• LRTConfig.sol: This contract serves as a configuration manager. It implements role-based access control, manages supported assets, their deposit limits, and associated strategies. The contract allows the addition of new supported assets, updates to deposit limits and strategies, and provides getter and setter functions for accessing and modifying various protocol parameters.

• LRTDepositPool.sol: LRTDepositPool contract manages the depositing of LST assets. The contract tracks the distribution of assets among the deposit pool, node delegator contracts (NDCs), and an eigen layer. Users can stake LST assets, and the contract calculates the corresponding amount of rsETH to mint based on an oracle's asset prices. It also allows the addition of node delegator contracts, asset transfers to node delegators, and updates to the maximum node delegator count.

• NodeDelegator.sol: The NodeDelegator facilitates the depositing of assets into strategies. It interacts with the LRT configuration and implements pausing and reentrancy protection. The contract allows the LRT manager to approve the maximum amount of an asset to the eigen strategy manager, deposit assets into strategies, and transfer assets back to the LRT deposit pool. Additionally, it provides functions to fetch asset balances and individual asset balances deposited into strategies through the node delegator.

• LRTOracle.sol: This contract serving as an oracle . It calculates exchange rates for assets against ETH and RSETH. The contract utilizes external price oracles through the IPriceFetcher interface and fetches asset prices, allowing users to query exchange rates.

• RSETH.sol: The RSETH contract is an ERC-20 token, featuring pausing functionality and role-based access controls for minting and burning. Can be associated with a specific LRT configuration contract. The contract allows for the management of roles, pausing operations, and updates to the LRT configuration.

• ChainlinkPriceOracle.sol: The contract is for fetching asset-to-ETH exchange rates from Chainlink price feeds, allowing dynamic updates of price oracles.

• LRTConfigRoleChecker.sol: The LRTConfigRoleChecker is an abstract contract facilitating role-based access control for configuration.

Privileged Roles

Some privileged roles exercise powers over the Controller contract:

• LRT-Manager

    modifier onlyLRTManager() {
        if (!IAccessControl(address(lrtConfig)).hasRole(LRTConstants.MANAGER, msg.sender)) {
            revert ILRTConfig.CallerNotLRTConfigManager();
        }
        _;
    }

• LRT-Admin

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

Approach Taken-in Evaluating The Kelp DAO

Accordingly, I analyzed and audited the subject in the following steps;

1. Core Protocol Contract Overview:

   I focused on thoroughly understanding the codebase and providing recommendations to improve its functionality. The main goal was to take a close look at the important contracts and how they work together in the Kelp DAO Protocol.

   Main Contracts I Looked At

           LRTDepositPool.sol
           LRTConfig.sol
           NodeDelegator.sol
           LRTOracle.sol
           RSETH.sol
           ChainlinkPriceOracle.sol
   

   I started my analysis by examining the LRTDepositPool.sol contract it's important because the LRTDepositPool contract manages LST asset deposits, including functionalities such as tracking deposit distribution across the protocol, managing deposit limits, minting rseth tokens based on asset amounts and exchange rates, and handling node delegator contracts with features like adding to a queue, transferring assets, and updating the maximum node delegator count.

   Then, I turned our attention to the LRTConfig.sol contract second because it manages critical configuration parameters for the LRT and serves as a centralized repository for critical configuration parameters in the Kelp DAO protocol, managing details such as supported assets with associated deposit limits, contract addresses for key components, and mappings between tokens and their respective strategies.

   Then NodeDelegator.sol contract next as its pivotal role in managing the depositing and withdrawal of assets into and from strategies within the LRT protocol, So ensuring the security of operations that impact the overall protocol's integrity and user assets.

   Then audit LRTOracle.sol, as it provides essential functionality for determining exchange rates, enabling precise asset valuation within the protocol.

   And RSETH contract as it represents the ERC20 implementation for the rsETH token, requiring careful examination to ensure secure minting, burning, and proper access control roles, especially pertaining to the interaction with the LRT configuration.

2. Documentation Review:

   The protocol currently lacks documentation,So went to Review This Blogs for a more detailed and technical explanation of Kelp DAO.

3. Manuel Code Review In this phase, I initially conducted a line-by-line analysis, following that, I engaged in a comparison mode.

   • Line by Line Analysis: Pay close attention to the contract's intended functionality and compare it with its actual behavior on a line-by-line basis.

   • Comparison Mode: Compare the implementation of each function with established standards or existing implementations, focusing on the function names to identify any deviations.

4. In summary, I have identified some vulnerabilities. I hope addressing these issues will enhance the security of the Kelp DAO Protocol.

   • The deposit limit for assets can be reduced by the manager, which could lock user assets if the limit is set below existing deposits.

   • Asset strategies can be changed by any admin, but there is no logic to withdraw existing funds from the old strategy, which could result in lost funds.

   • Strategies can be set per asset, but the contract code may expect a single global strategy, potentially causing funds to be sent to incorrect strategies.

   • The total balance calculation iterates through node delegate contracts, but does not prevent reentrancy or take a snapshot, so a deposit made during iteration would be missed, giving an incorrect low total balance.

   • The asset strategy concept is defined but strategies are not actually used or passed asset data, undermining extensibility and potentially causing unexpected behaviors.

   • Token transfers using transferFrom do not check allowance first, which could cause reverts if allowance is too low.

   • Strategy function calls do not validate success or return values, so failures or invalid returns would be ignored, potentially leaving funds in invalid states.

• Addressing these issues through strengthened input validation, error handling, and refactoring dependent relationships would help secure the contracts against exploitation or unintentional bugs.

Architecture Description and Diagram

Architecture of the key contracts that are part of the Kelp DAO | rsETH protocol:

This diagram illustrates the interaction among the key components of the Kelp Dao protocol.

• User deposits assets into the LRTDepositPool contract. An arrow from the user to the deposit pool showing the deposit.

• The LRTDepositPool contract interacts with the LRTOracle contract to get the current price of the deposited assets, represented by an arrow between them labeled "Price".

• Based on the asset prices, the LRTDepositPool then interacts with the RSETH contract to mint the equivalent amount of RSETH tokens for the user, shown by an arrow between them labeled "Mint RSETH".

• The RSETH tokens are then sent back to the user, represented by an arrow from RSETH back to the user labeled "RSETH".

• The LRTDepositPool contract then interacts with the NodeDelegator contract, shown with an arrow between them labeled "Transfer Assets".

• The NodeDelegator contract delegates the received assets to strategies on Eigenlayer, represented by an arrow pointing down to a box drawn as "Eigenlayer".

• The NodeDelegator is also able to transfer assets back to the LRTDepositPool if needed, shown with an arrow between them labeled "Transfer Back".

So in summary, it shows the core user deposit and withdrawal process, integration with oracle pricing, and delegation of assets to Eigenlayer strategies.

Architecture Feedback

1. Compose a comprehensive and detailed documentation for the Kelp DAO protocol, covering all pertinent aspects, including its purpose, features, smart contracts, security measures, community engagement strategies, user guidelines, and development roadmap.

2. Node Delegator: Manages the movement of liquid staked assets to contracts for each operator, automating reward redemption for restakers.

3. Leverage zEIP-23 for formally verified external calls in NodeDelegator to Eigen for highest security assurance.

4. I strongly recommend getting formal verification done on contracts to provide the highest level of security.

5. Support multiple LST integrations from the start to benefit from diversification and maximize API compatibility for future integrations.

6. Implement EIP-1271 signature verification in deposit/withdrawal functions to authenticate calls from approved wallets, preventing front-running attacks.

Codebase Quality

Overall, I consider the quality of the Kelp DAO codebase to be excellent. The code appears to be very mature and well-developed. We have noticed the implementation of various standards adhere to appropriately. Details are explained below:

Systemic & Centralization Risks

The analysis provided highlights several significant systemic and centralization risks present in the Kelp Dao protocol. These risks encompass concentration risk in LRTConfig, LRTDepositPool, NodeDelegator risk and more, third-party dependency risk, and centralization risks arising from the existence of an “owner” role in specific contracts.

Here's an analysis of potential systemic and centralization risks in the provided contracts:

Systemic Risks:

1. No having, fuzzing and invariant tests could open the door to future vulnerabilities.

2. The dynamic array nodeDelegatorQueue in LRTDepositPool contract has the potential for high gas costs if the length becomes excessively large. This could lead to transaction failures or expensive operations, especially during functions like addNodeDelegatorContractToQueue.

3. Nothing prevents node operators in LRTDepositPool contract from colluding to manipulate the system through their control over funds.

4. The maxApproveToEigenStrategyManager function in NodeDelegator contract approves the maximum amount of an asset to the Eigen Strategy Manager without a specific limit. This can pose a risk if the approval is misused or if there are vulnerabilities in the Eigen Strategy Manager contract.

5. In LRTOracle contract the getRSETHPrice function calculates the RSETH price based on the total ETH in the pool and the total supply of RSETH. If there are inaccuracies in the underlying data or if the total ETH calculation is incorrect, it can lead to incorrect RSETH prices.

6. The Protocol uses a single source (Chainlink) for fetching asset prices. If this single source is compromised, experiences downtime, or provides inaccurate data, it can impact the reliability of the entire oracle system.

Centralization Risks:

1. Only Administrators have the authority to perform these functions.

   • Update the LRTConfig contract address (single point of control over config)
   • Add new node delegator contract addresses to the queue (centralized control over node operators)
   • Transfer assets from the deposit pool to node delegators (centralized redistribution of funds)
   • Update the maximum number of node delegators (centralized control over network size)
   • Pause the contract in emergency situations (centralized pause control)
   • Unpause the contract after being paused (centralized unpause control)
   • Update supported asset configurations like price oracles (centralized config control)
   • Update deposit limits for assets (centralized limits control)
   • Change asset strategies (centralized strategy control)
   • Upgrade the contract to new versions (centralized upgrade control)
   • Grant/revoke admin roles for future admins (centralized governance control)
   • Set/change critical addresses stored in LRTConfig (centralized dependency control)
   • Adjust any other parameters defined in LRTConfig (broad centralized config control)
   • Admin can grant/revoke privileged roles like manager at will without community input
   • All critical functions, in NodeDelegator including asset approval, depositing into strategies, and transferring assets, can only be performed by the LRT manager
   • All critical functions, in LRTOracle contract including updating price oracles, pausing/unpausing the contract, and managing the configuration, can only be performed by the LRT manager.
   • The dynamic array nodeDelegatorQueue in LRTDepositPool contract can be manipulated by the LRTAdmin role, introducing centralization risks in managing the array of node delegator contracts.

Properly managing these risks and implementing best practices in security and decentralization will contribute to the sustainability and long-term success of the Kelp Dao protocol.

Conclusion

In general, the Kelp Dao project exhibits an interesting and well-developed architecture we believe the team has done a good job regarding the code, but the identified risks need to be addressed, and measures should be implemented to protect the protocol from potential malicious use cases. Additionally, it is recommended to add and improve the documentation and comments in the code to enhance understanding and collaboration among developers. It is also highly recommended that the team continues to invest in security measures such as mitigation reviews, audits, and bug bounty programs to maintain the security and reliability of the project.

Time spent:

20 hours