Dopex - Yanchuan'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

When the price of rdpx falls below the strike price, the put options can be settled. At this point, collateral tokens (WETH) will be transferred from VaultLP to rdpxV2Core, and the quantity of collateral tokens in VaultLP will be updated.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol
359    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
360      ethAmount
361    );

VaultLP checks the quantity of collateral tokens before updating the value of _totalCollateral (line 201). However, since collateral tokens are standard ERC20 tokens, anyone can directly call the collateral.transfer() function to transfer tokens into VaultLP. This action would change the value obtained from collateral.balanceOf(), causing the check at line 201 to fail, resulting in the failure of option settlement.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
199  function subtractLoss(uint256 loss) public onlyPerpVault {
200    require(
201      collateral.balanceOf(address(this)) == _totalCollateral - loss,
202      "Not enough collateral was sent out"
203    );
204    _totalCollateral -= loss;
205  }

Proof of Concept

Below is the test code where 1 WETH was transferred to VaultLP using weth.transfer(address(vaultLp), 1 ether) before settling the options.

  function testSettle() public {
    weth.mint(address(1), 1 ether);
    deposit(1 ether, address(1)); 

    vault.purchase(1 ether, address(this));

    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;

    weth.transfer(address(vaultLp), 1 ether);
    priceOracle.updateRdpxPrice(0.010 gwei);
    vault.settle(ids);

    assertEq(vault.totalActiveOptions(), 0);

  }

As the test results below show, the option settlement has failed.

[nian@localhost 2023-08-dopex]$ forge test --match-test testSettle  -vv
[⠆] Compiling...
[⠑] Compiling 1 files with 0.8.19
[⠘] Solc 0.8.19 finished in 5.29s
Compiler run successful!

Running 1 test for tests/perp-vault/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 765048)
Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.03s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/perp-vault/Unit.t.sol:Unit
[FAIL. Reason: Not enough collateral was sent out] testSettle() (gas: 765048)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

foundry

Recommended Mitigation Steps

The simplest solution would be to remove this check.

diff --git a/contracts/perp-vault/PerpetualAtlanticVaultLP.sol.orig b/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
index 5758161..1179b17 100644
--- a/contracts/perp-vault/PerpetualAtlanticVaultLP.sol.orig
+++ b/contracts/perp-vault/PerpetualAtlanticVaultLP.sol
@@ -197,10 +197,6 @@ contract PerpetualAtlanticVaultLP is ERC20, IPerpetualAtlanticVaultLP {
 
   /// @inheritdoc      IPerpetualAtlanticVaultLP
   function subtractLoss(uint256 loss) public onlyPerpVault {
-    require(
-      collateral.balanceOf(address(this)) == _totalCollateral - loss,
-      "Not enough collateral was sent out"
-    );
     _totalCollateral -= loss;
   }
 

The options can now be settled successfully.

[nian@localhost 2023-08-dopex]$ forge test --match-test testSettle  -vv
[⠒] Compiling...
[⠒] Compiling 10 files with 0.8.19
[⠆] Solc 0.8.19 finished in 11.47s
Compiler run successful!

Running 1 test for tests/perp-vault/Unit.t.sol:Unit
[PASS] testSettle() (gas: 630304)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 3.08s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

When a delegate calls the withdraw() function to make a withdrawal, the value of totalWethDelegated is not updated.

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

Proof of Concept

In the following test code, the user first deposits 1 WETH and then proceeds to make a withdrawal. After the withdrawal, the value of totalWethDelegated should be 0.

  function testWithdraw() public {
    rdpxV2Core.addToDelegate(1 * 1e18, 10e8);
    rdpxV2Core.withdraw(0);
    assertEq(rdpxV2Core.totalWethDelegated(), 0);
  }

The test result shows that the value of totalWethDelegated is not 0.

[nian@localhost 2023-08-dopex]$ forge test --match-test testWithdraw -vv
[⠔] Compiling...
[⠑] Compiling 5 files with 0.8.19
[⠃] Solc 0.8.19 finished in 9.11s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Assertion failed.] testWithdraw() (gas: 146066)
Logs:
  Error: a == b not satisfied [uint]
        Left: 1000000000000000000
       Right: 0

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.33s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Unit.t.sol:Unit
[FAIL. Reason: Assertion failed.] testWithdraw() (gas: 146066)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

