Skip to content

Latest commit

 

History

History
682 lines (511 loc) · 24.1 KB

2023-08-Dopex.md

File metadata and controls

682 lines (511 loc) · 24.1 KB

Findings Summary

ID Description Severity
H-01 Settle can be blocked from WETH collateral token owner High
M-01 minAmountOut in RdpxV2Core::curveSwap is prone to big slippage Medium
L-01 User can inflate balance of reserve contract in order to get bond with bigger discount Low
L-02 Malicious delegate bond consumer can lock delegate bond owner’s WETH indefinitely Low
L-03 Assets aren’t being removed properly Low

[H-01] Settle can be blocked from ‘WETH` collateral token owner

Impact

When dpxETH price deppegs from the price of Eth settle can be called by the protocol admin. It will remove passed options/positions and this will bring back the backing of the 2 tokens. If user knows that his option is going to be passed as an argument to the settle function he can send only 1 wei to the vaultLp collateral token (WETH, confirmed by the sponsors) which will make PerpetualAtlanticVaultLp.substractLoss revert, and avoid getting slashed.

Proof of Concept

Admin going to call settle passing option owned by Bob. Bob knows and immediately transfers 1 wei to the vaultLp ’s collateral which is WETH.

contracts/perp-vault/PerpetualAtlanticVault.sol

function settle(
    uint256[] memory optionIds
  )
    external
    nonReentrant
    onlyRole(RDPXV2CORE_ROLE)
    returns (uint256 ethAmount, uint256 rdpxAmount)
  {
    _whenNotPaused();
    _isEligibleSender();

    updateFunding();

    for (uint256 i = 0; i < optionIds.length; i++) {
      uint256 strike = optionPositions[optionIds[i]].strike;
      uint256 amount = optionPositions[optionIds[i]].amount;

      // check if strike is ITM
      _validate(strike >= getUnderlyingPrice(), 7);

      ethAmount += (amount * strike) / 1e8;
      rdpxAmount += amount;
      optionsPerStrike[strike] -= amount;
      totalActiveOptions -= amount;

      // Burn option tokens from user
      _burn(optionIds[i]);

      optionPositions[optionIds[i]].strike = 0;
    }

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

//@audit problem will occur on this line since substractLoss uses strict equality which will be broken by directly transfering tokens to the underlying contract
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).subtractLoss(
      ethAmount
    );
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP)
      .unlockLiquidity(ethAmount);
    IPerpetualAtlanticVaultLP(addresses.perpetualAtlanticVaultLP).addRdpx(
      rdpxAmount
    );

    emit Settle(ethAmount, rdpxAmount, optionIds);
  }
contracts/perp-vault/PerpetualAtlanticVaultLP.sol

function subtractLoss(uint256 loss) public onlyPerpVault {
    //@audit require will revert because collateral.balanceOf(address(this)) will have 1 wei more
		require(
      collateral.balanceOf(address(this)) == _totalCollateral - loss,
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

Poc

function testAuditSettleCantBeCalledIfUserSends1WeiToCollateral() 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;

        priceOracle.updateRdpxPrice(0.2 gwei); // initial price * 10
        uint256[] memory strikes = new uint256[](1);
        strikes[0] = 0.015 gwei;

        priceOracle.updateRdpxPrice(0.01 gwei);
				
//@audit send 1 wei to break accounting
        weth.transfer(address(vaultLp), 1 wei);
        vm.expectRevert();
        vault.settle(ids);
    }

Tools Used

Manual

Recommended Mitigation Steps

contracts/perp-vault/PerpetualAtlanticVaultLP.sol

function subtractLoss(uint256 loss) public onlyPerpVault {
    //@audit require will revert because collateral.balanceOf(address(this)) will have 1 wei more
		require(
-     collateral.balanceOf(address(this)) == _totalCollateral - loss,
+     collateral.balanceOf(address(this)) >= _totalCollateral - loss,  
      "Not enough collateral was sent out"
    );
    _totalCollateral -= loss;
  }

[M-01] minAmountOut in RdpxV2Core._curveSwap is prone to big slippage

Impact

Big slippage caused by wrong decimals calculation when using deppeg functions of the RdpxV2Core contract.

Proof of Concept

This function is being called every time RdpxV2Core admin wants to bring the deppeg of the dpxETH/ETH tokens back by minting and burning tokens based on the current case.

Even though protocol will be deployed on Arbitrum this vulnerability can still be exploited.

We assume _amount is in 1e18 decimals but the slippage will be the same no matter the decimals because the same decimals will be added to both sides of the equation.

//@audit    left: 18 + 8 - 8 = 18  right: 18 + 8 + 5 = 31 - 16 = 15
 uint256 minOut = _ethToDpxEth
            ? (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
            : (((_amount * getEthPrice()) / 1e8) - (((_amount * getEthPrice()) * slippageTolerance) / 1e16));

On the left side of the equation we will receive value with 18 decimals, while on the right side there will be 15 decimals. And now when we have let's say 5e18 - 2e15, result will be 4.99e18 instead of 3e18.

Tools Used

Manual

Recommended Mitigation Steps

Modify the code by changing the number by which the whole amount is divided:

 // calculate minimum amount out
        uint256 minOut = _ethToDpxEth
-            ? (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e16))
-            : (((_amount * getEthPrice()) / 1e8) - (((_amount * getEthPrice()) * slippageTolerance) / 1e16));
+            ? (((_amount * getDpxEthPrice()) / 1e8) - (((_amount * getDpxEthPrice()) * slippageTolerance) / 1e13))
+            : (((_amount * getEthPrice()) / 1e8) - (((_amount * getEthPrice()) * slippageTolerance) / 1e13));

[L-01] User can inflate balance of reserve contract in order to get bond with bigger discount

Impact

When normal and delegate bonding is being used, there is a discount to the WETH and rDPX tokens, which is calculated by the formula: bondDiscountFactor * sqrt(rdpx2TreasuryReserves).

User who wants to receive a bigger discount can simply transfer the precalculated amount of rDPX tokens to the address of the RdpxReserve, and inflate the balance. Execute bond or bondWithDelegate passing valid arguments so the discount calculated passes the validation: _validate(bondDiscount < 100e8, 14); Then at the _transfer function discount is sent back to the user, and the other part of the provided collateral is split amongst the other parts of the system.

Arbitrum makes this vulnerability even easier for the user because of its FIFO transactions.

Proof of Concept

Let’s look at how the discount is calculated in the code:

contracts/core/RdpxV2Core.sol

//@audit IRdpxReserve(addresses.rdpxReserve).rdpxReserve() is easy to be manipulated
uint256 bondDiscount = (
                bondDiscountFactor * Math.sqrt(IRdpxReserve(addresses.rdpxReserve).rdpxReserve()) * 1e2
            ) / (Math.sqrt(1e18)); // 1e8 precision
contracts/reserve/RdpxReserve.sol

function rdpxReserve() external view returns (uint256) {
    return IERC20WithBurn(rdpx).balanceOf(address(this));
  }
tests/rdpxV2Core/Unit.t.sol

function testBond() public {
uint256 userRdpxBalance = rdpx.balanceOf(address(this));
uint256 userwethBalance = weth.balanceOf(address(this));
(uint256 rdpxRequired, uint256 wethRequired) = rdpxV2Core.calculateBondCost(1 * 1e18, 0);
console.log("rdpxRequired", rdpxRequired);
console.log("wethRequired", wethRequired);

rdpx.transfer(address(rdpxReserveContract), 800 * 1e18);
uint256 userRdpxBalance2 = rdpx.balanceOf(address(this));
(uint256 rdpxRequired2, uint256 wethRequired2) = rdpxV2Core.calculateBondCost(1 * 1e18, 0);
console.log("rdpxRequired", rdpxRequired2);
console.log("wethRequired", wethRequired2);
}

Results from the test provided as POC:

Results from the test provided as POC:

rdpxRequired: 1225000000000000000

wethRequired: 806250000000000000

rdpxRequired: 998753109500000000

wethRequired: 749688277375000000

Amounts can be up to 49.5% off from the both collaterals.

Tools Used

Manual

Recommended Mitigation Steps

Consider storing the balance of the RdpxReserve contract as state variable and update it appropriately after bond related functions are executed.

[L-02] Malicious delegate bond consumer can lock delegate bond owner’s WETH indefinitely

Impact

Delegate bonds are used by people who have one of the 2 tokens needed in order to bond in the protocol. WETH token holders will call addToDelegate function by providing the amount and fees to lock in the bond. This will open a Delegate Position which can be used by rDPX token holders. Vulnerability happens when WETH owner wants to call RdpxV2Core.withdraw ,but delegatee has seen this opportunity earlier and executed bondWithDelegate function making the validation inside the function pass with wethRequired to be equal to the free collateral at the position.

_validate(delegate.amount - delegate.activeCollateral >= wethRequired, 5);

The cost of this vulnerability increases exponentially relative to the amount provided by the bond owner. This is not a problem since the delegatee can redeem his rDPX at any time after the bond has reached his maturity. On the other hand, delegates can call withdraw and receive all his WETH only if there are no delegate bonds made to them and their activeCollateral is equal to 0.

Proof of Concept

Delegate wants to open new position by calling addToDelegate:

contracts/core/RdpxV2Core.sol
/**
   * @notice Lets users delegate WETH
   * @param  _amount The amount of WETH to delegate
   * @param  _fee The fee to charge for the delegated WETH
   * @return uint256 the ID of the delegate
   **/
  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);
  }

Malicious user sees this newly created position and call bondWithDelegate passing arguments so wethRequired to be equal to the difference between delegate.amount and delegate.activeCollateral resulting in their substraction being 0:

contracts/core/RdpxV2Core.sol

function _bondWithDelegate(
    uint256 _amount,
    uint256 rdpxBondId,
    uint256 delegateId
  ) internal returns (BondWithDelegateReturnValue memory returnValues) {
    // Compute the bond cost
    (uint256 rdpxRequired, uint256 wethRequired) = calculateBondCost(
      _amount,
      rdpxBondId
    );

    // update ETH token reserve
    reserveAsset[reservesIndex["WETH"]].tokenBalance += wethRequired;

    Delegate storage delegate = delegates[delegateId];

    // update delegate active collateral
		//@audit substraction will result in equality
    _validate(delegate.amount - delegate.activeCollateral >= wethRequired, 5);
    delegate.activeCollateral += wethRequired; //@audit this will be eqaul to the total amount locked by delegate

    // update total weth delegated
    totalWethDelegated -= wethRequired;

    // Calculate the amount of bond token to mint for the delegate and user based on the fee
    (uint256 amount1, uint256 amount2) = _calculateAmounts(
      wethRequired,
      rdpxRequired,
      _amount,
      delegate.fee
    );

    // update user amounts
    // ETH token amount remaining after LP for the user
    uint256 bondAmountForUser = amount1;

    // Mint bond token for delegate
    // ETH token amount remaining after LP for the delegate
    uint256 delegateReceiptTokenAmount = _stake(delegate.owner, amount2);

    returnValues = BondWithDelegateReturnValue(
      delegateReceiptTokenAmount,
      bondAmountForUser,
      rdpxRequired,
      wethRequired
    );
  }

So when owner needs his WETH he won’t be able to call withdraw :

contracts/core/RdpxV2Core.sol

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;
				//@audit amountWithdrawn will be equal to 0, resulting in revert
        _validate(amountWithdrawn > 0, 15);
        delegate.amount = delegate.activeCollateral;

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

        emit LogDelegateWithdraw(delegateId, amountWithdrawn);
    }

Function will be reverting since amountWithdraw will be equal to 0. amountWithdrawn = delegate.amount - delegate.activeCollateral;

Tools Used

Manual

Recommended Mitigation Steps

It is challenging to conclude a particular fix because there are many aspects that have to be considered, but the most important thing is how to incentivise users to bond without the risk of getting their collateral locked.

[L-03] Assets aren’t being removed properly

Impact

  • Removing an asset not removing the right one from the assetTokens array.
  • Indexes are not handled correctly when removing an asset. If the removed asset was the last one in the assetTokens array, its index will remain instead of being set to 0. More information on the impact is given at the end of the PoC section, as it is easier to explain with tokens.

Proof of Concept

#1

AssetTokens array is not consistent and does not store the correct available tokens.

The test provided shows this. The setUp method in the test adds RDPX, WETH, DPXETH. When remove RDPX, it will remove DPXETH from the assetTokens array, because it is the last one.

Note: It will always remove the last element of assetTokens, not the specified one.

Note2: Use caps for the tokens in all the examples

function testRemoveFromReserveToken() public {
	  console.log("-----------Before-----------");
	  console.log("reserveAsset array length ", rdpxV2Core.getReserveAssetLength());
	  console.log("reserveToken array length ", rdpxV2Core.getReserveTokenLength());
	
	  console.log("reserveAsset array");
	  (, , string memory symbolToken0) = rdpxV2Core.reserveAsset(0);
	  (, , string memory symbolToken1) = rdpxV2Core.reserveAsset(1);
	  (, , string memory symbolToken2) = rdpxV2Core.reserveAsset(2);
	  (, , string memory symbolToken3) = rdpxV2Core.reserveAsset(3);
	  console.log(symbolToken0, symbolToken1, symbolToken2, symbolToken3);
	
	  console.log("reserveToken array: ", rdpxV2Core.reserveTokens(0), rdpxV2Core.reserveTokens(1), rdpxV2Core.reserveTokens(2));
	
	  console.log("reservesIndex mapping");
	  console.log("RDPX - ", rdpxV2Core.reservesIndex("RDPX"));
	  console.log("WETH - ", rdpxV2Core.reservesIndex("WETH"));
	  console.log("DPXETH - ", rdpxV2Core.reservesIndex("DPXETH"));
	
	  console.log();
	  console.log("Remove RDPX from the asset reserve.");
	  console.log();
	  rdpxV2Core.removeAssetFromtokenReserves("RDPX");
	
	  console.log("-----------After-----------");
	  console.log("reserveAsset array length ", rdpxV2Core.getReserveAssetLength());
	  console.log("reserveToken array length ", rdpxV2Core.getReserveTokenLength());
	
	  console.log("reserveAsset array");
	  (, , string memory symbolToken0After) = rdpxV2Core.reserveAsset(0);
	  (, , string memory symbolToken1After) = rdpxV2Core.reserveAsset(1);
	  (, , string memory symbolToken2After) = rdpxV2Core.reserveAsset(2);
	  console.log(symbolToken0After, symbolToken1After, symbolToken2After);
	
	  console.log("reserveToken array: ", rdpxV2Core.reserveTokens(0), rdpxV2Core.reserveTokens(1));
	
	  console.log("reservesIndex mapping");
	  console.log("RDPX - ", rdpxV2Core.reservesIndex("RDPX"));
	  console.log("WETH - ", rdpxV2Core.reservesIndex("WETH"));
	  console.log("DPXETH - ", rdpxV2Core.reservesIndex("DPXETH"));
	
	  console.log();
	  console.log("Remove DPXETH from the asset reserve.");
	  console.log();
	  rdpxV2Core.removeAssetFromtokenReserves("DPXETH");
	
	  console.log("-----------After2-----------");
	  console.log("reserveAsset array length ", rdpxV2Core.getReserveAssetLength());
	  console.log("reserveToken array length ", rdpxV2Core.getReserveTokenLength());
	
	  console.log("reserveAsset array");
	  (, , string memory symbolToken0After2) = rdpxV2Core.reserveAsset(0);
	  (, , string memory symbolToken1After2) = rdpxV2Core.reserveAsset(1);
	  console.log(symbolToken0After2, symbolToken1After2);
	
	  console.log("reserveToken array: ", rdpxV2Core.reserveTokens(0));
	
	  console.log("reservesIndex mapping");
	  console.log("RDPX - ", rdpxV2Core.reservesIndex("RDPX"));
	  console.log("WETH - ", rdpxV2Core.reservesIndex("WETH"));
	  console.log("DPXETH - ", rdpxV2Core.reservesIndex("DPXETH"));
}

The output:

Logs:
-----------Before-----------
reserveAsset array length  4
reserveToken array length  3
reserveAsset array
ZERO RDPX WETH DPXETH
reserveToken array:  RDPX WETH DPXETH
reservesIndex mapping
RDPX -  1
WETH -  2
DPXETH -  3

Remove RDPX from the asset reserve.

-----------After-----------
reserveAsset array length  3
reserveToken array length  2
reserveAsset array
ZERO DPXETH WETH
reserveToken array:  RDPX WETH
reservesIndex mapping
RDPX -  0
WETH -  2
DPXETH -  1

Remove DPXETH from the asset reserve.

-----------After2-----------
reserveAsset array length  2
reserveToken array length  1
reserveAsset array
ZERO WETH
reserveToken array:  RDPX
reservesIndex mapping
RDPX -  0
WETH -  1
DPXETH -  0

#2

If the admin removes and then wants to add another asset, the assetTokens array will not store the correct tokens.

Let's take the previous code as a starting point and extend it.

After we removed RDPX and DPXETH, the assetTokens array will store RDPX as it was pushed first and will not be removed when removeAssetFromtokenReserves() is called as I explained in the previous example. But if we add RDPX again, the assetTokens array will have RDPX twice, instead of WETH and RDPX.

// the previous unit test code from above
...

console.log();
console.log("Add RDPX to the asset reserve.");
console.log();
rdpxV2Core.addAssetTotokenReserves(address(rdpx), "RDPX");

console.log("-----------After Add(RDPX)-----------");
console.log("reserveAsset array length ", rdpxV2Core.getReserveAssetLength());
console.log("reserveToken array length ", rdpxV2Core.getReserveTokenLength());

console.log("reserveAsset array");
(, , string memory symbolToken0AfterAdd) = rdpxV2Core.reserveAsset(0);
(, , string memory symbolToken1AfterAdd) = rdpxV2Core.reserveAsset(1);
(, , string memory symbolToken2AfterAdd) = rdpxV2Core.reserveAsset(2);
console.log(symbolToken0AfterAdd, symbolToken1AfterAdd, symbolToken2AfterAdd);

console.log("reserveToken array: ", rdpxV2Core.reserveTokens(0), rdpxV2Core.reserveTokens(1));

console.log("reservesIndex mapping");
console.log("RDPX - ", rdpxV2Core.reservesIndex("RDPX"));
console.log("WETH - ", rdpxV2Core.reservesIndex("WETH"));
console.log("DPXETH - ", rdpxV2Core.reservesIndex("DPXETH"));

Extended output:

Add RDPX to the asset reserve.

-----------After Add(RDPX)-----------
reserveAsset array length  3
reserveToken array length  2
reserveAsset array
ZERO WETH RDPX
reserveToken array:  RDPX RDPX
reservesIndex mapping
RDPX -  2
WETH -  1
DPXETH -  0

#3

If the order of the necessary tokens is not the same as in tests, upon removal it will break the reservesIndex mapping.

Consider this order of adding - WETH, RDPX, DPXETH. Now run the same test from example #1. When removing RDPX, the mapping is fine, but if also remove DPXETH, RDPX in the mapping will be set to 2, which is wrong because we have already removed it.

The output will be this:

Logs:
-----------Before-----------
reserveAsset array length  4
reserveToken array length  3
reserveAsset array
ZERO WETH RDPX DPXETH
reserveToken array:  WETH RDPX DPXETH
reservesIndex mapping
RDPX -  2
WETH -  1
DPXETH -  3

Remove RDPX from the asset reserve.

-----------After-----------
reserveAsset array length  3
reserveToken array length  2
reserveAsset array
ZERO WETH DPXETH
reserveToken array:  WETH RDPX
reservesIndex mapping
RDPX -  0
WETH -  1
DPXETH -  2

Remove DPXETH from the asset reserve.

-----------After2-----------
reserveAsset array length  2
reserveToken array length  1
reserveAsset array
ZERO WETH
reserveToken array:  WETH
reservesIndex mapping
RDPX -  2
WETH -  1
DPXETH -  0

And if he adds DPXETH back into the mapping, RDPX and DPXETH will point to index 2.

Add DPXETH to the asset reserve.

-----------AfterAdd-----------
reserveAsset array length  3
reserveToken array length  2
reserveAsset array
ZERO WETH DPXETH
reserveToken array:  WETH DPXETH
reservesIndex mapping
RDPX -  2
WETH -  1
DPXETH -  2

This can lead to a loss of funds because every time rDPX is used in the code, it will use a dpxETH token address and more importantly the token balance since rDPX is removed. It probably won't happen often, but if the admin needs to change the assets, due to changing the token address, etc., the ongoing manipulations of the arrays will break their consistency. Besides the asset token balances not being correct, users will have to wait for the admin to add the necessary assets so that the functions work again and don't revert due to missing token addresses.

If the following scenario occurs and both assets point to index 2. If at this point a user call bond() (an existing one), the function will calculate the required rDPX and WETH, transfer the WETH to the contract, update the WETH balance, apply the premium, and then when execute _transfer() inside, the decaying bond amount will be decreased, the rDPX amount will be withdraw, but when it reaches the RDPX token balance update, it won't update it, but actually will update the DPXETH token balance which will break all calculations.

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

Tools Used

Manual, Foundry

Recommended Mitigation Steps

function removeAssetFromtokenReserves(
  string memory _assetSymbol
) external onlyRole(DEFAULT_ADMIN_ROLE) {
  uint256 index = reservesIndex[_assetSymbol];
  _validate(index != 0, 18);

  // remove the asset from the mapping
  reservesIndex[_assetSymbol] = 0;

  // @audit check this seem it make the pre-last element with other index
  // add new index for the last element
- reservesIndex[reserveTokens[reserveTokens.length - 1]] = index;
+	if (reservesIndex[reserveTokens[reserveTokens.length - 1]] != 0) {
+   reservesIndex[reserveTokens[reserveTokens.length - 1]] = index;
+ }

  // update the index of reserveAsset with the last element
  reserveAsset[index] = reserveAsset[reserveAsset.length - 1];

+	reserveTokens[index - 1] = reserveTokens[reserveTokens.length - 1];

  // remove the last element
  reserveAsset.pop();
  reserveTokens.pop();

  emit LogAssetRemovedFromtokenReserves(_assetSymbol, index);
}