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

The inequalities acting as safety boundaries in _curveSwap() are incorrect and allow for overshoot, i.e. upperDepeg() may lower the price below 1 and lowerDepeg() may raise the price above 1. Either the protocol itself causes the overshoot, or the attacker front-swaps the balances in the same direction so that the protocol's call then overshoots. The attacker can then profit by reversing the overshoot.

Proof of Concept

RdpxCoreV2.upperDepeg() can be called when the price of dpxETH/ETH > 1. It mints and swaps _amount dpxETH for wETH in _curveSwap() which requires that dpxEthBalance + _amount <= (ethBalance + dpxEthBalance) / 2. This is supposedly so that the pool's balance of dpxETH can be brought up to but not exceed its wETH balance, since the price is 1 precisely when the balances are equal. However, this fails to account for the fact that the bonding curve is convex, i.e. when (ethBalance - dpxEthBalance) / 2 (the maximum allowed here) of dpxETH is swapped, when dpxETH is more expensive (and thus dpxEthBalance < ethBalance), then more than this amount is received in wETH, which makes the wETH balance greater than the dpxETH balance. This implies that the price of dpxETH now is less than 1.

If the protocol itself overshoots an attacker can simply follow up with a swap towards equal balances, which then gives him dpxETH at a discount (which can e.g. then be sold off 1:1 for wETH as the peg is maintained). If the protocol doesn't intend to overshoot the attacker can still make it overshoot by frontrunning upperDepeg() with a swap of some dpxETH for (> some) wETH. Afterwards, a reverse swap of his received wETH will return more dpxETH than he spent in his initial swap since this trade is now performed at an improved price level.

An equivalent situation obtains in RdpxCoreV2.lowerDepeg().

Recommended Mitigation Steps

The current calculation of the equal balance point as the average of the balances is correct only for a linear bonding curve, i.e. a constant sum curve (or an infinite amplification factor in Curve's StableSwap). (For a constant product curve the balances would be equal at sqrt(ethBalance * dpxEthBalance).)

For the StableSwap curve the balances are equal at D/2. Unfortunately the pool's _get_D() is internal. But the external get_virtual_price() returns _get_D() * 1e18 / ERC20(self.lp_token).totalSupply() so D can be obtained from this. The correct inequality to use for upperDepeg() is therefore dpxEthBalance + _amount <= D/2, and for lowerDepeg() it is ethBalance + _amount <= D/2.

Note that the fact that _curveSwap() reverts when the inequality constraint is broken is in itself an issue (reported separately as "DoS of lowerDepeg() and upperDepeg()") since an attacker could simply cause it to revert instead of profiting from the overshoot. The solution proposed here eliminates the exploitable overshoot but therefore also reduces the margin against reverting. This is resolved in the other issue.