Denial of Service (DOS) on `depositAsset`

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L132-L134 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L56-L58 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L47-L51 https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L79

Vulnerability details

Impact

Malicious user is able to DoS the function depositAsset, by increasing the balance of LRTDepositPool for an underlying asset.

Proof of Concept

There is a condition inside depositAsset which checks if the balance of the protocol for an underlying token is reached to the depositLimit (limit will be set by MANAGER role), then depositAssets doesn't let anyone to deposit that underlying asset:

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

So imagine this scenario (See also getAssetCurrentLimit(), getTotalAssetDeposits(), getAssetDistributionData() and you see how getAssetCurrentLimit depends on balance of LRTDepositPool):

• Assume the depositLimit of stETH is 10 (not real number, just an example).
• Attacker transfers 10 stETH directly to LRTDepositPool (without calling depositAsset, just through stETH contract).
• Now stETH.balanceOf(LRTDepositPool) returns 10 and also getTotalAssetDeposits() returns 10.
• getAssetCurrentLimit returns 0, because:

  lrtConfig.depositLimitByAsset(asset) - getTotalAssetDeposits(asset); // <--- (10 - 10) = 0

• Bob calls depositAssets and wants to mint some amount of rsETH.
• Bob gets a revert MaximumDepositLimitReached because the balance of protocol for asset stETH is reached to depositLimit and no one is able to mint any rsETH until the MANAGER calls updateAssetDepositLimit and set a higher limit.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a mapping:

mapping(address asset => uint256 amount)

which stores how much asset is deposited through depositAsset.

Assessed type

DoS

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

Attacker transfers 10 stETH directly to LRTDepositPool (without calling depositAsset, just through stETH contract). No one would do this non-economical attack because no shares were minted uinless you're doing this for donation attack as a first stacker.

[c4-judge]

fatherGoose1 marked the issue as unsatisfactory: Invalid