Incorrect slippage check in the AMO2.rebalanceUp can be attacked by MEV

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L323-L325 https://github.com/code-423n4/2023-05-xeth/blob/main/src/AMO2.sol#L335-L351

Vulnerability details

Impact

The AMO2.rebalanceUp uses AMO2.bestRebalanceUpQuote function to avoid MEV attack when removing liquidity with only one coin. But the bestRebalanceUpQuote does not calculate the slippage correctly in this case, which is vulnerable to be attacked by MEV sandwich.

Proof of Concept

AMO2.rebalanceUp can be executed sucessfully only if the preRebalanceCheck returns true.

        bool isRebalanceUp = preRebalanceCheck();
        if (!isRebalanceUp) revert RebalanceUpNotAllowed();

So as the logic in the preRebalanceCheck, if isRebalanceUp = ture, the token balances in the curve should satisfy the following condition

xETHBal / (stETHBal + xETHBal) > REBALANCE_UP_THRESHOLD 

The default value of REBALANCE_UP_THRESHOLD is 0.75 :

uint256 public REBALANCE_UP_THRESHOLD = 0.75E18;

It's always greater than 0.5 . So when removing liquidity for only xETH, which is the rebalanceUp function actually doing, the slippage will be positive instead of negative. You will receive more than 1 uint of xETH when you burn the lp token of 1 virtual price.

But the slippage caculation is incorrect in the bestRebalanceUpQuote:

    // min received caculate in bestRebalanceUpQuote
        bestQuote.min_xETHReceived = applySlippage(
            (vp * defenderQuote.lpBurn) / BASE_UNIT
        );

    // applySlippage function
    function applySlippage(uint256 amount) internal view returns (uint256) {
        return (amount * (BASE_UNIT - maxSlippageBPS)) / BASE_UNIT;
    }

It uses the target amount subtracted slippage as the min token amount received. It is equivalent to expanding the slippage in the opposite direction. It makes the contractQuote can't protect the defenderQuote, increasing the risk of a large sandwich.

Tools Used

Manual review

Recommended Mitigation Steps

rebalanceUp and rebalanceDown should use different slippage calculation methods in the applySlippage.

Assessed type

MEV

[c4-judge]

kirk-baird marked the issue as primary issue

[d3e4]

Whether you will receive more or less xETH per LP token is accounted for in the price vp. (vp * defenderQuote.lpBurn) / BASE_UNIT is the amount of xETH received when burning lpBurn LP tokens. We want to receive as much xETH as possible, but at least applySlippage of that, e.g. 99%. It doesn't matter what direction we are exchanging; we always want to receive as much as possible for whatever fix amount we provide. So the slippage should always be calculated as less than 100% of the ideal amount received.

[c4-sponsor]

vaporkane marked the issue as sponsor disputed

[kirk-baird]

I agree with the comments that slippage is calculated as a percentage of the received token. This is the desirable behaviour and therefore this issue is invalid.

[c4-judge]

kirk-baird marked the issue as unsatisfactory: Invalid

[5z1punch]

Whether you will receive more or less xETH per LP token is accounted for in the price vp. (vp * defenderQuote.lpBurn) / BASE_UNIT is the amount of xETH received when burning lpBurn LP tokens. We want to receive as much xETH as possible, but at least applySlippage of that, e.g. 99%. It doesn't matter what direction we are exchanging; we always want to receive as much as possible for whatever fix amount we provide. So the slippage should always be calculated as less than 100% of the ideal amount received.

In fact, the price vp only accounts the token amounts when the peg pool is balance, which means stETH:xEth is 1:1 in the pool. The case in the comment is a from the point of a defi user instead of a rebalance mechanism. The REBALANCE_UP_THRESHOLD = 0.75 , and REBALANCE_DOWN_THRESHOLD = 0.68 . So when calling rebalanceUp, there must be more than 75% xETH in the pool and after the rebalance, there should be more than 68% xETH in the pool. Otherwise the pool is unbalanced again. So I you use vp minus slippage as the contractQuote, the rebalance up can reduce the xETH proportion to lower than 50%, which makes the pool fall into the rebalance down process.

