cergyk - ValantisModule::initializePosition Unlimited slippage can be incurred on initialization of position

[sherlock-admin3]

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 the module_.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

1. Executor wants to set a new module on a vault and calls setModule().
2. Attacker sees the transaction in the mempool and front-runs it, buying large amount of tokens from the pool.
3. Executor's transaction gets executed, effectively setting the new module and depositing the liquidity into the pool at an unbalanced price.
4. Attacker back-runs and makes a profit from their sandwich attack.

Code Snippet

• ValantisHOTModule.sol#L148

Tool used

Manual Review

Recommendation

Allow the executor to set slippage protections in their call to ValantisModule::initializePosition.

[CergyK]

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:

• 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 with StandardManager::rebalance which enables executor to set slippage checks. This, even though setModule acts as a rebalance between old and new module

[sherlock-admin3]

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:

• 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 with StandardManager::rebalance which enables executor to set slippage checks. This, even though setModule acts as a rebalance between old and new 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.

[0xjuaan]

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

[WangSecurity]

@CergyK do you have any counterarguments to the comment above?

[CergyK]

@WangSecurity

@0xjuaan is correct on this, this escalation can be invalidated

[WangSecurity]

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).

[WangSecurity]

Result:
Invalid
Unique

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• CergyK: rejected