Update in strategy will cause wrong issuance of shares

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L70 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L49 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L84 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L122-L123

Vulnerability details

Impact

If there is update in strategy, getTotalAssetDeposits(asset) will reduce for asset. Because it checks for the assets deposited in NodeDelegator. NodeDelegator.getAssetBalance() will check for asset balance for the strategy of the user. If strategy is updated, then the balance of old strategy won't be taken into account which will reduce the totalDeposits. This will reduce the rsETH share price and cause more rsETH shares to be minted for the depositors. Thus, depositors can immediately deposit and their shares are worth more than their deposit.

Proof of Concept

contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle public lrtOracle;
    NodeDelegator public nodeDelegator;
    ILRTConfig public ilrtConfig;
    MockEigenStrategyManager public mockEigenStraregyManager;
    MockStrategy public mockStrategy1;
    MockStrategy public mockStrategy2;

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

        //deploy rsETH
        proxyAdmin = new ProxyAdmin();
        RSETH tokenImpl = new RSETH();
        TransparentUpgradeableProxy tokenProxy = new TransparentUpgradeableProxy(
                address(tokenImpl),
                address(proxyAdmin),
                ""
            );
        rseth = RSETH(address(tokenProxy));

        //deploy lrtConfig
        proxyAdmin = new ProxyAdmin();
        LRTConfig lrtConfigImpl = new LRTConfig();
        TransparentUpgradeableProxy lrtConfigProxy = new TransparentUpgradeableProxy(
                address(lrtConfigImpl),
                address(proxyAdmin),
                ""
            );

        lrtConfig = LRTConfig(address(lrtConfigProxy));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        // rseth.initialize(address(admin), address(lrtConfig));
        // add rsETH to LRT config
        // lrtConfig.setRSETH(address(rseth));
        // add oracle to LRT config

        // deploy LRTOracle
        ProxyAdmin proxyAdmin1 = new ProxyAdmin();
        lrtOracle = new LRTOracle();
        TransparentUpgradeableProxy contractProxy1 = new TransparentUpgradeableProxy(
                address(lrtOracle),
                address(proxyAdmin1),
                ""
            );
        lrtOracle = LRTOracle(address(contractProxy1));

        // deploy NodeDelegator
        proxyAdmin1 = new ProxyAdmin();
        nodeDelegator = new NodeDelegator();
        contractProxy1 = new TransparentUpgradeableProxy(
            address(nodeDelegator),
            address(proxyAdmin1),
            ""
        );
        nodeDelegator = NodeDelegator(address(contractProxy1));

        lrtConfig.initialize(
            admin,
            address(stETH),
            address(rETH),
            address(cbETH),
            address(rseth)
        );
        rseth.initialize(admin, address(lrtConfig));
        lrtOracle.initialize(address(lrtConfig));
        nodeDelegator.initialize(address(lrtConfig));
        lrtDepositPool.initialize(address(lrtConfig));

        mockEigenStraregyManager = new MockEigenStrategyManager();
        mockStrategy1 = new MockStrategy(address(stETH), 0);
        mockStrategy2 = new MockStrategy(address(stETH), 0);

        vm.startPrank(admin);
        lrtConfig.setContract(
            keccak256("LRT_DEPOSIT_POOL"),
            address(lrtDepositPool)
        );
        lrtConfig.setContract(
            keccak256("EIGEN_STRATEGY_MANAGER"),
            address(mockEigenStraregyManager)
        );
        lrtConfig.grantRole(keccak256("MANAGER"), manager);
        lrtConfig.setRSETH(address(rseth));
        vm.stopPrank();

        vm.startPrank(manager);
        lrtOracle.updatePriceOracleFor(
            address(stETH),
            address(new LRTOracleMock())
        );
        lrtOracle.updatePriceOracleFor(
            address(rETH),
            address(new LRTOracleMock())
        );
        lrtOracle.updatePriceOracleFor(
            address(cbETH),
            address(new LRTOracleMock())
        );
        // lrtConfig.addNewSupportedAsset(address(randomToken), 100_000 ether);
        // lrtOracle.updatePriceOracleFor(
        //     address(randomToken),
        //     address(new LRTOracleMock())
        // );
        vm.stopPrank();

        vm.startPrank(admin);
        lrtConfig.updateAssetStrategy(address(stETH), address(mockStrategy1));

        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        address[] memory nodeDelegatorContracts = new address[](1);
        nodeDelegatorContracts[0] = address(nodeDelegator);
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorContracts);
        vm.stopPrank();
    }

function test_update_strategy_cause_wrong_minting_of_shares() public {
        //NOTE: comment line 207-211 in setup for this
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(stETH), 10 ether); //NOTE: 10000000000000000000 rsETH amount minted
        vm.stopPrank();
        vm.startPrank(manager);
        lrtDepositPool.transferAssetToNodeDelegator(
            0,
            address(stETH),
            10 ether
        );
        vm.stopPrank();
        vm.startPrank(admin);
        lrtConfig.updateAssetStrategy(address(stETH), address(mockStrategy2));
        lrtConfig.updateAssetStrategy(address(rETH), address(mockStrategy2));
        lrtConfig.updateAssetStrategy(address(cbETH), address(mockStrategy2));

        vm.stopPrank();
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(stETH), 10 ether); //NOTE:   2500000000000000000 rsETH amount minted even after fixing deposit bug
        vm.stopPrank();
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

