Dopex - T1MOH's results

Lines of code

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

Vulnerability details

Impact

Malicious user can donate as little as 1 wei of WETH to PerpetualAtlanticVaultLP.sol. In this case internal variable _totalCollateral is not equal to actual WETH balance. And balance check in function subtractLoss() is too strict, making it revert.

As a result, options can't be settled

Proof of Concept

This check is too strict. It will revert in case of free donation to contract.

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

Correct would be to check >=, but there is no sense in this check then

Tools Used

Manual Review

Recommended Mitigation Steps

Because of possibility of free donation, there is no sense in this check.

However you can create function like sync() to set _totalCollateral = weth.balanceOf(address(this)), and then call it before interaction: https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L346-L361

+   perpetualAtlanticVaultLP.syncTotalCollateral();

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

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

ReLP removes liquidity from RDPX/ETH UniV2 pool, swaps ETH for RDPX and adds liquidity again. It results in increasing of RDPX price in pool. On step 2 (swap) it calculates min amount of RDPX to receive, but price RDPX/ETH is used instead of ETH/RDPX. It results in calculated amount mintokenAAmount to be much less than intended. Current price of RDPX is 20 USD, ETH is 1800 USD. Received amount is (1800/20) ^ 2 = 8100 times less than intended, slippage is around 99.99%

Proof of Concept

I created PoC which imitates behaviour of reLP(): remove liquidity and swap tokenB for tokenA

  function testSlippageInReLP() public {

    // set slippage to 0% to ease calculations
    uint256 liquiditySlippageTolerance = 0;
    uint256 slippageTolerance = 0;

    // suppose RDPX/ETH pool has these reserves
    uint256 tokenALpReserve = 10_000e18;
    uint256 tokenBLpReserve = 100e18;
    uint256 totalLpSupply = 1_000e18; // In uniV2 it is sqrt(Xreserve * Yreserve)
    uint256 tokenAPrice = 0.01e8; // precission 1e8

    // Suppose this amount is removed
    uint256 tokenAToRemove = 100e18;

    //@note formula from
    // https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L239-L240
    uint256 lpToRemove = (tokenAToRemove * totalLpSupply) / tokenALpReserve;
    assertEq(lpToRemove, 10e18);

    //@note formula from
    // https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L250-L251
    uint256 mintokenAAmount = tokenAToRemove -
      ((tokenAToRemove * liquiditySlippageTolerance) / 1e8);
    assertEq(mintokenAAmount, 100e18);

    //@note formula from
    // https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L252-L255
    uint256 mintokenBAmount = ((tokenAToRemove * tokenAPrice) /
      1e8) -
      ((tokenAToRemove * tokenAPrice) * liquiditySlippageTolerance) /
      1e16;
    assertEq(mintokenBAmount, 1e18);

    /* REMOVE LIQUIDITY
    * It is imitating test, therefore make internal calculations instead of actual swap
    * lpToRemove is 10e18, i.e. 1% of reserves. It means contract receive 100e18 tokenA and 1e18 tokenB
    */
    // https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L257
    uint256 amountB = 1e18;

    //@note formula from
    // https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L273-L275
    mintokenAAmount =
      (((amountB / 2) * tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAPrice * slippageTolerance) / 1e16);
    
    // tokenAPrice = 0.01e8. It means 1 RDPX = 0.01 ETH. We swap 0.5 ETH, and expect to receive 50 RDPX
    assertEq(mintokenAAmount, 50e18); // But we get 50e18 / 100 ^ 2 = 0.005e18

    console.log("Actual price of swap (1e8 precission): ");
    console.log((amountB / 2) * 1e8 / mintokenAAmount); // 100e8 instead of 0.01e8
    console.log();
    console.log("Expected price of swap (1e8 precission): ");
    console.log(tokenAPrice);
  }

Logs:
  Error: a == b not satisfied [uint]
        Left: 5000000000000000
       Right: 50000000000000000000
  Actual price of swap (1e8 precission): 
  10000000000
  
  Expected price of swap (1e8 precission): 
  1000000

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

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

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

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

Current implementation calculates minOut incorrect. It uses reverse price, and swap with this calculated minOut has lower expected minOut than intended. As a result, automatic calculation of slippage tolerance is incorrect, and protocol suffers loss if uses it

