c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTDepositPool.sol#L56 https://github.com/code-423n4/2023-11-kelp/blob/main/src/LRTConfig.sol#L94

Vulnerability details

Impact

The function getAssetCurrentLimit() in LRTDepositPool can unexpectedly revert on underflow under certain circuimstances, breaking expected behaviour.

Proof of Concept

The function getAssetCurrentLimit() is responsible for returning the amount of asset the user can currently deposit. This is done by subtracting the current deposits from the current asset deposit limit and returning the result

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

    /// @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); // @audit this will revert unexpectedly if the limit is less than current deposits
    }

The asset deposit limit can be set by the admin in LRTConfig's updateAssetDepositLimit() function.

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

    /// @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);
    }

Observe that, if the deposit limit is being reduced there is no check on whether the current deposits are more than the deposit limit, meaning that the deposit limit can become less than the current deposit amount. If this happens the result of the subtraction will be negative and the function getAssetCurrentLimit() will revert on underflow.

Furthermore, an attacker can induce this bug by frontrunning a transaction that reduces the deposit limit. The attacker needs to deposit just over that new limit in a frontrunning transaction, breaking the behaviour for anyone calling getAssetCurrentLimit() with that same asset.

Tools Used

Manual Review

Recommended Mitigation Steps

Before doing the subtraction, check if lrtConfig.depositLimitByAsset(asset) < getTotalAssetDeposits(asset). If it is then return 0, otherwise return the result of lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset).

Assessed type

Under/Overflow

c4-pre-sort commented 11 months ago

raymondfam marked the issue as insufficient quality report

c4-pre-sort commented 11 months ago

raymondfam marked the issue as duplicate of #116

c4-judge commented 11 months ago

fatherGoose1 changed the severity to QA (Quality Assurance)