Remember to update the value of totalWethDelegated when a user makes a withdrawal.

diff --git a/contracts/core/RdpxV2Core.sol.orig b/contracts/core/RdpxV2Core.sol
index e28a65c..da68f37 100644
--- a/contracts/core/RdpxV2Core.sol.orig
+++ b/contracts/core/RdpxV2Core.sol
@@ -984,6 +984,8 @@ contract RdpxV2Core is
     _validate(amountWithdrawn > 0, 15);
     delegate.amount = delegate.activeCollateral;
 
+    totalWethDelegated -= amountWithdrawn;
+
     IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
 
     emit LogDelegateWithdraw(delegateId, amountWithdrawn);

Now the test case has passed.

[nian@localhost 2023-08-dopex]$ forge test --match-test testWithdraw -vv
[⠰] Compiling...
[⠔] Compiling 5 files with 0.8.19
[⠑] Solc 0.8.19 finished in 8.96s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Unit.t.sol:Unit
[PASS] testWithdraw() (gas: 134444)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.52s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

When users swap tokens, there is almost always slippage. To minimize losses caused by slippage, when performing swap operations on Uniswap, users can set a lower limit for the amount of tokens they receive. If the actual amount of tokens received falls below this lower limit, the swap operation will revert.

// https://docs.uniswap.org/contracts/v2/reference/smart-contracts/router-02
function swapExactTokensForTokens(
  uint amountIn,
  uint amountOutMin,
  address[] calldata path,
  address to,
  uint deadline
) external returns (uint[] memory amounts);

In ReLPContract.sol, the variable slippageTolerance represents this lower limit value, with a default value of 5e5, indicating the maximum acceptable slippage of 0.5%.

  /// @notice The slippage tolernce in swaps in 1e8 precision
  uint256 public slippageTolerance = 5e5; // 0.5%

  function setSlippageTolerance(
    uint256 _slippageTolerance
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    require(
      _slippageTolerance > 0,
      "reLPContract: slippage tolerance must be greater than 0"
    );
    slippageTolerance = _slippageTolerance;
  }

During a swap operation, The minimum amount of output tokens is based on the amount of input tokens and slippageTolerance. However, the calculation formula here is incorrect. The value of calculated mintokenAAmount is very small. This can lead to significant financial losses.

    // calculate min amount of tokenA to be received
    mintokenAAmount =
      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

The variable tokenAPrice denotes the price of tokenA in relation to tokenB. The accurate formula is amountB = amountA * tokenAPrice. To compute the amount of tokenA, use the formula amountA = amountB / tokenAPrice. Consequently, the proper formula for calculating mintokenAAmount is as follows:

mintokenAAmount = ((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice) - (((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice) * slippageTolerance / 1e8).

Proof of Concept

For testing convenience, I have added print statements within the reLP() function to display the values of some variables.

    // calculate min amount of tokenA to be received
    mintokenAAmount =
      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);

    console.log("tokenAPrice = %s", tokenAInfo.tokenAPrice);
    console.log("amountB / 2 = %s", amountB / 2);
    console.log("mintokenAAmount = %s", mintokenAAmount);
    console.log("slippageTolerance = %s", slippageTolerance);

    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(
        amountB / 2,
        mintokenAAmount,
        path,
        address(this),
        block.timestamp + 10
      )[path.length - 1];

    console.log("tokenAAmountOut = %s", tokenAAmountOut);

In the following test code, 10 rDPX and 2 WETH are added to the Uniswap V2 pool. Additionally, 10 rDPX tokens are minted to the rdpxReserveContract. Finally, the bond() function is invoked to mint bonds. The expected result is that when reLP() calls swapExactTokensForTokens(), the swap operation fails due to exceeding the slippage tolerance, causing the transaction to revert.

  function testreLP() public {
    uniV2LiquidityAMO = new UniV2LiquidityAMO();

    // set addresses
    uniV2LiquidityAMO.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );

    // give amo premission to access rdpxV2Core reserve tokens

    rdpxV2Core.addAMOAddress(address(uniV2LiquidityAMO));

    rdpxV2Core.approveContractToSpend(
      address(rdpx),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpxV2Core.approveContractToSpend(
      address(weth),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpx.transfer(address(rdpxV2Core), 50e18);
    weth.transfer(address(rdpxV2Core), 10e18);

    rdpxPriceOracle.updateRdpxPrice(2e7);
    // add liquidity
    uniV2LiquidityAMO.addLiquidity(10e18, 2e18, 0, 0);

    rdpx.mint(address(rdpxReserveContract), 10 ether);

    // set address in reLP contract and grant role
    reLpContract.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxReserveContract),
      address(uniV2LiquidityAMO),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );
    reLpContract.grantRole(reLpContract.RDPXV2CORE_ROLE(), address(rdpxV2Core));

    reLpContract.setreLpFactor(9e4);

    uniV2LiquidityAMO.approveContractToSpend(
      address(pair),
      address(reLpContract),
      type(uint256).max
    );

    rdpxV2Core.setIsreLP(true);

    vm.expectRevert();
    rdpxV2Core.bond(7 * 1e18, 0, address(this));
  }

