Kelp DAO | rsETH - evmboi32's results

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTOracle.sol#L70 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L47-L51 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L79-L88

Vulnerability details

Impact

Anyone could break the rsETH price by directly sending tokens to LRTDepositPool or NodeDelegator. Users can receive less ether than intended for their contribution.

Proof of Concept

• Let's take a look at how the price is calculated

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

• To get the amount of assets on the contract it uses getTotalAssetDeposits function which calls the getAssetDistributionData

function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
    (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
        getAssetDistributionData(asset);
    return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
}

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

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

• Here the assetLyingInDepositPool and assetLyingInNDCs are retrieved directly by checking the balance of the contract.
• So let's say that rsEthSupply = 10 ETH and there is only 1 asset in the deposit pool cbETH and the balance of the LRTDepositPool = 10 ETH.
• So now the price of rsETH is calculated as follows return totalETHInPool / rsEthSupply;
• Thi results in 10 ETH / 10 ETH = 1 ETH which is correct. (totalETHInPool is the sum of all assets in the pool, but in this example, we only have cbETH as the supported asset.)
• But if someone was to suddenly send 30 ETH to the LRTDepositPool the result would be 40 / 10 = 4 ETH (totalETHInPool would now be 40 as the assetLyingInDepositPool increases, but rsEthSupply still remain at 10).
• This means that 1 rsETH is worth 4 ETH. So when depositing to a new strategy users will get 4x less eth than intended. (eg. they need to deposit 4 ETH to the depositPool to get 1 rsETH back).

Tools Used

VS Code

Recommended Mitigation Steps

It is not smart to rely on a balance that is retrieved by IERC20(asset).balanceOf function. Instead, use a state variable that increases every time a user deposits to LRTDepositPool and a variable that increases when tokens are sent to delegator.

LRTDepositPool

// state variable
+   mapping(address => uint256) public assetsDeposited;

function depositAsset(
    address asset,getAssetDistributionData
    uint256 depositAmount
)
    external
    whenNotPaused
    nonReentrant
    onlySupportedAsset(asset)
{
    // checks
    if (depositAmount == 0) {
        revert InvalidAmount();
    }
    if (depositAmount > getAssetCurrentLimit(asset)) {
        revert MaximumDepositLimitReached();
    }

    if (!IERC20(asset).transferFrom(msg.sender, address(this), depositAmount)) {
        revert TokenTransferFailed();
    }

+   assetsDeposited[asset] += depositAmount;
    // interactions
    uint256 rsethAmountMinted = _mintRsETH(asset, depositAmount);

    emit AssetDeposit(asset, depositAmount, rsethAmountMinted);
}

function transferAssetToNodeDelegator(
    uint256 ndcIndex,
    address asset,
    uint256 amount
)
    external
    nonReentrant
    onlyLRTManager
    onlySupportedAsset(asset)
{
    address nodeDelegator = nodeDelegatorQueue[ndcIndex];
    if (!IERC20(asset).transfer(nodeDelegator, amount)) {
        revert TokenTransferFailed();
    }
+   assetsDeposited[asset] -= amount;
+   nodeDelegator.increaseAssetsDeposited(asset, amount);
}

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));
+   assetLyingInDepositPool = assetsDeposited[asset];


    uint256 ndcsCount = nodeDelegatorQueue.length;
    for (uint256 i; i < ndcsCount;) {
-       assetLyingInNDCs += IERC20(asset).balanceOf(nodeDelegatorQueue[i]);
+       assetLyingInNDCs += nodeDelegatorQueue[i].assetsDeposited[asset];

        assetStakedInEigenLayer += INodeDelegator(nodeDelegatorQueue[i]).getAssetBalance(asset);
        unchecked {
            ++i;
        }
    }
}

// state variable
+   function increaseAssetsDeposited(
+       address asset,
+       uint256 amount
+   ) external
+     onlyNodeDelegator 
+   {
+         assetsDeposited[asset] += amount;
+   }

NodeDelegator

// state variable
+   mapping(address => uint256) public assetsDeposited;

+   function increaseAssetsDeposited(
+       address asset,
+       uint256 amount
+   ) external
+     onlyLRTDepositPool 
+   {
+         assetsDeposited[asset] += amount;
+   }

function transferBackToLRTDepositPool(
    address asset,
    uint256 amount
)
    external
    whenNotPaused
    nonReentrant
    onlySupportedAsset(asset)
    onlyLRTManager
{
    address lrtDepositPool = lrtConfig.getContract(LRTConstants.LRT_DEPOSIT_POOL);
    
+   assetsDeposited[asset] -= amount;
+   lrtDepositPool.increaseAssetsDeposited(asset, amount);

    if (!IERC20(asset).transfer(lrtDepositPool, amount)) {
        revert TokenTransferFailed();
    }
}

Assessed type

Other