code423n4 commented 1 year ago

Lines of code

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

Vulnerability details

Impact

At the start of every epoch the protocol admins will call calculateFunding in PerpetualAtlanticVault.sol to calculate the funding of options for an array of strikes prices in the next epoch. They do this before calling provideFunding in the core contract which calls the underlying payFunding in the vault contract. The provided funds are then dripped to the LPs throughout the epoch.

The problem with the current mechanism is that calculateFunding does not have sufficient access control, and therefore can be called by anyone. This might not sound like a problem, but a malicious user can deliberately frontrun any legitimate admin calls to this method at the start of the epoch, providing a single strike price. Provided this strike is in the array provided by the admin (which it should be because the admin calculates funding for all the active options) then the admin call to calculateFunding will revert due to a check. A malicious user can do this for as many strikes as there are. So if there were 50 different strikes, then the malicious user could force 50 different calls to calculateFunding, rather than a single call with the array of all the strikes.

This will cause a delay, which means the premium calculations are likely to be lower (it could be higher based on the Black Scholes algorithm, but I expect the highest premium will usually be at the start of an epoch) and therefore the funding payments to LPs are also lower than they could/should have been.

Proof of Concept

At the start of every epoch the admin calls calculateFunding to calculate the funding to be drip fed to LPs based on the total active options. The first part of this method is interesting:

  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
      );

As you can see, there is no access control besides _isEligibleSender, and this is only relevant when calling from a contract, therefore it does not protect against a malicious EOA. An admin is going to want to call the method with all the active strikes (or as many as gas allows) at the start of the epoch. However there is a validation that the funding for the strike hasn't already been calculated for the latest funding payment pointer. This is a now a problem because a malicious user can grief the admin by calling calculateFunding with a single strike; the funding for this strike has now been calculated for the latest funding payment pointer. Now, when the admin provides the same strike in the array of strikes, the execution will revert.

Instead of being able to calculate funding for multiple strikes in a single transaction, a malicious user can grief the admin by forcing funding calculations for 1 strike at a time. Like I mentioned above, this delays the funding calculations and therefore leads to probably (dependent on Black Scholes parameters) lower premiums and therefore rewards to LPs.

Tools Used

Manual review

Recommended Mitigation Steps

The calculateFunding method should either have some additional access control to only allow the admin or core contract (via an admin) to call it, or the validation mentioned above could be exchanged in favour of a continue clause; if the funding for the strike has already been calculated for the current epoch, then simply move on to the next strike in the array rather than reverting.

Assessed type

Access Control

c4-pre-sort commented 1 year ago

bytes032 marked the issue as primary issue

c4-pre-sort commented 1 year ago

bytes032 marked the issue as sufficient quality report

c4-sponsor commented 1 year ago

witherblock marked the issue as disagree with severity

witherblock commented 1 year ago

Can be easily avoided by calling functions using a fail safe bot and a fast RPC, also regardless of the time difference, since the network is arbitrum, txs are processed very fast so the difference in funding is negligible

GalloDaSballo commented 1 year ago

I agree with the griefing, but I don't see it being weaponized Additionally a set of try catches could allow this to be avoided Lastly the change for a few minutes would be extremely small

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)