Dopex - nirlin's results

A rebate system for option writers in the Dopex Protocol.

General Information

Platform: Code4rena

Start Date: 21/08/2023

Pot Size: $125,000 USDC

Total HM: 26

Participants: 189

Period: 16 days

Judge: GalloDaSballo

Total Solo HM: 3

Id: 278

League: ETH

Dopex

Findings Distribution

Researcher Performance

Rank: 18/189

Findings: 5

Award: $1,031.85

Analysis:
grade-a

🌟 Selected for report: 0

🚀 Solo Findings: 0

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L764-L783 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205

Vulnerability details

Impact

Settle function in rdpxv2Core.sol can be dossed by sending the 1wei of weth into the perpetualAtlanticValutLP.sol

Proof of Concept

When the settle function is called, it calls the subtractLoss function on Lp vault, that balances out the total collateral by subtracting the loss accured from it.

But if we take a look at the subtract loss function we can see a hard check for the balance:

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

So if anyone send 1wei of token into this contract it will be dossed as the hard check for the balance will fail

I made the following change in the test case for the settle function where address(1) sends exactly 1wei of token in lp vault right before the settle function is called.

  function testSettle() public {
        rdpxV2Core.bond(5 * 1e18, 0, address(this));
        rdpxV2Core.bond(1 * 1e18, 0, address(this));

        (, uint256 rdpxReserve1, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
        (, uint256 wethReserve1, ) = rdpxV2Core.getReserveTokenInfo("WETH");

        vault.addToContractWhitelist(address(rdpxV2Core));
        uint256[] memory _ids = new uint256[](1);

        // test OTM options
        vm.expectRevert(
            abi.encodeWithSelector(
                PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector,
                7
            )
        );
+        deal(address(weth),address(1), 100e18);
+        vm.startPrank(address(1));
+       // transfer some weth token to the lp vault
+        weth.transfer(address(vault), 1);
+        vm.stopPrank();
+        rdpxV2Core.settle(_ids);

        // settle invalid option id
        _ids[0] = 1;
        vm.expectRevert(
            abi.encodeWithSelector(
                PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector,
                7
            )
        );
        rdpxV2Core.settle(_ids);

        (uint256 strike1, uint256 amount1, ) = vault.optionPositions(0);

        // test ITM options
        _ids[0] = 0;
        rdpxPriceOracle.updateRdpxPrice(1e7);
        rdpxV2Core.settle(_ids);

        (, uint256 rdpxReserve2, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
        (, uint256 wethReserve2, ) = rdpxV2Core.getReserveTokenInfo("WETH");

        assertEq(rdpxReserve1 - amount1, rdpxReserve2);
        assertEq(wethReserve1 + ((amount1 * strike1) / 1e8), wethReserve2);

        rdpxV2Core.bond(10 * 1e18, 0, address(this));
        rdpxV2Core.bond(1 * 1e18, 0, address(this));

        (, rdpxReserve1, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
        (, wethReserve1, ) = rdpxV2Core.getReserveTokenInfo("WETH");

        // test for multiple options with settled option id
        _ids = new uint256[](3);
        _ids[0] = 0;
        _ids[1] = 2;
        _ids[2] = 3;
        vm.expectRevert(
            abi.encodeWithSelector(
                PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector,
                7
            )
        );
        rdpxV2Core.settle(_ids);

        // settle multiple options at different strikes
        _ids[0] = 1;
        _ids[1] = 2;
        _ids[2] = 3;

        (strike1, amount1, ) = vault.optionPositions(1);
        (uint256 strike2, uint256 amount2, ) = vault.optionPositions(2);
        (uint256 strike3, uint256 amount3, ) = vault.optionPositions(3);

        rdpxPriceOracle.updateRdpxPrice(1e6);
        rdpxV2Core.settle(_ids);

        (, rdpxReserve2, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
        (, wethReserve2, ) = rdpxV2Core.getReserveTokenInfo("WETH");

        assertEq(rdpxReserve1 - amount1 - amount2 - amount3, rdpxReserve2);
        assertEq(
            wethReserve1 +
                ((amount1 * strike1) / 1e8) +
                ((amount2 * strike2) / 1e8) +
                ((amount3 * strike3) / 1e8),
            wethReserve2
        );
    }

This function fails with the following error output:

Failing tests: Encountered 1 failing test in tests/perp-vault/Unit.t.sol:Unit [FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 913849)

Which is the error thrown in subtract loss.

Admin can reverse the doss by calling the emergency withdraw and than sending back the exact required tokens and that is hassle and someone can again DOS by simply sending 1wei token or build a bot for it or even frontrunning each settle transaction. So it essentially becomes a permanent loss.

Tools Used

Dreaming about code after finding nothing for a while

chnage the subtractLoss() as following:

  function subtractLoss(uint256 loss) public onlyPerpVault {

    require(
-        collateral.balanceOf(address(this)) == _totalCollateral - loss,
+       collateral.balanceOf(address(this)) >= _totalCollateral - loss,

      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

Assessed type

DoS

#0 - c4-pre-sort

2023-09-09T09:54:08Z

bytes032 marked the issue as duplicate of #619

#1 - c4-pre-sort

2023-09-11T16:14:19Z

bytes032 marked the issue as sufficient quality report

#2 - c4-judge

2023-10-21T07:15:59Z

GalloDaSballo marked the issue as satisfactory

Awards

15.9268 USDC - $15.93

Labels

bug
2 (Med Risk)
satisfactory
sufficient quality report
duplicate-850

External Links

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L237-L241 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L405-L459 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L462-L524

Vulnerability details

Impact

perpetualAtlanticVault.sol functionality is dependent upon the funding duration which can be changed by the admin and can lead to bricking the whole calculations in the perpetualAtlanticVault.sol

Proof of Concept

Initially funding duration is set to seven days which can be changed using the following function:

  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    fundingDuration = _fundingDuration;
  }

And one of the important function that is dependent upon the funding duration is:

  /// @inheritdoc	IPerpetualAtlanticVault
  function nextFundingPaymentTimestamp()
    public
    view
    returns (uint256 timestamp)
  {

    return genesis + (latestFundingPaymentPointer * fundingDuration);
  }

Which is used overall in the contract to calculate start times and end times and while paying the fundings in drip vested manner

Lets consider the following function and it will explain the overall problem in other functions too:

  function updateFunding() public {
    // what if even we don't update this funding payment pointer, will it make any difference at all?

    updateFundingPaymentPointer();
    uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];
    uint256 startTime = lastUpdateTime == 0
      ? (nextFundingPaymentTimestamp() - fundingDuration)
      : lastUpdateTime;
    lastUpdateTime = block.timestamp;

    collateralToken.safeTransfer(
      addresses.perpetualAtlanticVaultLP,
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );

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

    emit FundingPaid(
      msg.sender,
      ((currentFundingRate * (block.timestamp - startTime)) / 1e18),
      latestFundingPaymentPointer
    );
  }

lets say initially the funding duration was 7 days for first 4 epochs. And after 4 epochs admin/protcol governance decide that the ideal parameter should be 5 days. What would happen, lets see

consider the following code snippet :

 uint256 startTime = lastUpdateTime == 0
      ? (nextFundingPaymentTimestamp() - fundingDuration)
      : lastUpdateTime;
    lastUpdateTime = block.timestamp;

lets say the lastUpdateTime == 0

And 4 epochs have passed, which means 4 weeks have gone by after the epoch, 35 days, if genesis timestamp is at 7th day after deployment, so 28 days after genesis. But because new funding duration is 5 days the start time becomes:

startTime = (genesis (lets say initially it was the 7th day) + (pointer ( which is 4) * fundingDuration(which is 5)) - fundingDuration

so startTime = genesis + 20 days.

So our startTime decreases 8 days and collide with the previous star time stamps, for which the funding could be already paid and will result in double payment.

Similarly funding could be skipped for certain time in between if the funding duration is increased instead of decreased. And whole drip vesting is dependent upon this logic.

Also funding duration plays important part in premium calculation to calculate the time till expiry. That will be also bricket

     uint256 timeToExpiry = nextFundingPaymentTimestamp() -
        (genesis + ((latestFundingPaymentPointer - 1) * fundingDuration));

Tools Used

Manual review

Two solution here, either make the funding duration constant at the time of deployment, if not the whole contract will need a refactor here

Assessed type

Math

#0 - c4-pre-sort

2023-09-08T06:23:39Z

bytes032 marked the issue as duplicate of #980

#1 - c4-pre-sort

2023-09-11T08:19:59Z

bytes032 marked the issue as sufficient quality report

#2 - c4-judge

2023-10-20T11:12:26Z

GalloDaSballo marked the issue as satisfactory

Findings Information

Labels

bug
2 (Med Risk)
downgraded by judge
satisfactory
sufficient quality report
duplicate-761

Awards

158.2271 USDC - $158.23

External Links

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1156

Vulnerability details

Impact

In bond cost function the pointer is not updated to latest pointer which leads to wrong calculation of expiry timing. Expiring timing is the main key factor to calculate the premium, wrong expiry time leads to wrong premium hence, loss for either user or the protocol.

Proof of Concept

Have a look at the following function:

function calculateBondCost(
    uint256 _amount,
    uint256 _rdpxBondId
  ) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
    uint256 rdpxPrice = getRdpxPrice();

    
    if (_rdpxBondId == 0) {
      // if bond id is zero it is non decaying bonds we talking about
      // non decaying bonds, we passed in the amount and the bond Id
        uint256 bondDiscount = (bondDiscountFactor *
          Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) *
          1e2) / (1e9); // 1e8 precision
        // (bondDiscoundFactor * squaretootof(rdpxReserve) * 100) / (squaretootof(1e18))

      _validate(bondDiscount < 100e8, 14);
      // rdpx is 25% so we need to make the 25% discount on the value it creates lets say the amount is 100e8
      // so its 25% is 25, so discount should be calculated on 25
      rdpxRequired =  ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount *   DEFAULT_PRECISION) / (DEFAULT_PRECISION * rdpxPrice * 1e2);
      wethRequired =((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /(DEFAULT_PRECISION * 1e2);
    } else {
      // if rdpx decaying bonds are being used there is no discount
      rdpxRequired =
        (RDPX_RATIO_PERCENTAGE * _amount * DEFAULT_PRECISION) /
        (DEFAULT_PRECISION * rdpxPrice * 1e2);

      wethRequired =
        (ETH_RATIO_PERCENTAGE * _amount) /
        (DEFAULT_PRECISION * 1e2);
    }

    uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price



    // @note - update the pointer first.
    uint256 timeToExpiry = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).nextFundingPaymentTimestamp() - block.timestamp;
    if (putOptionsRequired) {
      wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
        .calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
    }
  }

Here we can see expiry time is calculated at:

    uint256 timeToExpiry = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).nextFundingPaymentTimestamp() - block.timestamp;

And formula for nextFundingPaymentTimestamp() given in the perpetualAtlanticVault.sol as:

genesis + (latestFundingPaymentPointer * fundingDuration)

But latestFundingPaymentPointer could be for the behind the latest epoch, which is why we always call the function updateFundingPaymentPointer() before making any critical calculation.

As we can see when we calculate the premium in atlantic vault in calculateFunding function first the pointer is updated by calling the updateFundingPaymentPointer() as in following function from atlantic vault:

  function calculateFunding(
    uint256[] memory strikes
  ) external nonReentrant returns (uint256 fundingAmount) {
    _whenNotPaused();
    _isEligibleSender();

    updateFundingPaymentPointer();

    .....> skipped the code in between

      uint256 timeToExpiry = nextFundingPaymentTimestamp() -
        (genesis + ((latestFundingPaymentPointer - 1) * fundingDuration));

      uint256 premium = calculatePremium(
        strike,
        amount,
        timeToExpiry,
        getUnderlyingPrice()
      );

      latestFundingPerStrike[strike] = latestFundingPaymentPointer;
      fundingAmount += premium;

      // Record number of options that funding payments were accounted for, for this epoch
      // this accounting is only done for the epoch that start after the genesis
      fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;

      // record the number of options funding has been accounted for the epoch and strike
      fundingPaymentsAccountedForPerStrike[latestFundingPaymentPointer][
        strike
      ] += amount;

      // Record total funding for this epoch
      // This does not need to be done in purchase() since it's already accounted for using `addProceeds()`
      totalFundingForEpoch[latestFundingPaymentPointer] += premium;

      emit CalculateFunding(
        msg.sender,
        amount,
        strike,
        premium,
        latestFundingPaymentPointer
      );
    }
  }

But in our case in the rdpxV2Core step of updating pointer is skipped.

Tools Used

Caffeine, neurons, vscode and alot of frustration.

This could have been prevented by updating the pointer in either of three functions in first line :

  1. bond()
  2. delgateBond()
  3. calculateBondCost

If we choose first two we have to call the function in both the function. But putting it in last function compensate above two too.

call the updateFundingPaymentPointer in the calculateBondCost() and modify the code as following:

 function calculateBondCost(
    uint256 _amount,
    uint256 _rdpxBondId
  ) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
  + vault.updateFundingPaymentPointer()
    uint256 rdpxPrice = getRdpxPrice();

    
    if (_rdpxBondId == 0) {
      // if bond id is zero it is non decaying bonds we talking about
      // non decaying bonds, we passed in the amount and the bond Id
        uint256 bondDiscount = (bondDiscountFactor *
          Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) *
          1e2) / (1e9); // 1e8 precision
        // (bondDiscoundFactor * squaretootof(rdpxReserve) * 100) / (squaretootof(1e18))

      _validate(bondDiscount < 100e8, 14);
      // rdpx is 25% so we need to make the 25% discount on the value it creates lets say the amount is 100e8
      // so its 25% is 25, so discount should be calculated on 25
      rdpxRequired =  ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount *   DEFAULT_PRECISION) / (DEFAULT_PRECISION * rdpxPrice * 1e2);
      wethRequired =((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /(DEFAULT_PRECISION * 1e2);
    } else {
      // if rdpx decaying bonds are being used there is no discount
      rdpxRequired =
        (RDPX_RATIO_PERCENTAGE * _amount * DEFAULT_PRECISION) /
        (DEFAULT_PRECISION * rdpxPrice * 1e2);

      wethRequired =
        (ETH_RATIO_PERCENTAGE * _amount) /
        (DEFAULT_PRECISION * 1e2);
    }

    uint256 strike = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .roundUp(rdpxPrice - (rdpxPrice / 4)); // 25% below the current price



    // @note - update the pointer first.
    uint256 timeToExpiry = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).nextFundingPaymentTimestamp() - block.timestamp;
    if (putOptionsRequired) {
      wethRequired += IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
        .calculatePremium(strike, rdpxRequired, timeToExpiry, 0);
    }
  }

