Closed c4-submissions closed 10 months ago
raymondfam marked the issue as insufficient quality report
raymondfam marked the issue as duplicate of #530
raymondfam marked the issue as duplicate of #711
raymondfam marked the issue as sufficient quality report
raymondfam marked the issue as high quality report
raymondfam marked the issue as not a duplicate
raymondfam marked the issue as primary issue
@serial-coder As it seems you aren't active on Discord, please be aware that no, you're not allowed to comment on issues until Post-judging QA, which you would know if you had read the documentation sent to you when you were granted backstage access. Please check the docs I have sent to you in discord and if possible, remove these comments until the appropriate time.
manoj9april (sponsor) disputed
We don't think this is a bug. Its okay if stETH token balance rebases at any point of time.
Edge case, and doesn't impose any risk to users. QA
fatherGoose1 changed the severity to QA (Quality Assurance)
fatherGoose1 marked the issue as grade-b
After deep review and discussion with the Kelp team, I do not see this as an issue. The rebasing will cause Kelp's stETH balance to increase, while the exchange rate will stay the same. This is how stETH performs yield generation. Contrast this to the other accepted LSTs, their balance stays the same while their exchange rate increases over time.
From the sponsor: For cbETH and rETH , the exchange rate increases corresponding to its staking yields. And for stETH, balance increases corresponding to its staking yield. Hence gradual price increase in rsETH due to stETH balance rebases is actually correct.
fatherGoose1 marked the issue as grade-c
Lines of code
https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L56-L58 https://github.com/code-423n4/2023-11-kelp/blob/f751d7594051c0766c7ecd1e68daeb0661e43ee3/src/LRTDepositPool.sol#L119-L144
Vulnerability details
Impact
Each asset (LST) intended for deposit must adhere to a deposit limit of 100,000e18. It's important to note that
stETH
, being a rebasing token, has a variable balance that can fluctuate (balance could go up and down).Due to the nature of stETH's balance changes, the
depositLimit
invariant may become inaccurate. Consequently, deposits may be temporarily blocked until thestETH
balance falls below the deposit limit or until the manager updates it.Proof of Concept
stETH has two rebasing scenarios, let's delve into each of them.
Positive Rebase:
If a user deposits stETH and it's close to reaching the deposit limit. On the next deposit, he will follow these steps:
getAssetCurrentLimit()
to determine the allowable deposit amount before reaching the asset deposit limit.X
(the amount of tokens they can deposit).depositAsset(stETH, X)
.getAssetCurrentLimit()
will revert due togetTotalAssetDeposits()
returning a value higher than 100,000e18.getAssetCurrentLimit()
, will be blocked.Here is a coded PoC:
Import
console
andStdUtils
at the top of theLRTDepositPoolTest.t.sol
file.Place the PoC in the
LRTDepositPoolDepositAsset
contract.Can be run with:
Negative Rebase
In a rare situation, the amount of
stETH
can decrease, called a negative rebase.There is a scenario where the asset limit is reached, but a negative rebase will make some extra space and whoever sees this will be able to mint additional
rsETH
.stETH
deposit limit.stETH
negative rebase allowing them to deposit an additional amount until they reach the limit again.rsETH
is minted for this specific user.Tools Used
Manual Review, Foundry
Recommended Mitigation Steps
It's challenging to provide a coded recommendation due to the inherent nature of the stETH token and its rebasing mechanism, which cannot be halted. However, I offer the following suggestion:
Consider using
wstETH
as it simplifies integration with DeFi and eliminates the rebasing functionality. This can provide a more stable and predictable environment for your application.Assessed type
ERC20