Dopex - ABAIKUNANBAEV's results

Lines of code

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

Vulnerability details

Impact

There is totalWethDelegated param that accounts for how much of WETH was provided by the delegators. It first increased in addToDelegate() function and then it's substracted when somebody calls _bondWithDelegate to bond with their decaying bond and to use delegator's WETH. However, the variable is not updated anyhow when the delegator withdraws his WETH.

Proof of Concept

1. Let's say the delegator calls addToDelegate() and deposits 5 WETH:

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

totalWethDelegated += _amount;

The variable is now 10.

2. Then imagine 5 of his WETH was used inside of _bondWithDelegate so the variable is now 5.

3. The delegator wants to withdraw the unused WETH and calls withdraw:

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

But this time the variable is not updated and totalWethDelegated still 5. Now if someone wants to use this 5 delegated WETH, his tx may revert due to underflow in _bondWithDelegate:

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

Tools Used

Manual review.

Recommended Mitigation Steps

Update totalWethDelegated inside of withdraw().

Assessed type

Math

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L168 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L172 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L210 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L215 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L321

Vulnerability details

Impact

Inside of UniV2LiquidityAmo.sol, there are several functions that interact with RdpxV2Core.sol and transfer tokens between the contracts. The problem is that some functions are missing sync() to update the reserves correctly. For example, sync is called when rDPX tokens are transferred from ReLPContract to the core after performing swaps:

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

Proof of Concept

First of all, it's _sendTokensToRdpxV2Core() that is called inside of addLiquidity(), removeLiquidity and swap to send the unused tokens back to the RdpxV2Core.sol:

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L160-178 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

  IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      tokenABalance
    );
    IERC20WithBurn(addresses.tokenB).safeTransfer(
      addresses.rdpxV2Core,
      tokenBBalance
    );

Also there are transfers of tokens from RdpxV2Core to perform liquidity operations in the amo contract. As they are directly transferred from the amo and not from the core, they will not be synchronized correctly:

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L210-214

IERC20WithBurn(addresses.tokenA).safeTransferFrom(
      addresses.rdpxV2Core,
      address(this),
      tokenAAmount
    );
    IERC20WithBurn(addresses.tokenB).safeTransferFrom(
      addresses.rdpxV2Core,
      address(this),
      tokenBAmount
    );

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV2LiquidityAmo.sol#L321-325

 IERC20WithBurn(token1).safeTransferFrom(
      addresses.rdpxV2Core,
      address(this),
      token1Amount
    );

Tools Used

Manual review.

Recommended Mitigation Steps

Call sync() on these functions.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

This issue may look like my another issue about not calling sync() on several functions. In that case, the internal account of reserves was affected. In this issue, it's different as it may lead to the WETH being used on completely different purposes (and possibly lost). The problem is that, in RpdxV2Core.sol, the sync() function is not called automatically in addToDelegate(). So if it's not called by somebody, the balances will remain not sync. This is a serious issue as in the sync() function the total balance of WETH is synchronized the following way:

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

balance = balance - totalWethDelegated;

Basically, it decreases the WETH reserve balance by totalWethDelegated making it not accessible to be used for different purposes (for example, liquidity provision or pegging of dpxETH / ETH). If the function is not sync immediately, it opens up a time window when this delegated collateral can be used not for delegating.

Proof of Concept

1. The users calls addToDelegate() and deposits 10 WETH. The tokenBalance of WETH is 20 WETH:

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

reserveAsset[i].tokenBalance = balance;

2. Now as the function sync() is not called, the IERC20weth.balanceOf(address(this)) of WETH token will be 30 WETH.

3. This can lead to this WETH being withdrawn in emergencyWithdraw() if it's called before sync() which is unexpected behavior as the initial intention of the delegator was to just delegate his WETH and not for his WETH to be somehow withdrawn.

Tools Used

Manual review.

Recommended Mitigation Steps

Add sync() at the end of the addToDelegate() function.

Assessed type

Other