Assessed type

Math

#0 - c4-pre-sort

2023-09-10T12:54:28Z

bytes032 marked the issue as duplicate of #237

#1 - c4-pre-sort

2023-09-12T06:09:05Z

bytes032 marked the issue as sufficient quality report

#2 - c4-pre-sort

2023-09-14T09:35:05Z

bytes032 marked the issue as duplicate of #761

#3 - c4-judge

2023-10-20T11:57:27Z

GalloDaSballo marked the issue as satisfactory

#4 - c4-judge

2023-10-21T07:54:55Z

GalloDaSballo changed the severity to 2 (Med Risk)

Awards

24.8267 USDC - $24.83

Labels

bug
2 (Med Risk)
downgraded by judge
satisfactory
sufficient quality report
duplicate-153

External Links

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L286-L295

Vulnerability details

Impact

Postion of funds can be list if some of thefunds haven't been deposited to the underlying Uniswap pools. There's always a chance of such event since Uniswap pools take balanced token amounts

Proof of Concept

Relp contract calls the addLiquidity() function on the uniswap v2 pool, if we have a look at this function in uniswap v2 router, it calls another internal function '_addLiquidity()' which have the following code:

    function _addLiquidity(
        address tokenA,
        address tokenB,
        uint amountADesired,
        uint amountBDesired,
        uint amountAMin,
        uint amountBMin
    ) internal virtual returns (uint amountA, uint amountB) {
        // create the pair if it doesn't exist yet
        if (IUniswapV2Factory(factory).getPair(tokenA, tokenB) == address(0)) {
            IUniswapV2Factory(factory).createPair(tokenA, tokenB);
        }
        (uint reserveA, uint reserveB) = UniswapV2Library.getReserves(factory, tokenA, tokenB);
        if (reserveA == 0 && reserveB == 0) {
            (amountA, amountB) = (amountADesired, amountBDesired);
        } else {
            uint amountBOptimal = UniswapV2Library.quote(amountADesired, reserveA, reserveB);
            if (amountBOptimal <= amountBDesired) {
                require(amountBOptimal >= amountBMin, 'UniswapV2Router: INSUFFICIENT_B_AMOUNT');
                (amountA, amountB) = (amountADesired, amountBOptimal);
            } else {
                uint amountAOptimal = UniswapV2Library.quote(amountBDesired, reserveB, reserveA);
                assert(amountAOptimal <= amountADesired);
                require(amountAOptimal >= amountAMin, 'UniswapV2Router: INSUFFICIENT_A_AMOUNT');
                (amountA, amountB) = (amountAOptimal, amountBDesired);
            }
        }
    }

