code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L534-L541

Vulnerability details

Impact

RdpxV2Core.lowerDepeg() and RdpxV2Core.upperDepeg() can be DoS'd by frontrunning with deposits/withdrawals/swaps on the Curve pools (and in the case of lowerDepeg() also the UniswapV2 pool) to cause an inequality constraint in RdpxV2Core._curveSwap() to be violated.

Proof of Concept

RdpxV2Core.lowerDepeg() burns dpxETH to restore the peg of dpxETH to wETH. It takes _rdpxAmount and _wethAmount from the reserves, swaps _rdpxAmount for amountOfWethOut on UniswapV2 and then swaps _wethAmount + amountOfWethOut for dpxETH on Curve, which is then burned.

The second swap is performed in RdpxV2Core._curveSwap() which validates that ethBalance + (_wethAmount + amountOfWethOut) <= (ethBalance + dpxEthBalance) / 2, where ethBalance and dpxEthBalance are the balances in the Curve pool.

The vulnerability is that all of the terms, except _wethAmount, in the above inequality can be manipulated by an attacker such that the inequality is violated, causing _curveSwap() to revert and thus also lowerDepeg(). Specifically the inequality can be broken by increasing ethBalance, by decreasing dpxEthBalance, or by increasing amountOfWethOut.

The attacker may deposit wETH in the Curve pool to increase ethBalance, or withdraw dxpETH to decrease dpxEthBalance, or effectively do both at the same time by swapping wETH for dpxETH. Or he may similarly manipulate the price in the UniswapV2 pool such that more amountOfWethOut is received than expected.

This purpose of the inequality check is supposedly to bring the two balances to equilibrium but not beyond, and one can therefore assume that lowerDepeg() will be called with values that will make it fall very close to the limit of equality. The attacker would therefore only need to use marginal amounts, and since lowerDepeg() will revert he can immediately undo his operation afterwards (e.g. as a sandwich), avoiding any loss due to slippage (up but some small fees).

A very similar situation obtains in RdpxV2Core.upperDepeg(), with the difference that only the Curve pool balances in the inequality can be manipulated (there is no first swap on UniswapV2).

Recommended Mitigation Steps

Instead of reverting, only use the amount that is needed and return the remaining funds to the reserves.

The attacker is effectively performing part of the peg restoration himself, which causes the intended full restoration to overshoot. So instead of simply reverting, this fix would incorporate the attacker's restorative contribution to the protocol's advantage. Thus the attack would have no greater effect on the peg than normal operations directly on the liquidity pools.

Note that the inequality is actually incorrect. This is reported separately as "_curveSwap() safety boundaries overshoot; MEV attack on peg restoration" and the proposed solution is simply to use another inequality. In this recommended mitigation the "amount that is needed" should therefore be defined by that new inequality.