Dopex - hals's results

Lines of code

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

Vulnerability details

Impact

• RdpxDecayingBonds contract as per the documentation

  This is an ERC721 contract that mints decaying bonds to users which store a virtual amount of rDPX (to be fulfilled by the Treasury Reserves). These bonds are minted to users as rebates for losses incurred on Dopex Option Products and can also be used to emit indirect rDPX. The term decaying is used as the bond expires after a period of time.

• So the minter can mint a decaying bond for any user as a rebate for option losses; the function is called with three arguments:

  • to : which is the address of the user to mint the bonds for
  • expiry : the timestamp of the bond expiry
  • rdpxAmount : which is the amount of rdpx to bond

• But there's no check if the assigned bond expiry is in the future or not; which might result in minting an expired bond to the user.

• So the user will not be able to get a rebate by redeeming/bonding this expired bond.

Proof of Concept

RdpxDecayingBonds contract/mint function

File:2023-08-dopex/contracts/decaying-bonds/RdpxDecayingBonds.sol
Line 114-125:
  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);
  }

Tools Used

Manual Testing.

Recommended Mitigation Steps

Check the bond expiry to be > block.timestamp before assigning it:

  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);
+   require(rdpxAmount > block.timestamp);
    bonds[bondId] = Bond(to, expiry, rdpxAmount);

    emit BondMinted(to, bondId, expiry, rdpxAmount);
  }

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

• All the price calculations in the protocol are done assuming a returned price of dpxETH-ETH with 1e8 decimal; which is wrong.

• The RdpxEthOracle implemented oracle returns 1e18 instead of 1e8, so a change would be required by updating the oracle or the main contracts.

• This will result in wrong calculations in all the functions that calls the asset to collateral price.

Proof of Concept

RdpxEthOracle::getLpPriceInEth function:

   function getLpPriceInEth() external view override returns (uint) {
        uint totalSupply = pair.totalSupply();
        (uint r0, uint r1, ) = pair.getReserves();
        uint sqrtK = HomoraMath.sqrt(r0.mul(r1)).fdiv(totalSupply); // in 2**112
        uint px0 = getETHPx(token0); // in 2**112
        uint px1 = 2 ** 112; // in 2**112
        // fair token0 amt: sqrtK * sqrt(px1/px0)
        // fair token1 amt: sqrtK * sqrt(px0/px1)
        // fair lp price = 2 * sqrtK * sqrt(px0 * px1)
        // split into 2 sqrts multiplication to prevent uint overflow (note the 2**112)
        uint lpPriceIn112x112 = sqrtK
            .mul(2)
            .mul(HomoraMath.sqrt(px0))
            .div(2 ** 56)
            .mul(HomoraMath.sqrt(px1))
            .div(2 ** 56);

        return (lpPriceIn112x112 * 1e18) >> 112; // @audit : returned price is in 1e18 decimals not 1e8
    }

Some examples of the issue in the RdpxV2Core contract :

RdpxV2Core::getRdpxPrice function

    function getRdpxPrice() public view returns (uint256) {
        return
            IRdpxEthOracle(pricingOracleAddresses.rdpxPriceOracle)
                .getRdpxPriceInEth();
    }

RdpxV2Core::_calculateAmounts Line605

uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;

RdpxV2Core::_calculateAmounts Line608

uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;

RdpxV2Core::_transfer Line669

uint256 rdpxAmountInWeth = (_rdpxAmount * getRdpxPrice()) / 1e8;

RdpxV2Core::_transfer Line673-674

      uint256 extraRdpxToWithdraw = (discountReceivedInWeth * 1e8) /
        getRdpxPrice();

Tools Used

Manual Testing.

Recommended Mitigation Steps

Update the used oracle or the main contracts to adopt the 1e18 decimals instead of the currently implemented 1e8.

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

• In RdpxV2Core contract: the totalWethDelegated state variable represents the total wETH delegated by users to the protool.

• Users can delegate their wETH by calling addToDelegate function, and can un-delegate the unused balance of their delegated wETH by calling withdraw function.

• And the totalWethDelegated variable is increased when the users delegate their wETH by calling addToDelegate function; but this variable is not updated when the users withdraw their unused delegated wETH (when calling withdraw function): addToDelegate function/Line 964

      // add amount to total weth delegated
          totalWethDelegated += _amount;

• This will result in totalWethDelegated representing a delegated amount greater than the actual amount in the protocol which will lead to wrong calculations when the wETH asset reserve balance is updated in sync function.

• So this will lead to incorrect calculations wherever reserveAsset[reservesIndex["WETH"]].tokenBalance is used.

Proof of Concept

withdraw function

File:2023-08-dopex/contracts/core/RdpxV2Core.sol
Line 975-990:
  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);
  }

Tools Used

Manual Testing.

Recommended Mitigation Steps

Update totalWethDelegated variable in the withdraw function by deducting the amountWithdrawn:

  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);
+   totalWethDelegated -= amountWithdrawn;
    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

Context

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L995-L1008 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L160-L178 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L238 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L286 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV2LiquidityAmo.sol#L346

Vulnerability details

Impact

• In UniV2LiquidityAmo contract: _sendTokensToRdpxV2Core function is called internally to send back residual tokens to the RdpxV2Core contract after adding liquidity to uniswap v2 pools, removing liquidity & tokens swapping operations.

• In RdpxV2Core contract : there's a sync function where it syncs assets reserves with contract balances.

• Since these operations are using the reserves assets of RdpxV2Core contract and changing these assets balances; RdpxV2Core::sync function must be called in UniV2LiquidityAmo::_sendTokensToRdpxV2Core function.

• The absence of syncing reserves assets balances of RdpxV2Core will result in using incorrect assets reserves by the RdpxV2Core contract, resulting in incorrect calculations wherever these reserves balances are used as these reserves don't reflect the actual contract balances.

Proof of Concept

RdpxV2Core contract/sync function

File:2023-08-dopex/contracts/core/RdpxV2Core.sol
Line 995-1008:
  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();
  }

UniV2LiquidityAmo contract/_sendTokensToRdpxV2Core function

File:2023-08-dopex/contracts/amo/UniV2LiquidityAmo.sol
Line 160-178:
  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);
  }

UniV2LiquidityAmo contract/addLiquidity function

File:2023-08-dopex/contracts/amo/UniV2LiquidityAmo.sol
Line 238: _sendTokensToRdpxV2Core();

UniV2LiquidityAmo contract/removeLiquidity function

File:2023-08-dopex/contracts/amo/UniV2LiquidityAmo.sol
Line 286: _sendTokensToRdpxV2Core();

UniV2LiquidityAmo contract/swap function

File:2023-08-dopex/contracts/amo/UniV2LiquidityAmo.sol
Line 346: _sendTokensToRdpxV2Core();

Tools Used

Manual Testing.

Recommended Mitigation Steps

Update UniV2LiquidityAmo::_sendTokensToRdpxV2Core function to sync tokens balances in RdpxV2Core contract after each operation:

  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
    );
+   IRdpxV2Core(addresses.rdpxV2Core).sync();
    emit LogAssetsTransfered(msg.sender, tokenABalance, tokenBBalance);
  }

Assessed type

Context