[5z1punch]

Aha, I find a similar issue submitted by other wanden https://github.com/code-423n4/2023-05-xeth-findings/issues/35. This issue is the root cause and solution for the https://github.com/code-423n4/2023-05-xeth-findings/issues/35.

Actually, rebalanceDown can't leap the boundary about REBALANCE_UP_THRESHOLD if slippage is appropriate. Because the rebalanceDown mints more xETH and adds it to the pool. If it adds too much xETH, the lp minted will lower than the slippage allows.

But for the rebalanceUp, the slippage protects nothing because of incorrect calculation method.

[5z1punch]

Hi @kirk-baird , just to make sure you don't miss the issue

[kirk-baird]

@vaporkane you're input would be valuable if that's okay.

Okay so the first point I am taking out of this is

In fact, the price vp only accounts the token amounts when the peg pool is balance, which means stETH:xEth is 1:1 in the pool.

Does vp * defenderQuote.lpBurn / BASE_UNIT always give us the correct value of xETH that would be received when burning lpBurn amount of LP tokens?

[romeroadrian]

Aha, I find a similar issue submitted by other wanden #35. This issue is the root cause and solution for the #35.

Actually, rebalanceDown can't leap the boundary about REBALANCE_UP_THRESHOLD if slippage is appropriate. Because the rebalanceDown mints more xETH and adds it to the pool. If it adds too much xETH, the lp minted will lower than the slippage allows.

But for the rebalanceUp, the slippage protects nothing because of incorrect calculation method.

I can't see how this is linked to #35.

When the pool is unbalanced you won't get exactly the virtual price amount when you redeem it as one coin. This would imply that there's always 1:1 relation which is not what curve does. But I think it is fair to use it as a slippage check, which is the sponsor's intention here.

What the warden is trying to argue is that, let's say that virtual price is 1.05 so 1 LP should be redeemed as 1.05. Due to imbalance (rebalance up should require 75% of xETH using the default settings), if you redeem it as one coin (xETH) you won't actually get 1.05, you will get a bit more because you are removing the coin which has more liquidity. Note that we are talking about curve's stable pool model, where balance differences are eased.

Note also that this is a safeguard measure against a defender supplied parameter. The calculation is only used if the argument provided by the off chain process ends up being lower than the quote from calculation, which means that the calculation is just a lower bound safety measure.

[kirk-baird]

Thanks for the feedback @romeroadrian. To summarise this issue there is a miscalculation in the slippage protection which could allow for a small sandwich attack. The conditions are that a defender must send a transaction with the minimum xETH received to be significantly lower than the real price in which case the applySlippage() lower bound will be taken rather than the provided minimum. Since there is a bug in the applySlippage() calculations this may result in a sandwich attack.

I'm going to reinstate this issue as medium severity as there is a small bug in the slippage protection which under niche circumstances could result in a sandwich attack.

Noting, although related to #35 I do not see this as a direct duplicate since it is a slippage attack rather than over rebalancing.

[c4-judge]

kirk-baird marked the issue as satisfactory

[c4-judge]

kirk-baird marked the issue as selected for report

[romeroadrian]

Thanks for the feedback @romeroadrian. To summarise this issue there is a miscalculation in the slippage protection which could allow for a small sandwich attack. The conditions are that a defender must send a transaction with the minimum xETH received to be significantly lower than the real price in which case the applySlippage() lower bound will be taken rather than the provided minimum. Since there is a bug in the applySlippage() calculations this may result in a sandwich attack.

I'm going to reinstate this issue as medium severity as there is a small bug in the slippage protection which under niche circumstances could result in a sandwich attack.

Noting, although related to #35 I do not see this as a direct duplicate since it is a slippage attack rather than over rebalancing.

I'm not sure I would call this a bug, as the intention is not to prevent MEV (which is already mitigated by the min amount) but to prevent a rogue or malfunctioning defender. Just making sure my previous comment intention is fully understood, I'll leave the final decision (severity) to you.