Dopex - zaevlad's results

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#L995 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L720

Vulnerability details

Impact

RdpxV2Core contract WETH balance can be easily manipulated by a malicious user

Proof of Concept

The sync() functions has no access control and it allows to syncs asset reserves with contract balances.

  function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }
      reserveAsset[i].tokenBalance = balance;
    }

    emit LogSync();
  }

For WETH token it provide an additional calculation.

Imagine the situation when a maliciuos user will want to abuse the contract:

1. Malicious user call addToDelegate with a flashloaned WETH. It updates a storage variable totalWethDelegated += _amount;
2. Than he call sync() to update WETH with balance = balance - totalWethDelegated;
3. At the end he call withdraw() and get his WETH back. However totalWethDelegated remain super big as it is not updated in the 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;

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

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

So in future users will not be able to buy bond with delegate because it could revert here:

  function _bondWithDelegate(
    uint256 _amount, //DpxEth
    uint256 rdpxBondId,
    uint256 delegateId
  ) internal returns (BondWithDelegateReturnValue memory returnValues) {
    ...

    // update total weth delegated
    totalWethDelegated -= wethRequired;

    ...
  }

The contract will think that is has enough WETH, but in real it is not.

Tools Used

Manual review

Recommended Mitigation Steps

Provide an access control check to the sync() functions or make a balance update after all WETH operations.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

First depositor can deposit a single wei then donate to the vault to greatly inflate share ratio. Due to truncation when converting to shares this can be used to steal funds from later depositors.

Proof of Concept

Currently there is a possibility to deposit collateral to the PerpetualAtlanticVaultLP contract directly as for there is no access control or other limitations. Deposit function calls convertToShares to calculate the amount of shares:

  function convertToShares(
    uint256 assets
  ) public view returns (uint256 shares) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

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

So if there is no supply the depositor will get the amount of shares equal to the amount of assets.

So the first user can deposit 1 wei, wait for the second user to deposit his collateral and get 0 shares due to the calcalatios. After all the first depositor withdraw the full amount from the vault stealing some of second user assets.

Tools Used

Manual review

Recommended Mitigation Steps

Either during creation of the vault or for first depositor, lock a small amount of the deposit to avoid this.

Assessed type

MEV

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L238 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L286 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L346 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L160

Vulnerability details

Impact

Adding or removing liquidity and making a swap in the UniV2LiquidityAmo contract can lead to stale balances in the core contract of the protocol.

Proof of Concept

addLiquidity, removeLiquidity and swap functions at the end of execution call _sendTokensToRdpxV2Core that transfer some amounts of tokens to the core contract:

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

However that functions misses a call to sync() functions in the rdpxV2Core that will synchronize the balances, as it was made in the same functions in the UniV3LiquidityAMO contract (see below):

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

    // sync token balances
    IRdpxV2Core(rdpxV2Core).sync();

    emit LogAssetsTransfered(tokenABalance, tokenBBalance, tokenA, tokenB);
  }

The sync() function helps to keep the tokens balances info updated after each action:

  function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );

      if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
      }
      reserveAsset[i].tokenBalance = balance;
    }

    emit LogSync();
  }

Tools Used

Manual review

Recommended Mitigation Steps

Consider adding a sync() functions into the _sendTokensToRdpxV2Core for UniV2LiquidityAMO contract as well.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Token balances should be syncing after each bond purchase otherwise contract can handle stale balances for other operations.

Proof of Concept

We user purchase a bond or bond with delegates the call ends up in the _stake() function where some calculations are made:

  function _stake(
    address _to,
    uint256 _amount
  ) internal returns (uint256 receiptTokenAmount) {
    reserveAsset[reservesIndex["WETH"]].tokenBalance -= _amount / 2;

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

    // deposit into the rdpxV2ReceiptToken contract
    receiptTokenAmount = IRdpxV2ReceiptToken(addresses.rdpxV2ReceiptToken)
      .deposit(_amount / 2);

    // mint receipt token bonds
    _issueBond(_to, receiptTokenAmount);
  }

The problem is in division by 2. If the amount value will be odd some tokens will not be taken into account after calculations. Easy exapmle, if we divide 19 / 2 the result will be 9, and 1 token will be left. With a bigger amounts the lefover can be high as well.

Finaly the contract will be unbalanced greatly, it will has different amounts written into the storage variables and actual amount it has.

Tools Used

Manual review

Recommended Mitigation Steps

Add a sync() function to keep the balances updated after every bond purchase.

Assessed type

Math