The result shows that the swap operation was successful, and the transaction did not revert. The values of variables tokenAPrice, amountB, mintokenAAmount, slippageTolerance, and tokenAAmountOut have been printed. The value of slippageTolerance is 0.5%, and you can calculate the actual slippage tolerance as follows:1 - mintokenAAmount * tokenAPrice / DEFAULT_PRECISION / (amountB / 2), which results in 0.96. Therefore, in extreme cases, this could lead to losses of up to 96%. Let's calculate the actual slippage for this swap operation using the given method: 1 - tokenAAmountOut * tokenAPrice / DEFAULT_PRECISION / (amountB / 2), the result is 0.01459, which translates to 1.459%. This exceeds the 0.5% limit.

[nian@localhost 2023-08-dopex]$ forge test --match-test testreLP -vv
[⠆] Compiling...
[⠒] Compiling 5 files with 0.8.19
[⠆] Solc 0.8.19 finished in 10.72s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Call did not revert as expected] testreLP() (gas: 4003606)
Logs:
  tokenAPrice = 20000000
  amountB / 2 = 23055700137353372
  mintokenAAmount = 4588084327333321
  slippageTolerance = 500000
  tokenAAmountOut = 113596261351450530

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.29s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Call did not revert as expected] testreLP() (gas: 4003606)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

Calculate the value of mintokenAAmount using the correct formula.

