Kelp DAO | rsETH - squeaky_cactus's results

Lines of code

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

Vulnerability details

Impact

The LRTDepositPool has the concept of a maximum deposit amount, where the sum of the deposit amount and current holding will not exceed the maximum amount for that asset, configurable in lrtConfig.

An attempt to deposit too large of an amount should result in a revert with the custom MaximumDepositLimitReached error, however under certain conditions the revert of Reason: Arithmetic over/underflow will be encountered.

The outcome is still a failed transaction when attempting to deposit above the maximum, but with a different error and message, which could potentially negatively impact apps and their UX.

Description

In LRTDepositPool.depositAsset there is a precondition check that the sum of depositAmount is smaller than the current limit for that asset, where failure results in a revert with the MaximumDepositLimitReached error in LRTDepositPool

src/LRTDepositPool.sol#132-134

        if (depositAmount > getAssetCurrentLimit(asset)) {
            revert MaximumDepositLimitReached();
        }

The current asset limit is the current deposit limit minus the total of the assets already deposited, which is the sum of the deposit pool, NDCs and EigenLayer asset holdings, from LRTDepositPool.

src/LRTDepositPool.sol#56-58

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

The deposit limit may be updated by a trusted LRTManger role with LRTConfig.updateAssetDepositLimit().

src/LRTConfig.sol#94-104

    function updateAssetDepositLimit(address asset, uint256 depositLimit)
        external
        onlyRole(LRTConstants.MANAGER)
        onlySupportedAsset(asset)
    {
        depositLimitByAsset[asset] = depositLimit;
        emit AssetDepositLimitUpdate(asset, depositLimit);
    }

Without any restrictions on the depositLimit, such as it being greater than the previous limit, or less than the current deposits, it may be set to less than the currently deposited assets.

If the depositLimit is ever less than the current assets deposited, then the subtraction in getAssetCurrentLimit will cause underflow, which is caught and handled in Solidity with a revert and message Reason: Arithmetic over/underflow.

This is still the case of the user attempting to deposit more than is allowed and the reasonable expectation is to receive the customMaximumDepositLimitReached error, rather than the generic over/underflow revert.

Proof of Concept

Two test cases that highlight the issue, the first directly calling getAssetCurrentLimit and the second as part of depositing. They are written with the expectation of passing, where currently they fail, on correct implementation they will pass.

Place the following test file into /test and run with forge test --match-contract LRTDepositPoolAssetLimitUnderflow

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 {LRTDepositPoolTest} from "./LRTDepositPoolTest.t.sol";

contract LRTDepositPoolAssetLimitUnderflow is LRTDepositPoolTest {
    address public stEthAddress;

    function setUp() public override {
        super.setUp();
        lrtDepositPool.initialize(address(lrtConfig));
        stEthAddress = address(stETH);
        vm.startPrank(admin);
        lrtConfig.grantRole(LRTConstants.MANAGER, manager);
        vm.stopPrank();
    }

    function test_GetAssetCurrentLimit_after_depositLimitReducedBelowAssets() external {
        // Alice deposits the full asset limit
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(stEthAddress, 10 ether);
        vm.stopPrank();

        // Deposit limit reduced below deposited
        vm.prank(manager);
        lrtConfig.updateAssetDepositLimit(stEthAddress, 5 ether);

        // @audit getAssetCurrentLimit() is causing a "Arithmetic over/underflow" revert, rather than returning zero
        assertEq(lrtDepositPool.getAssetCurrentLimit(stEthAddress), 0 ether, "No further deposit allowance should remain");
    }

    function test_RevertWhenDepositAmountExceedsLimit_depositLimitReducedBelowAssets() external {
        // Alice deposits the full asset limit
        vm.startPrank(alice);
        stETH.approve(address(lrtDepositPool), 10 ether);
        lrtDepositPool.depositAsset(stEthAddress, 6 ether);
        vm.stopPrank();

        // Deposit limit reduced below deposited
        vm.prank(manager);
        lrtConfig.updateAssetDepositLimit(stEthAddress, 5 ether);

        // @audit depositAsset() reverts with a "Arithmetic over/underflow" revert, rather than MaximumDepositLimitReached
        vm.expectRevert(ILRTDepositPool.MaximumDepositLimitReached.selector);
        vm.prank(alice);
        lrtDepositPool.depositAsset(stEthAddress, 2 ether);
        assertEq(lrtDepositPool.getAssetCurrentLimit(stEthAddress), 0 ether, "No further deposit allowance should remain");
    }
}

Results

Failing tests:
Encountered 2 failing tests in test/LRTDepositPoolAssetLimitUnderflow.t.sol:LRTDepositPoolAssetLimitUnderflow
[FAIL. Reason: Arithmetic over/underflow] test_GetAssetCurrentLimit_after_depositLimitReducedBelowAssets() (gas: 205972)
[FAIL. Reason: Error != expected error: NH{q != 0x1751ef83] test_RevertWhenDepositAmountExceedsLimit_depositLimitReducedBelowAssets() (gas: 211905)*/

Tools Used

Manual review, Foundry test

Recommended Mitigation Steps

To avoid underflow, only perform the subtraction when the total deposits is less than the limit in LRTDepositPool

src/LRTDepositPool.sol#56-58

    function getAssetCurrentLimit(address asset) public view override returns (uint256) {
-        return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
+        uint256 totalDeposits = getTotalAssetDeposits(asset);
+        uint256 depositLimit = lrtConfig.depositLimitByAsset(asset);
+        return totalDeposits > depositLimit ? 0 : depositLimit - totalDeposits;
    }

Assessed type

Under/Overflow