search

code-423n4 / 2023-11-kelp-findings

13 stars 11 forks source link

KelpDAO generates less yield than it could do #64

Closed c4-submissions closed 10 months ago

c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L136-L138

Vulnerability details

Impact

Current implementation is following: 1) Users deposit to LRTDepositPool 2) Then manager manually transfers assets to NodeDelegator 3) And manager, again manually, deposits assets to EigenLayer

It was implemented to keep deposits open to LRTDepositPool while EigenLayer is paused. However users lose yield in current implementation because assets are not deposited immediately to EigenLayer. Yield is lost for timeframe between user's deposit and manual manager's deposit from NodeDelegator contract.

Proof of Concept

That's current design: user deposits to LRTDepositPool, then manager transfers asset to NodeDelegator manually, and then manager manually deposits to EigenLayer. Main reason is:

We are aware that EigenLayer has an mechanism to pause deposits in its protocol. Hence we created a layer called NodeDelegator which holds assets and deposits them into EigenLayer asset strategy. The intention is for these NodeDelegator contracts to deposit into Eigenlayer once deposit is available.

But described scenario should be executed only when EigenLayer is unavailable, otherwise deposit to EigenLayer should be performed immediately

Tools Used

Manual Review

Recommended Mitigation Steps

Refactor to following design:

Deposit to EigenLayer immediately in depositAsset() transaction. However wrap call to EigenLayer with try-catch, use current design flow with manual manager's deposit as fallback only, otherwise deposit is successful.

Assessed type

Other

c4-pre-sort commented 11 months ago

raymondfam marked the issue as insufficient quality report

raymondfam commented 11 months ago

Doc description mismatch but it's an intended design. QA at best because it also incurs fewer losses arising from slashings.

c4-pre-sort commented 11 months ago

raymondfam marked the issue as primary issue

c4-judge commented 10 months ago

fatherGoose1 marked the issue as unsatisfactory: Invalid

T1MOH593 commented 10 months ago

Why this issue was invalidated? There is no mismatch with doc, intended design is to keep deposits always open, not to use funds ineffectively most of the time. Intended design is to keep deposits opened in Kelp while EigenLayer paused deposits. And it works as intended when EigenLayer is paused.

However when EigenLayer is not paused (most of the time), Kelp doesn't receive yield for users' deposits due to this design. Because manager manually shall deposit funds to EigenLayer. Report highlights lost part of yield because of nature of such batch deposits.

fatherGoose1 commented 10 months ago

This report does not highlight a vulnerability, rather a design recommendation. @T1MOH593