diff --git a/contracts/reLP/ReLPContract.sol.orig b/contracts/reLP/ReLPContract.sol
index bc57fbd..de8386c 100644
--- a/contracts/reLP/ReLPContract.sol.orig
+++ b/contracts/reLP/ReLPContract.sol
@@ -271,8 +271,8 @@ contract ReLPContract is AccessControl {
 
     // calculate min amount of tokenA to be received
     mintokenAAmount =
-      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
-      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);
+      ((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice) -
+      (((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice) * slippageTolerance / 1e8);
 
     uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
       .swapExactTokensForTokens(

The test results are consistent with our expectations now.

[nian@localhost 2023-08-dopex]$ forge test --match-test testreLP -vv
[⠆] Compiling...
[⠒] Compiling 5 files with 0.8.19
[⠆] Solc 0.8.19 finished in 10.70s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[PASS] testreLP() (gas: 3776390)
Logs:
  tokenAPrice = 20000000
  amountB / 2 = 23055700137353372
  mintokenAAmount = 114702108183333026
  slippageTolerance = 500000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 5.36s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

There are three pieces of information in the bond: owner, expiry and rdpxAmount. It's better to update the owner when transferring it.

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/decaying-bonds/RdpxDecayingBonds.sol
39  // Structure to store the bond information
40  struct Bond {
41    address owner;
42    uint256 expiry;
43    uint256 rdpxAmount;
44  }

Proof of Concept

In the test code below, a bond is minted and then transferred to address(1).

  function testMint() public {
    rdpxDecayingBonds.mint(address(this), 1223322, 5e18);
    rdpxDecayingBonds.safeTransferFrom(address(this) ,address(1), 1);
    (address owner, , ) = rdpxDecayingBonds.bonds(1);
    assertEq(owner, address(1));
  }

The test results indicate that the owner is still address(this).

[nian@localhost 2023-08-dopex]$ forge test --match-test testMint  -vv
[⠔] Compiling...
[⠑] Compiling 1 files with 0.8.19
[⠘] Solc 0.8.19 finished in 2.08s
Compiler run successful!

Running 1 test for tests/RdpxDecayingBondsTest.t.sol:RdpxDecayingBondsTest
[FAIL. Reason: Assertion failed.] testMint() (gas: 232468)
Logs:
  Error: a == b not satisfied [address]
        Left: 0x7fa9385be102ac3eac297483dd6233d62b3e1496
       Right: 0x0000000000000000000000000000000000000001

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.69ms
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/RdpxDecayingBondsTest.t.sol:RdpxDecayingBondsTest
[FAIL. Reason: Assertion failed.] testMint() (gas: 232468)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

foundry

Recommended Mitigation Steps

Update the owner of the bond in the _beforeTokenTransfer function.

diff --git a/contracts/decaying-bonds/RdpxDecayingBonds.sol.orig b/contracts/decaying-bonds/RdpxDecayingBonds.sol
index e89349e..3160630 100644
--- a/contracts/decaying-bonds/RdpxDecayingBonds.sol.orig
+++ b/contracts/decaying-bonds/RdpxDecayingBonds.sol
@@ -167,6 +167,7 @@ contract RdpxDecayingBonds is
   ) internal override(ERC721, ERC721Enumerable) {
     _whenNotPaused();
     super._beforeTokenTransfer(from, to, tokenId, batchSize);
+    bonds[tokenId].owner = to;
   }
 
   // The following functions are overrides required by Solidity.

The tests have now passed.

[nian@localhost 2023-08-dopex]$ forge test --match-test testMint 
[⠒] Compiling...
[⠑] Compiling 6 files with 0.8.19
[⠘] Solc 0.8.19 finished in 8.94s
Compiler run successful!

Running 1 test for tests/RdpxDecayingBondsTest.t.sol:RdpxDecayingBondsTest
[PASS] testMint() (gas: 218579)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.04ms
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

There are three parties involved in this system: the project team, dpxETH bonders, and liquidity providers, each with different needs. The dpxETH bonders's goals are to mint as much dpxETH as possible, preferably with a discount. Liquidity providers deposit collateral tokens into VaultLP to earn returns, which come from two sources: (1) the premium deposited when the bonders purchase put options, and (2) rewards provided by the project team. The project team designed dpxETH with the intention of encouraging more bonders to mint dpxETH while also considering the long-term growth of the project and encouraging more participants in the system.

From the current design and code, it appears that the project team has focused more on the interests of bonders, which can be observed in several aspects: (1) Users receive a discount when minting dpxETH, reducing the cost and encouraging more dpxETH minting. (2) Users can use rDPX decaying bond to mint dpxETH without paying rdpx. The decaying bond has an expiration date, indirectly encouraging users to mint rdpxETH. (3) A portion of the rdpx received when minting dpxETH is generally burned, while the other half is distributed to veDPX holders, increasing the price of rDPX and allowing users to mint dpxETH with fewer rdpx tokens. (4) The system has a reLP mechanism that buys rdpx on Uniswap, further increasing the price of rdpx. (5) The project team rewards VaultLP in the form of WETH, but this WETH belongs to available collateral, increasing the collateral amount when users purchase put options, enabling them to mint more dpxETH.

However, these measures may harm the interests of liquidity providers: (1) When the rdpx price is below the strike price, options will be settled, resulting in losses for liquidity providers. (2) When the rdpx price is above the strike price, since Vault uses Perpetual puts options, the options will not end. While liquidity providers earn profits, they cannot withdraw them. (3) Since the project team encourages users to mint dpxETH, a significant amount of collateral may be locked, preventing withdrawals. (4) If liquidity providers want to withdraw, the funds can only come from collateral tokens deposited by other providers or rewards provided by the project team. But the rewards, being part of available collateral, can also be used as assets when users purchase options, further reducing the likelihood of successful withdrawals for liquidity providers.

Users deposit collateral tokens to earn returns, but the precondition is the ability to withdraw. If withdrawals are not possible, high returns are meaningless. Especially in a bull market, where both ETH and rdpx prices are rising, and rdpx's market cap is smaller, its price may increase faster than ETH's. This results in put options not being settled. When ETH's price rises to a certain level, users may want to sell ETH, but due to the insufficient available WETH in VaultLP, they even cannot withdraw.

Therefore, this could deter users from depositing collateral tokens into VaultLP, even if bonders want to mint dpxETH, they won't be able to due to the lack of available collateral in VaultLP. This may lead to the failure of dpxETH. As a project team, it's essential to consider the interests of all participants to promote the successful development of dpxETH.

Proof of Concept

In the following test code, a user deposited 1 ether into VaultLP. It can be observed that, after one year, when the user attempts to withdraw assets, this is not sufficient amount of assets available for withdrawal.

  function testRedeem() external {
    weth.approve(address(vaultLp), type(uint256).max);
    uint256 shares = vaultLp.deposit(1 ether, address(this));

    (, uint256 id) = vault.purchase(1 ether, address(this));
    uint256[] memory optionIds = new uint256[](1);
    optionIds[0] = id;  

    skip(86500);
    vault.updateFundingPaymentPointer(); 

    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 0.015 gwei;
    vault.calculateFunding(strikes);
    vault.payFunding();

    skip(86500*365); // 1 year past
    vault.calculateFunding(strikes);

    (uint256 red_weth, uint256 red_rdpx) = vaultLp.redeemPreview(shares);
    uint256 aval_weth = vaultLp.totalAvailableCollateral();

    console.log("red_weth: %s, red_rdpx: %s, aval_weth: %s", red_weth, red_rdpx, aval_weth);
  }

[nian@localhost 2023-08-dopex]$ forge test --match-test testRedeem -vv
[⠆] Compiling...
[⠢] Compiling 1 files with 0.8.19
[⠆] Solc 0.8.19 finished in 3.75s
Compiler run successful!

Test result: ok. 0 passed; 0 failed; 0 skipped; finished in 1.31s

Running 1 test for tests/perp-vault/Unit.t.sol:Unit
[PASS] testRedeem() (gas: 7118735)
Logs:
  red_weth: 1099999999999999998, red_rdpx: 0, aval_weth: 949999999999999998

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.33s
Ran 2 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review

Recommended Mitigation Steps

Here are some suggestions for the project team to consider:

(1) The rewards provided by the project team into VaultLP are exclusively as incentives for user deposits and should not be considered as available collateral. This approach ensures that these rewards are not locked when bonders purchase put options, allowing liquidity providers to withdraw their rewards freely.

(2) The rewards provided by the project team into VaultLP can be considered as available collateral, but only a portion of these funds can be used as collateral when purchasing put options. This portion can be defined as the putOptionsFactor, ranging from 0% to 100%. The project team can set the putOptionsFactor or automatically adjust it based on the proportion of available collateral to total collateral. Lower putOptionsFactor when the proportion is low and increase it when the proportion is high.

(3) Consider changing the bondDiscountFactor from uint256 to int256, where positive values represent rewards for purchasing dpxETH, and negative values represent excess funds. Adjust this value based on actual circumstances. However, it may not be a good idea as users typically prefer discounts over additional funds from a psychological perspective.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

The UniV2LiquidityAMO provides three functions, addLiquidity, removeLiquidity and swap, which the admin can use to add and remove liquidity from Uniswap, or swap assets. The addLiquidity and swap requires assets from *RdpxV2Core, and the assets obtained from removeLiquidity and swap are stored in RdpxV2Core. However, UniV2LiquidityAMO does not update the data of ReserveAssets in RdpxV2Core, which can lead to inconsistencies between the data recorded in ReserveAsset and the actual data of the stored assets.

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

Proof of Concept

In the following test code, there are 50 rDPX and 11 WETH in RdpxV2Core. The addLiquidity function is used to add liquidity to Uniswap by adding 5 rDPX and 1 WETH. After adding liquidity, the balances of rDPX and WETH in RdpxV2Core will decrease.

  function testV2Amo() public {
    uniV2LiquidityAMO = new UniV2LiquidityAMO();

    // set addresses
    uniV2LiquidityAMO.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );

    // give amo premission to access rdpxV2Core reserve tokens
    rdpxV2Core.addAMOAddress(address(uniV2LiquidityAMO));

    rdpxV2Core.approveContractToSpend(
      address(rdpx),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpxV2Core.approveContractToSpend(
      address(weth),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpx.transfer(address(rdpxV2Core), 50e18);
    weth.transfer(address(rdpxV2Core), 11e18);

    rdpxV2Core.sync();
    (, uint256 rdpxToken1,) = rdpxV2Core.getReserveTokenInfo("RDPX");
    (, uint256 wethToken1,) = rdpxV2Core.getReserveTokenInfo("WETH");

    // add liquidity
    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);

    (, uint256 rdpxToken2,) = rdpxV2Core.getReserveTokenInfo("RDPX");
    (, uint256 wethToken2,) = rdpxV2Core.getReserveTokenInfo("WETH");

    assertLt(rdpxToken2, rdpxToken1);
    assertLt(wethToken2, wethToken1);
  }

The test results indicate that after calling addLiquidity to add liquidity to Uniswap, the balances of rDPX and WETH recorded in the ReserveAsset of RdpxV2Core did not decrease.

[nian@localhost 2023-08-dopex]$ forge  test --match-test testV2Amo  -vv
[⠆] Compiling...
[⠘] Compiling 2 files with 0.8.19
[⠊] Solc 0.8.19 finished in 6.49s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Assertion failed.] testV2Amo() (gas: 2333888)
Logs:
  Error: a < b not satisfied [uint]
    Value a: 50000000000000000000
    Value b: 50000000000000000000
  Error: a < b not satisfied [uint]
    Value a: 11000000000000000000
    Value b: 11000000000000000000

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 2.59s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Assertion failed.] testV2Amo() (gas: 2333888)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

