Dopex - deadrxsezzz's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L576 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L104

Vulnerability details

Impact

Current rounding up makes too much of a difference with the asset's real price.

Proof of Concept

The current implementation of the RDPXETH oracle returns the price scaled up in 1e8. Note: this is not based only on the mock contract, but everywhere within RdpxV2Core and PerpetualAtlanticVault it is assumed so. As an example:

  function _calculateAmounts(
    uint256 _wethRequired,
    uint256 _rdpxRequired,
    uint256 _amount,
    uint256 _delegateFee
  ) internal view returns (uint256 amount1, uint256 amount2) {
    // Commented below for better clarity
    uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;  // @audit diving by 1e8, because getRdpxPrice is scaled by 1e8

This is how the strike price is calculated:

    uint256 currentPrice = getUnderlyingPrice(); // price of underlying wrt collateralToken
    uint256 strike = roundUp(currentPrice - (currentPrice / 4)); // @audit problematic roundup

  function roundUp(uint256 _strike) public view returns (uint256 strike) {
    uint256 remainder = _strike % roundingPrecision;
    if (remainder == 0) {
      return _strike;
    } else {
      return _strike - remainder + roundingPrecision;
    }
  }

  /// @dev the precision to round up to
  uint256 public roundingPrecision = 1e6;

At the time of report, the price of RDPX is $14.80 and ETH trades at $1,644.96. This means that getRdpxPrice would return a RDPX price of ~0.0089e8 or 8.9e5. Rounding up by 1e6, when the price is in 1e5/1e6 will make TOO MUCH of a difference.

With the current prices, even if the token drops 99%, the price after roundingUp will still report to 1e6.

Due to this huge rounding error, almost always put options will seem as if they're ITM and will wrongfully be paid out.

Tools Used

Manual review

Recommended Mitigation Steps

Either scale up the price from the oracle (e.g. to 1e18) or lower the roundingPrecision (e.g. to 1e2)

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Adversary can fully DoS the RdpxV2Core.sol

Proof of Concept

addToDelegate allows users to delegate WETH which other users are allowed to use. The total WETH delegated is tracked in a variable totalWethDelegated which is subtracted from the WETH balance of the contract, in order to properly keep track of the balance of the protocol. The problem is that if the delegate isn't used, users are free to withdraw it and by doing so totalWethDelegated isn't reduced. By repetitive delegating and withdrawing, a user can infinitely inflate the value of totalWethDelegated. By doing so, the user will break all accounting. All calls to sync will fail due to revert due to underflow (weth balance - totalWethDelegated). Considering both bond and bondWithDelegate make a call to reLP which makes a call to sync, all bond methods will be unusable. Furthermore, the attacker can always make sure totalWethDelegated is equal to the WETH balance, meaning that for the inner accounting, tokenBalance == 0 and all withdraws will be impossible.

  function addToDelegate(
    uint256 _amount,
    uint256 _fee
  ) external returns (uint256) {
    ... 

    // add amount to total weth delegated
    totalWethDelegated += _amount;  // @audit totalWethDelegated is increased

    ...
  }

  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;     // @audit totalWethDelegated is never decreased.

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Here's a Foundry POC showcasing the issue

  function testDelegateIssue() public { 
    uint256 delegateId = rdpxV2Core.addToDelegate(50 * 1e18, 10 * 1e8);
    rdpxV2Core.withdraw(delegateId);
    uint wethBalance = weth.balanceOf(address(rdpxV2Core));
    uint wethDelegated = rdpxV2Core.totalWethDelegated();
    console.log("WETH balance: ", wethBalance);
    console.log("wethDelegated: ", wethDelegated);
    vm.expectRevert(); // @audit - will revert due to underflow 
    rdpxV2Core.sync();
  }

And the Logs

[PASS] testDelegateIssue() (gas: 158057)
Logs:
  WETH balance:  0
  wethDelegated:  50000000000000000000

Tools Used

Manual review

Recommended Mitigation Steps

Decrease totalWethDelegated upon withdrawing a delegate

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L202

Vulnerability details

Impact

Possible full DoS for ReLPContract

Proof of Concept

When taking out a bond in RdpxV2Core if isReLPActive == true, a call to ReLPContract#reLP is made which 're-LPs the pool` (takes out an amount of RDPX, while still perfectly maintaining the token ratio of the pool).

    (uint256 reserveA, uint256 reserveB) = UniswapV2Library.getReserves(  // @audit - here it gets the reserves of the pool and assumes them as owned by the protocol
      addresses.ammFactory,
      tokenASorted,
      tokenBSorted
    );

    TokenAInfo memory tokenAInfo = TokenAInfo(0, 0, 0);

    // get tokenA reserves
    tokenAInfo.tokenAReserve = IRdpxReserve(addresses.tokenAReserve)
      .rdpxReserve(); // rdpx reserves

    // get rdpx price
    tokenAInfo.tokenAPrice = IRdpxEthOracle(addresses.rdpxOracle)
      .getRdpxPriceInEth();

    tokenAInfo.tokenALpReserve = addresses.tokenA == tokenASorted
      ? reserveA
      : reserveB;

    uint256 baseReLpRatio = (reLPFactor *
      Math.sqrt(tokenAInfo.tokenAReserve) *
      1e2) / (Math.sqrt(1e18)); // 1e6 precision

    uint256 tokenAToRemove = ((((_amount * 4) * 1e18) /
      tokenAInfo.tokenAReserve) *
      tokenAInfo.tokenALpReserve *   // @audit - here the total RDPX reserve in the pool is assumed to be owned by the protocol
      baseReLpRatio) / (1e18 * DEFAULT_PRECISION * 1e2);

    uint256 totalLpSupply = IUniswapV2Pair(addresses.pair).totalSupply();

    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) /
      tokenAInfo.tokenALpReserve;

