Dopex - ABA's results

Lines of code

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

Vulnerability details

Summary

A malicious actor can block all settle operations by directly transferring collateral to the LP vault (PerpetualAtlanticVaultLP), making all future settle operations revert. This happens because PerpetualAtlanticVaultLP incorrectly presumes that only deposited collateral and procedures increment contract collateral, without taking into account that anyone can directly send collateral to the LP vault.

In the above scenario all settle operations fail and users lose their earned rewards.

Vulnerability Details

A settle operation execution flow is as follows:

• RdpxV2Core::settle
  • PerpetualAtlanticVault::settle
    • PerpetualAtlanticVaultLP::[several call]

When a settle call reaches PerpetualAtlanticVault::settle, if the options to be settled are correct, the calculate wETH amount to be moved from the Atlantic LP vault is first sent from the vault and the internal accounting for the LP vault is called to update and confirm the loss:

  function settle(
    uint256[] memory optionIds
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
  {
    // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
    collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );

    // code ...

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );    

    // code ...
    }

The IPerpetualAtlanticVaultLP::subtractLoss verifies that the collateral amount has indeed been transferred out of the vault and notes it internally.

  function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

The issues is with the require condition that collateral.balanceOf(address(this)) must be equal with total accounted collateral minus the now incurred loss since the contract balance can be updated by anyone but _totalCollateral is only increased in case of a deposit or when adding proceeds gains, meaning it does not take into consideration any directly send balance.

Because of this, directly sending collateral to the vault will block all settles.

Follows is a coded POC test that shows the above scenario. Add it to tests\perp-vault\Unit.t.sol and it can be run with forge test --match-test testSettleIsBlocked -vv

function testSettleIsBlocked() public {
    // initial vault setup
    weth.mint(address(1), 1 ether);
    deposit(1 ether, address(1));

    vault.purchase(1 ether, address(this));

    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;

    // malicious user bob setup
    address bob = makeAddr("bob");
    weth.mint(bob, 1 ether);    
    
    // oracle update price so that it is in the money
    priceOracle.updateRdpxPrice(0.010 gwei); // ITM

    // before a valid "settle" call, bob transfers 1 WEI to the LP vault
    vm.prank(bob);
    weth.transfer(address(vaultLp), 1);

    // settle will revert
    vm.expectRevert("Not enough collateral was sent out");
    vault.settle(ids);
  }

Impact

A malicious actor can block all settle operations by directly transferring collateral to the Atlantic perpetual LP vault, making all future settle operations revert.

Tools Used

Manual review.

Recommendations

The entire vault LP should hold an internal accounting of exactly what balance it is tracking, relying on collateral.balanceOf(address(this)) is not a recommended practice and may lead to other issues in the future.

Assessed type

DoS

Lines of code

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

Vulnerability details

Issue details

Atlantic Perpetual PUT Options are currently are implemented without any expiration date. This is a high level design issue that needs to be resolved, as theoretically options that are set near ATL (all time lows) will last forever and those that are near local price lows can remain in the system for weeks to months.

    // Mint the option tokens
    tokenId = _mintOptionToken();
    optionPositions[tokenId] = OptionPosition({
      strike: strike,
      amount: amount,
      positionId: tokenId
    });

During this time funding is paid for all of them. From an economical point of view this system of options without expiry does not make any sense and can accumulate options indefinitely.

Options can only be closed if they are ITM (in the money) which is a risk free economical instrument for investors, one that should not exist:

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

Impact

Options not settled remain in the forever, system pays funding theoretically forever for them without any value added in the long term.

Tools Used

Manual review.

Recommendations

Implement an expiry period for options and check in the settle function that it either ITM or expired and remove it from the system.

Assessed type

Other

Lines of code

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

Vulnerability details

Issue Details

Distributing funding for Atlantic Vault PUT option writers needs to be done in 2 steps, without incrementing the epoch pointer, otherwise that funding is lost. Steps:

1. calculate the funding for all strikes up to this point
   • by calling PerpetualAtlanticVault::calculateFunding
   • must be done before current epoch ends, as it increments the epoch pointer if it is needed before executing the logic
