RdpxV2Core::_curveSwap calculates minOut variable incorrectly which will cause the protocol to lose money on Curve swaps

[code423n4]

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L544-L558
https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L273-L275

Vulnerability details

When dpxETH depegs, then either upperDepeg or lowerDepeg can be called in order to bring back dpxETH-ETH peg. Both functions internally call _curveSwap, which performs a swap on Curve. minOut is one of the parameters passed to Curve and is used in order to protect protocol from too big slippage. However, it is calculated incorrectly.

It is calculated in the following way:

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

Assume that 1 dpxETH = 1.25 ETH in order to illustrate that. Also, assume that slippageTolerance = 0.5%. Then, if we want to swap 1 ETH for dpxETH, minOut will equal 1.25 - 1.25 * 0.005 = 1.24375 (t's because getDpxEthPrice returns dpxETH price in ETH instead of the other way around). So, in the scenario when 1 dpxETH = 1.25 ETH <=> 0.8 dpxETH = 1 ETH, _curveSwap function will demand at least 1.24375 dpxETH for 1 ETH while the price of 1 ETH is 0.8 dpxETH.
In reality, we would like to swap dpxETH for ETH in case 1 dpxETH = 1.25 ETH, but it can be easily calculated that in that case, minOut = 0.796, instead of 1.24375 (we swap 1 dpxETH = 1.25 ETH and we only demand 0.796 ETH in return).

In the given example, minOut will equal 0.796 instead of 1.24375, which is 36.32% slippage instead of 0.5%. In reality, however, upperDepeg and lowerDepeg will probably be called when the dpxETH price is 1% off and in such a case, minOut = 0.985 instead of 1.005, which is ~2.5% slippage instead of 0.5%. Anyway, the slippage will be too big.

Note: Similar error is present in the ReLPContract::reLP function when mintokenAAmount is calculated for the second time (see the second link I've provided at the beginning).

Impact

Swaps on Curve will have incorrect minOut which will open up arbitrage opportunities and will make it possible that the protocol will just receive less money than it should each time swaps are performed.

Proof of Concept

Please set the function _curveSwap to public (not necessary, but will be easier to test this way, so that we don't have to call upperDepeg or lowerDepeg).
The point of this test is to show that minOut is calculated incorrectly and the test shows the scenario where we swap ETH for dpxETH where 1 dpxETH = 1.25 ETH (in reality, the opposite swap would be performed as I wrote earlier, but the point of this test is just to show that minOut is calculated incorrectly).

  function testCurveSlippage() public
  {
    // requires `RdpxV2Core::_curveSwap` to be set to public
    
    dpxEthPriceOracle.updateDpxEthPrice(125 * 1e6); // 1 dpxETH = 1.25 ETH, 1 ETH = 0.8 dpxETH
    weth.mint(address(rdpxV2Core), 1 ether);

    // Curve swap should give slightly less than 1 dpxETH for 1 ETH since the liquidity is still
    // - 200 dpxETH
    // - 200 WETH
    // we don't have to modify Curve pool liquidity so that it reflects the price change
    // the goal of this test is only to show that if 1 dpxETH if worth more than 1 ETH,
    // then `rdpxV2Core` will expect more than 1 dpxETH for 1 ETH, which is an incorrect behaviour
    // in the given example, if 1 dpxETH = 1.25 ETH, then we should expect to get at least 0.796 dpxETH
    // for 1 ETH, but `rdpxV2Core` will expect 1.24375

    // `rdpxV2Core` expects to get at least 1.24375 dpxETH for 1 ETH
    // we would get ~1 dpxETH since I didn't change ETH-dpxETH ratio in the pool, but `_curveSwap` will
    // still reject it
    vm.expectRevert("Exchange resulted in fewer coins than expected");
    rdpxV2Core._curveSwap(1 ether, true, false, 0);
    
    uint correctMinOut = 995 * dpxEthPriceOracle.getEthPriceInDpxEth() * 1e18 / (1000 * 1e8);
    console.log("correctMinOut =", correctMinOut);
    
    uint prevBalance = dpxETH.balanceOf(address(this));
    // but we should realistically expect to get at least 0.796 dpxETH
    IStableSwap(curvePool).exchange(0, 1, 1 ether, correctMinOut);
    console.log("received dpxETH:", dpxETH.balanceOf(address(this)) - prevBalance);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Change:

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

to:

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

Assessed type

Math

[c4-pre-sort]

bytes032 marked the issue as duplicate of #2172

[c4-pre-sort]

bytes032 marked the issue as not a duplicate

[c4-pre-sort]

bytes032 marked the issue as primary issue

[c4-pre-sort]

bytes032 marked the issue as high quality report

[c4-sponsor]

witherblock (sponsor) confirmed

[c4-judge]

GalloDaSballo marked issue #1558 as primary and marked this issue as a duplicate of 1558

[c4-judge]

GalloDaSballo marked the issue as satisfactory