Kelp DAO | rsETH - 0xbrett8571's results

Lines of code

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

Vulnerability details

Impact

The _mintRsETH calculates the amount of rsETH to mint based on the latest exchange rate from the LRTOracle. This means there is a window between when a user deposits tokens, and when _mintRsETH is called, where the user is exposed to exchange rate fluctuations. If the rate changes unfavorably during this window, the user will receive less rsETH than expected based on deposit-time rates.

Proof of Concept

The _mintRsETH function calculates the amount of rsETH to mint based on the latest exchange rate from the LRTOracle.

This means if there is a lag between when the user deposits tokens, and when _mintRsETH is called, the user is exposed to exchange rate changes during that time.

If the rate changes unfavorably in that window, they will receive less rsETH than expected based on deposit-time rates.

The root cause is that _mintRsETH always uses the latest rate from the oracle rather than the rate at the time of deposit. Link to the Issue

    /// @notice helps user stake LST to the protocol
    /// @param asset LST asset address to stake
    /// @param depositAmount LST asset amount to stake
    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);
    }


    /// @dev private function to mint rseth. It calculates rseth amount to mint based on asset amount and asset exchange
    /// rates from oracle
    /// @param _asset Asset address
    /// @param _amount Asset amount to mint rseth
    /// @return rsethAmountToMint Amount of rseth minted
    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);
    }

As you can see, the rsETH amount is calculated at the time of deposit but not used during minting.

• In depositAsset, the expected rsETH amount is calculated via getRsETHAmountToMint

• This happens at the time of deposit, so it uses the deposit-time exchange rate

• _mintRsETH is called later to actually mint the rsETH

• However, _mintRsETH recalculates the amount to mint based on the latest exchange rate

• So any rate change between deposit and _mintRsETH affects the final rsETH received

So this issue has a high likelihood of occurring any time there is a lag between deposit and minting, as exchange rates can fluctuate frequently.

Maximum Risk Impact

This can be exploited to minimize the amount of rsETH minted in a deposit.

Here is an example exploit scenario:

1. Attacker deposits 100 DAI when 1 DAI = 1 USD and the rsETH rate is 0.95 ETH.

   • Expected rsETH to mint is 95.

2. Between deposit and minting, the attacker temporarily manipulates the DAI/USD rate on the oracle to 1 DAI = 0.10 USD.

3. _mintRsETH uses this latest rate, so the rsETH amount calculated is now 9.5.

4. Attacker mints only 9.5 rsETH instead of the expected 95 rsETH based on deposit-time rates.

Demonstration

Here is some code that demonstrates how this could work:

// Deposit when 1 DAI = 1 USD
deposit(100 DAI) 

// Expected rsETH is 95
uint expectedRsETH = getExpectedMintAmount(100 DAI, 1 USD); 

// Manipulate DAI/USD rate to 0.10 USD
setManipulatedRate(0.10 USD);

// _mintRsETH uses manipulated rate 
uint rsETHReceived = _mintRsETH(100 DAI);

// rsETHReceived is 9.5 instead of 95 due to manipulated rate
assertEq(rsETHReceived, 9.5);  
assertLt(rsETHReceived, expectedRsETH);

This demonstrates how manipulating the rate between deposit and minting can severely impact the rsETH amount received.

Tools Used

Manual

Recommended Mitigation Steps

• Pass expected rsETH amount calculated at deposit time to _mintRsETH instead of recalculating
• Store exchange rate and eth amount at time of deposit, use same data for minting
• Add deposit-time exchange rate data to logs to allow off-chain verification

Assessed type

Oracle

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTConfig.sol#L70-L76 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTConfig.sol#L70-L76

Vulnerability details

Impact

The addNewSupportedAsset function does not validate asset addresses. This allows anyone to add malicious or invalid contracts as supported assets. These could manipulate protocol behavior or steal funds when interacted with.

Proof of Concept

