Dopex - glcanvas's results

Lines of code

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

Vulnerability details

Impact

First transaction in block might receive more shares than the same transaction at the second position. This behavior is un-honest, and may entail reputational losses in combination with the current implementation.

Proof of Concept

Let's consider deposit:

 function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {
    // 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);
...
}

Calling perpetualAtlanticVault.updateFunding(); might increase _totalCollateral amount, and because shares amount depends on _totalCollateral and totalSupply i.e.: shares = amount * supply / collateral.

So, lets consider next case:

Current supply = 100, collateral = 100

• Start first transaction with amount 100 minted, shares calculated as `100 * 100 / 100 = 100z;
• Calling updateFunding, now supply = 100, collateral = 150;
• First transaction minted 100 shares, and now state: supply = 200, collateral = 250
• Second transaction started, with amount 100 => shares = 100 * 200 / 250 = 80, so user here receive not the same proportional amount of shares as first one.

Honest behavior only possible if updateFunding will be called before shares calculated.

Tools Used

Manual review

Recommended Mitigation Steps

Move line perpetualAtlanticVault.updateFunding(); at the beginning of the deposit.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145-L175 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L218-L235 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L274-L284 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L502-L524 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L462-L496

Vulnerability details

Impact

An attacker can use flash loan and withdraw significant part of funding intended for collateral providers.

Proof of Concept

Let's consider code snippet from deposit:

    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
    perpetualAtlanticVault.updateFunding();
    collateral.transferFrom(msg.sender, address(this), assets);
    _mint(receiver, shares);
    _totalCollateral += assets;

from redeem:

    (assets, rdpxAmount) = redeemPreview(shares);
    require(assets != 0, "ZERO_ASSETS");
    _rdpxCollateral -= rdpxAmount;
    beforeWithdraw(assets, shares);
    _burn(owner, shares);
    collateral.transfer(receiver, assets);
    IERC20WithBurn(rdpx).safeTransfer(receiver, rdpxAmount);

Next consider convertToShares:

    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();
    uint256 totalVaultCollateral = totalCollateral() +
      ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
    return
      supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);

And convertToAssets:

    uint256 supply = totalSupply;
    return
      (supply == 0)
        ? (shares, 0)
        : (
          shares.mulDivDown(totalCollateral(), supply),
          shares.mulDivDown(_rdpxCollateral, supply)
        );

in the other words, if we simplify formulas above: shares = asserts * supply / collateral asserts = shares * collateral / supply

This implementation gives us possibility to withdraw significant amount of funding from VaultLP contract use flash loan.

Possible case:

1. Hacker figure out that new funding available to transfer from Vault to VaultLP (via Vault.updateFunding());
2. Hacker loans a lot of ETH and call deposit it in VaultLP, let's assume before this transaction: supply = 100, collateral = 100;
3. Hacker provide 50 eth and minted shares: shares = 50 * 100 / 100 = 50;
4. Next, after shares calculated the Vault.updateFunding() called, and it updates collateral from 100 to 120;
5. Hacker's shares minted: supply = 150, collateral = 170, hacker has 50 shares;
6. In the same transaction hacker call redeem, and his amount to withdraw will be: amount = 50 * 170 / 150 = 56.6;
7. Therefore at single transaction hacker withdraws significant part of rewards intended for honest users.

This can be fixed if we move updateFunding at the beginning of the deposit function:

1. Hacker deposits same 50 eth, but now supply = 100, collateral = 120 => shares = 41.6;
2. Hacker redeem his shares: 41.6 * 170 / 141.6 = 50 eth;
3. With fix above this attack impossible.

Tools Used

Manual review

Recommended Mitigation Steps

In function deposit move line perpetualAtlanticVault.updateFunding(); to the first place.

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

Contract state might become in incorrect state. Users funds will be locked, and only emergencyWithdraw can save the situation. But still -- contract will be in non working state. Protocol can lose their users and confidence.

Proof of Concept

Let's consider withdraw function:

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

Delegator can withdraw all unused amount:

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;

but we additionally handle totalWethDelegated which is responsible for amount of total delegated tokens.

When user withdraws his delegate amount, the contract must update totalWethDelegated, but this doesn't happened.

This lead to:

• Incorrect totalWethDelegated amount;
• Impossibility to call sync function;
• Possibility to stop whole contract.

Possible case to Stop the system:

• I create a delegate bonds (1 times with 10eth, for example);
• I cancel them immediately;
• Then If contract has 11 eth (really has on his balance), I call sync() function, and now balance will be decreased to 1 eth. This will be disaster because users may lost their money, and contract state will be incorrect.

Tools Used

Manual review

Recommended Mitigation Steps

When you're doing withdraw decease totalWethDelegated.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L19-L21 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L162-L170 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L157 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol#L41

Vulnerability details

Impact

Decaying bonds was introduced to encourage those who received a loss in the protocol, and how it was intended -- these bonds can be used by harmed users only.

But in current implementation discount can be used by anyone, not only by harmed user. Because initial bonds owner can transfer these bonds like plain old NFT token.

This leads to additional profit for those who do not deserve a discount.

Proof of Concept

Let's consider bond:

  struct Bond {
    address owner;
    uint256 expiry;
    uint256 rdpxAmount;
  }

here owner field which filled in mint function:

function mint(
    address to,
    uint256 expiry,
    uint256 rdpxAmount
  ) external onlyRole(MINTER_ROLE) {
    _whenNotPaused();
    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
    uint256 bondId = _mintToken(to);
    bonds[bondId] = Bond(to, expiry, rdpxAmount);
    emit BondMinted(to, bondId, expiry, rdpxAmount);
  }

Bond get his expiry, amount and owner, and storages in bonds array, and token was written to ERC721 layout too.

In core contract owner checks like NFT owner not field in Bond structure.

      _validate(
        IRdpxDecayingBonds(addresses.rdpxDecayingBonds).ownerOf(_bondId) ==
          msg.sender,
        9
      );

That's incorrect, because owner might be changed with transferFrom, which is implemented in ERC721 contract.

So, possible case when Bob got his decaying bond, then transfer this bond to Alice, and Alice uses this discount.

Tools Used

Manual audit

Recommended Mitigation Steps

Either Forbid to transfer bonds, or remove field owner from

  struct Bond {
    address owner;
    uint256 expiry;
    uint256 rdpxAmount;
  }

Assessed type

ERC721

[L-01] Fix Typos

links to typos: https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L84 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L117

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

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L816 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L240 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L117 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L99 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L60 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L52

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

[L-02] Make constants instead of raw values

Replace all occurrences of constant like 1e10 with ONE_HUNDRED_PERCENT

List:

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L658 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L658 1e10 with ONE_HUNDRED_PERCENT

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L669 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L673 1e8 with ONE_TOKEN

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L947 100e8 with ONE_HUNDRED_PERCENT

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L949 1e16 with MILI_ETHER

And in all other files the same.

[L-03] Unify withdraw functions

From now, we have different implementations of emergencyWithdraw

  function emergencyWithdraw(
    address[] calldata tokens
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    IERC20WithBurn token;

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

    emit LogEmergencyWithdraw(msg.sender, tokens);
  }

  function emergencyWithdraw(
    address[] calldata tokens
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    _whenPaused();
    IERC20WithBurn token;

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

    emit EmergencyWithdraw(msg.sender, tokens);
  }

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

These functions must be the same.

Make single contract EmegencyWithdraw.sol and implement all your contract with inheritance from this contract.

[L-04] Don't assign role on msg.sender

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/dpxETH/DpxEthToken.sol#L25 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/dpxETH/DpxEthToken.sol#L26

DpxEthToken minted/burned by Core contract, and here msg.sender get MINTER and BURNER role, which must be reassigned latte. At the first creator can mint multiple tokens to any address before transfer role. At the second creator can give role to another address.

This is bad solution. Better use approach like in UniswapV3 (I described it in analysis report).

[L-05] User might have a lot of decaying bonds

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

If user has a lot of bonds there might be a problem to receive all list of items. Due to limit of external nodes like Infura.

Better use pagable pattern -- i.e. request only subpart of bonds.

[L-06] Unused role

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

bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE");

This role created, but not assigned.

[L-07] Copy-paste

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

This: (currentFundingRate * (block.timestamp - startTime)) / 1e18 might be assigned to variable. This improves readability and saves gas.

[L-08] Unreached code

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

lastUpdateTime == 0 never will be 0 because it's moved anyway in updateFundingPaymentPointer(); so rewrite this code to: uint256 startTime = lastUpdateTime;

[L-09] wrong function naming

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L240 addAssetTotokenReserves -> addAssetToTokenReserves

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L270 removeAssetFromtokenReserves -> removeAssetFromTokenReserves

[L-10] Unused array

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

amoAddresses only filled, but not used anyway.

[L-11] Incorrect error params

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

Due to unclear error code and bad design -- developers created a lot of mistakes while create validation code

_validate(_amount > 0, 17);

error code shouldn't be 17, because: E17: "Zero address"

[L-12] Incorrect comment

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

[L-13] Improve _transfer

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

1. Swap if clauses to be consistent with calculateBondCost;
2. Use constant ONE_HUNDRED_PERSENT instead of 1e10;
3. Move out to variable: (_rdpxAmount * rdpxBurnPercentage) / 1e10
4. Use constant DEFAULT_PRECISION instead of 1e8.

[Q-01] Set addresses might fail

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L181-L212 // Transfer rDPX and ETH token from user You already transferred ETH, so rewrite to: // Transfer rDPX token from user

Current implementation might fail if any address will be changed twice due to safeApprove implementation

collateralToken.safeApprove(
      addresses.perpetualAtlanticVaultLP,
      type(uint256).max
    );

see:

    function safeApprove(IERC20 token, address spender, uint256 value) internal {
        // safeApprove should only be called when setting an initial allowance,
        // or when resetting it to zero. To increase and decrease it, use
        // 'safeIncreaseAllowance' and 'safeDecreaseAllowance'
        require(
            (value == 0) || (token.allowance(address(this), spender) == 0),
            "SafeERC20: approve from non-zero to non-zero allowance"
        );
        _callOptionalReturn(token, abi.encodeWithSelector(token.approve.selector, spender, value));
    }

This can be handled by:

  function setLpAllowance(bool increase) external onlyRole(DEFAULT_ADMIN_ROLE) {
    increase
      ? collateralToken.approve(
        addresses.perpetualAtlanticVaultLP,
        type(uint256).max
      )
      : collateralToken.approve(addresses.perpetualAtlanticVaultLP, 0);
  }

But this is not convenient, due to a lot of additional actions. Better separate setAddresses by multiple atomic setters like:

setOptionPricing
setAssetPriceOracle
...

[Q-02] Not needed to handle ATM options

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

Current implementation handles ATM options, but your doc told that you don't settle ATM options https://docs.dopex.io/option-fundamentals/option-settlement

More-over here not any meaning to handle it, because you got same amount of funds but in another tokens.

So, forbid to settle ATM option, and implement option settlement as was described in my previous report.

[Q-03] Rename variable to symbols

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L70 string[] public reserveTokens;

Actually, they are symbols, not tokens.