Kelp DAO | rsETH - pipidu83's results

Lines of code

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

Vulnerability details

Impact

The function getAssetCurrentLimit can revert unexpectedly if getTotalAssetDeposits(asset) is greater than lrtConfig.depositLimitByAsset(asset), which can happen after updating deposit limits.

This vulnerability should be marked as MEDIUM as this is a public view function and could then be used by other contracts, which would rightly expect a return value equal to 0 in case more assets were deposited than the current limit, and potentially malfunction on this revert.

Another side effect of lesser severity is that the depositAsset will revert with no named reason when getAssetCurrentLimit reverts. It would be cleaner to catch this revert with a custom error.

Proof of Concept

Let's take a closer look at the getAssetCurrentLimit function below

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

First asset1 is initially added by LRTConstants.MANAGER (thanks to the addNewSupportedAsset function in the LRTConfig contract) with a depositLimitByAsset[asset1] set to 1e18.

Let's assume a user then deposits an amount 5e17 of asset1 in the depositAsset function.

Let's now assume LRTConstants.MANAGER then updates the depositLimitByAsset[asset1] with the updateAssetDepositLimit function in the LRTConfig contract to 0.

In this situation, the getTotalAssetDeposits function the returns 5e17 for asset1, and [depositLimitByAsset] is then equal to 0, leading the getAssetCurrentLimit function to revert when called with asset1 as an input.

Tools Used

Manual Review / Visual Studio

Recommended Mitigation Steps

This vulnerability can be fixed with a simple check in the getAssetCurrentLimit function as per below:

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

in this case the function will not revert and will simply return 0 when getTotalAssetDeposits(asset) is greater than lrtConfig.depositLimitByAsset(asset), preventing the potentially negative effects described above.

Assessed type

Under/Overflow