Kelp DAO | rsETH - zhaojie's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L136 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L70

Vulnerability details

Impact

When depositAsset, the price of RsETH is related to the depositAmount. The larger the depositAmount is, the higher the price of RsETH is, and the less RsETH quantity can be obtained. this is unfair to users who deposit a lot of assets at once, and the price of RsETH is higher than the actual expected price because the assets in the pool are calculated in advance.

LRTDepositPool#depositAsset:

    function depositAsset(address asset, uint256 depositAmount)

Proof of Concept

LRTDepositPool#depositAsset,transfer asset first, then mint RsETH:

  function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        .....
@>      if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
            revert TokenTransferFailed();
        }
@>      uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);
        emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
    }

LRTOracle#getRSETHPrice, totalETHInPool include the asset that is currently being deposited,the value of rsEthSupply is before deposit.

  function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();
        .....
        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            address asset = supportedAssets[asset_idx];
            uint256 assetER = getAssetPrice(asset);
            //@audit totalAssetDeposit include the asset that is currently being deposited
@>          uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            totalETHInPool += totalAssetAmt * assetER;
            unchecked {
                ++asset_idx;
            }
        }
@>      return totalETHInPool / rsEthSupply;
    }

Tools Used

vscode manual

Recommended Mitigation Steps

Get the price of RsETH before transferring the asset (calculate the number of Mint RsETH)

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

+    (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);
+    if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
+        revert TokenTransferFailed();
+    }
+    IRSETH(address(lrtConfig.rsETH())).mint(msg.sender, rsethAmountToMint);

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

When RsETH totalSupply is 0, users deposit 1wei's assets, and then other users will only receive a small amount of rsETH when they redeposit (such as 1eth), protocol will not work.

Proof of Concept

LRTDepositPool#depositAsset calculates the number of RsETH tokens in mint based on the price of deposited assets and RsETH.

    rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();

The price returned by lrtOracle.getRSETHPrice is determined by the total amount of RsETH and the total amount of assets(rETH+stETH+cbETH) deposited, as well as the price.

When the number of RsETH in the pool is only 1wei, if the user deposits 1eth, a large price will be returned.

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }
        return totalETHInPool / rsEthSupply;

When the number of RsETH in the pool is only 1wei, redeposit 1 eth, in lrtOracle.getRSETHPrice function totalETHInPool is 1 eth, rsEthSupply is 1, totalETHInPool/rsEthSupply value is very big, RsETH price will be high, the user will only get a few RsETH token. The asset(rETH) is transferred to the pool before the price is calculated so the totalETHInPool is 1eth.

This can happen when the protocol is just started, or for some other reason RsETH totalSupply is 0.

Test code:

    function test_DepositAsset2() external {
        vm.startPrank(alice);

        rETH.approve(address(lrtDepositPool), 2 ether);
        // first time deposit 1wei
        lrtDepositPool.depositAsset(rETHAddress, 1);
        // then deposit 1wei
        lrtDepositPool.depositAsset(rETHAddress, 1 ether);
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        vm.stopPrank();
        //aliceBalanceAfter = 1
        console.log(aliceBalanceAfter);
    }

Running test_DepositAsset1 yields the normal amount of RsETH, while test_DepositAsset2 deposits 1wei and then 1eth and only 1wei RsETH.

forge test --match-test test_DepositAsset1 -vv
forge test --match-test test_DepositAsset2 -vv

Place the test file in the test directory:

pragma solidity 0.8.21;

import {BaseTest} 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 {LRTOracle} from "src/LRTOracle.sol";

import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import {ProxyAdmin} from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";
import "forge-std/console.sol";

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

contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracle public lrtOracle;

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

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

        LRTOracle lrtOracleImpl = new LRTOracle();
        MockPriceOracle priceOracle = new MockPriceOracle();

        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
                address(lrtOracleImpl),
                address(proxyAdmin),
                ""
            );

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

        lrtOracle = LRTOracle(address(lrtOracleProxy));
        lrtOracle.initialize(address(lrtConfig));

        lrtConfig.grantRole(LRTConstants.MANAGER, admin);
        lrtConfig.setContract(
            LRTConstants.LRT_DEPOSIT_POOL,
            address(lrtDepositPool)
        );

        lrtOracle.updatePriceOracleFor(address(rETH), address(priceOracle));
        lrtOracle.updatePriceOracleFor(address(stETH), address(priceOracle));
        lrtOracle.updatePriceOracleFor(address(cbETH), address(priceOracle));

        //console.log("getAssetPrice:",lrtOracle.getAssetPrice(address(rETH)));

        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracle));
        vm.stopPrank();
    }
}

contract LRTDepositPoolDepositAsset is LRTDepositPoolTest {
    address public rETHAddress;

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

        // initialize LRTDepositPool
        lrtDepositPool.initialize(address(lrtConfig));

        rETHAddress = address(rETH);

        // add manager role within LRTConfig
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();
    }

    function test_DepositAsset1() external {
        vm.startPrank(alice);

        rETH.approve(address(lrtDepositPool), 1 ether);

        lrtDepositPool.depositAsset(rETHAddress, 1 ether);
        // alice balance of rsETH after deposit
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        vm.stopPrank();

        console.log(aliceBalanceAfter);
    }

    function test_DepositAsset2() external {
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), 2 ether);
        lrtDepositPool.depositAsset(rETHAddress, 1);
        lrtDepositPool.depositAsset(rETHAddress, 1 ether);
        // alice balance of rsETH after deposit
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        vm.stopPrank();
        console.log(aliceBalanceAfter);
    }
}

Tools Used

vscode manual

Recommended Mitigation Steps

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
         ...
-        if (rsEthSupply == 0) {
+        if (rsEthSupply <= 1 ether) {
            return 1 ether;
         }
         ...
    }

Assessed type

Error

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L109 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L84

Vulnerability details

Impact

LRTConfig#updateAssetStrategy will affect the price of RsETH, when the administrator calls updateAssetStrategy the user may get more RsETH at a lower price.

Proof of Concept

The price of RsETH is affected by the total amount of deposited assets. When the total amount decreases, the price also decreases, and users can obtain more RsETH when they deposit.

The total amount of assets is calculated including the balance of assetStrategy:

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        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;
            }
        }
    }

    function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));

        uint256 ndcsCount = nodeDelegatorQueue.length;
        for (uint256 i; i < ndcsCount;) {
            assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
@>          assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
            unchecked {
                ++i;
            }
        }
    }

    function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
@>      return IStrategy(strategy).userUnderlyingView(address(this));
    }

Therefore, when the administrator changes the assetStrategy, if the balance of the new assetStrategy is different from that of the old one, the total amount of assets will change. If the new assetStrategy has less balance than the old one, the price of RsETH will go down. Although the administrator can modify the balance of the new assetStrategy to be consistent with the old one, but, if it is not in the same transaction, the user can use the frontrunning to deposit assets before the administrator corrects the balance and obtain more RsETH at a lower price.

Tools Used

vscode manual

Recommended Mitigation Steps

Check whether the balance changes before and after the update in LRTConfig#updateAssetStrategy

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L56 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L94

Vulnerability details

Impact

If the depositLimit exceeds the total amount of asset deposits, the deposit will always revert.

Proof of Concept

To deposit an asset in the pool, the depositAsset function will check whether the depositAmount exceeds the maximum value.

    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        ....
        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }
        ....
    }
    
    function getAssetCurrentLimit(address asset) public view override returns (uint256) {
        return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
    }

The maximum value can be reset by the administrator, and if the reset value is smaller than getTotalAssetDeposits(asset),depositAsset will always fail to execute.

    function updateAssetDepositLimit(
        address asset,
        uint256 depositLimit
    )
        external
        onlyRole(LRTConstants.MANAGER)
        onlySupportedAsset(asset)
    {
        depositLimitByAsset[asset] = depositLimit;
        emit AssetDepositLimitUpdate(asset, depositLimit);
    }

Tools Used

vscode manual

Recommended Mitigation Steps

When updateAssetDepositLimit, check whether the depositLimit will exceed the total amount

Assessed type

Other