addNewSupportedAsset takes an asset address as input. But does not verify that this address is a valid ERC20 token contract.

This means an arbitrary address could be added as a "supported" asset, not just real tokens.

• Could add scam or malicious contracts as approved assets that steal funds or manipulate protocol behavior.
• Integrations with the listed assets may fail or act unpredictably if they are not actual tokens.
• Hurts integrity of asset whitelist, since it could contain non-token contracts.
• UI/tooling may display invalid assets as tradeable options.

The addNewSupportedAsset external function which:

• Takes an asset address and deposit limit as parameters
• Checks the MANAGER role using the OpenZeppelin onlyRole modifier
• Calls the internal _addNewSupportedAsset function to add the asset

The key issue is that addNewSupportedAsset does not verify that the asset address passed in is a valid ERC20 token contract. It only relies on the access control check from onlyRole.

So any address could be passed and added as a supported asset, even if it is not an actual token contract.

addNewSupportedAsset function

function addNewSupportedAsset(address asset, uint256 depositLimit) external {
  // No input validation on `asset` 
  _addNewSupportedAsset(asset, depositLimit);
}

This allows adding any arbitrary address as a supported asset.

An attacker could:

1. Deploy a malicious token contract:

contract MalToken {
  function transferFrom(address, address, uint) external {
    // Drain funds
  }
}

2. Get it added as a supported asset:

LRTConfig(config).addNewSupportedAsset(malToken, 100);

3. When transfers are attempted on the fake token, it drains funds:

IERC20(malToken).transferFrom(user, pool, 10); // Calls malicious contract

This test demonstrates adding a malicious contract, and it draining funds when used as a "supported" asset.

// test/LRTConfig.t.sol

import "forge-std/Test.sol";
import {LRTConfig} from "../src/LRTConfig.sol";

contract Exploit is Test {
  LRTConfig config;
  MalToken malToken;

  function testAddMaliciousAsset() public {

    malToken = new MalToken(); // malicious contract

    config.addNewSupportedAsset(address(malToken), 100);

    assertTrue(config.isSupportedAsset(address(malToken)));

    vm.expectCall(address(malToken), abi.encodeWithSelector(
     MalToken.transferFrom.selector, address(0), address(0), 10
    ));

    IERC20(address(malToken)).transferFrom(address(0), address(0), 10);

  }
}

contract MalToken {
  function transferFrom(address from, address to, uint amount) external {
    // drain funds 
  }
}

But the Likelihood of this happening

• Requires permission to call addNewSupportedAsset (MANAGER role holder)
• If improper access controls, could be exploited

Tools Used

Manual review

Recommended Mitigation

In addNewSupportedAsset, validate asset addresses are proper ERC20 contracts before adding:

function addNewSupportedAsset(address asset, uint256 depositLimit) external {

+ require(ERC20(asset).totalSupply() > 0, "Invalid ERC20 token");

  // Additional ERC20 validity checks

  // ...

}

This prevents invalid addresses from being added.

Assessed type

Invalid Validation

Kelp aims to provide liquidity rewards for staked ETH assets by integrating with the EigenLayer protocol. It consists of core contracts for configuration, deposits, delegating to strategies, pricing, and receipt tokens.

I reviewed the contracts through manual analysis, focused on security, intended behavior, and integration.

LRTConfig: An upgradeable contract which is responsible for storing the configuration of the Kelp product. It is also responsible for storing the addresses of the other contracts in the Kelp product.

LRTDepositPool: An upgradeable contract which receives the funds deposited by users into the Kelp product. From here, the funds are transferred to NodeDelegators contracts.

NodeDelegator: These are contracts that receive funds from the LRDepositPool and delegate them to the EigenLayer strategy. The funds are then used to provide liquidity on the EigenLayer protocol. It is also an upgradeable contract.

LRTOracle: An upgradeable contract which is responsible for fetching the price of Liquid Stasking Tokens tokens from oracle services.

RSETH: Receipt token for depositing into the Kelp product. It is an upgradeable ERC20 token contract.

