getDepositAssetCurrentLimit() may underflow if total asset deposits > deposit limit

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In LRTDepositPool.sol contract, there is a function called getAssetCurrentLimit() that calculates the difference between lrtConfig.depositLimitByAsset(asset) and getTotalAssetDeposits(asset). However, due to the possibility of increasing the total asset deposits above the deposit limit level, there is a chance that the function will underflow and, consequently, depositAsset() will revert as well.

Proof of Concept

getTotalAssetDeposits(asset) consists of 3 components: assetLyingInDepositPool, assetLyingInNDCs`:

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

function getTotalAssetDeposits(address asset) public view override returns (uint256 totalAssetDeposit) {
        (uint256 assetLyingInDepositPool, uint256 assetLyingInNDCs, uint256 assetStakedInEigenLayer) =
            getAssetDistributionData(asset);
        return (assetLyingInDepositPool + assetLyingInNDCs + assetStakedInEigenLayer);
    }

The problem is that only the assetLyingInDepositPool can be controlled in the system due to the depositAsset() check:

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

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

But assetLyingInNDCs and assetLyingInDepositPool can be increased due to the fact that the tokens can be sent to the node delegator contracts directly and without using transferAssetToNodeDelegator(). This can effectively lead to a situation where the limit may reach the level (even if it's not intentionally by the node delegator) where it's more than the current deposit limit for a specific type of asset. depositAsset() where getAssetCurrentLimit() is used will also underflow. Node delegators can also send tokens directly and put them into the strategy without using deposit pool functionality at all.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider changing the way of how total deposits are calculated.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #116

[c4-judge]

fatherGoose1 changed the severity to QA (Quality Assurance)

[c4-judge]

fatherGoose1 marked the issue as grade-b

[rodiontr]

I think it can easily be medium as it effectively allows to front-run every time the depositLimit is reached.

There can be even a scenario where one user deposits and after his deposit depositLimit is reached. And another user just front-runs him by directly sending tokens to the NDC or deposits himself and make his tx revert. This would allow implement this attack anytime when the depositLimit is almost reached and somebody will always lose gas money due to the tx revert.

So it’s "leak value with a hypothetical attack path with stated assumptions, but external requirements".

And if the issue #479 is considered as medium and it only shows “something that can happen in the future but it’s not the fact that it would happen”, and moreover, it’d be governance issue if the admins add such asset with different decimals. In contrary, this problem is already present in this code, why this is not treated as a medium?

thanks for reviewing! Would like to hear your thoughts on this

[fatherGoose1]

@rodiontr This does not warrant medium severity. It is not an "attack". A permissionless protocol does not care which user is depositing. It is not in their interest to protect the right for users' deposit transactions to successfully complete as opposed to a griefer.

On top of that, I struggle to see any incentive for a griefer to perform exercise this action. It's an expensive maneuver that is only present under certain conditions, with no benefit to the griefer.

[rodiontr]

@rodiontr This does not warrant medium severity. It is not an "attack". A permissionless protocol does not care which user is depositing. It is not in their interest to protect the right for users' deposit transactions to successfully complete as opposed to a griefer.

On top of that, I struggle to see any incentive for a griefer to perform exercise this action. It's an expensive maneuver that is only present under certain conditions, with no benefit to the griefer.

Thank you for the answer. I think there are plenty of gas-griefing cases with no beneifit to a griefer. It (this issue) just points out the losses for the user as every time the deposit limit is close enough, somebody can always lose money as their deposit will be front-runned and they will not be able to not only deposit but also they will lose their funds. In addition to this, the probability of something happening as the criteria should be secondary factor. If there is a deposit limit, it’s supposed to be reached multiple times or so for different assets and every time somebody will be gas-griefed.

And also about permissionlessness: I agree that protocol shouldn’t always care about how transactions are placed but it creates functionality (deposit limits) and therefore creates new attack vectors (even if they are not intentional) and it should be created in a way when users don’t lose anything. So if this problem can be mitigated multiple ways, why it should stay present ?