Open c4-submissions opened 11 months ago
raymondfam marked the issue as sufficient quality report
raymondfam marked the issue as duplicate of #39
raymondfam marked the issue as not a duplicate
raymondfam marked the issue as high quality report
raymondfam marked the issue as primary issue
gus-staderlabs (sponsor) confirmed
fatherGoose1 marked the issue as satisfactory
fatherGoose1 marked the issue as selected for report
Slippage control makes more sense in DEXes. But in staking protocols, where prices are mostly stable and users can reject if it txn takes longer time. But it is a useful recommendation. I think we can move it to QA.
manoj9april marked the issue as disagree with severity
similar to #102
Sticking with medium. There is no protection for the user in the current implementation. minShares
checks are commonly implemented in staking contracts.
Lines of code
https://github.com/code-423n4/2023-11-kelp/blob/c5fdc2e62c5e1d78769f44d6e34a6fb9e40c00f0/src/LRTDepositPool.sol#L119
Vulnerability details
Impact
The depositAsset function of the LRTDepositPool contract, enables users to deposit assets into the protocol, getting RSETH tokens in return. The function doesn't have any type of slippage control; this is relevant in the context of the depositAsset function, since the amount of tokens received by the user is determined by an interaction with an oracle, meaning that the amount received in return may vary indefinitely while the request is waiting to be executed.
Also the users will have no defense against price manipulations attacks, if they where to be found after the protocol's deployment.
Proof of Concept
As can be observed by looking at it's parameters and implementation, the depositAsset function of the LRTDepositPool contract, doesn't have any type of slippage protection:
Meaning that users have no control over how many RSETH tokens they will get in return for depositing in the contract.
The amount of tokens to be minted, with respect to the deposited amount, is determined by the getRsETHAmountToMint function of the same contract (inside of _mintRsETH):
As can be observed, this function uses two oracle interactions to determine how many tokens to mint, getAssetPrice and getRSETHPrice (with getRSETHPrice internally calling getAssetPrice as well).
The getAssetPrice function queries the external oracle for the asset price:
Meaning that the user has no way to predict how many RSETH they will get back at the moment of minting, as the price could be updated while the request is in the mempool.
Here is a very simple foundry script that shows that an oracle price modification, can largely impact the amount of tokens received in return by the user (implying that the depositAsset function has no protection against it). Place it in the
test
folder to preserve dependencies:Recommended Mitigation Steps
And additional parameter could be added to the depositAsset function, to let users decide the minimum amount of tokens to be received, with a relative check after minting.
Assessed type
Other