Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_curveSwap() wrong formula #1802

Closed
code423n4 opened this issue Sep 5, 2023 · 4 comments
Closed

_curveSwap() wrong formula #1802

code423n4 opened this issue Sep 5, 2023 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-1558 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality

Comments

@code423n4
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

RdpxV2Core._curveSwap An error in the formula for calculating the minimum amount of minOut for dpxEth may cause the slippage protection to fail.

Proof of Concept

_curveSwap()The code implementation is as follows:

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth,
    bool validate,
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
    IStableSwap dpxEthCurvePool = IStableSwap(addresses.dpxEthCurvePool);

    // First compute a reverse swapping of dpxETH to ETH to compute the amount of ETH required
    address coin0 = dpxEthCurvePool.coins(0);
    (uint256 a, uint256 b) = coin0 == weth ? (0, 1) : (1, 0);

    // validate the swap for peg functions
    if (validate) {
      uint256 ethBalance = IStableSwap(addresses.dpxEthCurvePool).balances(a);
      uint256 dpxEthBalance = IStableSwap(addresses.dpxEthCurvePool).balances(
        b
      );
      _ethToDpxEth
        ? _validate(
          ethBalance + _amount <= (ethBalance + dpxEthBalance) / 2,
          14
        )
        : _validate(
          dpxEthBalance + _amount <= (ethBalance + dpxEthBalance) / 2,
          14
        );
    }

    // calculate minimum amount out
    uint256 minOut = _ethToDpxEth
@>    ? (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

From the code above we know that if _ethToDpxEth=true uses the formula
minOut = (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))

but getDpxEthPrice() is the dpxEth price.

The correct formula should be _amount * 1e8 / getDpxEthPrice()

Tools Used

Recommended Mitigation Steps

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth,
    bool validate,
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
....

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


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

    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

Assessed type

Context

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Sep 5, 2023
code423n4 added a commit that referenced this issue Sep 5, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #2172

@c4-pre-sort
Copy link

bytes032 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added the sufficient quality report This report is of sufficient quality label Sep 12, 2023
@c4-pre-sort
Copy link

bytes032 marked the issue as duplicate of #970

@c4-judge c4-judge added duplicate-1558 satisfactory satisfies C4 submission criteria; eligible for awards and removed duplicate-970 labels Oct 18, 2023
@c4-judge
Copy link
Contributor

GalloDaSballo marked the issue as satisfactory

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-1558 satisfactory satisfies C4 submission criteria; eligible for awards sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

3 participants