Dopex - pep7siup's results

Lines of code

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

Vulnerability details

Impact

Immediate loss on vault liquidity provider side (Option Writers). All collateral in VaultLP could be drained within the same transaction if the protocol's offline algorithm detects the profits.

Proof of Concept

The issue revolves around a lack of validation condition that allows immediate exercise of options when the market price falls below a certain threshold (roundingPrecision). This leads to a situation where the option can be exercised without any constraints, resulting in a positive profit for the option buyer, causing an immediate loss to the vault liquidity providers.

1. If the current market price (currentPrice) falls within the range (0, roundingPrecision), Alice can buy an In-the-money option and can immediately exercise it at strike price of roundingPrecision which is 1e6 or 0.01weth thanks to roundUp. Alice can purchase an option with a strike price (strike) and an amount (amount) such that strike * amount > premium.
2. Alice exercises the option, receiving strike * amount - premium as profit.
3. As a result, the vault liquidity provider faces an immediate loss of collateral, as the option was exercised without any expiry constraints.

Tools Used

Manual Review

Recommended Mitigation Steps

Do not allow option purchase if the market price falls below the roundingPrecision. FYI, setting rdpxCoreV2.putOptionsRequired flag to false is not sufficient since it can be front-run.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/amo/UniV3LiquidityAmo.sol#L332

Vulnerability details

Impact

Eventhough the RdpxV2Core contract implements a ERC721Holder, allowing itself to receive ERC721 tokens, it has no functionality for managing them. For example, it does not include functions for transferring the tokens to other addresses, approving other addresses to manage the tokens, or checking the balance of tokens held by the contract. For this reason, any ERC721 tokens which are recoverERC721 to the RdpxV2Core will be stuck there forever. More severely, if the NonfungiblePositionManager NFTs managed by the UniV3LiquidityAmo were to withdrawn to the RdpxV2Core, the entire collateral committed to these NFTs can never be redeemed, resulting in a loss of funds.

// Findings are labeled with '<= FOUND'
// File: contracts/amo/UniV3LiquidityAmo.sol
324:  function recoverERC721(
325:    ...
330:    INonfungiblePositionManager(tokenAddress).safeTransferFrom(
331:      address(this),
332:      rdpxV2Core, // <= FOUND: rdpxV2Core cannot handle ERC721s sent from UniV3LiquidityAmo
333:      token_id
334:    );
335:    emit RecoveredERC721(tokenAddress, token_id);
336:  }

Proof of Concept

1. Alice, a system admin, attempts to recoverERC721 an ERC721 token to rdpxV2Core.

2. Since rdpxV2Core has no function to handle other types of ERC721 tokens, the "recovered" ERC721 token gets stuck in rdpxV2Core forever.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider sending the ERC721 token to a trusted address (address _to) that can handle all ERC721 tokens.

Assessed type

ERC721

Lines of code

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

Vulnerability details

Impact

The formula to calculate tokenAToRemove amount in ReLPContract.reLP function is incorrect where the tokenAInfo.tokenAReserve is used instead of rdpx_supply or IERC20(addresses.tokenA).totalSupply() (check line 233) as specified in the documentation

// Findings are labeled with '<= FOUND'
// File: contracts/reLP/ReLPContract.sol
202:  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {
				...
232:    uint256 tokenAToRemove = ((((_amount * 4) * 1e18) /
233:      tokenAInfo.tokenAReserve) * // <= FOUND: should be rdpx_supply
234:      tokenAInfo.tokenALpReserve *
235:      baseReLpRatio) / (1e18 * DEFAULT_PRECISION * 1e2);
					...
307:  }

Since tokenAInfo.tokenAReserve is used, which is always much less than rdpx_supply, leading to the computed tokenAToRemove values that are much larger than expected. This can ultimately cause the whole bonding process to fail, even with a reasonable target amount of bond, due to the scaling factor of rdpx_supply / tokenAReserve.

