Dopex - carrotsmuggler's results

Lines of code

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

Vulnerability details

Impact

The function subtractLoss subtracts the loss amount from the vault's LP during a settlement. There is also a require statement which makes sure the balance in the contract lines up with the accounting.

require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
_totalCollateral -= loss;

The issue is that this condition can be easily violated with an external donation. The value in _totalColalteral is changed during deposits, redeems, premium additions, and losses. Importantly, it is NOT dependent on the actual contract balance. So even before loss reporting, the amount of collateral in the contract is not necessarily equal to _totalCollateral, since any user can send weth to the contract directly.

Lets assume the _totalcollateral is 100 WETH, and the actual balance is 101 WETH due to an external donation. During a settle call on the vault, WETH is withdrawn from the LP vault. Lets assume this amount is 50 WETH.

collateralToken.safeTransferFrom(
    addresses.perpetualAtlanticVaultLP,
    addresses.rdpxV2Core,
    ethAmount
);
IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
    ethAmount
);

Lets assume ethAmount here is 50 WETH. Now, when subtractLoss is called, collateral in contract = 101 - 50 = 51 WETH, and _totalCollateral-loss = 100 - 50 = 50 WETH. This will cause the require statement to fail, and revert the transaction.

Since only the perp vault can call this function, there is no way for admins to fix this. This will break the whole settle process.

Proof of Concept

Below POC is scavenged from the test suite. A donation is made which cause settlement to revert. If the donation is removed or commented out, the settlement goes through.

function testAttackSettle() public {
        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;
        // skip(86500); // expire

        priceOracle.updateRdpxPrice(0.010 gwei); // ITM

        // Donation
        weth.transfer(address(vaultLp), 1 ether);
        vm.expectRevert("Not enough collateral was sent out");
        vault.settle(ids);
    }

Tools Used

Foundry

Recommended Mitigation Steps

Relax the require statement to allow the balance to be higher.

require(
      collateral.balanceOf(address(this)) >= _totalCollateral - loss,
      "Not enough collateral was sent out"
    );

Assessed type

Invalid Validation

Lines of code

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

Vulnerability details

Impact

The function deposit in PerpetualAtlanticVaultLP.sol processes deposits from users into the LP Vault. It first checks the shares to be minted, and then updates the funding state.

require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");
perpetualAtlanticVault.updateFunding();

The updateFunding function call transfers pending premium funds from the Vault to the LP. This basically transfers in the premium payments pending since the last update time. The issue is that the shares is calculated before the premium is collected.

The vault acts similarly to ERC4626 vaults, but in this implementation actually handles multiple tokens, WETH and Rdpx. On deposits, users are minted shares proportional to their deposit. The amount of shares depends on the assets already in the vault. Since here the shares are calculated before transferring in more funds, the vault actually calculates the proportion of shares on a smaller pool of assets, and thus mints the user more shares than they should have received.

A common test for ERC4626 vaults is that if a user deposits some assets, and then immediately redeems the shares gained form that, the user will always end up with the same or less than the amount of tokens they started with. Since in this case the user is minted more shares than they should have been, they can redeem more tokens than they deposited. This is shown in the POC.

Proof of Concept

The following POC carries out the following steps:

1. Funding etc to make vault operational. An optionis purchased, so there are pending funding payments.
2. A user deposits 1 ETH. The minted shares are recorded.
3. The user then redeems the same number of shares. The user receives more than 1 ETH back.

If the vault was primed first, with a updateFunding call, then the user receives the exact amount of eth they deposited. This call is in the POC as a comment, and if uncommented, will show that the user receives the correct amount of ETH back.

function testAttack() external {
        // Setup
        deposit(1 ether, address(this));
        vault.purchase(1 ether, address(1));
        skip(86500);
        uint256 wethBalLP = weth.balanceOf(address(vaultLp));
        uint256 wethBalBefore = weth.balanceOf(address(this));
        emit log_named_uint("wethBalLP before", wethBalLP);

        // Uncomment to check other variation
        // vault.updateFunding();

        // Deposit
        uint256 lpBalBefore = vaultLp.balanceOf(address(this));
        weth.approve(address(vaultLp), type(uint256).max);
        vaultLp.deposit(1 ether, address(this));
        uint256 lpMinted = vaultLp.balanceOf(address(this)) - lpBalBefore;
        emit log_named_uint("lpGained", lpMinted);

        // redeem
        vaultLp.redeem(lpMinted, address(this), address(this));
        uint256 wethAfter = weth.balanceOf(address(this));
        emit log_named_uint("wethBalBefor", wethBalBefore);
        emit log_named_uint("wethBalAfter", wethAfter);
    }

