Dopex - SBSecurity's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L758-L783 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L198-L205

Vulnerability details

Impact

When dpxETH price deppegs from the price of Eth settle can be called by the protocol admin. It will remove passed options/positions and this will bring back the backing of the 2 tokens. If user knows that his option is going to be passed as an argument to the settle function he can send only 1 wei to the vaultLp collateral token (WETH, confirmed by the sponsors) which will make PerpetualAtlanticVaultLp.substractLoss revert, and avoid getting slashed.

Proof of Concept

Admin going to call settle passing option owned by Bob. Bob knows and beforehand transfers 1 wei to the vaultLp ’s collateral which is WETH.

contracts/perp-vault/PerpetualAtlanticVault.sol

function settle(
    uint256[] memory optionIds
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
  {
    _whenNotPaused();
    _isEligibleSender();

    updateFunding();

    for (uint256 i = 0; i < optionIds.length; i++) {
      uint256 strike = optionPositions[optionIds[i]].strike;
      uint256 amount = optionPositions[optionIds[i]].amount;

      // check if strike is ITM
      _validate(strike >= getUnderlyingPrice(), 7);

      ethAmount += (amount * strike) / 1e8;
      rdpxAmount += amount;
      optionsPerStrike[strike] -= amount;
      totalActiveOptions -= amount;

      // Burn option tokens from user
      _burn(optionIds[i]);

      optionPositions[optionIds[i]].strike = 0;
    }

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

//@audit problem will occur on this line since substractLoss uses strict equality which will be broken by directly transfering tokens to the underlying contract
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
      .unlockLiquidity(ethAmount);
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(
      rdpxAmount
    );

    emit Settle(ethAmount, rdpxAmount, optionIds);
  }

contracts/perp-vault/PerpetualAtlanticVaultLP.sol

function subtractLoss(uint256 loss) public onlyPerpVault {
    //@audit require will revert because collateral.balanceOf(address(this)) will have 1 wei more
		require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

There is a test written to demonstrate how to vulnerability will occur:

function testAuditSettleCantBeCalledIfUserSends1WeiToCollateral() public {
    weth.mint(address(1), 1 ether);
    deposit(1 ether, address(1));

    vault.purchase(1 ether, address(this));

    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;

    priceOracle.updateRdpxPrice(0.2 gwei); // initial price * 10
    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;

    priceOracle.updateRdpxPrice(0.01 gwei);

    //@audit send 1 wei to break accounting
    weth.transfer(address(vaultLp), 1 wei);
    vm.expectRevert();
    vault.settle(ids);
}

Tools Used

Manual

Recommended Mitigation Steps

Instead of using strict equality, ensure that the collateral token have enough balance to satisfy the condition.

contracts/perp-vault/PerpetualAtlanticVaultLP.sol

function subtractLoss(uint256 loss) public onlyPerpVault {
    //@audit require will revert because collateral.balanceOf(address(this)) will have 1 wei more
    require(
-     collateral.balanceOf(address(this)) == _totalCollateral - loss,
+     collateral.balanceOf(address(this)) >= _totalCollateral - loss,  
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
}

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2023-08-dopex/blame/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L515-L558 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L545-L549

Vulnerability details

Impact

Big slippage caused by wrong decimals calculation when using deppeg functions of the RdpxV2Core contract.

Proof of Concept

This functions is being called everytime RdpxV2Core admin wants to bring the deppeg of the dpxETH/ETH tokens back by minting and burning tokens based on the current case.

Even though protocol will be deployed on Arbitrum this vulnerability can still be exploited.

We assume _amount is in 1e18 decimals but the slippage will be the same no matter the decimals, because the same decimals will be added to the both sides of the equation.

//@audit    left: 18 + 8 - 8 = 18  right: 18 + 8 + 5 = 31 - 16 = 15
 uint256 minOut = _ethToDpxEth
            ? (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
            : (((_amount * getEthPrice()) / 1e8) - (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

On the left side of the equation we will receive value with 18 decimals, while on the right side there will be 15 decimals. And now when we have let's say 5e18 - 2e15, result will be 4.99e18 instead of 3e18.

Tools Used

Manual

Recommended Mitigation Steps

Modify the code by changing the number by which the whole amount is divided:

 // calculate minimum amount out
        uint256 minOut = _ethToDpxEth
-            ? (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
-            : (((_amount * getEthPrice()) / 1e8) - (((_amount * getEthPrice()) * slippageTolerance) / 1e16));
+            ? (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e13))
+            : (((_amount * getEthPrice()) / 1e8) - (((_amount * getEthPrice()) * slippageTolerance) / 1e13));

Assessed type

Decimal