In the function _sendTokensToRdpxV2Core(), call the sync() function of RdpxV2Core to update the data in ReserveAsset.

diff --git a/contracts/amo/UniV2LiquidityAmo.sol.orig b/contracts/amo/UniV2LiquidityAmo.sol
index ce92d43..dc8dad2 100644
--- a/contracts/amo/UniV2LiquidityAmo.sol.orig
+++ b/contracts/amo/UniV2LiquidityAmo.sol
@@ -149,6 +149,8 @@ contract UniV2LiquidityAMO is AccessControl {
       token.safeTransfer(msg.sender, token.balanceOf(address(this)));
     }
 
+    IRdpxV2Core(addresses.rdpxV2Core).sync();
+
     emit LogEmergencyWithdraw(msg.sender, tokens);
   }
 

Performing the test again, you should observe that after adding liquidity to Uniswap, the assets balances recorded in ReserveAsset are reduced.

[nian@localhost 2023-08-dopex]$ forge  test --match-test testV2Amo  -vv
[⠢] Compiling...
[⠒] Compiling 2 files with 0.8.19
[⠑] Solc 0.8.19 finished in 7.23s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[PASS] testV2Amo() (gas: 2339509)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.54s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

During the reLP process, part of liquidity is removed from Uniswap V2. Half of The WETH obtained from Uniswap V2 is then swapped into rDPX. Afterward, the addLiquidity function is called to re-add this portion of rDPX and WETH back to Uniswap V2. In an ideal scenario, the amount of WETH and rDPX (in ETH value) are equal, and there will be no remaining WETH in the ReLPContract. However, in practical scenarios, the actual amounts of WETH and rDPX added to Uniswap V2 may not be equal, and there will often be some remaining rDPX or WETH in the ReLPContract.

    (, , uint256 lp) = IUniswapV2Router(addresses.ammRouter).addLiquidity(
      addresses.tokenA,
      addresses.tokenB,
      tokenAAmountOut,
      amountB / 2,
      0,
      0,
      address(this),
      block.timestamp + 10
    );

    // transfer the lp to the amo
    IERC20WithBurn(addresses.pair).safeTransfer(addresses.amo, lp);
    IUniV2LiquidityAmo(addresses.amo).sync();

    // transfer rdpx to rdpxV2Core
    IERC20WithBurn(addresses.tokenA).safeTransfer(
      addresses.rdpxV2Core,
      IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
    );
    IRdpxV2Core(addresses.rdpxV2Core).sync();