Output:

Running 1 test for tests/perp-vault/Unit.t.sol:Unit
[PASS] testAttack() (gas: 677624)
Logs:
  wethBalLP before: 1000000000000000000
  lpGained: 1000000000000000000
  wethBalBefor: 2998950000000000000000
  wethBalAfter: 2998974999999999999999

We see here that by depositing and immediately redeeming, the user can extract a part of the pending premium. If the user takes a flashloan and does the same with a large amount, they can extract a majority of the pending premium.

Tools Used

Foundry

Recommended Mitigation Steps

Update vault state before calculating shares. Call perpetualAtlanticVault.updateFunding(); At the very beginning, like in the redeem function.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The addToDelegate function is used by users to deposit ETH to be used by other users to bond with rdpx. The function withdraw does the opposite, and lets users receive their deposited eth back if not used. The amount of WETH deposited for delegated bonding is tracked by the variable totalWethDelegated. This is increased when users deposit WETH using addToDelegate.

    totalWethDelegated += _amount;

However, this value is not decremented in the withdraw function. Thus totalWethDelegated does not actually track the amount of available WETH for delegated bonding. This not only makes the public variable meaningless, but also breaks the sync() function since totalWethDelegated can be much larger than the actual WETH balance in the contract.

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

This bit in the sync function will revert. Also large amounts of WETH can be flashloaned, deposited and immediately withdrawn to unilaterally increase the totalWethDelegated value. With enough iterations, this can be used to make the totalWethDelegated value hit uint256.max which will break the addToDelegate function.

Proof of Concept

No POC is provided since it is evident from looking at the function that totalWethDelegated is not decremented.

