Dopex - 836541's results

Lines of code

github.com/code-423n4/2023-08-dopex/blob/main/contracts/perp-vault/PerpetualAtlanticVaultLP.sol#L145

Vulnerability details

Proof of Concept

In PerpetualAtlanticVaultLP, users can deposit WETH to get shares, which can be redeemed for WETH yield (and RDPX in the future). The rewards are payed in drip-manner and also at the start of each epoch (related to the funding rate of the entire epoch). These epoch funding payments have two things to consider:

• Predictable: an epoch has seven days and the protocol has the public nextFundingPaymentTimestamp() to check
  • Any user knows when the funding payment will happen
• Increase of Shares valuation: the funding payment covers the entire epoch, so it considerably increases the WETH Supply, which makes the share valuation to increase:
  • The value of a share in WETH is calculated using the formula: value = (shares * totalWethSupply) / sharesSupply. Each epoch start pays the WETH yield, so totalWethSupply gets bigger and affects the equation numerator in a way that the value of a share becomes bigger. The following function is the reference:

  function _convertToAssets(
    uint256 shares
  ) internal view virtual returns (uint256 assets, uint256 rdpxAmount) {
    uint256 supply = totalSupply;
    return
      (supply == 0)
        ? (shares, 0)
        : (
          shares.mulDivDown(totalCollateral(), supply),
          shares.mulDivDown(_rdpxCollateral, supply)
        );

So, with this two properties in mind, an attacker can exploit this in the following steps:

1. Attacker checks nextFundingPaymentTimestamp() to know when the funding payment will happen and picks a close timestamp to start the second step

2. Attacker deposits and get shares at the valuation of totalSupply() (total WETH supply) as X

3. Epoch Funding Payment happens and increases totalSupply(). Now the valuation of a share is X + how much the payment increased the totalSupply(). Let's say it's X + 0.1X (10% increase)

4. Attacker immediately redeem his shares

5. Result: Attacker got shares in a valuation X and instantly redeem them for a valuation 1.1X for a profit of around 10% WETH in a Risk-Free trade, since the funding payment has a predictable timestamp for any epoch.

The reason why the attacker can do this is a lack of fees in the redeem(), a function to redeem shares for underlying assets. While most Yield protocols has a withdrawFee to prevent this kind of attack, PerpetualAtlanticVaultLP dont, which causes this vulnerability.

Impact

Attacker can steal a considerable amount of funds without giving liquidity to protocol

Tools Used

Manual Audit

Recommended Mitigation Steps

There are two possibilities:

1. Add an internal function to calculate a redeem fee
2. Lock redeems for an epoch duration

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Proof of Concept

1. The method deposit checks how many assets the user may receive in the line require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
2. After that, deposit calls updateFundings, which is the method used to update the "rewards" to PerpetualAtlanticVaultLP.sol. Also, updateFundings calls updateFundingPaymentPointer, which is a method that updates the fundings payment of the entire epoch.
3. However, shares in an ERC-4626-Like protocol are calculated in the following way: shares = (assets to deposit * Total Supply of Shares) / Total Supply of Underlying Tokens. So, the amount of collateral the protocol has impacts the denominator, so (+) collateral supply = (-) shares get per deposit

  function convertToShares(
    uint256 assets
  ) public view returns (uint256 shares) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

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

4. updateFundings increases the supply of collateral for the protocol, changing the demoninator. However, shares of the first depositor are calculated before the updateFundings, effectively making the first depositor of each epoch to get more shares than It should.

  function deposit(
    uint256 assets,
    address receiver
  ) public virtual returns (uint256 shares) {
    // Check for rounding error since we round down in previewDeposit.
    require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

    perpetualAtlanticVault.updateFunding();

    // Need to transfer before minting or ERC777s could reenter.
    collateral.transferFrom(msg.sender, address(this), assets);

    _mint(receiver, shares);

    _totalCollateral += assets;

    emit Deposit(msg.sender, receiver, assets, shares);
  }

5. The calculation of assets to get for the redeem of shares is the following: assets = (Shares to burn * CollateralTotalSupply) / SharesTotalSupply. So, the updateFundings increases the CollateralTotalSupply, which is in the numerator, so it effectivelly also increases the valuation of a shares.

6. In conclusion, the first depositor of an epoch can get more shares he should and redeem it for a higher valuation.

  function _convertToAssets(
    uint256 shares
  ) internal view virtual returns (uint256 assets, uint256 rdpxAmount) {
    uint256 supply = totalSupply;
    return
      (supply == 0)
        ? (shares, 0)
        : (
          shares.mulDivDown(totalCollateral(), supply),
          shares.mulDivDown(_rdpxCollateral, supply)
        );
  }

Impact

An epoch for Perpetual Options is only 7 days, which allows this bug to be triggered four times a month. Considering the valuation of shares increases by just 10% each epoch, around 1 WETH can be "lost" each 10 epochs (70 days). Also, if protocol is deployed in a chain with MEV, attackers can use this for Risk-Free trades since he can be more sure that is going to be the first depositor of the epoch.

Tools Used

Manual Review

Recommended Mitigation Steps

Protocol should updateFundings() before the calculation of shares in deposit method.

Assessed type

Timing

Lines of code

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

Vulnerability details

Proof of Concept

Consider totalAsset() as the total value of underlying assets deposited in PerpetualAtlanticVaultLP.sol and totalSupply() as the total value of existing shares. When users deposit in PerpetualAtlaticVaultLP.sol, shares are minted so he can redeem in the future to get Yield. The calculation of how many shares he gets for an amount of deposited assets is: shares = (assets deposited * totalSupply()) / totalAsset() and happens in this function:

  function convertToShares(
    uint256 assets
  ) public view returns (uint256 shares) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

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

Here's how the contract is vulnerable:

1. A new PerpetualAtlanticVaultLP.sol has been deployed and configured, no depositors yet
2. Attacker deposits 1 wei and gets 1 share. Thus, totalAsset()==1, totalSupply()==1.
3. Attacker front-runs the deposit of a victim who wants to deposit 2e18 (2 WETH). For that, he uses ERC20::transfer to transfer 1e18 (1 WETH) directly to the contract without using deposit method so he inflates the denominator of shares calculation. Thus, totalAsset()==1e18 + 1, totalSupply()==1.
4. Next, the victim's transaction takes place. The victim gets shares = (1 * 2e18) / (1e18 + 1), which results in 1.9999, but since integer truncation happens in Solidity, the actual value is 1 share.
5. totalSupply() == 3 WETH. So, attacker redeems his 1 share, which is 50% of the totalSupply() pool (1 for Victim and 1 for attacker) and gets 1.5 WETH, a profit of 50% of his initial deposit.

Impact

Since the protocol is initially being deployed in a MEVless chain, the probability of the attack is low. However, the impact is high because of the steal of funds, so the protocol needs to be protected against any possibility of frontrun.

Tools Used

Manual Audit

Recommended Mitigation Steps

UniswapV2 fixed this with two steps:

1. First, the first mint it actually mints the first 1000 shares to the zero-address

2. Second, it requires that the minted shares are not 0

The second step is already implemented in deposit function, so only the first step is required.

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Proof of Concept

The epoch in the protocol is used as the "expiry date" of options and the frequency which fundings are payed to depositors in PerpetualAtlanticVaultLP.sol. The standard epoch duration is set to 7 days in the fundingDuration state variable at PerpetualAtlanticVault.sol.

uint256 public fundingDuration = 7 days;

It can also be changed via updateFundingDuration method in PerpetualAtlanticVault.sol#L237.

  function updateFundingDuration(
    uint256 _fundingDuration
  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    fundingDuration = _fundingDuration;
  }

The function nextFundingPaymentTimestamp() calculates the ending of an epoch using the state variable fundingDuration.

  function nextFundingPaymentTimestamp()
    public
    view
    returns (uint256 timestamp)
  {
    return genesis + (latestFundingPaymentPointer * fundingDuration);
  }

• The calculation is genesis + (latestFundingPaymentPointer * fundingDuration, which can be translated to genesis + (Counter of Epochs * Duration of an Epoch). It assumes that all epochs have the same duration, which is a mistake:

1. We are in epoch 10 and fundingDuration == 7 days, so nextFundingPaymentDuration() returns genesis + 70 days.
2. Admin changes the fundingDuration to 10 days.

• Now nextFundingPaymentDuration() returns genesis + 100 days. However, as an epoch now lasts 10 days, the 30 days difference from one fundingDuration value to another isn't correct.

3. This can cause a range of different impacts accordingly to the new fundingDuration value since important methods updateFundings, updateFundingPaymentPointer and purchase (all from PerpetualAtlanticVault.sol) uses nextFundingPaymentTimestamp in some way.

Impact

First, we need to understand the function that uses the vulnerable nextFundingPaymentTimestamp and is linked to purchase and updateFundings. It's updateFundingPaymentPointer and it's purpose is to pay an epoch when it ends and also update the latestFundingPaymentPointer, which is the counter of epochs. To do that, first it checks if block.timestamp (current time) is higher or equal to nextFundingPaymentTimestamp, so it only updates if the current epoch ended. That's done in a loop way to update even if the counter is 2 epochs outdated.

  function updateFundingPaymentPointer() public {
    // 1 - LOOPS UNTIL CURRENT TIME IS LOWER THAN EPOCH ENDING
    while (block.timestamp >= nextFundingPaymentTimestamp()) {
    /// some code
      
    // 2 - PAY THE LAST EPOCH BEFORE STARTING A NEW ONE
            collateralToken.safeTransfer(
          addresses.perpetualAtlanticVaultLP,
          (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
            1e18
        );
    /// some code
    // 3 - UPDATE THE CODE TO HANDLE THE CURRENT EPOCH 
    latestFundingPaymentPointer += 1;

Impacts are scenarios of denial of service, wrong funding payments and breaking the correct calculation of nextFundingPaymentTimestamp for an unlimited amount of time:

• Scenario of updating fundingDuration to a lower value:

1. fundingDuration = 7 days and latestFundingPaymentPointer - 7 (Counter of Epochs = 7, so we are in epoch 7). With this state variables, nextFundingPaymentTimestamp == genesis + 49 days
2. Admin updates fundingDuration to 5 days via updateFundingDuration.
3. Now, nextFundingPaymentTimestamp == genesis + 35 days. So, even that fundingDuration changed to 5 days, which should only produce a difference of 2 days to the original output, the function is now returning a difference of 14 days, effectively acting like the contract is 2-3 epochs away from the actual epoch.
4. updateFundingPaymentPointer will loop and increment lastFundingPaymentPointer a few times to get updated to make nextFundingPaymentTimestamp get updated again

while (block.timestamp >= nextFundingPaymentTimestamp() { 
// ...
            collateralToken.safeTransfer(
          addresses.perpetualAtlanticVaultLP,
          (currentFundingRate * (nextFundingPaymentTimestamp() - startTime)) /
            1e18
        );
// ... 
   lastFundingPaymentPointer += 1; 
}

5. Now that lastFundingPaymentPointer was incremented unduly, two things happen with the protocol:

• currentFundingRate of the epoch, which is a value based on sold option's premiums is lost since it's based in the current epoch and the protocol skipped the current epoch

• lastUpdatePaymentPointer, which is the counter of epochs now is forever wrong. If before the output of nextFundingPaymentTimestamp was genesis + 49 days, even if admin changes back to 7 days the fundingDuration, nextFundingPaymentTimestamp will output a bigger value since the calculation relies on lastFundingPaymentPointer. This will make every epoch lasts more than it should, since all epoch-related functions relies on nextFundingPaymentTimestamp.

• Scenario of updating fundingDuration to a higher value.

1. Let's suppose fundingDuration = 7 days and latestFundingPaymentPointer - 7 (Counter of Epochs = 7, so we are in epoch 7). With this state variables, nextFundingPaymentTimestamp == genesis + 49 days
2. Admin changes fundingDuration to 9 days.
3. Now nextFundingPaymentTimestamp == genesis + (7 * 9), which is genesis + 63 days.
4. updateFundingPaymentPointer isn't affected anymore, since it relies that block.timestamp >= nextFundingPaymentTimestamp. However, updateFundings(), which is the function used in PerpetualAtlanticVault.sol and called in important functions, like purchase (used to buy Options), redeem and deposit (both used in PerpetualAtlanticVaultLP.sol for ERC-4626-Like Yield Farm) is now affected:

  function updateFunding() public {

    updateFundingPaymentPointer(); 

    uint256 currentFundingRate = fundingRates[latestFundingPaymentPointer];

    uint256 startTime = lastUpdateTime == 0
      ? (nextFundingPaymentTimestamp() - fundingDuration)
      : lastUpdateTime;
    lastUpdateTime = block.timestamp;

    collateralToken.safeTransfer(
      addresses.perpetualAtlanticVaultLP,
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );
    // ...
 } 

• The function pays depositors of PerpetualAtlanticVaultLP.sol in a drip-maner way since it's called by a lot of methods and transfers to VaultLP a quantity of funds based on how much time of the epoch has already passed currentFundingRate * (block.timestamp - startTime).
• However, now that nextFundingPaymentTimestamp output is bigger than it should, updateFundingPaymentPointer will take longer to update the epoch, so the epoch will efectively last longer.

5. The calculation currentFundingRate * (block.timestamp - startTime) will start to get unexpected values, since the epoch duration wasn't expected to be so big.

6. Because of (5), more funds than it should will be transfered to VaultLP depositors. Also, safeTransfer (using SafeERC20.sol library) will revert if PerpetualAtlanticVault.sol doesn't have enough funds, which probably will happen since the protocol was expecting to drip-maner pay a 8 days epoch, not a bigger one.

7. So, denial of service will happen and even if admin updates updateFunding to the standard value 7 days again, the nextFundingPaymentTimestamp can have unexpected actions since his calculation is wrong and can't handle how much days passed correctly.

Tools Used

Manual Review

Recommended Mitigation Steps

1. Add an array to list all fundingDuration values used.
2. Add a mapping to map how many epochs passed for each fundingDuration used
3. Make updateFundingPaymentPointer, which is the function that updates epochs to interact with this mapping
4. Now, nextFundingPaymentTimestamp can correctly calculate the end of an epoch

  function nextFundingPaymentTimestamp()
    public
    view
    returns (uint256 timestamp)
  {
    uint256 value; 
    for (uint x; x < fundingDurationValues.length - 1; ++ x) {
       uint256 fundingDurationValue = fundingDurationValues[x]; 
       value += epochsForEachFundingDuration[fundingDurationValue] * fundingDurationValue; 
    return genesis + value;
  }

Assessed type

Math