The problem is that the protocol wrongfully assumes that it owns all of the liquidity within the pool. This leads to faulty calculations. In best case scenario wrong amounts are passed. However, when the protocol doesn't own the majority of the pool LP balance, this could lead to full DoS, as lpToRemove will be calculated to be more than the LP balance of UniV2LiquidityAmo and the transaction will revert.

This can all be easily proven by a simple PoC (add the test to the given Periphery.t.sol) Note: there's an added console.log in ReLPContract#reLP, just before the transferFrom in order to better showcase the issue

    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) /
      tokenAInfo.tokenALpReserve;

    console.log("lpToRemove value:    ", lpToRemove);  // @audit - added console.log to prove the underflow
    // transfer LP tokens from the AMO
    IERC20WithBurn(addresses.pair).transferFrom(
      addresses.amo,
      address(this),
      lpToRemove
    );

  function testReLpContract() public {
    testV2Amo();

    // set address in reLP contract and grant role
    reLpContract.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxReserveContract),
      address(uniV2LiquidityAMO),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );
    reLpContract.grantRole(reLpContract.RDPXV2CORE_ROLE(), address(rdpxV2Core));

    reLpContract.setreLpFactor(9e4);

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
    uniV2LiquidityAMO.approveContractToSpend(
      address(pair),
      address(reLpContract),
      type(uint256).max
    );

    rdpxV2Core.setIsreLP(true);


    (uint256 reserveA, uint256 reserveB, ) = pair.getReserves();
    weth.mint(address(2), reserveB * 10);
    rdpx.mint(address(2), reserveA * 10);
    vm.startPrank(address(2));
    weth.approve(address(router), reserveB * 10);
    rdpx.approve(address(router), reserveA * 10);
    router.addLiquidity(address(rdpx), address(weth), reserveA * 10, reserveB * 10, 0, 0, address(2), 12731316317831123);
    vm.stopPrank();
    
    console.log("UniV2Amo balance isn't enough and will underflow");
    uint pairBalance = pair.balanceOf(address(uniV2LiquidityAMO));
    console.log("UniV2Amo LP balance: ", pairBalance);

    vm.expectRevert("ds-math-sub-underflow");
    rdpxV2Core.bond(1 * 1e18, 0, address(this));
}