function withdraw(
    uint256 delegateId
  ) external returns (uint256 amountWithdrawn) {
    _whenNotPaused();
    _validate(delegateId < delegates.length, 14);
    Delegate storage delegate = delegates[delegateId];
    _validate(delegate.owner == msg.sender, 9);

    amountWithdrawn = delegate.amount - delegate.activeCollateral;
    _validate(amountWithdrawn > 0, 15);
    delegate.amount = delegate.activeCollateral;

    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Tools Used

Manual Review

Recommended Mitigation Steps

Decrement totalWethDelegated in the withdraw function.

totalWethDelegated -= amountWithdrawn;

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The value stored in tokenAinfo.tokenAPrice stores the price of Rdpx in ETH. Thus it holds the ETH per Rdpx amount. This evident from the following calculation, where this is multiplied by the amount of Rdpx.

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

Here tokenAToRemove is the amount of Rdpx to remove, and tokenAInfo.tokenAPrice is the price of Rdpx in ETH. Thus the result is the amount of ETH to remove. This is used to set the minimum amount of eth expected from the liquidity removal.

Next these ETH tokens are swapped for Rdpx, and the minimum of amount of Rdpx tokens expected is calculated.

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

However, the issue here is that the amountB, which is the amount of ETH, is multiplied by the Rdpx price instead of dividing it.

tokenAprice stores ETH / Rdpx ratio. So to calculate the amount of Rdpx, we need to divide the price, amount Rdpx = amount ETH / (ETH / Rdpx)

Since the oracle price is multiplied instead of being divided, this give the wrong minimum amount. This will cause the contract to either expect too little, leading to slippage loss, or too much, leading to a revert. Thus depending on the price, it can lead to a DOS of the contract.

Proof of Concept

It is clear from the code, that both instances A and B cannot be correct. Both are multiplying the amount and the price, but in one case amount is in Rdpx, and the other is in Eth, andthe price in both cases is the same value. One must be a division.

// Instance A
uint256 mintokenBAmount = ((tokenAToRemove * tokenAInfo.tokenAPrice)


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

Tools Used

Manual Review

Recommended Mitigation Steps

Divide the amountB by the price, instead of multiplying it.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The _curveswap function takes an amount parameter, and then either converts dpxeth into eth or the other way around depending on the passed _ethToDpxEth bool value using curve LP.

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

For slippage, a minimum amount out is calculated in the above shown snippet if not passed into the function already. The oracle price is given by the function getDpxEthPrice, which reports the amount of ETH per dpxETH. Since it is ETH / dpxETH, it must be multiplied with the dpxETH amount. However in the above snippet we see that it i used wrongly.

If _ethToDpxEth is true, then ETH is being converted to dpxETH, and the passed _amount is the amount of ETH to be converted. But if _ethToDpxEth is true, the minimum is calculated as amount * getDpxEthPrice(). Since getDpxEthPrice() is ETH / dpxETH, we are calculating ETH * ETH / dpxETH. This is wrong. So the wrong value of minimum amount will be calculated.

The issue is that if the oracle shows a price higher than 1 ETH / dpxETH, then the eth-to-dpxeth minimum amount will be calculated to be higher than the input amount. This will lead to the swap always reverting.

More proof that getDpxEthPrice() is ETH / dpxETH is given in the function upperDepeg. Here, the upperdepeg condition is checked as follows.

_validate(getDpxEthPrice() > 1e8, 10);

This shows that the oracle price is ETH / dpxETH, and not dpxETH / ETH.

Proof of Concept

In the testUpperDepeg test function, we see that the price is set to more than 1e8. This shows that the oracle price is in ETH/dpxETH.

dpxEthPriceOracle.updateDpxEthPrice(103012950);

Tools Used

Manual Review

Recommended Mitigation Steps

Flip the ternary operator in _curveswap. Currently the wrong code path is taken for the wrong value of ethToDpxEth. Flipping the ternary statements will resolve the issue.

Assessed type

Math

Lines of code

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

Vulnerability details

Impact

The PerpetualAtlanticVault.sol:purchase function allows users to buy options after paying a premium. This is called from the core contract, and returns the premium value which will be deducted from the user. The price or the premium to be paid is calculated in the following snippet.

uint256 timeToExpiry = nextFundingPaymentTimestamp() - block.timestamp;

// Get total premium for all options being purchased
premium = calculatePremium(strike, amount, timeToExpiry, 0);

The timeToExpiry is calculated as the time difference between block.timestamp and the next funding epoch timestamp. The issue is that users can bond close to the epoch timestamp to have cheaper premiums due to lower time to expiry.

According to the docs here, the Atlantic Perpetual PUTS Options section states that the premium will be calculated according to the BlackScholes model.

The BlackScholes model for put options pricing is as follows:

Where T-t is the time to expiry. All options decay with time as can be seen from the exponential decay term, thus the premium of the option also goes down as the time to expiry decreases.

So if a user times their purchase close to the epoch timestamp, their timeToExpiry will be very low, which translates to a very cheap option price. These options dont expire, and can be held until settlement. So users can time their bonding to get cheaper options and thus cheaper bonding costs compared to users who dont time their bonding purchases.

Proof of Concept

A POC couldn't be developed due to the Mock contracts returning a fixed premium value and not implementing the BlackScholes model. However the snippet above clearly shows that time to expiry depends on block.timestamp, and the option premiums always decrease according to the Scholes model. Thus the issue is valid.

Tools Used

Manual Review

Recommended Mitigation Steps

Calculate timeToExpiry as difference between two epoch timestamps. That way every user will be charged the same premium provided the price and strike remain the same.

Assessed type

Timing

Lines of code

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

Vulnerability details

Impact

The function bondWithDelegate is used to bond rdpx tokens to WETH tokens provided by delegators. The function takes an array of amounts and delegation ids, and loops over the arrays and takes the amount form each delegate id sent.

for (uint256 i = 0; i < _amounts.length; i++) {
    // Validate amount
    _validate(_amounts[i] > 0, 4);

The only validation done is that each amount is non-zero. When a user bonds this way, part of the bond is staked on the user's account, and the rest is staked on the delegator's account. The user can choose to set the value of amount[] as low as possible, and the transaction will go through as long as the amount is non zero.

So if a malicious user takes a bunch of delegations and match small amounts for each of them, the delegators will get small amount of the bonds. These amounts can be so small that it isnt worth it for the delegators to redeem them on expiry due to gas costs. The attacker can also choose to match the same delegation multiple times in small amounts, and this also give rise to the same scenario.

The attacker gets an inherent advantage since their actions are batched, but the delegators have to redeem their bonds one at a time, and so will have to pay far higher gas costs. This can be used to grief the delegators.

Proof of Concept

This issue arises from there being no minimum amount to match delegations. This can be seen in the following snippet.

 for (uint256 i = 0; i < _amounts.length; i++) {
      // Validate amount
      _validate(_amounts[i] > 0, 4);

Tools Used

Manual Review

Recommended Mitigation Steps

Specify a minimum _amounts[i] value so that the delegators are not griefed by gas costs.

Assessed type

Other