Proof of Concept

Note that argument _amount is in terms of DpxEth. Assume now 1 DpxEth = 1.01 ETH, i.e. getDpxEthPrice() = 1.01e8 and getEthPrice() = 0.99e8

  /**
   * @notice Lets users mint DpxEth at a 1:1 ratio when DpxEth pegs above 1.01 of the ETH token
   * @param  _amount The amount of DpxEth to mint
   * @param minOut The minimum amount out
   **/
  function upperDepeg(
    uint256 _amount,
    uint256 minOut
  ) external onlyRole(DEFAULT_ADMIN_ROLE) returns (uint256 wethReceived) {
    _isEligibleSender();

    _validate(getDpxEthPrice() > 1e8, 10);

    IDpxEthToken(reserveAsset[reservesIndex["DPXETH"]].tokenAddress).mint(
      address(this),
      _amount
    );

    // Swap DpxEth for ETH token
    wethReceived = _curveSwap(_amount, false, true, minOut);

    ...
  }

Look how minOut is calculated inside of _curveSwap():

  function _curveSwap(
    uint256 _amount,
    bool _ethToDpxEth, // false
    bool validate, // true
    uint256 minAmount
  ) internal returns (uint256 amountOut) {
    ...

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

    // Swap the tokens
    amountOut = dpxEthCurvePool.exchange(
      _ethToDpxEth ? int128(int256(a)) : int128(int256(b)),
      _ethToDpxEth ? int128(int256(b)) : int128(int256(a)),
      _amount,
      minAmount > 0 ? minAmount : minOut
    );
  }

If called from upperDepeg(), this formula will be used:

    uint256 minOut = (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

But getEthPrice() is price of 1 ETH in terms of DpxEth, correct usage is to divide by this price

Suppose follow scenario:

1. 1 DpxEth = 1.01 ETH; 1 ETH = 0.99 DpxEth; reserves are 1000 DpxEth and 1010 ETH
2. Traded amount is 5 DpxEth
3. slippageTolerance is 0 to ease calculation
4. Then minOut = 5e18 * 0.99e8 / 1e8 = 4.95 ETH. But correct would be 5.05 ETH As a result contract can lose portion of tokens

Similar issue when trade ETH to DpxETH, price is also reversed

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L563-L569

Vulnerability details

Impact

fundingDuration initially is 7 days. It's the main variable with genesis and latestFundingPaymentPointer to calculate bounds of funding epochs. However this calculation will break if fundingDuration is changed.

Setting fundingDuration > 7 days will expand current epoch to too big value. For example if you set fundingDuration = 14 days while there were 10 epochs, current epoch will last 14 * 10 - 7 * 10 = 70 days.

Some other inconsistencies introduced within this root cause, I can explain later if needed

Proof of Concept

1. Suppose genesis = 10_000, fundingDuration = 50, latestFundingPaymentPointer = 10 (10 epochs already passed). Now nextFundingPaymentTimestamp() returns 10_000 + 50 * 10 = 10_500.

  function nextFundingPaymentTimestamp()
    public
    view
    returns (uint256 timestamp)
  {
    return genesis + (latestFundingPaymentPointer * fundingDuration);
  }

2. Note that current timestamp is 10_500 in this case.
3. Now fundingDuration is set to 100. nextFundingPaymentTimestamp() will return 10_000 + 100 * 10 = 11_000.
4. But current timestamp is 10_500, end epoch ends in 11_000. But intended duration is 100

This view function nextFundingPaymentTimestamp() is used internally , therefore impacts protocol's behaviour

Tools Used

Manual Review

Recommended Mitigation Steps

Easy solution is to disallow updating of fundingDuration Or you can refactor internal calculations to handle this case

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L904-L907

Vulnerability details

Impact

In fact function bond() is swap of WETH + RDPX to DpxEth. User must specify _amount of DpxETh he wants to get, but can't specify max amounts of RDPX and WETH that will be used. Worth noting that required amounts of WETH and RDPX depend on dynamic factors: rdpxPrice and rdpxReserve, that can change between sending of transaction and confirming

Proof of Concept

1. Function calculateBondCost() is used to calculate required amounts of WETH and RDPX. However they depend on dynamic rdpxPrice:

  function calculateBondCost(
    uint256 _amount,
    uint256 _rdpxBondId
  ) public view returns (uint256 rdpxRequired, uint256 wethRequired) {
@>  uint256 rdpxPrice = getRdpxPrice();

    if (_rdpxBondId == 0) {
      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)) *
          _amount *
          DEFAULT_PRECISION) /