Additionally, if the rdpxV2Core contract enables the isReLPActive flag, this issue can make the bonding process inoperable if rdpx_supply / tokenAReserve is too large. Furthermore, a malicious attacker can use a reasonably smaller fund to force the removal of all liquidity provider (LP) tokens from the AMO, swap ETH to rDPX to pump the price, and simultaneously exploit the pump in the rDPX price with a flash MEV attack.

Proof of Concept

1. Alice, who is aware of this vulnerability, interacts with the contract.
2. She performs actions that trigger the incorrect calculation of tokenAToRemove using tokenAInfo.tokenAReserve.
3. This leads to tokenAToRemove values that are much larger than expected, causing issues in the bonding process.
4. In addition, Alice may choose to exploit this vulnerability by using a smaller fund to force the removal of LP tokens from the AMO, manipulating the price of rDPX, and performing flash MEV attacks.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to fix the calculation of tokenAToRemove by using rdpx_supply instead of tokenAInfo.tokenAReserve. This change will ensure that lpToRemove stays within the expected range and aligns with the specifications outlined in the documentation.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The impact of this vulnerability is that out-of-the-money (OTM) options can never be forfeited, allowing option writers to enjoy perpetual premium fundings. This can lead to financial imbalances within the system.

Proof of Concept

The sponsor-provided documentation specifies that options can be settled or forfeited. However, this contract does not implement the forfeit functionality.

1. The active options becomes more and more OTM. The higher the OTM range is, the more respective funding the protocol has to pay due to Black-Scholes. Those options should be forfeited to cut the loss.
2. The protocol attempts to forfeit the options, but the contract does not have the functionality to forfeit OTM options.
3. As a result, the option writers continues to receive premium fundings for the OTM option indefinitely.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to implement a forfeit function that allows the protocol to cut losses on out-of-the-money (OTM) options. This function should stop paying funding for forfeited OTM options, saving protocol's reserve funds.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The vulnerability occurs due to the multiplication of amountB by tokenAInfo.tokenAPrice (Line 273-275) while it's supposed to be a division. As a result, if tokenAPrice becomes larger than 1e8 due to market movement, mintokenAAmount can become larger than expectation, which can lead to failed swaps (INSUFFICIENT_OUTPUT_AMOUNT). This, in turn, can render the ReLP process inoperable.

// Findings are labeled with '<= FOUND'
// File: contracts/reLP/ReLPContract.sol
202:  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {
203:    ...
272:    // calculate min amount of tokenA to be received
273:    mintokenAAmount =
274:      (((amountB / 2) * tokenAInfo.tokenAPrice) / 1e8) -
275:      (((amountB / 2) * tokenAInfo.tokenAPrice * slippageTolerance) / 1e16);// <= FOUND: wrong formular to calc mintokenAAmount
276:    uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
277:      .swapExactTokensForTokens(
278:        amountB / 2,
279:        mintokenAAmount,
280:        path,
281:        address(this),
282:        block.timestamp + 10
283:      )[path.length - 1];
284:...
306:  }

Proof of Concept