And the logs:

[PASS] testReLpContract() (gas: 3946961)
Logs:
  UniV2Amo balance isn't enough and will underflow
  UniV2Amo LP balance:  2235173550604750304
  lpToRemove value:     17832559500122488916

Tools Used

Manual review

Recommended Mitigation Steps

Change the logic and base all calculations on the pair balance of UniV2LiquidityAmo

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L273-#L275

Vulnerability details

Impact

Wrong calculation might of mintokenAAmount might lead to less RDPX received and loss of funds.

Proof of Concept

In ReLPContract#reLP we perform a swap of WETH for RDPX. In order to prevent sandwich attacks, slippage protection is used, with mintokenAAmount being calculated within the reLP function. However, due to faulty calculations it will not work properly as wrong formula is used. Context: tokenA -> RDPX ; tokenB -> WETH

    mintokenAAmount =
      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    tokenAInfo.tokenAPrice = IRdpxEthOracle(addresses.rdpxOracle)
      .getRdpxPriceInEth();

As we can see, tokenAInfo.tokenAPrice is the RDPX price in ETH, meaning amountA * tokenAInfo.tokenAPrice / 1e8 = amountB. To confirm this, we can see this same math equation a few lines above in the same function:

    uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice) /
      1e8) -
      ((tokenAToRemove * tokenAInfo.tokenAPrice) * liquiditySlippageTolerance) /
      1e16;

However, when calculating mintokenAAmount, the current logic is (amountB * tokenAInfo.tokenAPrice) / 1e8, when in reality it should be (amountB * 1e8) / tokenAInfo.tokenAPrice (basically reversing the equation from above).

This faulty calculation would lead to improper minAmount calculation and would allow for sandwich attacks.

Tools Used

Manual review

Recommended Mitigation Steps

Change the formula from mintokenAAmount = (amountB * tokenAInfo.tokenAPrice) / 1e8 to mintokenAAmount = (amountB * 1e8) / tokenAInfo.tokenAPrice

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1169-#L1173

Vulnerability details

Impact

People will be unable to bond if discount is higher than 50%

Proof of Concept

When a user is calculating their bond cost, if it is not towards a decaying bond, there is a discount of up to 100%. However, due to flaw in logic any discount over 50% will cause the transaction to revert.

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

      _validate(bondDiscount < 100e8, 14);

      rdpxRequired =
        ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) *  // @audit - this line will revert
          _amount *
          DEFAULT_PRECISION) /
        (DEFAULT_PRECISION * rdpxPrice * 1e2);

  uint256 public constant RDPX_RATIO_PERCENTAGE = 25 * DEFAULT_PRECISION;

Both RDPX_RATIO_PERCENTAGE and bondDiscount are scaled up to 1e8. RDPX_RATIO_PERCENTAGE is a constant with value 25e8. If bondDiscount > 50e8, RDPX_RATIO_PERCENTAGE - (bondDiscount / 2) will revert due to underflow. This is obviously an invariant, as the check above makes sure to validate bondDiscount is up to 100e8.

This can be proven by a very simple PoC

  function testDiscountRevertIssue() public { 
    rdpxV2Core.setBondDiscount(5.1e6); // = 51%
    vm.expectRevert(stdError.arithmeticError);
    (uint256 rdpxRequired, uint256 wethRequired) = rdpxV2Core.calculateBondCost(1 * 1e18, 0);
  }

Tools Used

Manual review

Recommended Mitigation Steps

Either limit the discounts to 50% or change the formula used.

Assessed type

Math