@>      (DEFAULT_PRECISION * rdpxPrice * 1e2);

      wethRequired =
     ((ETH_RATIO_PERCENTAGE - (bondDiscount / 2)) * _amount) /
        (DEFAULT_PRECISION * 1e2);
    } else {
      // if rdpx decaying bonds are being used there is no discount
      rdpxRequired =
        (RDPX_RATIO_PERCENTAGE * _amount * DEFAULT_PRECISION) /
@>      (DEFAULT_PRECISION * rdpxPrice * 1e2);

      wethRequired =
        (ETH_RATIO_PERCENTAGE * _amount) /
        (DEFAULT_PRECISION * 1e2);
    }

    ...
}

2. And what is more important, put option price depends on current block.timestamp ( in other words on time left to settle this option, it is calculated via Black Scholes formula). Suppose there is left 5 minutes in current epoch, then put option is cheap. But transaction is mined after 6 minutes when new epoch just started, it makes put option much more expensive than intended. And user pays much more

Tools Used

Manual Review

Recommended Mitigation Steps

  function bond(
    uint256 _amount,
    uint256 rdpxBondId,
-   address _to
+   address _to,
+   uint256 wethMax,
+   uint256 rdpxMax
  ) public returns (uint256 receiptTokenAmount) {
    _whenNotPaused();
    // Validate amount
    _validate(_amount > 0, 4);

    // Compute the bond cost
    (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
      _amount,
      rdpxBondId
    );
+   require(rdpxRequired <= rdpxMax && wethRequired <= wethMax);
  }

Implement the same in bondWithDelegate()

Assessed type

MEV

Judge has assessed an item in Issue #425 as 2 risk. The relevant finding follows:

6. UniLiquidityAmo contracts doesn’t synchronize reserve balances of RdpxV2Core in some cases Impact Developer from Dopex said that “we keep the balances to check the health of dpxEth”. I talk about this balances: https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L61

/// @notice Array containg the reserve assets ReserveAsset[] public reserveAsset;

/// @dev Struct containing the the rdpxV2Core reserve asset data struct ReserveAsset { address tokenAddress; uint256 tokenBalance; string tokenSymbol; } tokenBalance can differ from actual balance in this cases:

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L313-L322 function recoverERC20( address tokenAddress, uint256 tokenAmount ) external onlyRole(DEFAULT_ADMIN_ROLE) { // Can only be triggered by owner or governance, not custodian // Tokens are sent to the custodian, as a sort of safeguard TransferHelper.safeTransfer(tokenAddress, rdpxV2Core, tokenAmount);

emit RecoveredERC20(tokenAddress, tokenAmount);

} https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L119-L133 function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) { for (uint i = 0; i < positions_array.length; i++) { Position memory current_position = positions_array[i]; INonfungiblePositionManager.CollectParams memory collect_params = INonfungiblePositionManager.CollectParams( current_position.token_id, rdpxV2Core, type(uint128).max, type(uint128).max );

  // Send to custodian address
  univ3_positions.collect(collect_params);
}

} https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178 function _sendTokensToRdpxV2Core() internal { uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf( address(this) ); uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf( address(this) ); // transfer token A and B from this contract to the rdpxV2Core IERC20WithBurn(addresses.tokenA).safeTransfer( addresses.rdpxV2Core, tokenABalance ); IERC20WithBurn(addresses.tokenB).safeTransfer( addresses.rdpxV2Core, tokenBBalance );

emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);

} Recommended Mitigation Steps Call IRdpxV2Core(rdpxV2Core).sync() in all the referenced code blocks

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/reLP/ReLPContract.sol#L286-L295

Vulnerability details

Impact

This function performs 3 operations:

1. Remove liquidity
2. Swap half of received tokenB to tokenA (i.e. swap ETH to RDPX)
3. Add liquidity: provide second half of tokenB and received tokenA amount from swap