While updating the strategy, ensure that balance deposited in previous strategy for same asset is accounted while minting the shares.

Assessed type

ERC4626

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

gus-staderlabs (sponsor) disputed

[gus-stdr]

The Eigenlayer has a 1-1 mapping for asset and its strategy. We don't believe this an issue with KelpDAO or its code but with Eigenlayer if it upgrades its protocol to have multiple strategies for an asset. Then if this occurs, KelpDAO will also have to upgrade its contracts to ensure the compatibility to Eigenlayer. Also, if Eigenlayer changes strategies, they are responsible to migrate the internal accounting to the new address.

[manoj9april]

Currently eigenlayer's implementation allows them to have multiple startegies for any asset. -> But we can continue to deposit into one strategy for a asset and it won't be a problem for us. That is assuming 1:1 mapping won't harm us.

Above assumptions will only create problem in case, eigen layer wants to spread the current funds into multiple strategies for any asset in future upgrades. And in this case, they will provide proper heads up ahead of time and we can upgrade along with them accordingly.

In case they change their strategy for any asset, we can point to new strategy address and it should work fine. And in this case eigen layer should should take care of all migration of funds from old to new strategy.

But as currently nothing has been decided at eigen layer end. I believe we are good with our current assumption of 1:1. It would be a overkill to assume anything else as of now.

Please check the attached conversation with eigen layer team. Discord: https://discord.com/channels/1089434273720832071/1096331191243788379

[fatherGoose1]

Kelp has provided adequate reasoning behind the current design. I agree that it is unlikely that a breaking change from the Eigenlayer protocol would not be properly communicated ahead of time. Designing around this potentiality would impose added risk in the current design.

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid

[radeveth]

Hey, @fatherGoose1.

I disagree with the reasons provided above.

We don't believe this an issue with KelpDAO or its code but with Eigenlayer if it upgrades its protocol to have multiple strategies for an asset.

The problem lies specifically in how Kelp DAO upgrades the strategy for assets. Nothing related to the functionality of EigenLayer Strategies.

Let's view at the code of startegy upgradeing

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

As we can see, when an EigenLayer Strategy for an asset is changed, there is currently no mechanism in place to migrate user deposits from the old strategy to the new one. It is precisely in this scenario that the vulnerability arises.

If a strategy update occurs, the function getTotalAssetDeposits(asset) will show a reduced value for that asset. This reduction happens because the function references assets deposited in NodeDelegator. Specifically, NodeDelegator.sol#getAssetBalance() assesses the asset balance based on the user's current strategy. When a strategy update takes place, the balance associated with the previous strategy is no longer considered, leading to a decrease in totalDeposits. As a result, the share price of rsETH decreases, prompting the system to mint more rsETH shares for depositors. Consequently, depositors can make immediate deposits, gaining shares that are more valuable than the actual amount they deposited.

This vulnerability describe significant flaws in the KelpDAO protocol flow. Nothing related to the functionality of EigenLayer Strategies.

But as currently nothing has been decided at eigen layer end. I believe we are good with our current assumption of 1:1. It would be a overkill to assume anything else as of now.

The recommendations in this report do not make any assumptions about the implementation of 1:1 mapping. Instead, they specifically suggest updating the updateAssetStrategy() function. in a way thet update would enable the transfer of user deposits from the old strategy to the new strategy once the new strategy is set. (as in my issue #644 writes)

I believe the issue has been misjudged and should be mitigated to High Severity Issue.

Have a good one.

[manoj9april]

@radeveth KelpDao will only update the strategy address when eigenLayer updates strategy for any asset (which eigenLayer currently has no plans as such).

Even if above scenario takes place. EigenLayer should provide proper communications ahead of time. And we can go through follow below steps:

• eigen layer communicates of strategy update
• KelpDao pauses its protocol for a mutually decided duration
• eigenLayer migrates its state from old contracts to new contracts.
• KelpDao points to new strategy contract
• KelpDao resumes

if migration of contract state is not possible at eigenLayer and they(eigenlayer) decomission their old strategy, only in this scenario KelpDao will have to withdraw its funds from old strategy and redelgate it to new strategy. And these steps will be done in complete visibility and transparency with proper timelocks. But sees no harm or loss to funds or miscalculation.

[fatherGoose1]

This will be upgraded to MEDIUM.

The warden accurately explains how Kelp's internal accounting will be broken if they decide to use the updateAssetStrategy() function. If Kelp changes the strategy address, rsETH share prices will decrease in a way that should not occur.

While Kelp describes the list of actions that they would take in the case they would need to change the strategy address, the implementation does not protect against the possibility of the broken accounting.

[c4-judge]

fatherGoose1 changed the severity to 2 (Med Risk)

[radeveth]

Hey, @fatherGoose1.

This has been marked as medium, but it still has the unsatisfactory label.

Have a good one

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report