The addLiquidity interface in Uniswap V2 is as follows:

  function addLiquidity(
    address tokenA,
    address tokenB,
    uint256 amountADesired,
    uint256 amountBDesired,
    uint256 amountAMin,
    uint256 amountBMin,
    address to,
    uint256 deadline
  ) external returns (uint256 amountA, uint256 amountB, uint256 liquidity);

amountA and amountB are the amounts of tokens added to Unisswap V2.

The return value of IUniswapV2Router(addresses.ammRouter).addLiquidity should not be ignored. If there is any remaining WETH, this portion of WETH needs to be handled properly; otherwise, it could be permanently locked.

Proof of Concept

The following test code first adds 5 rDPX and 1 WETH to Uniswap V2, and then calls the reLP function.

  function testreLP() public {
    uniV2LiquidityAMO = new UniV2LiquidityAMO();

    // set addresses
    uniV2LiquidityAMO.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );

    // give amo premission to access rdpxV2Core reserve tokens
    rdpxV2Core.addAMOAddress(address(uniV2LiquidityAMO));

    rdpxV2Core.approveContractToSpend(
      address(rdpx),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpxV2Core.approveContractToSpend(
      address(weth),
      address(uniV2LiquidityAMO),
      type(uint256).max
    );

    rdpx.transfer(address(rdpxV2Core), 50e18);
    weth.transfer(address(rdpxV2Core), 10e18);

    rdpxV2Core.sync();
    // add liquidity
    uniV2LiquidityAMO.addLiquidity(5e18, 1e18, 0, 0);
    rdpxV2Core.sync();

    // set address in reLP contract and grant role
    reLpContract.setAddresses(
      address(rdpx),
      address(weth),
      address(pair),
      address(rdpxV2Core),
      address(rdpxReserveContract),
      address(uniV2LiquidityAMO),
      address(rdpxPriceOracle),
      address(factory),
      address(router)
    );

    reLpContract.setreLpFactor(9e4);
    uniV2LiquidityAMO.approveContractToSpend(
      address(pair),
      address(reLpContract),
      type(uint256).max
    );

    uint256 reLpWETHBalance1 = weth.balanceOf(address(reLpContract));
    assertEq(reLpWETHBalance1, 0);

    reLpContract.reLP(1e18);

    uint256 reLpWETHBalance2 = weth.balanceOf(address(reLpContract));
    assertEq(reLpWETHBalance2, 0);
  }