Scope

Note: The "Libraries Used" column indicates any external libraries or contracts that are utilized by the respective contract.

Approach

My review involved:

• Manual code analysis of logic and control flow
• Checking input validation and exit conditions
• Identifying trust relationships between contracts
• Assessing access control and privilege escalation risks
• Evaluating protocol integration points and failure modes
• Considering economic incentives and tokenomics

Architecture

Kelp has a modular architecture with well-defined contract responsibilities:

• LRTConfig: Stores addresses and parameters
• LRTDepositPool: Frontend for user deposits
• NodeDelegator: Delegates assets to EigenLayer
• LRTOracle: Provides asset price feeds
• RSETH: Receipt token for deposits

Diagram of the Kelp architecture showing the key contracts and their interactions:

graph TD
    subgraph Kelp DAO
        LRTConfig(LRTConfig<br/>Config & Address Storage)
        LRTDepositPool(LRTDepositPool<br/>Deposit Handler & RSETH Minting)
        NodeDelegator(NodeDelegator<br/>Asset Delegation)
        LRTOracle(LRTOracle<br/>Price Feeds)
        RSETH(RSETH<br/>Receipt Token)
    end

    User==>LRTDepositPool
    LRTDepositPool==Mint RSETH==>RSETH
    LRTDepositPool==Price Data==>LRTOracle
    LRTDepositPool-.->LRTConfig
    NodeDelegator-.->LRTConfig
    LRTDepositPool==Delegates Assets==>NodeDelegator
    
    classDef grey fill:#dddddd,stroke:#ffffff,stroke-width:4px,color:#000000;
    class LRTConfig,LRTOracle,LRTDepositPool,NodeDelegator,RSETH grey

    style LRTConfig fill:#5DA5DA,stroke:#ffffff,stroke-width:4px,color:#ffffff
    style LRTDepositPool fill:#FAA43A,stroke:#ffffff,stroke-width:4px,color:#ffffff
    style NodeDelegator fill:#60BD68,stroke:#ffffff,stroke-width:4px,color:#ffffff
    style LRTOracle fill:#F17CB0,stroke:#ffffff,stroke-width:4px,color:#ffffff
    style RSETH fill:#B2912F,stroke:#ffffff,stroke-width:4px,color:#ffffff

Key Points

• LRTConfig stores addresses and parameters for other contracts
• LRTDepositPool is the frontend for user deposits
• It mints RSETH tokens in sync with deposits
• NodeDelegator receives assets from LRTDepositPool and delegates to strategies
• LRTOracle provides pricing data to the system
• Modular architecture allows flexibility and separation of concerns

The core dependency is the unified configuration in LRTConfig, which must be secure and authorize only valid contract addresses.

LRTConfig

The LRTConfig contract is relied upon by other contracts for address lookups via the getContract and getToken functions:

    function getContract(bytes32 contractKey) external view override returns (address) {
        return contractMap[contractKey];
    }

// Example usage in LRTDepositPool
address lrtOracle = LRTConfig.getContract(LRT_ORACLE);

If an incorrect address is returned, it could lead to interactions with unauthorized contracts.

The key risk identified is that setContract allows the admin to update any address mapping. Proper access control is needed to restrict this.

LRTOracle

The LRTOracle provides pricing data that is consumed by other contracts: getAssetPrice

    function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (uint256) {
        return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);
    }

An vulnerability in getAssetPrice or getRSETHPrice could allow manipulated rates.

The main risk is that the LRTManager can set the price oracles, opening up privilege escalation.

RSETH

The RSETH contract must mint tokens in sync with LRTDepositPool based on exchange rates.

A mismatch in logic between the two could lead to economic exploits:

The key risk is there is no supply cap or validation in mint, so it must be used correctly.

This separation of concerns follows best practices. The use of upgradeable proxy contracts also allows flexibility.

Some dependencies to note:

