Lack of slippage control on LRTDepositPool.depositAsset

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

The depositAsset function of the LRTDepositPool contract, enables users to deposit assets into the protocol, getting RSETH tokens in return. The function doesn't have any type of slippage control; this is relevant in the context of the depositAsset function, since the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.

Also the users will have no defense against price manipulations attacks, if they where to be found after the protocol's deployment.

Proof of Concept

As can be observed by looking at it's parameters and implementation, the depositAsset function of the LRTDepositPool contract, doesn't have any type of slippage protection:

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

Meaning that users have no control over how many RSETH tokens they will get in return for depositing in the contract.

The amount of tokens to be minted, with respect to the deposited amount, is determined by the getRsETHAmountToMint function of the same contract (inside of _mintRsETH):

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

As can be observed, this function uses two oracle interactions to determine how many tokens to mint, getAssetPrice and getRSETHPrice (with getRSETHPrice internally calling getAssetPrice as well).

The getAssetPrice function queries the external oracle for the asset price:

    function getAssetPrice(address asset) public view onlySupportedAsset(asset) returns (uint256) {
        return IPriceFetcher(assetPriceOracle[asset]).getAssetPrice(asset);
    }

Meaning that the user has no way to predict how many RSETH they will get back at the moment of minting, as the price could be updated while the request is in the mempool.

Here is a very simple foundry script that shows that an oracle price modification, can largely impact the amount of tokens received in return by the user (implying that the depositAsset function has no protection against it).
Place it in the test folder to preserve dependencies:

// SPDX-License-Identifier: UNLICENSED

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 { TransparentUpgradeableProxy } from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
import { ProxyAdmin } from "@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol";

import "forge-std/console.sol";

contract LRTOracleMock {

    uint256 assetPrice = 1e18;

    function setAssetPrice(uint256 newPrice) external {
        assetPrice = newPrice;
    }

    function getAssetPrice(address) external view returns (uint256) {
        return assetPrice;
    }

    function getRSETHPrice() external pure returns (uint256) {
        return 1e18;
    }
}

contract MockNodeDelegator {
    function getAssetBalance(address) external pure returns (uint256) {
        return 1e18;
    }
}

contract LRTDepositPoolTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    LRTOracleMock public mockOracle;

    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));
        mockOracle = new LRTOracleMock();

        // 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 oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(mockOracle));

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

        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_OracleCanModifyAssetMinted() external {
        
        vm.prank(alice);
        rETH.approve(address(lrtDepositPool), 2 ether);
        vm.prank(bob);
        rETH.approve(address(lrtDepositPool), 2 ether); 

        uint256 aliceDepositTime = block.timestamp;
        vm.prank(alice);
        lrtDepositPool.depositAsset(rETHAddress, 2 ether);

        mockOracle.setAssetPrice(1e17);

        uint256 bobDepositTime = block.timestamp;
        vm.prank(bob);
        lrtDepositPool.depositAsset(rETHAddress, 2 ether);

        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
        
        console.log(aliceBalanceAfter);
        console.log(bobBalanceAfter);    

        console.log(aliceDepositTime);
        console.log(bobDepositTime); 

        assertEq(aliceDepositTime, bobDepositTime);
        assertGt(aliceBalanceAfter, bobBalanceAfter);

    }

}

Recommended Mitigation Steps

And additional parameter could be added to the depositAsset function, to let users decide the minimum amount of tokens to be received, with a relative check after minting.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #39

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-sponsor]

gus-staderlabs (sponsor) confirmed

[c4-judge]

fatherGoose1 marked the issue as satisfactory

[c4-judge]

fatherGoose1 marked the issue as selected for report

[manoj9april]

Slippage control makes more sense in DEXes. But in staking protocols, where prices are mostly stable and users can reject if it txn takes longer time.
But it is a useful recommendation.
I think we can move it to QA.

[c4-sponsor]

manoj9april marked the issue as disagree with severity

[manoj9april]

similar to #102

[fatherGoose1]

Sticking with medium. There is no protection for the user in the current implementation. minShares checks are commonly implemented in staking contracts.