The test results show that after executing reLP, the amount of WETH in the ReLPContract is not zero. This portion of WETH is permanently locked in the ReLPContract.

[⠢] Compiling...
[⠘] Compiling 1 files with 0.8.19
[⠊] Solc 0.8.19 finished in 6.52s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Assertion failed.] testreLP() (gas: 2956253)
Logs:
  Error: a == b not satisfied [uint]
        Left: 1015878819174905
       Right: 0

Test result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 3.52s
Ran 1 test suites: 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery
[FAIL. Reason: Assertion failed.] testreLP() (gas: 2956253)

Encountered a total of 1 failing tests, 0 tests succeeded

Tools Used

Foundry

Recommended Mitigation Steps

The remaining WETH in the ReLPContract should be transferred to RdpxV2Core.

diff --git a/contracts/reLP/ReLPContract.sol.orig b/contracts/reLP/ReLPContract.sol
index bc57fbd..7d0291e 100644
--- a/contracts/reLP/ReLPContract.sol.orig
+++ b/contracts/reLP/ReLPContract.sol
@@ -303,6 +303,13 @@ contract ReLPContract is AccessControl {
       addresses.rdpxV2Core,
       IERC20WithBurn(addresses.tokenA).balanceOf(address(this))
     );
+
+    // transfer remaining weth to rdpxV2Core
+    IERC20WithBurn(addresses.tokenB).safeTransfer(
+      addresses.rdpxV2Core,
+      IERC20WithBurn(addresses.tokenB).balanceOf(address(this))
+    );
+
     IRdpxV2Core(addresses.rdpxV2Core).sync();
   }
 

