code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L302-L303 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L440 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L376-L380

Vulnerability details

Impact

A malicious user can DOS the funding payment logic of the PerpetualAtlanticVault, meaning for a given funding cycle, they can force LPs in the PerpetualAtlanticVaultLP contract to not receive any funding. Specifically, the attacker can force the payFunding function of the PerpetualAtlanticVault to revert on all calls for that funding period. This is the function where the admin is actually sending the funding payment for that period from the RdpxV2Core contract. The only requirement for this attack is there being a new funding period for which the admin has not yet paid the funding for & there being a call to settle an ITM option. These are simple requirements which have a fairly high likelihood of occurring in practice.

Proof of Concept

The attack begins with the beginning of a new funding period for which the payFunding function has not yet been triggered by the admin. It is highly likely that in general a call to this function will be delayed, as the logic allows for paying out the entire period's yield, irrespective of when the function is actually called during a funding period (e.g. no incentive to call it right at the beginning of the period). Let's assume that at this period in time, totalActiveOptions is equal to 1_000e18, which is just the amount of rDPX being covered. Let's also assume for simplicity that no new options have been purchased during this new funding period, meaning fundingPaymentsAccountedFor[latestFundingPaymentPointer] is equal to 0. Here, I am assuming that latestFundingPaymentPointer is pointing to this new funding cycle (note: this will happen only after the next call to updateFunding, but that detail is unimportant).

Now let's assume that there's a call to settle an ITM option. This could have either been triggered during this cycle, or triggered in the previous cycle, but the tx was not yet mined due to blockchain congestion. The attacker will frontrun this call to settle with a call to calculateFunding which is defined as follows:

function calculateFunding(
  uint256[] memory strikes
) external nonReentrant returns (uint256 fundingAmount) {
  _whenNotPaused();
  _isEligibleSender();

  updateFundingPaymentPointer();

  for (uint256 i = 0; i < strikes.length; i++) {
    _validate(optionsPerStrike[strikes[i]] > 0, 4);
    _validate(
      latestFundingPerStrike[strikes[i]] != latestFundingPaymentPointer,
      5
    );
    uint256 strike = strikes[i];

    uint256 amount = optionsPerStrike[strike] -
      fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][
        strike
      ];

    ...

    // Record number of options that funding payments were accounted for, for this epoch
    fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;

    // record the number of options funding has been accounted for the epoch and strike
    fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][
      strike
    ] += amount;

    ...
  }
}

The relevant logic here is that by front-running a call to settle, the attacker is able to run the following logic (for all options, including the option which is about to be exercised): fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;. Let's assume for this example that there were two outstanding options, each with different strike prices (one is ITM and one is OTM). The attacker has called the calculateFunding function with both these strike values, which will mean fundingPaymentsAccountedFor[latestFundingPaymentPointer] is now 1_000e18. This is because there's no check of whether these options are ITM or not.

Following this call, the settle function is called:

function settle(
  uint256[] memory optionIds
)
  external
  nonReentrant
  onlyRole(RDPXV2CORE_ROLE)
  returns (uint256 ethAmount, uint256 rdpxAmount)
{
  _whenNotPaused();
  _isEligibleSender();

  updateFunding();

  for (uint256 i = 0; i < optionIds.length; i++) {
    uint256 strike = optionPositions[optionIds[i]].strike;
    uint256 amount = optionPositions[optionIds[i]].amount;

    // check if strike is ITM
    _validate(strike >= getUnderlyingPrice(), 7);

    ethAmount += (amount * strike) / 1e8;
    rdpxAmount += amount;
    optionsPerStrike[strike] -= amount;
    totalActiveOptions -= amount;

    // Burn option tokens from user
    _burn(optionIds[i]);

    optionPositions[optionIds[i]].strike = 0;
  }

  ...
}

The relevant logic here is that for the option is being settled, the totalActiveOptions amount is being decremented by the amount of rDPX that option was covering: totalActiveOptions -= amount;. Let's assume for this example that it was 500e18 rDPX, meaning totalActiveOptions now equals 1_000e18-500e18=500e18.

Now with this done, let's see what happens when the admin attempts to call payFunding in order to pay the funding yield for this cycle. This has the following check, which is intended to check that the funding amount being paid covers all obligations:

_validate(
  totalActiveOptions == 
    fundingPaymentsAccountedFor[latestFundingPaymentPointer],
  6
);

We can see now that there is an issue. As shown earlier, totalActiveOptions is 500e18, while fundingPaymentsAccountedFor[latestFundingPaymentPointer] is 1_000e18. This check will revert, and so this payFunding function will not be able to be called.

Tools Used

Manual review

Recommended Mitigation Steps

Consider reworking this strict validation check in the payFunding function to account for when options are being settled.

Assessed type

DoS

c4-pre-sort commented 1 year ago

bytes032 marked the issue as duplicate of #1798

c4-pre-sort commented 1 year ago

bytes032 marked the issue as sufficient quality report

c4-judge commented 11 months ago

GalloDaSballo changed the severity to QA (Quality Assurance)