Dopex - chaduke's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. DOS attack can be launched to PerpetualAtlanticVaultLP.subtractLoss(). This is because the condition collateral.balanceOf(address(this)) == _totalCollateral - loss will always fail if a malicious user donates some collateral tokens to the PerpetualAtlanticVaultLP contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

PerpetualAtlanticVaultLP.subtractLoss() allows the PerpetualAtlanticVault contract to readjust the value of _totalCollateral based on the input loss. It checks that collateral.balanceOf(address(this)) == _totalCollateral - loss

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

However, if a malicious user donates some collateral tokens to the PerpetualAtlanticVaultLP contract, then the condition collateral.balanceOf(address(this)) == _totalCollateral - loss will break forever, and therefore, subtractLoss() will be blocked forever.

Therefore, a user can easily launch a DOS attack to PerpetualAtlanticVaultLP.subtractLoss() and PerpetualAtlanticVault.settle() (since it calls the former) by sending free collateral tokens to the PerpetualAtlanticVaultLP contract.

Tools Used

VSCode

Recommended Mitigation Steps

Change the condition collateral.balanceOf(address(this)) == _totalCollateral - loss to collateral.balanceOf(address(this)) >= _totalCollateral - loss to prevent donation-based DOS attack:

  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/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L324-L336

Vulnerability details

Impact

Detailed description of the impact of this finding. All ERC721 tokens recovered by UniV3LiquidityAmo.recoverERC721() into rdpxV2Core will be lost in rdpxV2Core forever. The main reason is that there is no emergency withdrawl function in RdpxV2Core.sol for ERC721 and no other functions can perform any operations on those recovered ERC721 tokens.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

UniV3LiquidityAmo.recoverERC721() allows one to recover ERC721 tokens. However, the ERC721 tokens will be sent to rdpxV2Core:

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

Unfortunately, once sent to rdpxV2Core, there is no emergency withdrawl function in RdpxV2Core.sol to withdraw them. No other functions exists to deal with these ERC721 tokens. Therefore, they will be lost in the contract forever.

Tools Used

VScode

Recommended Mitigation Steps

Add emergency withdrawl functions for ERC721 tokens in RdpxV2Core.sol as well.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. PerpetualAtlanticVaultLP.deposit() calls previewDeposit(assets) before calling perpetualAtlanticVault.updateFunding(), as a result, the depositor will receive more shares they he deserves.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

PerpetualAtlanticVaultLP.deposit() calls perpetualAtlanticVault.updateFunding() so that due funds can be sent from the PerpetualAtlanticVault to the PerpetualAtlanticVaultLP, which would increase the _totalCollateral.

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

While the redeem() function calls perpetualAtlanticVault.updateFunding() as the first task; PerpetualAtlanticVaultLP.deposit() calls previewDeposit(assets) first before calling perpetualAtlanticVault.updateFunding(). As a result, the number of shares of LP tokens will be larger than it should be since the due funds have not been transferred to the PerpetualAtlanticVaultLP contract yet, which would increase the _totalCollateral value.

As a result, a depositor will receive more shares of LP tokens they he deserves, which means loss of funds to the contract and thus to some other users.

Tools Used

VSCode

Recommended Mitigation Steps

Call perpetualAtlanticVault.updateFunding() first in deposit():

 function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {

+    perpetualAtlanticVault.updateFunding();

    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

-    perpetualAtlanticVault.updateFunding();

    // Need to transfer before minting or ERC777s could reenter.
    collateral.transferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    _totalCollateral += assets;

    emit Deposit(msg.sender, receiver, assets, shares);
  }

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. RdpxV2Core.withdraw() fails to decrease totalWethDelegated. As a result, the state of totalWethDelegated and reserveAsset[i].tokenBalance for Weth might be wrong.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The RdpxV2Core.withdraw() allows the owner of a delegate to withdraw the portion of the weth that is not in the portion of activeCollateral.

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

However, the function does not decrease totalWethDelegated. As a result, the accounting totalWethDelegated, and thus reserveAsset[i].tokenBalance for Weth are all wrong. Such wrong contract state can lead to unpredictable future result.

Tools Used

VScode

Recommended Mitigation Steps

Decrease totalWethDelegated for RdpxV2Core.withdraw():

  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;

