Dopex - gjaldon's results

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L359-L361 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L200-L203 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L772-L774

Vulnerability details

Impact

A strict validation in PerpetualAtlanticVaultLP.subtractLoss() makes it possible for anyone to disable RdpxV2Core.settle() just by sending 1 WETH to PerpetualAtlanticVaultLP.

Proof of Concept

The check collateral.balanceOf(address(this)) == _totalCollateral - loss found in subtractLoss() below can be easily made to fail by anyone just by sending 1 unit of collateral to PerpetualAtlanticVaultLP. In this case, the collateral is WETH.

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

PerpetualAtlanticVaultLP does not provide any functions to re-sync _totalCollateral with the WETH balance of the contract so this leads to permanent disabling of RdpxV2Core.settle(). Only RdpxV2Core.settle() is affected because it is the only external function that calls VaultLP.subtractLoss() internally.

The Dopex protocol relies on settle() to increase the percentage of dpxETH's backing reserves in WETH and bring back the peg of dpxETH when rDPX price drops. This essentially disables one of the core functionalities of the Peg Stability Module and makes it difficult for the protocol to maintain dpxETH's peg. This causes damage to the protocol and the dpxETH stablecoin that is difficult to quantify.

Tools Used

Manual Review

Recommended Mitigation Steps

Change subtractLoss()'s validation to be less strict like so:

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

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L190-L196 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145-L175 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L262-L266 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L218-L229

Vulnerability details

Impact

Earlier depositors in PerpetualAtlanticVaultLP can wait for PerpetualAtlanticVault to add proceeds which the vault does every time it updates funding. Once proceeds are added, these earlier depositors can redeem their shares which are now worth more since addProceeds() does not increase the number of shares and shares price is based on the total collateral / total supply of shares.

Proof of Concept

This issue can be reproduced with the following test:

  function testInitialDepositorsStealWeth() external {
    address alice = address(1);
    setApprovals(alice);
    mintWeth(10 ether, alice);
    deposit(10 ether, alice);

    // PerpVault adds proceeds to VaultLP via updateFunding() or updateFundingPaymentPointer()
    mintWeth(100 ether, address(vault));
    vm.startPrank(address(vault));
    weth.transfer(address(vaultLp), 100 ether);
    vaultLp.addProceeds(100 ether);
    vm.stopPrank();

    assertEq(vaultLp.totalCollateral(), 110 ether);
    
    // Alice redeems shares as soon as TotalCollateral has increased
    uint256 aliceShares = vaultLp.balanceOf(alice);
    vm.startPrank(alice);
    vaultLp.redeem(aliceShares, alice, alice);
    vm.stopPrank();

    assertEq(vaultLp.totalCollateral(), 0);
    assertEq(weth.balanceOf(alice), 110 ether);
  }

Copy the test above to tests/perp-vault/Integration.t.sol and run it with :

