Kelp DAO | rsETH - 0xrugpull_detector's results

Lines of code

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

Vulnerability details

Here's attack scenario when rsETH contract is just deployed.

1. A hacker mint 1 wei of rsETH to become first depositor of new rsETH.
2. The hacker send large amount of asset token to DepositPool, thus inflate LRTOracle.getRSETHPrice().
3. Subsequent depositors will get nothing or less rsETH to be minted because of inflated RsETH price.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L151-L157

    function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

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

    function getRsETHAmountToMint(
        address asset,
        uint256 amount
    )
        public
        view
        override
        returns (uint256 rsethAmountToMint)
    {
        // setup oracle contract
        address lrtOracleAddress = lrtConfig.getContract(LRTConstants.LRT_ORACLE);
        ILRTOracle lrtOracle = ILRTOracle(lrtOracleAddress);

        // calculate rseth amount to mint based on asset amount and asset exchange rate
        rsethAmountToMint = (amount * lrtOracle.getAssetPrice(asset)) / lrtOracle.getRSETHPrice();
    }

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTOracle.sol#L52-L79

    function getRSETHPrice() external view returns (uint256 rsETHPrice) {
        ...
        for (uint16 asset_idx; asset_idx < supportedAssetCount;) {
            ...
            uint256 totalAssetAmt = ILRTDepositPool(lrtDepositPoolAddr).getTotalAssetDeposits(asset);
            ...
        }

        return totalETHInPool / rsEthSupply;
    }

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

    function getAssetDistributionData(address asset)
        public
        view
        override
        onlySupportedAsset(asset)
        returns (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer)
    {
        // Question: is here the right place to have this? Could it be in LRTConfig?
        assetLyingInDepositPool = IERC20(asset).balanceOf(address(this));
        ...
    }

Impact

First depositor of rsETH can steal subsequent depositor's fund.

Tools Used

Manual Review

Recommendation

LRTDepositPool._mintRsETH should mint specific amount to zero address when total supply is zero.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L151-L157

    function _mintRsETH(address _asset, uint256 _amount) private returns (uint256 rsethAmountToMint) {
+        if (IRSETH(rsethToken).totalSupply() == 0)
+            IRSETH(rsethToken).mint(address(0), MINIMUM_AMOUNT);    

        (rsethAmountToMint) = getRsETHAmountToMint(_asset, _amount);

        address rsethToken = lrtConfig.rsETH();
        // mint rseth for user
        IRSETH(rsethToken).mint(msg.sender, rsethAmountToMint);
    }

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L109-L122 https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L121-L124

Vulnerability details

Proof of Concept

Same strategy can be set for different assets like stETH or wstETH.

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

    function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)
    {
        UtilLib.checkNonZeroAddress(strategy);
        if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;
    }

In that case, getAssetBalance() might return sum of asset balances assigned to strategy.

https://github.com/code-423n4/2023-11-kelp/blob/main/src/NodeDelegator.sol#L121-L124

    function getAssetBalance(address asset) external view override returns (uint256) {
        address strategy = lrtConfig.assetStrategy(asset);
        return IStrategy(strategy).userUnderlyingView(address(this));
    }

Impact

It might cause double accounting of strategy balance.

Tools Used

Manual Review

Recommendation

Should check if strategy is already assigned to asset.

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

+   mapping(address strategy => address asset) public override strategyAsset;
    function updateAssetStrategy(
        address asset,
        address strategy
    )
        external
        onlyRole(DEFAULT_ADMIN_ROLE)
        onlySupportedAsset(asset)
    {
        UtilLib.checkNonZeroAddress(strategy);
        if (assetStrategy[asset] == strategy) {
            revert ValueAlreadyInUse();
        }
        assetStrategy[asset] = strategy;

+        if (strategyToAsset[strategy] == asset) {
+            revert ValueAlreadyInUse();
+        }
+        strategyToAsset[strategy] = asset;
    }

Assessed type

Invalid Validation