It calculates the balanced amount of amountA and amountB to be transferred from the user internally not what user wanted to send, so there is always a chance that due to slippage or some calculation, some amount of tokenA and tokenB is left in the contract which can accumulate overtime as relp can be called very often if it is toggle on.

Not token A in not the problem here due to following snippet in Relp:

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L302-L305

    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );

token A is sent back to core. But the tokenB go no where remainning stuck forever in contract. And unlike other contracts, this contract have no emergency withdraw either .

For more reference Jeiwan submission in good entry:

Reference Submission

Tools Used

Inspired from Jeiwan submission in good entry contest which was heavily based upon the uniswap.

Add a lines to send back the tokenB too like tokenA

    IERC20WithBurn(addresses.tokenB).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenB).balanceOf(address(this))
    );

Assessed type

Token-Transfer

#0 - c4-pre-sort

2023-09-07T12:41:00Z

bytes032 marked the issue as duplicate of #1259

#1 - c4-pre-sort

2023-09-07T12:41:24Z

bytes032 marked the issue as not a duplicate

#2 - c4-pre-sort

2023-09-07T12:46:44Z

bytes032 marked the issue as duplicate of #1286

#3 - c4-pre-sort

2023-09-11T07:50:55Z

bytes032 marked the issue as not a duplicate