2. distribute the funding to the vault
   • by calling RdpxV2Core::provideFunding
   • does not increment the funding pointer
   • must be done after the calculateFunding call and before an call that updates the funding pointer

     • protocol documentation indicates it is done in the next epoch (chronologically but before the pointer is incremented)

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

Another important observation is that APP (Atlantic Perpetual PUTS Options vault) owner can modify the epoch duration at will via PerpetualAtlanticVault::updateFundingDuration to any value.

  /**
   * @notice Updates the funding duration
   * @dev    Can only be called by the owner
   **/
  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    fundingDuration = _fundingDuration;
  }

A major issue occurs when updating the funding duration to any value that would set the protocol into the next epoch without funding being collected up to this point. Since distributing funding (2) depends on its calculation (1) which increment the pointer if it is the case, by setting a lower funding duration we are basically "thrusted into the future" from internal protocol accounting point of view and missed collecting and distributing funding amount to option writers.

POC

A theoretical POC:

• funding duration is the default 7 days;
• after 6 days have passed, the Dopex team decides to reduce funding duration to 5 days
• team calls updateFundingDuration to 5 days
• from an internal accounting point of view, the protocol has passed 1 day into its new epoch period without funding being calculated or distributed for the previous chronological epoch
• attempting to distribute funding now requires calculateFunding to be called but this updates the current funding pointer
• users can still buy options but the more the team waits before updating the epoch pointer the more funding is lost (since it is still accounted to the old epoch pointer), thus the team is needed to call updateFundingPaymentPointer or updateFunding (because calculateFunding can be called only once per strike price per epoch and doing so to update the pointer will result in funding being lost this way)
• overall any funding from the first 6 days of options buys will be lost

A coded POC follows, with the added observations:

• add the following 3 functions (2 tests and 1 helper) to tests\perp-vault\Unit.t.sol
• the nature of this issue is better shown with 2 tests that highlight what happens if a correct funding behavior is applied (testDecreasingFundingDuration_CorrectFundingBehavior test) and if an incorrect behavior, the currently existing implementation, is applied (testDecreasingFundingDuration_SlashFunding)
• also add import "forge-std/console.sol"; to file import header
• running the tests with forge test --match-test testDecreasingFundingDuration -vvv will run both of them at the same time and you can see the difference side by side

  function doFundingSetup() public returns (uint256[] memory) {
    // setup tests
    vault.setLpAllowance(true);
    uint256 lpBalanceBefore = weth.balanceOf(address(vaultLp));
    assertEq(lpBalanceBefore, 0);
    deposit(100 ether, address(this));
    uint256 lpBalanceAfter = weth.balanceOf(address(vaultLp));
    assertEq(lpBalanceAfter, 100 ether);

    // initial options purchase
    vault.purchase(1 ether, address(this));

    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;

    // start with a fresh pointer
    skip(86500);
    vault.updateFundingPaymentPointer();

    return strikes;
  }

  function testDecreasingFundingDuration_CorrectFundingBehavior() public {
    uint256[] memory strikes = doFundingSetup();
    uint256 cumulatedFundingAmount = 0;

    skip(75600); // after 21/24 h have passed

    cumulatedFundingAmount += vault.calculateFunding(strikes);
    console.log("Correctly paying funding up until before the update in duration; funding:", cumulatedFundingAmount);

    vault.updateFundingDuration(72000); // change to 20 from 24h
    
    cumulatedFundingAmount += vault.calculateFunding(strikes);
    console.log("Total funding while taking into consideration fragmented epoch:", cumulatedFundingAmount);
  }

  function testDecreasingFundingDuration_SlashFunding() public {
    uint256[] memory strikes = doFundingSetup();

    skip(75600); // after 21/24 h have passed

    // here the call to calculate funding is missing

    vault.updateFundingDuration(72000); // change to 20 from 24h
    
    uint256 fundingAccrued = vault.calculateFunding(strikes);
    console.log("Total funding without taking into consideration fragmented epoch:", fundingAccrued);
  }