// File: tests/ReLP.t.sol
// @audit-issue H-wrongMinTokenAAmount
contract ReLPTest is Setup {
    UniV2LiquidityAMO uniV2LiquidityAMO;
    address attacker = makeAddr("attacker");

    function getPathFor(
        address tokenIn,
        address tokenOut
    ) private pure returns (address[] memory) {
        address[] memory path = new address[](2);
        path[0] = tokenIn;
        path[1] = tokenOut;
        return path;
    }

    function _initV2Amo() 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), 1000e18);
        weth.transfer(address(rdpxV2Core), 100e18);

        // add liquidity
        uniV2LiquidityAMO.addLiquidity(1000e18, 100e18, 0, 0);
        uniV2LiquidityAMO.approveContractToSpend(
            address(pair),
            address(reLpContract),
            type(uint256).max
        );

        rdpxV2Core.setIsreLP(true);
    }

    function _initReLP() public {
        // 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);
    }

    function _simulateLargeTokenAPrice() public {
        // Default Setup simulate rdpxPriceInWeth < 1
        // to simulate rdpxPriceInWeth > 1, we need reserveWeth > reserveRdpx
        (uint256 reserveRdpx, uint256 reserveWeth) = UniswapV2Library
            .getReserves(address(factory), address(rdpx), address(weth));
        assertTrue(reserveRdpx > reserveWeth, "Failed: rdpxPriceInWeth > 1");

        // swap to pump rdpx price > 1 weth
        uint256 amountWethIn = (reserveRdpx - reserveWeth) * 2;
        router.swapExactTokensForTokens(
            amountWethIn,
            0,
            getPathFor(address(weth), address(rdpx)),
            address(this),
            block.timestamp
        );

        (reserveRdpx, reserveWeth) = UniswapV2Library.getReserves(
            address(factory),
            address(rdpx),
            address(weth)
        );
        assertTrue(reserveRdpx < reserveWeth, "Failed: rdpxPriceInWeth < 1");

        rdpxPriceOracle.updateRdpxPrice((reserveWeth * 1e8) / reserveRdpx);
    }

    function testReLp_minTokenAAmount_largeTokenAPrice_reverts(
        uint256 bondAmount
    ) public {
        _initV2Amo();
        _initReLP();
        _simulateLargeTokenAPrice();

        // @audit-info wrong equation makes minTokenAAmount larger than expected, ReLPContract.reLP will revert when calling swapExactTokensForTokens
        vm.assume(bondAmount > 0.1e18);
        vm.assume(bondAmount < 10e18);
        vm.expectRevert("UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT");
        rdpxV2Core.bond(bondAmount, 0, address(this));
    }
}

Result:

forge test -vv --mt testReLp_minTokenAAmount_largeTokenAPrice_reverts Running 1 test for tests/ReLP.t.sol:ReLPTest [PASS] testReLp_minTokenAAmount_largeTokenAPrice_reverts(uint256) (runs: 256, μ: 3733822, ~: 3733829)

The test result shows that given a large tokenA price (rdpxInWeth > 1), rdpxV2Core.bond calls would fail with "UniswapV2Router: INSUFFICIENT_OUTPUT_AMOUNT" error.

Tools Used

Foundry

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to divide amountB by tokenAPrice when calculating mintokenAAmount.

mintokenAAmount =
      (((amountB / 2) * 1e8 / tokenAInfo.tokenAPrice)) -
      ((amountB / 2) * slippageTolerance / tokenAInfo.tokenAPrice); // 1e8 decimal precisions of slippage & tokenAPrice were canceled out

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The logics to compute minOut based on _ethToDpxEth direction are put in wrong order. Put it in details, when _ethToDpxEth is True, minOut is supposed to be minDpxEthOut which should be computed by multiplying ethAmountIn with ethPrice instead of DpxEthPrice as depicted at Lines 546-547. We can estimate how much smaller the minOut is compared to the correct one via following equation:

wrong minOut / right minOut = (_amount * getDpxEthPrice()) / 1e8) / (_amount * getEthPrice()) / 1e8) = (getDpxEthPrice() ** 2) / 1e16

This means, for every x% drop of getDpxEthPrice, we would trade with an compounded drop of (x^2)% below market price. This will open the trade to MEV attack.

In the case of lowerDepeg, if certain conditions are met, an attacker can sandwich the transaction with a pump-and-dump of DpxEth, resulting in a suboptimal amount of DpxEth tokens received from the trade which makes the re-pegging process cost more resources.

In the case of upperDepeg, similar conditions can be exploited, allowing an attacker to manipulate the Eth price.

Proof of Concept

Scenario for lowerDepeg:

1. Alice initiates a lowerDepeg transaction with specific conditions met: getDpxEthPrice < 1e8 and _ethToDpxEth = true.

2. An attacker, Bob, monitors the pending transactions and identifies Alice's lowerDepeg transaction.

3. Bob quickly submits a transaction that sandwiches Alice's transaction with a pump-and-dump of DpxEth, manipulating the price.

4. Alice's transaction goes through but with a suboptimal amount of DpxEth tokens received, making the re-peg less efficient.

Scenario for upperDepeg:

1. Alice initiates an upperDepeg transaction with specific conditions met: getEthPrice < 1e8.

2. Bob, the attacker, spots Alice's transaction.

3. Bob quickly submits a transaction that manipulates the Eth price, making Alice's transaction suboptimal in terms of Eth received.

4. Alice's transaction results in receiving fewer Eth tokens than expected.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, consider switching the logic in the _ethToDpxEth cases to ensure that the minAmount is set correctly for both lowerDepeg and upperDepeg functions.

  uint256 minOut = _ethToDpxEth
      ? (((_amount * getEthPrice()) / 1e8) -
        (((_amount * getEthPrice()) * slippageTolerance) / 1e16))
      : (((_amount * getDpxEthPrice()) / 1e8) -
        (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16));

Assessed type

MEV

Lines of code

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

Vulnerability details

Impact

The impact of this vulnerability is that it can lead to fund loss for options writers or the protocol, depending on the scenario.

1. If the epoch duration is shortened such that the nextFundingPaymentTimestamp is less than the lastUpdateTime, the eligible pending funds within the period (before changing duration) will be lost. Consider calling function updateFundingPaymentPointer after shortening the duration, the new nextFundingPaymentTimestamp() is less than lastUpdateTime at Line 464, which in turn skips the fund transfering block. It then attempts to increment the latestFundingPaymentPointer to move to the next epoch. The pending funds in the old epoch is officially un-accounted, causing loss of funds for the VaultLP participants.

2. If the epoch duration is lengthened, the next call to updateFunding would skip the updateFundingPaymentPointer logic due to false value from comparison at line 463. Therefore, the latestFundingPaymentPointer remains the same, creating a prolonged epoch with positive funding rate (fundingRates not cleared at the time of duration changing). This prolonged epoch would allow excessive funds being sent to the vault LP. This means option writers can enjoy the existing funding rate for a time period much longer than the original epoch, leading to fund losses for the protocol.

Proof of Concept

Scenario 1 (Shortened Epoch Duration):

1. Alice, the protocol admin, shortens the epoch duration, causing the new nextFundingPaymentTimestamp() to be less than lastUpdateTime.

2. Alice calls updateFunding(), which moves latestFundingPaymentPointer to a higher epoch without accounting for pending funds in the old epoch.

3. The currentFundingRate is reset to 0, and lastUpdateTime is updated to block.timestamp, but no funds are transferred to the PerpetualAtlanticVaultLP.

4. Option writers in the old epoch experience losses.

Scenario 2 (Lengthened Epoch Duration):

1. Alice lengthens the epoch duration.

2. The latestFundingPaymentPointer remains the same.

3. The next call to updateFunding() skips the updateFundingPaymentPointer() and keeps sending funds to the vault LP until new Epoch flip which is longer than the original timestamp.

4. Option writers enjoy the existing funding rate for a longer time, effectively leading to fund losses for the protocol.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability:

1. When changing the funding duration, ensure that any pending funds are settled to debt or pending funds accounts. Then, change the funding duration and reset lastUpdateTime to 0 to start fresh at the next epoch start.

2. For lengthening the epoch duration, consider allowing it only if the current funding rate is 0 or implementing safeguards to prevent potential fund losses due to prolonged existing funding rates.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

UniV2LiquidityAmo._sendTokensToRdpxV2Core lacks a sync callback to rdpxV2Core. This can cause incorrect accounting in the rdpxV2Core's backing reserve. This incorrect accounting may lead to disrupted operations inside the rdpxV2Core contract.