$ forge test --mt testInitialDepositorsStealWeth`

The test shows Alice as a depositor who has deposited prior to the PerpVault adding proceeds to the VaultLP. This example shows Alice starting off with 10 ether and ending up with 110 ether, 100 ether of which are from the PerpVault's proceeds.

Tools Used

Manual Review, Forge Test

Recommended Mitigation Steps

So that earlier depositors can not immediately withdraw the WETH deposited by the PerpVault, the PerpVault can use deposit() instead of doing a direct transfer of WETH and then call addProceeds(). If the PerpVault would like to distribute the WETH to depositors, they can instead burn() their shares to decrease the total supply of shares and increase their price. This requires a burn() function to be added to VaultLP which is only accessible to the PerpVault.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L268-L284 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L118-L135 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L364-L366

Vulnerability details

Impact

Price of VaultLP shares significantly decrease after RDPX collateral is added by PerpVault due to incorrect decimals conversion. This impacts depositors who deposit after RDPX collateral is added to the VaultLP (RDPX balance of VaultLP goes from 0 to non-zero). Post-RDPX collateral depositors will get minted significantly less shares and will lose a portion of the WETH they deposited to Pre-RDPX collateral depositors.

Proof of Concept

Add the below test to tests/perp-vault/Integration.t.sol and run it with forge test --mt testPostRdpxCollateralIssue:

  function testPostRdpxCollateralIssue() external {
    // 1 rdpx = 0.2 WETH
    priceOracle.updateRdpxPrice(0.2 ether);

    address alice = address(1);
    setApprovals(alice);
    mintWeth(10 ether, alice);
    deposit(10 ether, alice);

    // PerpVault adds rdpx to VaultLP
    mintRdpx(500 ether, address(vault));
    vm.startPrank(address(vault));
    rdpx.transfer(address(vaultLp), 500 ether);
    vaultLp.addRdpx(500 ether);
    vm.stopPrank();

    // Bob deposits after RDPX collateral was added to VaultLP
    address bob = address(2);
    setApprovals(bob);
    mintWeth(10 ether, bob);
    deposit(10 ether, bob);

    uint256 aliceShares = vaultLp.balanceOf(alice);
    uint256 bobShares = vaultLp.balanceOf(bob);

    assertGt(aliceShares, bobShares);

    vm.prank(alice);
    vaultLp.redeem(aliceShares, alice, alice);

    vm.prank(bob);
    vaultLp.redeem(bobShares, bob, bob);

    assertGt(weth.balanceOf(alice), weth.balanceOf(bob));
    assertGt(weth.balanceOf(alice), 10 ether);
    assertLt(weth.balanceOf(bob), 10 ether);
    assertGt(rdpx.balanceOf(alice), rdpx.balanceOf(bob));

    console.log("Shares alice and bob", aliceShares, bobShares);
    console.log("Weth balance of alice and bob", weth.balanceOf(alice), weth.balanceOf(bob));
    console.log("Rdpx balance of alice and bob", rdpx.balanceOf(alice), rdpx.balanceOf(bob));
  }

The above test shows that Alice, who has deposited prior to the RDPX collateral being added to the VaultLP, ends up with significantly more shares than Bob, who has deposited after the RDPX collateral was added to the VaultLP. The difference in shares is so significant that Alice ends up with almost 2x as much WETH as she deposited. Bob, on the other hand, ends up with less than 1% of his WETH deposit.

This issue is cause by the incorrect decimals conversion in convertToShares() which is called when a user deposit()s.

uint256 totalVaultCollateral = totalCollateral() + ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8);
return supply == 0 ? assets : assets.mulDivDown(supply, totalVaultCollateral);

rdpxPrinceInAlphaToken is an 18 decimals value and so is _rdpxCollateral since RDPX token has 18 decimals. Multiplying both values will lead to a 36 decimals value but the calculation only divides it by 1e8 instead of 1e18. This bloats totalVaultCollateral by at least 8 decimals. And since shares is computed as (assets * supply) / totalVaultCollateral, the bloated totalVaultCollateral leads to significantly less shares being minted.

Tools Used

Manual Review, Forge Test

Recommended Mitigation Steps

Change the convertShares() code to:

uint256 totalVaultCollateral = totalCollateral() + ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e18);

This corrects the conversion and no longer bloats totalVaultCollateral.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L335 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L296-L300 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L921

Vulnerability details

Impact

There is a decimal issue in settle() that bloats the amount of WETH being withdrawn from VaultLP. This over-withdrawal of WETH comes at the expense of liquidity providers in the VaultLP who staked their WETH to protect the floor price of dpxETH.

This leads to _activeCollateral no longer being accurate and reflecting a much smaller amount than the total number of locked collateral in Put Options. There is no way to update _activeCollateral directly to fix the issue. This leads to being unable to settle() some of the remaining Options even if Admin sends back the over-withdrawn WETH back to the VaultLP. Also, sending back the over-withdrawn WETH without increasing the _activeCollateral will mean depositors will be able to withdraw more collateral than they are supposed to.

Overall, this puts the protocol in a bad state and makes the Put Options feature unreliable.

Proof of Concept

The steps that lead to the issue are:

1. Assuming purchase() calculations are correct and Put options are required, options are purchased and minted every time users bond(). Purchasing options would lock some WETH collateral in the VaultLP to ensure that Put Options can be exercised once they are In-The-Money.
2. RDPX price drops and is now below the strike of a few Options.
3. Admin will now run settle() on these Options that are In-The-Money. As long as there are enough WETH in VaultLP, this leads to the over-withdrawal of WETH and the inaccurate accounting of _activeCollateral.

This is because settle() makes an incorrect decimal conversion when calculating the WETH amount it is supposed to withdraw from the VaultLP when exercising the Put Options:

ethAmount += (amount * strike) / 1e8;

amount and strike above are both fixed-point 18-decimal values but they are only being divided by 1e8 instead of 1e18. This leads to ethAmount being bloated by 10 decimals. strike is the RDPX price in WETH which is in 18 decimals as is stated in the comments of the RdpxEthOracle.getRdpxPriceInEth() function that it relies on:

    /// @notice Returns the price of rDPX in ETH
    /// @return price price of rDPX in ETH in 1e18 decimals
    function getRdpxPriceInEth() external view override returns (uint price) {

amount is the amount of RDPX required for bonding which would also be 18 decimals since RDPX is an 18-decimal token.

The damage caused by this miscalculation bloating ethAmount is in settle() withdrawing and unlocking more WETH than it should:

    // Transfer collateral token from perpetual vault to rdpx rdpxV2Core
    collateralToken.safeTransferFrom(
      addresses.perpetualAtlanticVaultLP,
      addresses.rdpxV2Core,
      ethAmount
    );
    // Transfer rdpx from rdpx rdpxV2Core to perpetual vault
    IERC20WithBurn(addresses.rdpx).safeTransferFrom(
      addresses.rdpxV2Core,
      addresses.perpetualAtlanticVaultLP,
      rdpxAmount
    );

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
      .unlockLiquidity(ethAmount);

Tools Used

Manual Review

Recommended Mitigation Steps

Change settle()'s computation of ethAmount with the following code:

ethAmount += (amount * strike) / 1e8;

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L539-L551 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L286 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L429-L434

Vulnerability details

Impact

When Core calls purchase(), it is overpaying the premium for the Put Options by 1e10x. What it is paying is so bloated that the amount is even much larger than the value it is backing.

Proof of Concept

The below test can be added to tests/perp-vault/Unit.t.sol to make the issue more obvious:

import { IOptionPricing } from "contracts/interfaces/IOptionPricing.sol";
import { OptionPricingSimple } from "contracts/libraries/OptionPricingSimple.sol";
import "forge-std/console.sol";
// import statements above are necessary for the test to run

  function testCalculatePremiumIssue() external {
    IOptionPricing _optionPricing = new OptionPricingSimple(100, 1e9);
    vm.prank(address(vault));
    weth.approve(address(vaultLp), 0);
    vault.setAddresses(
      address(_optionPricing),
      address(priceOracle),
      address(volOracle),
      address(1),
      address(rdpx),
      address(vaultLp),
      address(this)
    );

    uint256 timeTillExpiry = 7 days;
    uint256 premium = vault.calculatePremium(
      0.015 ether, // 1e18 because rdpxPrice is 18 decimals
      500 ether,
      timeTillExpiry,
      0.02 ether  // 1e18 because rdpxPrice is 18 decimals
    );

    console.log("Premium: ", premium);
  }

In the above test, we replace the MockOptionPricing contract with the OptionPricingSimple contract that comes with the repo. The test does not do any assertions since we are only interested to see the log output of calculatePremium() which is:

Premium: 10000000000000000000000000000

That output is the premium that Core is paying for in WETH, which amounts to 1e28 WETH. That means the premium is 10M Ether, which is clearly overpriced. The issue lies in the decimal conversion in calculatePremium() which uses 1e8 instead of 1e18:

  function calculatePremium(
    uint256 _strike,
    uint256 _amount,
    uint256 timeToExpiry,
    uint256 _price
  ) public view returns (uint256 premium) {
    premium = ((IOptionPricing(addresses.optionPricing).getOptionPrice(
      _strike,
      _price > 0 ? _price : getUnderlyingPrice(),
      getVolatility(_strike),
      timeToExpiry
    ) * _amount) / 1e8);
  }

If 1e18 was used, we would get 1e18 (1 Ether) premium instead, which is a more realistic value for a 10 Ether Total Spot Price (500 ether * 0.02 ether) worth of Option.

Tools Used

Manual Review, Forge Test

Recommended Mitigation Steps

Change the divisor in calculatePremium() from 1e8 to 1e18 like so:

  function calculatePremium(
    uint256 _strike,
    uint256 _amount,
    uint256 timeToExpiry,
    uint256 _price
  ) public view returns (uint256 premium) {
    premium = ((IOptionPricing(addresses.optionPricing).getOptionPrice(
      _strike,
      _price > 0 ? _price : getUnderlyingPrice(),
      getVolatility(_strike),
      timeToExpiry
    ) * _amount) / 1e18);
  }

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L699-L744 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L819-L885 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L598-L615

Vulnerability details

Impact

WETH delegators provide their WETH so that other users that only have RDPX can use the delegated WETH in the pool to bond. In exchange, WETH delegators charge a fee for the usage of their WETH. Due to issues with the calculation of the receipt tokens, the Delegatee (RDPX provider) would get 10-20x more receipt tokens than the Delegator. If the calculations were correct, the Delegator would get more receipt tokens because of the delegate fee.

Proof of Concept

_calculateAmounts() is used in bondWithDelegate() to calculate the amount of receipt tokens the Delegator (WETH provider) and the Delegatee (RDPX provider) are getting.

We copy the _calculateAmounts() logic and put it in a contract that we put in Remix:

//SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;

import { Math } from "@openzeppelin/contracts/utils/math/Math.sol";
import "hardhat/console.sol";

contract Dopex {
    function _calculateAmounts() external view returns (uint256 amount1, uint256 amount2) {
        uint256 _wethRequired = 1 ether;
        uint256 _rdpxRequired = 5 ether;
        uint256 _amount = 2000 ether;
        uint256 _delegateFee = 5e8;
        uint256 rdpxPrice = 0.2 ether;

        // Commented below for better clarity
        uint256 rdpxRequiredInWeth = (_rdpxRequired * rdpxPrice) / 1e8;

        // amount required for delegatee
        amount1 = ((rdpxRequiredInWeth * _amount) /
        (rdpxRequiredInWeth + _wethRequired));
        
        // account for delegate fee
        amount1 = (amount1 * (100e8 - _delegateFee)) / 1e10;

        amount2 = _amount - amount1;

        console.log("Delegatee Amount: ", amount1);
        console.log("Delegator Amount: ", amount2);
    }
}

After deploying the above contract in Remix and then running the _calculateAmounts() function, we get the following logs:

Delegatee Amount: 1899999999810000000018
Delegator Amount: 100000000189999999982

As can be seen, we are getting a much larger amount of 1,899 for the Delegatee while the Delegator gets only 100. That means that the Delegator would lose out on ~90% of the receipt tokens they were supposed to get and will experience a loss on the WETH they delegated. On the other hand, the Delegate would almost 10x their RDPX value and get ~10x the receipt tokens they were supposed to get.

This issue is caused by using the wrong divisor of 1e8 in _calculateAmounts():

uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;

Since getRdpxPrice() and _rdpxRequired are 18 decimals, the divisor should be 1e18 instead of 1e8.

Tools Used

Manual Review, Remix

Recommended Mitigation Steps

We can fix it by replacing the divisor of 1e8 with 1e18 in _calculateAmounts() like so:

-uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e8;
+uint256 rdpxRequiredInWeth = (_rdpxRequired * getRdpxPrice()) / 1e18;

Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/reLP/ReLPContract.sol#L202-L312 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L930 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L873

Vulnerability details

Impact

Incorrect calculation of minimum out of Token B in reLP() and minimum out of Token A will always revert bond() and bondWithDelegate() when reLP is active. This means that the re-LP feature can not be used to burn RDPX and keep its price up.

Proof of Concept

We need to correct the testReLpContract() test by setting RDPX price to a 1e18 value. The test was incorrectly set to use 1e8. We can add the following code to the start of testReLpContract() test:

// RDPX Price must be a 1e18 value since RDPX has 18 decimals
rdpxPriceOracle.updateRdpxPrice(1e18);

Running the test now will return an UniswapV2Router: INSUFFICIENT_B_AMOUNT error because we are passing a bloated mintokenBAmount to UniswapV2Router.removeLiquidity() of 35823626367177226988400000000:

 uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice) /
   1e8) -
   ((tokenAToRemove * tokenAInfo.tokenAPrice) * liquiditySlippageTolerance) /
   1e16;

35823626367177226988400000000 is larger by 10 decimals and this would lead to Core.bond() and Core.bondWithDelegate() to always fail when isReLPActive is true.

minTokenAAmount also returns a value that is bloated by 10 decimals:

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

Calculations for both minTokenBAmount and minTokenAAmount will need to be fixed for reLP() to work as intended.

Tools Used

Manual Review, Forge Test

Recommended Mitigation Steps

Computation for minimum out of Token B can be replaced with:

 -uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice) /
   -1e8) -
   -((tokenAToRemove * tokenAInfo.tokenAPrice) * liquiditySlippageTolerance) /
   -1e16;
 +uint256 mintokenBAmount = ((lpToRemove * reserveB) / totalLpSupply) * (1e8 - liquiditySlippageTolerance) / 1e8;

Apart from correcting the decimals, we are also replacing the wrong usage of tokenAToRemove * tokenAPrice since that gives us Token A amounts instead of Token B amounts.

Computation for minimum out of Token A can be replaced with:

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

For Token A's calculation, we only need to fix the decimals.

Once the fixes are applied, running the test should now fail with incorrect assertions which shows reLP() ran without reverting.

Assessed type

Uniswap

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L162 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L262-L266 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L218-L229 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVault.sol#L346-L357

Vulnerability details

Impact

VaultLP does not allow redemption of shares when it leads to 0 WETH being withdrawn. VaultLP can end up in a state where it has a small amount of WETH and most of its reserves is in RDPX instead. This can happen when most of its WETH is locked as collateral for Put Options and all these Put Options were exercised/settled due to RDPX price dropping.

In this state, many depositors in the LP who have lost their WETH will not even be able to redeem their shares for the RDPX compensation.

Proof of Concept

The following steps will reproduce the issue:

1. Depositors deposit() 100_000e18 WETH to VaultLP
2. RdpxV2Core purchased so many options (since so many users were bonding) that it locked up all 100_000e18 WETH collateral as backing for all the options.
3. RDPX price dropped so much that all the Put Options had to be settled() to bring dpxETH peg back up. This replaces all the 100_000e18 WETH with RDPX.
4. Since total WETH collateral of VaultLP is now 0, none of the depositors can redeem() their shares due to this check:

require(assets != 0, "ZERO_ASSETS");

The above check will always fail since the computation for assets looks like:

shares.mulDivDown(totalCollateral(), supply)

totalCollateral() will return 0 because there is no WETH collateral left in the VaultLP. So the above computation will be shares * 0 / supply which will always equal 0. So even if the depositor has 1_000e18 shares and a lot of RDPX they are supposed to be able to withdraw, they will not be able to. In effect, they would lose 100% of their WETH deposited into VaultLP and get zero RDXP in compensation.

Tools Used

Manual Review

Recommended Mitigation Steps

The validation in redeem() can be replaced with the following:

require((assets + rdpxAmount) != 0, "ZERO_ASSETS");

That way, as long as there is any amount of RDPX or WETH they can withdraw, they will be able to do so.

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L1169-L1173 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L705-L708 https://github.com/code-423n4/2023-08-dopex/blob/main/contracts/core/RdpxV2Core.sol#L904-L907

Vulnerability details

Impact

When bond discount is higher than 51e8 due to a large bond discount factor, or large rDPX reserves in the treasury, or both, then bond() and bondWithDelegate() will be disabled when called with no rDPX decaying bond.

Proof of Concept

calculateBondCost() is a public function that's called by both bond() and bondWithDelegate(). The issue happens because of the following code in calculateBondCost():

      rdpxRequired =
        ((RDPX_RATIO_PERCENTAGE - (bondDiscount / 2)) *
          _amount *
          DEFAULT_PRECISION) /
        (DEFAULT_PRECISION * rdpxPrice * 1e2);

RDPX_RATIO_PERCENTAGE is a constant with a value of 25e8. Since we subtract bondDiscount / 2 from RDPX_RATIO_PERCENTAGE, then bondDiscount / 2 must be a value less than 25e8 or else the function will revert with an underflow error. However, nothing in calculateBondCost() ensures that bondDiscount is never more than 50e8.

The bond discount in calculateBondCost() is computed as:

      uint256 bondDiscount = (bondDiscountFactor *
        Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) *
        1e2) / (Math.sqrt(1e18)); // 1e8 precision

      _validate(bondDiscount < 100e8, 14);

The above calculations show that bond discount increases as bondDiscountFactor and rdpxReserve increase. It also validates that bondDiscount should never be higher than 100e8. So a value of 51e8 to 99e8 should be valid values for bondDiscount and yet they cause the computation for rdpxRequired to revert with an underflow error.

Tools Used

Manual Review

Recommended Mitigation Steps

If bondDiscount is meant to be a percentage, then it should be multiplied by the total rdpx amount and subtracted from the total rdpx amount rather than subtracted directly from RDPX_RATIO_PERCENTAGE. If it's not a percentage value, then it can be subtracted from the total rdpx amount.

The same fix can be applied to the wethRequired calculation.

Assessed type

Under/Overflow