RdpxV2Core._curveSwap incorrectly calculates minOut, inviting MEV attack

[code423n4]

Lines of code

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

Vulnerability details

Impact

The logics to compute minOut based on _ethToDpxEth direction are put in wrong order. Put it in details, when _ethToDpxEth is True, minOut is supposed to be minDpxEthOut which should be computed by multiplying ethAmountIn with ethPrice instead of DpxEthPrice as depicted at Lines 546-547. We can estimate how much smaller the minOut is compared to the correct one via following equation:

wrong minOut / right minOut
= (_amount * getDpxEthPrice()) / 1e8) / (_amount * getEthPrice()) / 1e8)
= (getDpxEthPrice() ** 2) / 1e16

This means, for every x% drop of getDpxEthPrice, we would trade with an compounded drop of (x^2)% below market price. This will open the trade to MEV attack.

In the case of lowerDepeg, if certain conditions are met, an attacker can sandwich the transaction with a pump-and-dump of DpxEth, resulting in a suboptimal amount of DpxEth tokens received from the trade which makes the re-pegging process cost more resources.

In the case of upperDepeg, similar conditions can be exploited, allowing an attacker to manipulate the Eth price.

Proof of Concept

Scenario for lowerDepeg:

1. Alice initiates a lowerDepeg transaction with specific conditions met: getDpxEthPrice < 1e8 and _ethToDpxEth = true.

2. An attacker, Bob, monitors the pending transactions and identifies Alice's lowerDepeg transaction.

3. Bob quickly submits a transaction that sandwiches Alice's transaction with a pump-and-dump of DpxEth, manipulating the price.

4. Alice's transaction goes through but with a suboptimal amount of DpxEth tokens received, making the re-peg less efficient.

Scenario for upperDepeg:

1. Alice initiates an upperDepeg transaction with specific conditions met: getEthPrice < 1e8.

2. Bob, the attacker, spots Alice's transaction.

3. Bob quickly submits a transaction that manipulates the Eth price, making Alice's transaction suboptimal in terms of Eth received.

4. Alice's transaction results in receiving fewer Eth tokens than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, consider switching the logic in the _ethToDpxEth cases to ensure that the minAmount is set correctly for both lowerDepeg and upperDepeg functions.

  uint256 minOut = _ethToDpxEth
      ? (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));

Assessed type

MEV

[c4-pre-sort]

bytes032 marked the issue as duplicate of #2172

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-pre-sort]

bytes032 marked the issue as duplicate of #970

[c4-judge]

GalloDaSballo marked the issue as nullified

[GalloDaSballo]

Frontrunning weirdness

[hungdoo]

According to Judge's comment on this issue #496 , I believe this issue can also fall into the same category of race condition.
On the other hand, the MEV attack is an example to illustrate the exploitation of the false slippage protection. Even though MEV is not qualified for Arbitrum, I remembered it was confirmed by the Sponsor to be acceptable.

Front-running is not a pre-requisite for this, an attacker and the sponsor may be in a race condition which is outside of either control, meaning this is plausible

[c4-judge]

GalloDaSballo marked the issue as partial-50

[GalloDaSballo]

Formula is correct

Explanation is not