+   totalWethDelegated  -= amountWithdrawn

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

  /**

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

Detailed description of the impact of this finding. nextFundingPaymentTimestamp() assumes that each funding duration is the same, which is not true, since an admin can change the funding duration via updateFundingDuration(), which should not be retrospective.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

PerpetualAtlanticVault.nextFundingPaymentTimestamp() assumes that each funding duration is the same

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

This assumption is not true since updateFundingDuration() can change it, but not retrospectively.

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

As a result, the way to calculate nextFundingPaymentTimestamp is wrong. It cannot be genesis + (latestFundingPaymentPointer * fundingDuration);.

Tools Used

VCCode

Recommended Mitigation Steps

Introduce a variable lastFundingPaymentTimestamp or do not allow the change of funding duration once latestFundingPaymentPointer >= 1.

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

+    return lastFundingPaymentTimestamp + fundingDuration; 
  
}


## Assessed type

Math

QA1. Some ERC20 tokens do not support allowance approval from non-zero to non-zero. So the following RdpxV2Core.approveContractToSpend() code will not work.

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

Correction: approve to zero first before approving to the designated amount for a safe approval:

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

+     uint256 allowance = IERC20WithBurn(_token).allowance(msg.sender, _spender);
+    if(allowance != 0) IERC20WithBurn(_token).approve(_spender, 0);


    IERC20WithBurn(_token).approve(_spender, _amount);
  }

QA2. Possible divide by zero error in perpetualAtlanticVault._updateFundingRate() due to lack of check of wether endTime == startTime for the case of fundingRates[latestFundingPaymentPointer] == 0.

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

Mitigation: we need to check endTime == startTime for the case of fundingRates[latestFundingPaymentPointer] == 0 as well.

 function _updateFundingRate(uint256 amount) private {
    if (fundingRates[latestFundingPaymentPointer] == 0) {
      uint256 startTime;
      if (lastUpdateTime > nextFundingPaymentTimestamp() - fundingDuration) {
        startTime = lastUpdateTime;
      } else {
        startTime = nextFundingPaymentTimestamp() - fundingDuration;
      }
      uint256 endTime = nextFundingPaymentTimestamp();
+      if (endTime == startTime) return;
      fundingRates[latestFundingPaymentPointer] =
        (amount * 1e18) /
        (endTime - startTime);
    } else {
      uint256 startTime = lastUpdateTime;
      uint256 endTime = nextFundingPaymentTimestamp();
      if (endTime == startTime) return;
      fundingRates[latestFundingPaymentPointer] =
        fundingRates[latestFundingPaymentPointer] +
        ((amount * 1e18) / (endTime - startTime));
    }
  }

QA3. UniV2LiquidityAmo.addLiquidity() (removeLiquidity() and swap()) might fail for some ERC20 tokens that revert on zero-transfer. The main reason is that it calls _sendTokensToRdpxV2Core() to send back ununsed tokenA and tokenB. However, there is no unused tokenA, then _sendTokensToRdpxV2Core() will faile for tokens that revert on zero-transfer.

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

The following _sendTokensToRdpxV2Core() function will fail if there is zero balance for tokenA or tokenB.

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

Mitigation: check whether the balance if zero or not, call safeTransfer only when the balance of non-zero:

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

+ if (tokenABalance > 0) 
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      tokenABalance
    );
  
+ if(tokenBBalance > 0) 
  IERC20WithBurn(addresses.tokenB).safeTransfer(
      addresses.rdpxV2Core,
      tokenBBalance
    );

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

QA4. UniV3LiquidityAmo.recoverERC20() fails to call IRdpxV2Core(rdpxV2Core).sync(), as a result, the balance of some reserve tokens in rdpxV2Core might not be synced after the calling of recoverERC20().

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

Mitigation: We need to call IRdpxV2Core(rdpxV2Core).sync():

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

+   IRdpxV2Core(rdpxV2Core).sync();
    emit RecoveredERC20(tokenAddress, tokenAmount);
  }

QA5. The ReLPContract.reLP() function has the following problems.

First, it lacks a slippage control when calling IUniswapV2Router(addresses.ammRouter).addLiquidity():

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

Second, it fails to send the dust tokenB to addresses.rdpxV2Core (only tokenA is considered) when there is any:

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

Both of them are necessary to correct the function.

QA6. An empty symbol reserve asset is added in the constructor, but two other data structures are not revised, including reserveTokens and reservesIndex.

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

We need to revise reserveTokens and reservesIndex as well to be consistent when add a new asset to reserve:

 constructor(address _weth) {
    _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
    weth = _weth;

    // add Zero asset to reserveAsset
    ReserveAsset memory zeroAsset = ReserveAsset({
      tokenAddress: address(0),
      tokenBalance: 0,
      tokenSymbol: "ZERO"
    });
    reserveAsset.push(zeroAsset);
    putOptionsRequired = true;

+    reserveTokens.push(_assetSymbol);
+    reservesIndex[_assetSymbol] = reserveAsset.length - 1;

  }

QA7. The emergency withdraw for RdpxDecayingBonds is supposed to send the tokens to To rather than msg.sender according to the NATSpec.

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");
    }
    IERC20WithBurn token;

    for (uint256 i = 0; i < tokens.length; i++) {
      token = IERC20WithBurn(tokens[i]);
-      token.safeTransfer(msg.sender, token.balanceOf(address(this)));
+      token.safeTransfer(to, token.balanceOf(address(this)));
    }
  }

QA8. The implementation of RdpxDecayingBonds.decreaseAmount() is not consistent with the name and the NatSpec. It sets the new amount of bonds[bondId].rdpxAmount instead of using amount as the amount to decrease.

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

Mitigation:

 function decreaseAmount(
    uint256 bondId,
    uint256 amount
  ) public onlyRole(RDPXV2CORE_ROLE) {
    _whenNotPaused();
-    bonds[bondId].rdpxAmount = amount;
+    bonds[bondId].rdpxAmount -= amount;
 
  }

QA9. The PerpetualAtlanticVault.payFunding() might be subject to reentrancy attack since it calls the transfer function first before setting fundingPaidFor[pointer] = true.

The mitigation should be like the following:

  function provideFunding()
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 fundingAmount)
  {
    _whenNotPaused();
    uint256 pointer = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .latestFundingPaymentPointer();
    _validate(fundingPaidFor[pointer] == false, 16);

+    fundingPaidFor[pointer] = true;

    fundingAmount = IPerpetualAtlanticVault(addresses.perpetualAtlanticVault)
      .payFunding();

    reserveAsset[reservesIndex["WETH"]].tokenBalance -= fundingAmount;

-    fundingPaidFor[pointer] = true;

    emit LogProvideFunding(pointer, fundingAmount);
  }