M-07 Unmitigated

[code423n4]

Lines of code

Vulnerability details

Mitigation of M-07: Issue NOT mitigated

Mitigated issue

M-07: Incorrect slippage check in the AMO2.rebalanceUp can be attacked by MEV Fix: https://github.com/code-423n4/2023-05-xeth/commit/630114ea6a3782f2f539b5936c524f8da62d0179

The issue is that since the pool is rebalanced around an imbalanced ratio with 68%-75% xETH the virtual price (which is valid for a 1:1 ratio) is underestimated, which means that the contractQuote in bestRebalanceUpQuote() will be too low, effectively having allowed for more slippage than intended.

Related issue - unable to rebalance down

A related potential issue is what happens in the case of rebalancing down. There the inverse holds: the slippage protection returns a contractQuote that is too high, which may then be the one chosen, which might cause the rebalancing down to revert. Thus the defender might be unable to rebalance down.

Mitigation review - get_virtual_price() is still wrong

The previous maxSlippageBPS has been replaced by upSlippage and downSlippage, both settable by the admin, which controls slippage in both directions independently. Previously the slippage was required to be between 0.06% and 15%, while it is now only required that it be less than 100%, which means that the remaining comment describing this is now incorrect.

Since the issue is that get_virtual_price() is inaccurate for imbalanced reserves, even if the slippage is zero the contractQuote might still be better than the defenderQuote and leave room for an MEV attack. Thus being able to set the slippage doesn't resolve this issue.

The related issue can be bypassed by allowing sufficient slippage when rebalancing down. Then the contractQuote can be made as low as needed. This is of course just to prevent reversion in rebalancing down, and the problem remains that get_virtual_price() is inaccurate.

Suggested mitigation steps

The correct price would be given by calc_withdraw_one_coin() for rebalancing up, and calc_token_amount() for rebalancing down, instead of by get_virtual_price(). But this may be manipulated. I suppose it would be possible to calculate these oneself based on the safer get_virtual_price(); a simple constant (but settable by the admin) multiplier to adjust it might be sufficient. But it might be better to avoid MEV attacks simply by not doing large rebalance operations, such that MEV attacks would not be profitable. There are already limits in place for this: rebalanceUpCap and rebalanceDownCap. If these can be made sufficiently small then MEV attacks can be deterred. However, because of the cooldown period after rebalancing this restriction might make the defender too slow. In that case some exemption must be made, essentially allowing large rebalance operations to be split in several smaller ones in immediate succession.

[c4-judge]

kirk-baird marked the issue as unmitigated

[c4-judge]

kirk-baird marked the issue as satisfactory