Kelp DAO | rsETH - Daniel526's results

Lines of code

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

Vulnerability details

Impact

The impact of this vulnerability is that users may receive a different amount of rseth than initially anticipated, leading to potential discrepancies and affecting the overall user experience within the protocol.

Proof of Concept

In the getRsETHAmountToMint function, the amount of rseth to mint is calculated based on the exchange rates between the deposited asset (LST) and rseth obtained from the oracle. The vulnerability lies in the dependency on external market conditions, specifically the oracle's reported prices.

function getRsETHAmountToMint(address asset, uint256 amount) public view override returns (uint256 rsethAmountToMint) {
    // setup oracle contract
    address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
    ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

    // calculate rseth amount to mint based on asset amount and asset exchange rate
    rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
}

In this code snippet:

• getAssetPrice(asset) retrieves the current price of the deposited asset from the oracle.
• getRSETHPrice() retrieves the current price of rseth from the oracle. Consider a decentralized finance (DeFi) protocol utilizing the LRTDepositPool contract. Users interact with the protocol by depositing LST (the deposited asset) and receiving an equivalent amount of rseth (a secondary asset) based on exchange rates obtained from an oracle. The exchange rates play a crucial role in determining the minted rseth amount.

1. User Deposit: User A decides to deposit 100 LST into the LRTDepositPool contract. At the time of deposit, the exchange rate between LST and rseth is favorable, with 1 LST being equivalent to 2 rseth.
2. Exchange Rate Fluctuation: Subsequently, before the minting process occurs, the market experiences a shift, and the exchange rate between LST and rseth becomes less favorable. Now, 1 LST is equivalent to only 1.5 rseth.
3. Minting Process: User A initiates the minting process by calling the depositAsset function. The getRsETHAmountToMint function calculates the rseth amount based on the deposit amount and the exchange rates obtained from the oracle.

// Assuming asset = LST, amount = 100
rsethAmountToMint = (100 * lrtOracle.getAssetPrice(LST)) / lrtOracle.getRSETHPrice();

4. Impact on Minted rseth: Due to the changed exchange rate, the calculated rsethAmountToMint is now lower than what User A might have expected based on the rate at the time of deposit.

// Assuming the new exchange rate is 1.5 (instead of 2)
rsethAmountToMint = (100 * 1) / 1.5; // Calculated rsethAmountToMint is now 66.67

5. User Receives Less rseth: User A receives only 66.67 rseth instead of the initially anticipated 100 rseth. The discrepancy is a result of the variable exchange rates between LST and rseth during the period between deposit and minting.

Tools Used

Manual

Recommended Mitigation Steps

A dynamic pricing mechanism should be implemented that considers the exchange rates at both the time of deposit and the time of minting. This ensures that the calculation is based on consistent and up-to-date values, reducing the impact of market fluctuations during the minting process.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The potential impact of this vulnerability is the execution failure of the transferAssetToNodeDelegator function when an invalid index is provided. This could result in the loss of assets, as the function attempts to transfer funds to a Node Delegator using an out-of-bounds index. If the index is not properly validated, it may cause assets to be transferred to an unintended or non-existent destination, leading to a permanent loss of those assets or even an unintended transaction revert.

Proof of Concept

The transferAssetToNodeDelegator function is designed to transfer assets to a specific Node Delegator based on the provided index. However, the code does not include a validation check to ensure that the given index is within the valid range of indices in the nodeDelegatorQueue array. If the index exceeds the length of the array, an attempt to access an invalid index will result in an exception.

function transferAssetToNodeDelegator(
    uint256 ndcIndex,
    address asset,
    uint256 amount
)
    external
    nonReentrant
    onlyLRTManager
    onlySupportedAsset(asset)
{
    // Vulnerability: Missing index validation
    address nodeDelegator = nodeDelegatorQueue[ndcIndex];
    if (!IERC20(asset).transfer(nodeDelegator, amount)) {
        revert TokenTransferFailed();
    }
}

The vulnerability lies in the assumption that the provided index is always valid, which could lead to unintended consequences if the index is out of bounds.

Tools Used

Manual

Recommended Mitigation Steps

Add a validation check to ensure that the provided index is within the valid range before attempting to access the nodeDelegatorQueue array.

function transferAssetToNodeDelegator(
    uint256 ndcIndex,
    address asset,
    uint256 amount
)
    external
    nonReentrant
    onlyLRTManager
    onlySupportedAsset(asset)
{
    // Mitigation: Validate the index before accessing the array
    require(ndcIndex < nodeDelegatorQueue.length, "Invalid index");

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

Assessed type

Context