#4 - c4-pre-sort

2023-09-11T15:37:48Z

bytes032 marked the issue as sufficient quality report

#5 - c4-pre-sort

2023-09-13T11:37:21Z

bytes032 marked the issue as duplicate of #1286

#6 - c4-judge

2023-10-10T17:52:40Z

GalloDaSballo changed the severity to 2 (Med Risk)

#7 - c4-judge

2023-10-18T12:13:16Z

GalloDaSballo marked the issue as satisfactory

Findings Information

Labels

analysis-advanced
grade-a
high quality report
edited-by-warden
A-03

Awards

832.8493 USDC - $832.85

External Links

<img src="https://user-images.githubusercontent.com/68193826/265804925-d7e1d5bb-e425-4d75-a573-c155c81a402a.svg" loading="lazy" width="30" alt="" class="image-9"> Description

Dopex is an advanced decentralised option trading solution. with this audit dopex is going to introduce a new coin that is pegged to the value of eth backed by the reserves assets of Ethereum and rdpx. Bonding will happen via options and will also lead to minting out of the stable coin.

To maintain the peg, dopex have meny noval mechanims, such as amo, Relp and over collateralizing and de collateralizing techniques.

Along with these there are vaults that will be managing the Lp and their funding via the premiums paid. For the first epoch option buyer will himself pay the premium and later on all the premium will be paid by protocol in start of each epoch.