Proof of Concept

1. The market DpxEth price is under-pegged, rdpxV2Core.lowerDepeg needs to be called to bring the price back up but not successful because most of WETH reserves were locked in UniV2LiquidityAmo.lpTokenBalance
2. Alice, an admin of the system, performs an removeLiquidity action on UniV2LiquidityAmo to fund to lowerDepeg process. However, since sync was not called, the rdpxV2Core's WETH reserve is not properly updated.
3. Because the silent mismatch of WETH reserve accounting went un-noticed, lowerDepeg keeps failing. Causing disruption in rdpxV2Core operations.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to add the IRdpxV2Core(addresses.rdpxV2Core).sync() step after token transfers. This will ensure that the contract's reserve accounting is correctly updated and aligned with the intended behavior of the rdpxV2Core contract.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The impact of this vulnerability is that excessive tokenB (WETH) is stuck in this contract indefinitely, leading to a loss of funds. This occurs because the contract does not guarantee that tokenB will be fully utilized when adding liquidity. When swapping amountB/2 to tokenAAmountOut, the price of tokenA becomes slightly larger, which can result in an excess of tokenB that cannot be retrieved from the contract.

Proof of Concept

// File: tests/rdpxV2-core/Periphery.t.sol
  function testReLpContract_stuckETH() public {
    uint256 reLpWethBalanceBefore = weth.balanceOf(address(reLpContract));

    // utilize existing test in tests/rdpxV2-core/Periphery.t.sol
    testReLpContract();

    uint256 reLpWethBalanceAfter = weth.balanceOf(address(reLpContract));
    assertEq(reLpWethBalanceAfter - reLpWethBalanceBefore, 0, "wETH left in ReLP");
  }

Result:

Logs: Error: wETH left in ReLP Error: a == b not satisfied [uint] Left: 1022407518327480 Right: 0 Failing tests: Encountered 1 failing test in tests/rdpxV2-core/Periphery.t.sol:Periphery [FAIL. Reason: Assertion failed.] testReLpContract_stuckETH() (gas: 4044033)

Test result confirms there are wETH left in the contract after reLP() was done. Those wETH is stuck forever.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, it is recommended to implement a mechanism to transfer the excess tokenB to rdpxV2Core.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Impact

Since PerpetualAtlanticVault.updateFundingPaymentPointer is a public function and is independent of RdpxV2Core's pause state, if the PerpetualAtlanticVault.updateFundingPaymentPointer function is called while the RdpxV2Core is paused for a duration longer than an Epoch period, the PerpetualAtlanticVault.latestFundingPaymentPointer becomes up-to-date. This forces the RdpxV2Core.provideFunding function to miss payment for the previous Epoch since it relies on PerpetualAtlanticVault.latestFundingPaymentPointer instead of its own, resulting in a loss of funds for option writers.

Proof of Concept

1. Alice, the protocol admin, pauses the RdpxV2Core for a period longer than an Epoch.

2. During this pause, the PerpetualAtlanticVault.updateFundingPaymentPointer function is freely called, updating the PerpetualAtlanticVault.latestFundingPaymentPointer to the current Epoch.

3. When the system is unpaused and RdpxV2Core.provideFunding is called for the current Epoch, it calculates funding based on the updated PerpetualAtlanticVault.latestFundingPaymentPointer, effectively skipping the payment for the previous Epoch.

4. Option writers who were supposed to receive funding for the previous Epoch do not receive their payments, resulting in a loss of eligible funds.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this vulnerability, the RdpxV2Core contract should manage its own latestFundingPaymentPointer rather than tracking the PerpetualAtlanticVault's latestFundingPaymentPointer. This would ensure that RdpxV2Core can accurately calculate and provide funding even if the system was paused for an extended period.

Assessed type

Other