• LRTConfig is relied on for address lookups
• LRTOracle provides critical pricing information
• RSETH must mint tokens correctly in sync with LRTDepositPool

Findings

Centralization Risks

• The LRTManager role has broad privileges such as pausing transfers and setting oracles. This could lead to centralized control over the system.

• The lrETH token address is set once at initialization but the admin can later change it. This poses a risk as the token is relied on for minting.

LRTManager Privileges

The LRTManager role has broad authority in several contracts:

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

// LRTOracle
[updatePriceOracleFor](https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L88-L99)

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

This allows an LRTManager to pause transfers and change price oracles. If a single account held this role, they could control deposits and pricing.

The main functions the role should not have are pausing, setting oracles, and updating config addresses.

rsETH Update

The rsETH token address is set at initialization in LRTConfig: Line 21

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

// LRTConfig
address public rsETH;

function initialize(address rsETH_) {
  rsETH = rsETH_; 
}

But the admin can later change this: setRsETH

    function setRSETH(address rsETH_) external onlyRole(DEFAULT_ADMIN_ROLE) {
        UtilLib.checkNonZeroAddress(rsETH_);
        rsETH = rsETH_;
    }

This poses a risk as rsETH is relied on for minting in LRTDepositPool. An unauthorized contract could lead to loss of funds.

Making rsETH immutable after initialization would prevent this centralized control over minting.

Tokenomics

• RSETH minting is proportional to the amount deposited. No supply cap is enforced. This could incentivize depositing incorrectly priced assets to mint excess RSETH.

The key minting logic is in _mintRsETH in LRTDepositPool:

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

It calculates the rsethAmount based on the deposit amount, gets the RSETH token address, and mints the tokens.

The issue is there is no logic enforcing a max cap on the total RSETH supply: mint

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

// No check on max supply

This means that if incorrect exchange rates are provided, an attacker could deposit at that rate and mint arbitrarily large amounts of RSETH.

For example:

1. Attacker sets a high rate for some asset in LRTOracle

2. Attacker deposits a small amount of that asset

3. _mintRsETH uses the high rate to mint a large amount of RSETH

Adding a max supply cap on RSETH would prevent this kind of exploit.

Logic Issues

• No validation of RSETH minting contract in LRTDepositPool (_mintRsETH). An unauthorized contract could be set via LRTConfig.

• Exchange rate in LRTOracle (getRSETHPrice) only considers LRTDepositPool balance. It does not account for assets already delegated through NodeDelegators. The total deposits could be inflated, affecting the rate.

No RSETH Validation in _mintRsETH

The _mintRsETH function gets the RSETH token address from LRTConfig but does not validate it:

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

This could allow an unauthorized contract to be set as the rsETH address and manipulate minting.

For example:

// Attacker contract
function mint(address to, uint amount) external {
  msg.sender.transfer(amount); // drain funds
}

The admin could point LRTConfig.rsETH to this contract. _mintRsETH would then drain funds instead of minting tokens.

This can be mitigated by:

• Checking rsETH address against a hardcoded verified address
• Using ERC165 to validate rsETH supports minting

Inflated Rates in getRSETHPrice

The getRSETHPrice function sums all deposits via LRTDepositPool only: getRSETHPrice

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

But it misses deposits already delegated through NodeDelegators, inflating the rate.

This can be fixed by tracking an independent totalSystemDeposits variable that sums across all contracts.

Recommendations

• Reduce LRTManager privileges to only critical operations like emergency shutdown. Move oracle control to admin only.

• Make rsETH immutable in LRTConfig after initialization.

• Enforce a cap on RSETH total supply in minting logic.

• In _mintRsETH, verify rsETH contract address using a hardcoded known good address.

• Maintain a separate total deposits variable in LRTOracle that tracks funds across all contracts.

Overall the system architecture is well designed but some key risks around privilege, logic, and incentives exist. Adding validation, immutable configuration, and supply caps would make the system more robust.

Time spent:

28 hours