The key contracts of the protocol for this Audit are:

  • RdpxV2Core: This function is responsible for performing all the functionalities by using other contracts. It is the entry point for most of things.

  • Rdpx Decaying Bond Contract: Only admin can mint these bonds and are source of compensation for the users for loss accured on the protocol.

  • PerpetualAtlanticVault Contract: Contract which is used to pay all the premiums and fundings to the lp's.

  • PerpetualAtlanticVaultlp Contract: Main entry point for the liquidity provider and is also used by the vault to keep record of funds and assets availible to be used.

  • AMOContract: Two type of AMO are being used, this concept is inspired from the FRAX. Use to add and remove liquidity to the pools

  • ReLP Contract: This contract simply removes the eth liquidity and swap back it into dpx to increase the its value and value of backing reserves.

Existing Patterns: The protocol uses standard Solidity and Ethereum patterns. It uses the ERC721 standards for most part and Accesibility pattern to grant different roles and also there is whitelisting and pausing mechanism too. Protocol is also partially based on ERC4626 standard too but does not follow it completely and it is intentional.

<img src="https://user-images.githubusercontent.com/68193826/265804925-d7e1d5bb-e425-4d75-a573-c155c81a402a.svg" loading="lazy" width="30" alt="" class="image-9"> Approach

I took the top to down approach for this audit, so general flow is as follow

