Malicious user can prevent rDPX options from being exercised

[code423n4]

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

[c4-pre-sort]

bytes032 marked the issue as duplicate of #619

[c4-pre-sort]

bytes032 marked the issue as sufficient quality report

[c4-judge]

GalloDaSballo marked the issue as satisfactory