Kelp DAO | rsETH - niser93's results

Lines of code

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

Vulnerability details

Impact

An erroneus deposit limit setting inside LRTConfig could block deposit functionalities inside LRTDepositPool due to Overflow error.

Let's we see LRTDepositPool.sol#L53-L58

/// @notice gets the current limit of asset deposit
/// @param asset Asset address
/// @return currentLimit Current limit of asset deposit
function getAssetCurrentLimit(address asset) public view override returns (uint256) {
    return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
}

So, getAssetCurrentLimit() simple compute a difference between lrtConfig.depositLimitByAsset() and getTotalAssetDeposits(), with respect to asset parameter. However, while totalAssetDeposit can never be more then assetDepositLimit, thanks to check inside depositAsset() at LRTDepositPool.sol#L132-L134, an account with MANAGER role could accidentally set the assetDepositLimit with a value lower than current totalAssetDeposit, using LRTConfig.sol#L91-L104

/// @dev Updates the deposit limit for an asset
/// @param asset Asset address
/// @param depositLimit New deposit limit
function updateAssetDepositLimit(
    address asset,
    uint256 depositLimit
)
    external
    onlyRole(LRTConstants.MANAGER)
    onlySupportedAsset(asset)
{
    depositLimitByAsset[asset] = depositLimit;
    emit AssetDepositLimitUpdate(asset, depositLimit);
}

Proof of Concept

We write own test inside LRTDepositPoolTest.t.sol/LRTDepositPoolGetAssetCurrentLimit

function test_zzzGetAssetCurrentLimitAfterAssetIsDeposited() external {
    vm.startPrank(manager);
    lrtConfig.updateAssetDepositLimit(address(stETH), 10 ether);
    vm.stopPrank();

    // deposit 1 ether stETH
    vm.startPrank(alice);
    stETH.approve(address(lrtDepositPool), 6 ether);
    lrtDepositPool.depositAsset(address(stETH), 6 ether);
    vm.stopPrank();

    assertEq(lrtDepositPool.getAssetCurrentLimit(address(stETH)), 4 ether, "Asset current limit is not set");

    vm.startPrank(manager);
    lrtConfig.updateAssetDepositLimit(address(stETH), 4 ether);
    vm.stopPrank();

    lrtDepositPool.getAssetCurrentLimit(address(stETH));
}

The result is

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 5.51ms
 
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/LRTDepositPoolTest.t.sol:LRTDepositPoolGetAssetCurrentLimit
[FAIL. Reason: Arithmetic over/underflow] test_zzzGetAssetCurrentLimitAfterAssetIsDeposited() (gas: 215238)

Tools Used

Visual ispection and Foundry

Recommended Mitigation Steps

It should be better to avoid an arbitrary update of depositLimit by MANAGER. Otherwise, we could use:

/// @notice gets the current limit of asset deposit
/// @param asset Asset address
/// @return currentLimit Current limit of asset deposit
function getAssetCurrentLimit(address asset) public view override returns (uint256) {
    if(lrtConfig.depositLimitByAsset(asset) >= getTotalAssetDeposits(asset))
        return lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset);
    else
        return 0;
}

Assessed type

Under/Overflow