1. rdpxV2Core.sol 2. perpetualAtlanticVault.sol 3. perpetualAtlanticVaultLP.sol 3. Amo and Relp 4. RdpxDecayingBonds.sol

Vaults, Amo and ReLp were reviewed side by side as they have close relationship with each other and balance out each other functionalities or build on top of each other and core interact with each of them. But decaying bonds contract can be reviewed separately.

<img src="https://user-images.githubusercontent.com/68193826/265804925-d7e1d5bb-e425-4d75-a573-c155c81a402a.svg" loading="lazy" width="30" alt="" class="image-9"> Codebase Quality

I would say the codebase quality is good but can be improved, there are checks in place to handle different roles, each standard is ensured to be followed. And if something is not fully being followed that have been informed. But still it can be improved in following areas

Codebase Quality CategoriesComments
Unit TestingCode base is well-tested but there is still place to add fuzz testing and invariant testing and foundry have now solid support for those, I would say best in class.
Code CommentsOverall comments in code base were not enough, more commenting can be done to more specifically describe specifically mathematical calculation. At many point if there were more detail commenting and natspec it would have been bit easy for the auditor and less question would have been asked from sponser, saving time for both. Specifically perpetual vault needs more comments as its flow is hard to understand
DocumentationDocumentation was to the point but not detailed enough, such novelty requires more documentation than one notion page, that can be improved and I am sure will be done as previous products have full proof documentation on dopex website.
OrganizationCode orgranization was great for core, decaying bonds, amo's and Relp. A bit of improvement is needed in vault and lp vault, specifically in vault every thing was all over the place. If some one is not familiar with the flow of function he will remain lost.

<img src="https://user-images.githubusercontent.com/68193826/265804925-d7e1d5bb-e425-4d75-a573-c155c81a402a.svg" loading="lazy" width="30" alt="" class="image-9"> Systematic and Centralisation Risks

1. Centralization Risk:

It is important to note that there are many previliged function in the system that if went rogue can cause significant problems for the protocol, for example roles for minter, fee setters, deployer and other roles granted using access control. But the bigger threat is that in every contract except for ReLP contract there is an emergency withdraw function that can be easily misused at some point and is a very big threat to the protocol.

Emergency withdraw function for either ERC20 or ERC721 or for both is present in following contract:

  1. core have the Emergency withdraw function
  2. V3 amo have recoverERC20 and recoverERC721 functions.
  3. Vault have emergency withdraw function
  4. Lp vault also have the emergency withdraw function.

Theses function may seem necessary in some cases but are constant risk to the protocol. Even for multisig wallets keys keep getting compromised.

2. Pools liquidity and manipulation risk

The whole system is overall very dependent upon the rdpx, dpx, and weth pools on uniswap but while anaysing these on a tool developed by chaos lab here:

https://community.chaoslabs.xyz/uniswap/twap

It can be seen that rdpx/weth pool on both uniswap v2 and v3 can be manipulated for a very less cost, and the highly dependence of dopex on these pool is an innate risk for the protocol.

Lets have a look here on both uniswap v2 and v3

Uniswap v2 analysis Uniswap V3 analysis

image

And even worse for dpx/weth pool as the manipulating the price by 10% only cost around $2000. And exhaustion cost is also way lower than v3 pool.

And dopex is highly dependence on these. so my recommendation is first focus on increasing the liquidity of these pools and making than more manipulation proof will be a good move.

<img src="https://user-images.githubusercontent.com/68193826/265804925-d7e1d5bb-e425-4d75-a573-c155c81a402a.svg" loading="lazy" width="30" alt="" class="image-9"> Conclusion

Overall dopex came up with a very strong solution with some weak points in commenting, docs and some centralisation risk. But the approach used for the core working of protocol is really solid and up to the industry standard and fixing above recommendation will make it even more robust.

Time spent:

20 hours

#0 - c4-pre-sort

2023-09-15T15:52:58Z

bytes032 marked the issue as sufficient quality report

#1 - c4-pre-sort

2023-09-15T15:54:42Z

bytes032 marked the issue as high quality report

#2 - c4-judge

2023-10-20T09:53:42Z

GalloDaSballo marked the issue as grade-a

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax © 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter