Dopex - dirk_y's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L772-L774 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L346-L361 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205

Vulnerability details

Impact

The Core contract admin can exercise/settle rDPX options at any point in time by calling settle to bring back the peg and back the backing reserves with more ETH.

However a malicious user can send dust WETH directly to the PerpetualAtlanticVaultLP.sol contract to cause a check in subtractLoss to consistently revert. This state can be persisted indefinitely and will prevent any rDPX options from being exercised and therefore the protocol admin cannot use this mechanism to control the DpxEth peg.

Proof of Concept

When the admin wants to exercise some options they can call settle in the core contract, specifying the option ids they want to exercise:

  function settle(
    uint256[] memory optionIds
  )
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 amountOfWeth, uint256 rdpxAmount)
  {
    _whenNotPaused();
    (amountOfWeth, rdpxAmount) = IPerpetualAtlanticVault(
      addresses.perpetualAtlanticVault
    ).settle(optionIds);

As you can see this makes the underlying call to settle in the PerpetualAtlanticVault contract. The interesting part of this method is the part where the WETH is transferred from the LP contract to the core contract, and then the "loss" is accounted in a following call:

    // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
    collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );
    // Transfer rdpx from rdpx rdpxV2Core to perpetual vault
    IERC20WithBurn(addresses.rdpx).safeTransferFrom(
      addresses.rdpxV2Core,
      addresses.perpetualAtlanticVaultLP,
      rdpxAmount
    );

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

where subtractLoss looks like:

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

Here there is check that the actual WETH balance of the contract is exactly equal to the accounted _totalCollateral minus the amount of WETH we have transferred out. However a malicious user can send WETH to the LP contract at any time without _totalCollateral being incremented, thereby causing the check to fail.

This behaviour can be demonstrated with a diff to the existing test suite (execute with forge test --match-path tests/rdpxV2-core/Unit.t.sol):

diff --git a/tests/rdpxV2-core/Unit.t.sol b/tests/rdpxV2-core/Unit.t.sol
index e11c284..8ac9a9e 100644
--- a/tests/rdpxV2-core/Unit.t.sol
+++ b/tests/rdpxV2-core/Unit.t.sol
@@ -318,19 +318,23 @@ contract Unit is ERC721Holder, Setup {
     (uint256 strike3, uint256 amount3, ) = vault.optionPositions(3);
 
     rdpxPriceOracle.updateRdpxPrice(1e6);
-    rdpxV2Core.settle(_ids);
 
-    (, rdpxReserve2, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
-    (, wethReserve2, ) = rdpxV2Core.getReserveTokenInfo("WETH");
+    // Send dust WETH to vault LP
+    weth.transfer(address(vaultLp), 1);
+    vm.expectRevert();
+    rdpxV2Core.settle(_ids);
 
-    assertEq(rdpxReserve1 - amount1 - amount2 - amount3, rdpxReserve2);
-    assertEq(
-      wethReserve1 +
-        ((amount1 * strike1) / 1e8) +
-        ((amount2 * strike2) / 1e8) +
-        ((amount3 * strike3) / 1e8),
-      wethReserve2
-    );
+    // (, 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
+    // );
   }
 
   function testWithdraw() public {

Tools Used

Manual review + Foundry

Recommended Mitigation Steps

Since subtractLoss in the LP contract can only be called by the PerpetualAtlanticVault.sol contract the require check should be removed from the method. The preceding safeTransferFrom would fail anyway if the required amount of WETH couldn't be pulled from the LP contract.

Below is a suggested diff:

diff --git a/contracts/perp-vault/PerpetualAtlanticVaultLP.sol b/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
index 5758161..1179b17 100644
--- a/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
+++ b/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
@@ -197,10 +197,6 @@ contract PerpetualAtlanticVaultLP is ERC20, IPerpetualAtlanticVaultLP {
 
   /// @inheritdoc	IPerpetualAtlanticVaultLP
   function subtractLoss(uint256 loss) public onlyPerpVault {
-    require(
-      collateral.balanceOf(address(this)) == _totalCollateral - loss,
-      "Not enough collateral was sent out"
-    );
     _totalCollateral -= loss;
   }
 

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L964 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L975-L990 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1001-L1003

Vulnerability details

Impact

Users that only hold WETH can delegate their WETH to the Dopex core contract to be used by rDPX token holders to later bond to. During this delegation process the totalWethDelegated variable is incremented. At any point in time a WETH delegate can choose to withdraw their delegate position.

However during this withdrawal of a delegate position the totalWethDelegated isn't decremented. This now means that any call to sync will decrement the internal accounted WETH reserve of the contract by more than intended. A malicious user can deliberately delegate and then withdraw repeatedly to reduce the internal WETH reserve balance to 0, rendering the contract unusable.

Proof of Concept

A user can delegate WETH by calling addToDelegate to delegate an amount of WETH. During this call the totalWethDelegated variable is incremented and WETH is transferred to the contract:

  function addToDelegate(
    uint256 _amount,
    uint256 _fee
  ) external returns (uint256) {
    _whenNotPaused();
    // fee less than 100%
    _validate(_fee < 100e8, 8);
    // amount greater than 0.01 WETH
    _validate(_amount > 1e16, 4);
    // fee greater than 1%
    _validate(_fee >= 1e8, 8);

    IERC20WithBurn(weth).safeTransferFrom(msg.sender, address(this), _amount);

    Delegate memory delegatePosition = Delegate({
      amount: _amount,
      fee: _fee,
      owner: msg.sender,
      activeCollateral: 0
    });
    delegates.push(delegatePosition);

    // add amount to total weth delegated
    totalWethDelegated += _amount;

    emit LogAddToDelegate(_amount, _fee, delegates.length - 1);
    return (delegates.length - 1);
  }

A user can later choose to withdraw any unused WETH by calling withdraw. In this method the WETH is transferred back to the original delegate, but the totalWethDelegated variable isn't decremented:

  function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;

    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Now, the sync method can be used to sync the accounted asset reserves with the actual contract balances. The purpose of this method is to allow the contract to use tokens sent directly to it. The WETH token is a special case since the delegated WETH isn't counted in the reserves until it is bonded with:

  function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }
      reserveAsset[i].tokenBalance = balance;
    }

    emit LogSync();
  }

The actual balance of WETH in the contract before and after a delegate + withdraw pair of transactions is the same, but totalWethDelegated is larger. Therefore, the reserve balance of WETH has been wrongly decremented, breaking the internal accounting.

Below is an addition to the existing test suite that demonstrates this behaviour:

diff --git a/tests/rdpxV2-core/Unit.t.sol b/tests/rdpxV2-core/Unit.t.sol
index e11c284..c32e62d 100644
--- a/tests/rdpxV2-core/Unit.t.sol
+++ b/tests/rdpxV2-core/Unit.t.sol
@@ -333,6 +333,14 @@ contract Unit is ERC721Holder, Setup {
     );
   }
 
+  function testTokenBalanceBroken() public {
+    rdpxV2Core.sync();
+    uint256 delegateId = rdpxV2Core.addToDelegate(50 * 1e18, 10 * 1e8);
+    rdpxV2Core.withdraw(delegateId);
+    vm.expectRevert(); // Reason: Arithmetic over/underflow
+    rdpxV2Core.sync();
+  }
+
   function testWithdraw() public {
     rdpxV2Core.addToDelegate(1 * 1e18, 10e8);
 

Tools Used

Manual review

Recommended Mitigation Steps

The totalWethDelegated variable should be decremented when withdraw is called. Below is a suggested diff:

diff --git a/contracts/core/RdpxV2Core.sol b/contracts/core/RdpxV2Core.sol
index e28a65c..68ca204 100644
--- a/contracts/core/RdpxV2Core.sol
+++ b/contracts/core/RdpxV2Core.sol
@@ -983,6 +983,7 @@ contract RdpxV2Core is
     amountWithdrawn = delegate.amount - delegate.activeCollateral;
     _validate(amountWithdrawn > 0, 15);
     delegate.amount = delegate.activeCollateral;
+    totalWethDelegated -= amountWithdrawn;
 
     IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
 

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

When a user calls bond or bondWithDelegate they specify the amount of DpxEth they want to bond for. As part of this call the cost of the bond (in WETH and rDPX) is calculated. The discount provided for a bond depends on the bond discount factor and the amount of rDPX in the reserve. The bond discount factor should remain relatively static since it is modified only by the admin, but the amount of rDPX in the reserve is constantly fluctuating.

This is therefore a problem, because the discount at the time where the users tries to bond might be different from the actual discount when the transaction is included in a block since the rDPX in the reserve may have changed. Therefore the bonder will end up paying more WETH and/or rDPX for the given amount of bonds than expected.

Since the bonders are still receiving a discount I have lowered the severity of this report from "high" to "medium", however I still consider this a bug because a bonder might expect/require a certain discount in order to make their whole trading strategy profitable.

Proof of Concept

For the sake of this report, let's focus on the normal bond path:

  function bond(
    uint256 _amount,
    uint256 rdpxBondId,
    address _to
  ) public returns (uint256 receiptTokenAmount) {
    _whenNotPaused();
    // Validate amount
    _validate(_amount > 0, 4);

    // Compute the bond cost
    (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
      _amount,
      rdpxBondId
    );

    IERC20WithBurn(weth).safeTransferFrom(
      msg.sender,
      address(this),
      wethRequired
    );

When calling bond, the amount of bond to mint is specified, and the cost of those bonds is then calculated with a call to calculateBondCost and the WETH transferred from the user (the rDPX is also transferred later). As you can see, there is no way for the user to specify the maximum amount of rDPX or WETH that they want to spend to purchase the bonds.

Now, let's have a look at the first part of the discount calculation logic:

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

    if (_rdpxBondId == 0) {
      uint256 bondDiscount = (bondDiscountFactor *
        Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) *
        1e2) / (Math.sqrt(1e18)); // 1e8 precision

As you can see, bondDiscount varies with bondDiscountFactor and the amount of rDPX in the reserve. Since the user has no control over the rDPX in the reserve, it is possible that this can change from block to block, thereby changing the discount and therefore the WETH & rDPX spent by the user.

This is now a classic "lack of slippage protection" bug that should be resolved as discussed below.

Tools Used

Manual review

Recommended Mitigation Steps

The bond and bondWithDelegate should include an additional slippage argument to cap the amount of either WETH or rDPX that they want to spend. Since the ratio of rDPX to WETH is fixed you technically need only a maxAmount for one of these; I would suggest maxRDPX.

Assessed type

Other

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