Now the results are correct.

[nian@localhost 2023-08-dopex]$ forge  test --match-test testreLP  -vv
[⠢] Compiling...
[⠒] Compiling 5 files with 0.8.19
[⠆] Solc 0.8.19 finished in 8.81s
Compiler run successful!

Running 1 test for tests/rdpxV2-core/Periphery.t.sol:Periphery
[PASS] testreLP() (gas: 2927932)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.53s
Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Assessed type

Uniswap

1. token_id on removeLiquidity() is always 0

removeLiquidity() is used to remove liquidity from Uniswap V3. pos.token_id is the nft token id of the liquidity to be removed. At the end of the function, pos.token_id is deleted from positions_mapping (line 263), so positions_mapping[pos.token_id].token_id must be 0. line 269 should be changed to emit log(pos.token_id) https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/amo/UniV3LiquidityAmo.sol#L269

262    positions_array.pop();
263    delete positions_mapping[pos.token_id];
264
265    // send tokens to rdpxV2Core
266    _sendTokensToRdpxV2Core(tokenA, tokenB);
267
268    emit log(positions_array.length);
269    emit log(positions_mapping[pos.token_id].token_id);

2. repeated checks

RdpxDecayingBonds can only be minted when the contract isn't paused. This checking is added in _beforeTokenTransfer(), there is no need to perform another check within the mint function. (line 119) https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/decaying-bonds/RdpxDecayingBonds.sol#L119

114  function mint(
115    address to,
116    uint256 expiry,
117    uint256 rdpxAmount
118  ) external onlyRole(MINTER_ROLE) {
119    _whenNotPaused();
120    require(hasRole(MINTER_ROLE, msg.sender), "Caller is not a minter");
121    uint256 bondId = _mintToken(to);
122    bonds[bondId] = Bond(to, expiry, rdpxAmount);
123
124    emit BondMinted(to, bondId, expiry, rdpxAmount);
125  }

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId,
    uint256 batchSize
  ) internal override(ERC721, ERC721Enumerable) {
    _whenNotPaused();
    super._beforeTokenTransfer(from, to, tokenId, batchSize);
  }

3. Logical "or" should be changed to "and"

The logical "or" should be changed to "and" because the logical "or" implies that as long as one address is not equal to address(0), the check will pass. Furthermore, since there are four parameters of address type, all four of these parameters need to be checked. https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L94-L97

  constructor(
    address _perpetualAtlanticVault,
    address _rdpxRdpxV2Core,
    address _collateral,
    address _rdpx,
    string memory _collateralSymbol
  )
    ERC20(
      "PerpetualAtlanticVault LP Token",
      _collateralSymbol,
      ERC20(_collateral).decimals()
    )
  {
    require(
      _perpetualAtlanticVault != address(0) || _rdpx != address(0),
      "ZERO_ADDRESS"
    );

4. Incorrect error codes

In RdpxV2Core.sol, error codes are defined to represent the reasons for transaction failures. However, it seems that the error codes used in the following _validate() functions are not appropriate.

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L410 should be changed to E4

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L533-L542 should be changed to E4

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/core/RdpxV2Core.sol#L1167 should add a new error code "E20 Bond discount is above 100%"

// E1: "Insufficient bond amount",
// E2: "Bond has expired",
// E3: "Invalid parameters",
// E4: "Invalid amount",
// E5: "Not enough ETH available from the delegate",
// E6: "Invalid bond ID",
// E7: "Bond has not matured",
// E8: "Fee cannot be more than 100",
// E9: "msg.sender is not the owner",
// E10: "Price is not above the upper peg",
// E11: "Amount greater that max permissible amount",
// E12: "Price is not lower than first lower peg",
// E13: "Amount greater than max permissible amount"
// E14: "Invalid delegate Id"
// E15: "Invalid amount"
// E16: "Funding already paid for the epoch"
// E17: "Zero address"
// E18: "Asset not found"
// E19: "Token not found"