Closed sherlock-admin3 closed 1 month ago
Escalate
This issue should be unduplicated from #80 since context and mitigation are different.
For this issue in particular, even if deviation check is set:
setModule
is inconsistent with StandardManager::rebalance
which enables executor to set slippage checks. This, even though setModule
acts as a rebalance between old
and new
moduleEscalate
This issue should be unduplicated from #80 since context and mitigation are different.
For this issue in particular, even if deviation check is set:
- Since the whole funds are migrated to the new vault, a small manipulation of the spot price can cause a non-negligible loss for LPs.
- The behavior of
setModule
is inconsistent withStandardManager::rebalance
which enables executor to set slippage checks. This, even thoughsetModule
acts as a rebalance betweenold
andnew
module
You've created a valid escalation!
To remove the escalation from consideration: Delete your comment.
You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.
If the new module used a different pool to the old module, this would fail since the attacker would not be to front-run and swap in the pool before any liquidity was provided to the new pool via initializePosition()
Hence, the attack requires the same SovereignPool to be used by both modules. If the same SovereignPool is used by both the old and new module, how is there any profit for the attacker?
Attacker buys tokens, moving price from A->2A
Executor calls setModule, withdraws liquidity and deposits back (both actions at price 2A)
Then the attacker sells tokens, moving price from 2A->A
Where is the loss for LP? They deposit and withdraw at the exact same price
@CergyK do you have any counterarguments to the comment above?
@WangSecurity
@0xjuaan is correct on this, this escalation can be invalidated
Planning to reject the escalation and leave the issue as it is. But, if it #80 is validated, planning to de-dup this issue, since it's not a duplicate of #80 (but correct me if i'm wrong).
Result: Invalid Unique
cergyk
high
ValantisModule::initializePosition Unlimited slippage can be incurred on initialization of position
Summary
Unlimited slippage can be incurred on an executor initializing a position because the slippage protection is disabled (set to 0).
Vulnerability Detail
To set a new module on a vault, an executor has to call
ArrakisStandardManager::setModule
which withdraws the vault's funds from the pool and transfers to the new module: ArrakisMetaVault.sol#L109.Then the executor needs to call
ValantisModule::initializePosition
through themodule_.call(payloads_[i]);
to deposit the funds into the new pool: ValantisHOTModule.sol#L148.However, the slippage protection is disabled in the call to
HOT::depositLiquidity
, allowing unlimited slippage to be incurred to the executor when initializing the position (depositing) in the pool: HOT.sol#L667-L668.Impact
Loss of funds for the vault/LPs due to slippage.
Scenario
See Excalidraw diagram
setModule()
.Code Snippet
Tool used
Manual Review
Recommendation
Allow the executor to set slippage protections in their call to
ValantisModule::initializePosition
.