Contract expects to provide second half of tokenB, however due to slippage (during swap) full amount can't be provided. At least 0.3% of amount won't be provided, and tokenB is never transferred out of ReLPContract.sol

Proof of Concept

UniswapV2's addLiquidity() can receive amounts of tokens in ratio of reserves. It must not change price. I.e if reserveA = 100e18 and reserveB = 10e18, user can only provide amounts of tokens when tokenAAmount = 10 * tokenBAmount

UniswapV2 has hardcoded fee of 0.3% in swaps.

1. To ease calculation assume ideal world without slippage, only with this fee 0.3%
2. reserveA = reserveB (I.e. tokens are stablecoins, it eases calculations)
3. tokenBAmountProvided is swapped to tokenA. Received tokenAAmount = 0.997 * tokenBAmountProvided
4. Ratio of reserves didn't change (suppose swapped amount is too small compared to reserves), and equals 1 as before
5. And now third step - provide liquidity. tokenBAmountProvided of tokenB and tokenAAmount of tokenA. But ratio of reserves equals to 1, pool can receive only 0.997 * tokenBAmountProvided of tokenB and tokenAAmount = 0.997 * tokenBAmountProvided of tokenA. That's the idea I point to. At least 0.003 of tokenB will remain in contract, it can't be provided

Let's take a look on code amountB is received during removeLiquidity

    (, uint256 amountB) = IUniswapV2Router(addresses.ammRouter).removeLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      lpToRemove,
      mintokenAAmount,
      mintokenBAmount,
      address(this),
      block.timestamp + 10
    );

First half of amountB is swapped to tokenA. Remember that fee 0.3% is applied to tokenAAmountOut, it's less than in ideal math model

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

And here liquidity provided. Remember that 1) due to the fee applied tokenAAmountOut is less than expected. 2) And remember that addLiquidity can't change reserve ratio. Because of fact 2, pool can't receive whole amountB / 2, at most 0.997 * amountB / 2

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );

In other words this share of tokenB (at least 0.3%) will remain every call

Tools Used

Manual Review

Recommended Mitigation Steps

Send remaining ETH to rdpxV2Core, or to admin

Assessed type

Math

1. Memory will be corrupted if add reserve with the same symbol

Impact

There is check to prevent adding asset with duplicate address, but it doesn't check whether tokenSymbol was previously used. If yes, old assetReserve will be overriden with the new one

Proof of Concept

Suppose there already exists "DPXETH" reserve with totalSupply = 1000. Now reserve "DPXETH" is added but with different address, old is overriden https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L240-L264

  function addAssetTotokenReserves(
    address _asset,
    string memory _assetSymbol
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(_asset != address(0), "RdpxV2Core: asset cannot be 0 address");

    for (uint256 i = 1; i < reserveAsset.length; i++) {
      require(
        reserveAsset[i].tokenAddress != _asset,
        "RdpxV2Core: asset already exists"
      );
    }

    ReserveAsset memory asset = ReserveAsset({
      tokenAddress: _asset,
      tokenBalance: 0,
      tokenSymbol: _assetSymbol
    });
    reserveAsset.push(asset);
    reserveTokens.push(_assetSymbol);

    //@audit HERE OLD RESERVE WILL BE OVERRIDEN WITH THE NEW ONE
@>  reservesIndex[_assetSymbol] = reserveAsset.length - 1;

    emit LogAssetAddedTotokenReserves(_asset, _assetSymbol);
  }

Recommended Mitigation Steps

    for (uint256 i = 1; i < reserveAsset.length; i++) {
      require(
        reserveAsset[i].tokenAddress != _asset,
        "RdpxV2Core: asset already exists"
      );

+     require(reserveAsset[i].tokenSymbol != _assetSymbol);
    }

2. Allow to set 0 approval

Impact

For example USDT reverts when set non-zero approval from non-zero approval. Current implementation doesn't allow to change approval for such token, because requires amount > 0

Proof of Concept

1. https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L133

  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(_token != address(0), "reLPContract: token cannot be 0");
    require(_spender != address(0), "reLPContract: spender cannot be 0");
    require(_amount > 0, "reLPContract: amount must be greater than 0");
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

2. https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L410

  function approveContractToSpend(
    address _token,
    address _spender,
    uint256 _amount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _validate(_token != address(0), 17);
    _validate(_spender != address(0), 17);
    _validate(_amount > 0, 17);
    IERC20WithBurn(_token).approve(_spender, _amount);
  }

Recommended Mitigation Steps

Remove this requirement, allow to pass 0

3. RdpxDecayingBonds.sol can't send native value

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/decaying-bonds/RdpxDecayingBonds.sol#L97

There is functionality to send native value in emergencyWithdraw(). But contract doesn't have payable and receive functions, therefore there won't be ether. And contract will never be able to perform transfer of native value

  function emergencyWithdraw(
    address[] calldata tokens,
    bool transferNative,
    address payable to,
    uint256 amount,
    uint256 gas
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _whenPaused();
    if (transferNative) {
      (bool success, ) = to.call{ value: amount, gas: gas }("");
      require(success, "RdpxReserve: transfer failed");
    }
    
    ...
  }

The same with UniV3LiquidityAmo.sol https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L344

Recommended Mitigation Steps

Remove functionality of native asset transfer or add payable function

4. RdpxV2Core.sol can issue bonds with immediate maturity

Impact

Maturity is not set explicitly in constructor, it is set via setter. And this function setBondMaturity() can be called in last transaction of deploy, such that user frontruns it and calls bond(). As a result bond is issued with maturity at current block.timestamp

Recommended Mitigation Steps

Pause protocol in constructor, then set all the values, and unpause

5. Confusing comment on upperDepeg()

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1047

Is is stated in docs and the code that upperDepeg() is executed when 1 DPXETH > 1 ETH. But in comment it's stated when 1 DPXETH > 1.01 ETH

  /**
@> * @notice Lets users mint DpxEth at a 1:1 ratio when DpxEth pegs above 1.01 of the ETH token
   * @param  _amount The amount of DpxEth to mint
   * @param minOut The minimum amount out
   **/
  function upperDepeg(
    uint256 _amount,
    uint256 minOut
  ) external onlyRole(DEFAULT_ADMIN_ROLE) returns (uint256 wethReceived) {
    _isEligibleSender();

@>  _validate(getDpxEthPrice() > 1e8, 10);

    ...
  }

6. UniLiquidityAmo contracts doesn't synchronize reserve balances of RdpxV2Core in some cases

Impact

Developer from Dopex said that "we keep the balances to check the health of dpxEth". I talk about this balances: https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L61

  /// @notice Array containg the reserve assets
  ReserveAsset[] public reserveAsset;

  /// @dev Struct containing the the rdpxV2Core reserve asset data
  struct ReserveAsset {
    address tokenAddress;
    uint256 tokenBalance;
    string tokenSymbol;
  }

tokenBalance can differ from actual balance in this cases:

1. https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L313-L322

  function recoverERC20(
    address tokenAddress,
    uint256 tokenAmount
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    // Can only be triggered by owner or governance, not custodian
    // Tokens are sent to the custodian, as a sort of safeguard
    TransferHelper.safeTransfer(tokenAddress, rdpxV2Core, tokenAmount);

    emit RecoveredERC20(tokenAddress, tokenAmount);
  }

2. https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L119-L133

  function collectFees() external onlyRole(DEFAULT_ADMIN_ROLE) {
    for (uint i = 0; i < positions_array.length; i++) {
      Position memory current_position = positions_array[i];
      INonfungiblePositionManager.CollectParams
        memory collect_params = INonfungiblePositionManager.CollectParams(
          current_position.token_id,
          rdpxV2Core,
          type(uint128).max,
          type(uint128).max
        );

      // Send to custodian address
      univ3_positions.collect(collect_params);
    }
  }

3. https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178

  function _sendTokensToRdpxV2Core() internal {
    uint256 tokenABalance = IERC20WithBurn(addresses.tokenA).balanceOf(
      address(this)
    );
    uint256 tokenBBalance = IERC20WithBurn(addresses.tokenB).balanceOf(
      address(this)
    );
    // transfer token A and B from this contract to the rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      tokenABalance
    );
    IERC20WithBurn(addresses.tokenB).safeTransfer(
      addresses.rdpxV2Core,
      tokenBBalance
    );

    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
  }

Recommended Mitigation Steps

Call IRdpxV2Core(rdpxV2Core).sync() in all the referenced code blocks