Output shows how in the correct case we get 0.1 ETH funding paid and in the incorrect one we get only 0.05 ETH paid, a 50% loss in funding:

Running 2 tests for tests/perp-vault/Unit.t.sol:Unit
[PASS] testDecreasingFundingDuration_CorrectFundingBehavior() (gas: 810384)
Logs:
  Correctly paying funding up until before the update in duration; funding: 50000000000000000
  Total funding while taking into consideration fragmented epoch: 100000000000000000

[PASS] testDecreasingFundingDuration_SlashFunding() (gas: 731499)
Logs:
  Total funding without taking into consideration fragmented epoch: 50000000000000000

Test result: ok. 2 passed; 0 failed; finished in 653.01ms

Impact

Updating the funding duration to any value that would set the protocol into the next epoch without funding being collected up to this point will result in the funding being lost.

Tools Used

Manual analysis

Recommendations

Modify updateFundingDuration so that:

• it is only callable from RdpxV2Core contract and make a RdpxV2Core::updateFundingDuration version as entry point
• in it, check if the new duration would cause a pointer increment
• if so:
  • call PerpetualAtlanticVault::calculateFunding
  • call RdpxV2Core::provideFunding
  • set the new update duration
  • call PerpetualAtlanticVault::updateFundingPaymentPointer

Doing it like so would resolve any funding issue with this scenario.

Assessed type

Timing

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L624-L685

Vulnerability details

Issue Details

Decaying bonds are minted to users they face a loss using one of the other Dopex ecosystem products. As a rebate for their loss, a decaying bondable rDPX bond is minted for the users with a expiry after which it is no longer valid and a amount based on the loss.

If a decaying bond is used, as a users, you don't have to send the rDPX from your balance as it will use the rDPX balance from the decaying bonds. Since the user still needs to add WETH the protocol overall still receives value, just less.

The decaying bond consumption logic has a design flaw that significantly reduces the usage of decaying bonds, ultimately leading to less liquidity coming into the protocol itself:

1. the amount of DPXETH to be bonded to cannot be more then the amount offered from the decaying bond
2. if the wished amount to be bounded surpasses the decaying bond amount, instead of users providing the difference, the transaction is reverted

Consider the following situations:

• the following users have:
  • User A: 0.4 DPXETH in decaying bonds
  • User B: 0.6 DPXETH in decaying bonds
  • User C: 0.3 DPXETH in decaying bonds
• User D wants to bond to 0.5 DPXETH, logically he will use either B because it is the happy path and the least effort to use (normal effort/reward theory)
• after that, user B bond has only 0.1 DPXETH
• say another user, E appear that wants 0.35 DPXETH, which uses user A's bonds
• now we have:
  • User A: 0.05 DPXETH in decaying bonds
  • User B: 0.1 DPXETH in decaying bonds
  • User C: 0.3 DPXETH in decaying bonds
• at this point normal Dopex ecosystem flows would generate, for example, another 2 decaying bonds:
  • User F: 0.5 DPXETH in decaying bonds
  • User G: 0.6 DPXETH in decaying bonds
• any new user that wishes to bound for 0.5 DPXETH will not chose to combine A+B+C, why would he do that when F or G exists?

The design logic issue is that, if users want to bond with amounts that are not within existing decaying bonds then they cannot do this without splitting their buys into several smaller ones. This will lead to users simply waiting for decaying bods within their limits to appear and chose those (efficiency). This further leaves all uncommon values (or small values) decaying bonds not used, why combine N when you can get the value from just 1 bond (it's even more cheap from a gas point of view). These small value bonds expire instead of driving revenue to the protocol.

Impact

Small value bonds will expire instead of driving revenue to the protocol.

Tools Used

Manual review

Recommendations

Modify bonding function to

• allow users to provide the difference between what a decaying bond can offer and the amount that they desire if they so choose so
• given an amount, to split it between a pool of IDs. This is different from pre-splitting the amount and matching it to existing IDs since the protocol should handle this, not the off-chain user.

Assessed type

Other