Platform: Code4rena
Start Date: 21/08/2023
Pot Size: $125,000 USDC
Total HM: 26
Participants: 189
Period: 16 days
Judge: GalloDaSballo
Total Solo HM: 3
Id: 278
League: ETH
Rank: 5/189
Findings: 5
Award: $3,040.21
π Selected for report: 3
π Solo Findings: 0
π Selected for report: LokiThe5th
Also found by: Nikki, __141345__, mahdikarimi, peakbolt, rvierdiiev, wintermute
1642.2054 USDC - $1,642.21
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:
PerpetualAtlanticVaultLP
allows redemptions at any time as long as there is excess collateral availableThis 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:
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).As the price-threshold for settlement is known, depositors into the PerpetualAtlanticVaultLP
have a few options to avoid this loss:
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.
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.
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
Manual code review. Foundry tests.
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.
DoS
#0 - c4-pre-sort
2023-09-10T07:31:45Z
bytes032 marked the issue as high quality report
#1 - c4-pre-sort
2023-09-15T13:16:59Z
bytes032 marked the issue as primary issue
#2 - c4-sponsor
2023-09-25T15:58:49Z
psytama (sponsor) confirmed
#3 - GalloDaSballo
2023-10-17T17:37:40Z
If you're ITM and you can redeem, then you should
if you're OTM then you can't redeem and you'll get settled against
I'm not convinced this is valid
#4 - GalloDaSballo
2023-10-20T14:24:39Z
After re-reading 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, I'll need to recheck all submissions as I think some dups of this have been missed
#5 - c4-judge
2023-10-20T14:24:45Z
GalloDaSballo marked the issue as selected for report
#6 - c4-judge
2023-10-20T14:26:30Z
GalloDaSballo changed the severity to 2 (Med Risk)
#7 - c4-judge
2023-10-20T14:27:43Z
GalloDaSballo changed the severity to 3 (High Risk)
#8 - GalloDaSballo
2023-10-20T14:28:11Z
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
π Selected for report: klau5
Also found by: 0x3b, 0xCiphky, 0xDING99YA, 0xWaitress, 0xbranded, 0xc0ffEE, 0xklh, 0xsurena, 0xvj, ABA, AkshaySrivastav, Anirruth, Aymen0909, Baki, Blockian, BugzyVonBuggernaut, DanielArmstrong, Evo, GangsOfBrahmin, HChang26, Inspex, Jiamin, Juntao, Kow, Krace, KrisApostolov, LFGSecurity, LokiThe5th, Mike_Bello90, Norah, Nyx, QiuhaoLi, RED-LOTUS-REACH, SBSecurity, Snow24, SpicyMeatball, T1MOH, Tendency, Toshii, Udsen, Yanchuan, __141345__, ak1, asui, auditsea, ayden, bart1e, bin2chen, blutorque, carrotsmuggler, chaduke, chainsnake, circlelooper, clash, codegpt, crunch, degensec, dirk_y, ge6a, gjaldon, grearlake, jasonxiale, juancito, ke1caM, kodyvim, kutugu, ladboy233, lanrebayode77, mahdikarimi, max10afternoon, mert_eren, nirlin, nobody2018, oakcobalt, parsely, peakbolt, pks_, pontifex, ravikiranweb3, rokinot, rvierdiiev, said, savi0ur, sces60107, sh1v, sl1, spidy730, tapir, tnquanghuy0512, ubermensch, visualbits, volodya, wintermute
0.0098 USDC - $0.01
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/core/RdpxV2Core.sol#L764-L783
Due to a strict balanceOf
requirement the settlement of put options can be blocked.
The PerpetualAtlanticVaultLP.sol contract holds the collateral that is used for writing put contract options to the RdpxV2Core.sol contract. The ability of the core contract to protect itβs positions through perpetual put options is critical to maintaining the price of DpxEthToken
.
By transferring 1 wei
of WETH to the PerpetualVaultLP.sol contract, an attacker can block the settle()
function call in the RdpxV2Core.sol
contract.
Due to the negligible cost to execute this attack (gas + 1 wei of the collateralToken
), and the severe impact it has on the token economic system of the project (put options are not executable, this no hedge against price movements), this issue has been evaluated to high severity.
In the PerpetualAtlanticVaultLP.sol
contract, the function substractLoss()
has a strict requirement which uses the underlying balanceOf
the collateral token. See here:
function subtractLoss(uint256 loss) public onlyPerpVault { require( collateral.balanceOf(address(this)) == _totalCollateral - loss, "Not enough collateral was sent out" ); _totalCollateral -= loss; }
In the require
statement the balanceOf(address(this))
must be strictly equal to _totalCollateral - loss
. This makes the contract vulnerable to denial of service attacks through direct transfers, which will cause the aforementioned check to always fail. In addition, there is no way to clear this excess collateral.
The below coded PoC can be placed inside the tests/perp-vault
directory of the contest and can be run with forge test --match-contract PoCNoSettlement
. It shows how the RdpxV2Core.sol
contract's calls to PerpetualAtlanticVault::settle()
revert after the amount of 1 wei
has been transferred directly to the PerpetualAtlanticVaultLP
contract. The PoC also has a control scenario for reference. Note: the revert scenario is explicitly checked, so both tests should pass
// SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.19; import { Test } from "forge-std/Test.sol"; import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol"; import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import { Setup } from "./Setup.t.sol"; import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol"; contract PoCNoSettlement is ERC721Holder, Setup { // ================================ HELPERS ================================ // function deposit(uint256 _amount, address _from) public { vm.startPrank(_from, _from); 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); vaultLp.deposit(_amount, address(1)); vm.stopPrank(); } // ================================ POC ================================ // function testSettlementControl() public { // Setup weth.mint(address(1), 1 ether); deposit(1 ether, address(1)); vault.purchase(1 ether, address(this)); uint256[] memory ids = new uint256[](1); ids[0] = 0; uint256[] memory strikes = new uint256[](1); strikes[0] = 0.015 gwei; skip(86500); // expire /** Control scenario flow: price moves -> puts in the money -> call to `settle` -> rdpx from address(this) to LP -> WETH from LP to address(this) */ // Update the rdpxPrice so that the puts are in the money priceOracle.updateRdpxPrice(0.010 gwei); uint256 wethBalanceBefore = weth.balanceOf(address(this)); uint256 rdpxBalanceBefore = rdpx.balanceOf(address(this)); // Settle the put options, we do not expect revert vault.settle(ids); uint256 wethBalanceAfter = weth.balanceOf(address(this)); uint256 rdpxBalanceAfter = rdpx.balanceOf(address(this)); // Check that the put option was settled assertEq(wethBalanceAfter - wethBalanceBefore, 0.15 ether); // asset settled; 1 rdpx worth of ether received by rdpxV2Core assertEq(rdpxBalanceBefore - rdpxBalanceAfter, 1 ether); // asset settled; 1 rdpx sent from rdpxV2Core to lp } // Attack Scenario function testNoSettlement() public { // Setup the attack scenario, same as control weth.mint(address(1), 1 ether); deposit(1 ether, address(1)); vault.purchase(1 ether, address(this)); uint256[] memory ids = new uint256[](1); ids[0] = 0; uint256[] memory strikes = new uint256[](1); strikes[0] = 0.015 gwei; skip(86500); // expire // We update the price so the puts are in the money priceOracle.updateRdpxPrice(0.010 gwei); // This is the attack, key difference from control scenario weth.transfer(address(vaultLp), 1 wei); // We test the hypothesis: // We expect the call to `settle` will be reverted with: "Not enough collateral was sent out" vm.expectRevert("Not enough collateral was sent out"); vault.settle(ids); // The call is reverted and the hypothesis, that direct transfers will prevent settlement, is accepted. } }
Manual review. Foundry.
An option would be to set the _totalCollateral
to equal collateral.balanceOf(address(this))
before the require
statement. This still supports the strict equality check.
From the code there is no reason to expect excess collateralToken
should be present in the contract, because both addProceeds
and deposit
ensure that it is added to _totalCollateral
in any case.
DoS
#0 - c4-pre-sort
2023-09-09T09:57:59Z
bytes032 marked the issue as duplicate of #619
#1 - c4-pre-sort
2023-09-11T16:15:07Z
bytes032 marked the issue as sufficient quality report
#2 - c4-judge
2023-10-20T19:34:32Z
GalloDaSballo marked the issue as satisfactory
π Selected for report: LokiThe5th
Also found by: 0xPsuedoPandit, 0xTiwa, 0xnev, 0xvj, Evo, Jiamin, Juntao, QiuhaoLi, T1MOH, Udsen, circlelooper, crunch, eeshenggoh, gjaldon, hals, josephdara, kutugu, minhtrng, niki, umarkhatab_465
125.228 USDC - $125.23
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 contest 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:
Contract | Function | Effect |
---|---|---|
rdpxV2Core.sol | getRdpxPrice() | Returns an 1e18 value when 1e8 expected |
calculateBondCost() | Deflates the rdpxRequired | |
calculateAmounts() | Inflates the rdpxRequiredInWeth | |
_transfer() | Inflates rdpxAmountInWeth and may cause possible underflow | |
UniV2LiquidityAmo | getLpPriceInEth() | Overestimates the lp value |
ReLp.sol | reLP() | Inflates min token amounts |
PerpetualAtlanticVault.sol | getUnderlyingPrice() | Returns 1e18 instead of 1e8 |
calculatePremium() | Inflates the premium calculation |
The RdpxEthPriceOracle.sol
file can be found here
It exposes the following functions used in the contests:
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 contest 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 contest 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.
Manual review. Communicated with sponsor to confirm RdpxEthPriceOracle
.
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.
Oracle
#0 - c4-pre-sort
2023-09-09T04:53:01Z
bytes032 marked the issue as primary issue
#1 - bytes032
2023-09-09T05:08:01Z
Excellent, quality written report.
#2 - c4-pre-sort
2023-09-09T05:10:26Z
bytes032 marked the issue as high quality report
#3 - bytes032
2023-09-15T13:09:14Z
Related issues:
#1075 #1008 #748 #397 #956
#4 - psytama
2023-09-25T13:37:08Z
The issue is in the oracle contract which returns 1e18.
#5 - c4-sponsor
2023-09-25T13:38:17Z
psytama (sponsor) confirmed
#6 - GalloDaSballo
2023-10-12T09:30:00Z
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
#7 - c4-judge
2023-10-15T18:39:50Z
GalloDaSballo marked the issue as selected for report
#8 - c4-judge
2023-10-20T18:28:14Z
GalloDaSballo changed the severity to 2 (Med Risk)
#9 - c4-judge
2023-10-20T18:28:24Z
GalloDaSballo changed the severity to 3 (High Risk)
π Selected for report: LokiThe5th
1182.6582 USDC - $1,182.66
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.
Assumptions:
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); }
Manual review. Foundry.
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.
Other
#0 - c4-pre-sort
2023-09-10T09:35:50Z
bytes032 marked the issue as primary issue
#1 - c4-pre-sort
2023-09-10T09:35:55Z
bytes032 marked the issue as high quality report
#2 - c4-sponsor
2023-09-25T19:26:15Z
psytama (sponsor) acknowledged
#3 - c4-judge
2023-10-15T18:05:40Z
GalloDaSballo marked the issue as selected for report
#4 - 141345
2023-10-23T01:36:04Z
This one seems dup of https://github.com/code-423n4/2023-08-dopex-findings/issues/1584.
The root cause is the same, after settlement, the market value of the vault will "jump", from strike to spot market price.
#5 - lokithe5th
2023-10-23T10:24:57Z
@141345 thank you for your comment. I initially thought the same (both this issue and #1584 being mine), but the underlying cause is different, as this issue isn't predicated on taking advantage of an anticipated settlement
call, it merely requires there to be rdpx
as collateral in the vault (external requirement which is why I submitted as medium). The issue simply describes the leakage of value that should have been captured by the protocol through their permissioned AMM, but can be bypassed.
I'm happy to provide additional context to the issue:
redeem
the amount of rdpx
returned is determined by neither the strike nor the spot market price, but is actually determined by the proportion of shares being burnt in relation to the current total _rdpxCollateral
:// Tracing it all the way from `redeem` ends here shares.mulDivDown(_rdpxCollateral, supply)
rdpx
back in proportion to the underlying value provided at deposit
without paying any swap fees to the V2 Permissioned AMM (owned by the protocol), meaning the protocol loses this fee (hence the leakage of value).This is a distinct root cause from #1584, where the public nature of the moneyness of the puts creates economic incentive to redeem
right before settle
and then deposit
to avoid losses and take up more shares after the value of the vault has fallen (i.e. this issue doesn't require an incoming settle
call).
Will accept the judge's ruling on this and am happy to provide more context should the judge require it.
#6 - GalloDaSballo
2023-10-25T09:18:52Z
Agree that these are different issues, this is an issue with the value received via redeem The other finding is an issue with how losses and gains are accounted (allowing for race conditions)
π Selected for report: catellatech
Also found by: 0xSmartContract, 0xnev, K42, LokiThe5th, RED-LOTUS-REACH, Sathish9098, __141345__, bitsurfer, hunter_w3b, mark0v, nirlin, rjs, rokinot
90.0998 USDC - $90.10
The contest focus was on the newly developed DpxEthToken
and it's supporting contracts. This is V2 for the Dopex team, who have built a thriving options ecosystem.
The key contracts in this contest are:
RdpxV2Core: This is the main entry oint for users into the system. A user can bond their rdpx
and WETH
through this contract. The discount factor in this contract determines the discount users receive for bonding. The bonding mechanism supports the backing reserves of rdpx
and WETH
. This contract also contains Curve style Peg Stability Module functionality.
RdpxV2Bonds: The bonds are minted as ERC721 tokens and are held by the aforementioned RdpxV2Core
contract, which is the only contract that can mint bonds.
PerpetualAtlanticVault: This contract is responsible for the perpetual options business logic. The contract allows the RdpxV2Core
contract to purchase perpetual put options with a strike price that is 25% below the current price. The price is denominated in rdpxPriceInEth
, and so these puts are in the money if either ETH
increases by more than 25% compared to rdpx
or if rdpx
weakens by more than 25% compared to ETH
.
PerpetualAtlanticVaultLP: This contract holds the collateral for put options and allows users to supply liquidity in the form of WETH
which is used as collateral by the PerpetualAtlanticVault
. In addition, it allows liquidity providers to earn yield on their deposited WETH
, as there is an epoch-based funding mechanism for holding the put options.
DpxEthToken: The DpxEthToken
is the star of the show. The Dopex team intends to make this a core asset used in their products. This token is intended to be pegged to ETH
and the token economic mechanisms of bonding and put options are crafted to support this peg.
The focus of the review was the implementation of the perpetual put options, the bonding mechanism and the business logic for the DpxEthToken
.
Manual review of the code and the provided protocol documents was done. Approximately 4 hours per day over the course of 10 days were spent reviewing the code.
The protocol consists of permissioned and non-permissioned components. The Algorithmic Market Operations module and other token management functionality cannot be made permissionless (at least at inception). The current architecture is suitable for a new product being deployed, which may require manual intervention in response to market actions. For example, when to exercise in the money put options requires team analysis and decision making which should not be automated at the start of the project.
Once the deployment has stabilized and the project token economics are up and running, moving to a less permissioned architecture should be considered.
Overall the code reviewed was of a great quality with good test coverage.
Category | Comments |
---|---|
Test suite | Both unit and integration tests in foundry were provided with good coverage |
Natspec | There were occasional, small natspec errors and instances where the comments were not clear |
It is not standard, but it is advisable to stress test the token economic assumptions in the protocol across market movements. The contracts are very exposed to ETH
price movements (see Token Economic Risks below).
The codebase is heavily permissioned and depends on the Dopex Team to manage treasury functions. Multiple protocol functions require role-based access. For example, the settlement of put options when the puts are in the money is triggered manually through an EOA or multisig, which in itself may introduce risk; it must be noted, however, that this functionality cannot be safely automated away at the moment.
In addition, there are multiple instances of deployer accounts being granted access to sensitive functions. This increases attack surface and should be avoided.
The DpxEthToken
and the supporting mechanisms are exposed to smart contract risk from the chosen WETH
token. A failure of trust in the aforementioned tokens contract will contaminate the DpxEthToken
. A possible consideration would be to use various WETH
tokens from multiple established projects.
There is significant concentrated asset risk in the audited contracts. The DpxEthToken
is backed by 75% ether
(used interchangeably here with ETH
and WETH
) and 25% rdpx
up front; in addition it is supported by a put option which backstops the collateral to be around 93% WETH
when the rdpx
price falls (75% straight WETH
, with an additional 25% rdpx
of which 75% is effectively WETH
through the put option).
The upside of this mechanic is that it becomes easier for DpxEth
to maintain it's peg to ETH
. This is also the biggest downside: WETH
price movements will greatly impact the TVL.
The edge case is also worth mentioning: in a market scenario where the WETH
price is falling, the rdpx
price is likely to come under pressure as well. If the prices of both of these assets fall simultaneously, and to similar degrees, the Core
contracts will be unable to exercise their put options (because the strike price is in rdpxInEth
).
It can be argued that this is by design:DpxEthToken
is, after all, intended to be pegged to ETH
. But it must be conceded that this leads to a scenario where the TVL can reduce drastically in USD terms in a relatively short time frame and the protocol would be unable to reduce it's exposure to these assets.
Protocol survival in times of market distress is crucial, and negative price movement in ETH
will trigger flight to stablecoins; away from both ETH
,rdpx
and DpxEth
, further putting pressure on all the assets. The slump in rdpx
in such a scenario is unavoidable (as evidenced by most tokens' historical price movements in downturns).
It is recommended that the team prepares and implements various hedges to account for such market scenarios. It is likely that the team already does this, but it is important to note nonetheless.
40 hours.
40 hours
#0 - c4-pre-sort
2023-09-15T15:56:40Z
bytes032 marked the issue as sufficient quality report
#1 - c4-judge
2023-10-20T10:05:38Z
GalloDaSballo marked the issue as grade-b