Kelp DAO | rsETH - turvy_fuzz's results

Lines of code

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

Vulnerability details

Impact

Function LRTDepositPool:depositAsset() does not check the minimum liquidity amount. This makes users' funds vulnerable to sandwich attacks.

depositAsset() will increase IRSETH(rsETHTokenAddress).totalSupply(), and thus change the exchange rate of RSETH.

return totalETHInPool / rsEthSupply;

Given the current network environment, most transactions in the mempool would be sandwiched. However, users may avoid this attack if they only send tx through Flashbot. I consider this a medium-risk issue.

Proof of Concept

Let's says a user wants to deposit 1000 LST in the pool hoping to get an expected amount of rsETH back. Attackers sees this and front runs trade as follows:

Mint more rsETH with 10,000 of the LST and extremely change the expected rsETH to mint for that LST User's tx enter here and execute depositAsset(). Trade finalize with unwanted rsETH amount given back to the user since no slippage specified

Tools Used

Visual Studio Code

Recommended Mitigation Steps

Function LRTDepositPool:depositAsset() should allow users to provide a min return and check the slippage. Always check how much liquidity a user receives in a transaction. A tx would not be sandwiched if it's not profitable.

We could learn a lot about MEV here.

Assessed type

MEV

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/LRTConfig.sol#L85 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L86

Vulnerability details

Impact

LRTOracle:getRSETHPrice() which is executed in all protocol pricing operations, includes all assets supported in the calculation. This asset is added with the addNewSupportedAsset() function, However, this sets the asset permanently and can't be undone if the asset returns same values when strategy is no longer handled and the asset is no longer supported on EigenLayer.

Proof of Concept

_addNewSupportedAsset() function sets assets as supported forever:

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

There is no function that can undo this.

LRTOracle:getRSETHPrice()

function getRSETHPrice() external view returns (uint256 rsETHPrice) {

        ...

        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); // @audit
            totalETHInPool += totalAssetAmt * assetER;

            unchecked {
                ++asset_idx;
            }
        }

        return totalETHInPool / rsEthSupply;
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Contract should have a removeAsset function

Assessed type

Other