Synthetics Perpetuals Exchange

Whenever a user deposits into the MarketVault, the lastDepositedTimestamp[receiver] is updated to the current block's timestamp. This is then used to ensure depositors have been in the vault for a MINIMUM_DEPOSIT_PERIOD when withdrawing or redeeming.

The problem is that a malicious user can deposit 1 wei of assets with another user as the receiver, updating the receiver's lastDepositedTimestamp. This can be used to prevent users from ever exiting their LP, leading to loss of funds.

Do not allow users to deposit for arbitrary receivers.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/edb9c78ac7b1a822e837d20d3b

0783dc2146c67a

The fee distribution interval early return logic is reversed such that fees can only be distributed inside of the DELAY window. After the DELAY window has passed fees can no longer be distributed.

// Distribute fees at regular intervals of every 1 hour if (lastTransferTimestamp + DELAY < block.timestamp) return;

Replace the interval early return logic with: lastTransferTimestamp + DELAY > block.timestamp.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/9f0bc8948ccb6719879a007b7a

a9bbdab1f26653

An arbitrary positionId can be provided to the modifyPosition function without validation that the msg.sender is the owner. As a result, users may modify or close any arbitrary position, even if it doesn’t belong to them.

Validate that the msg.sender is the owner of the supplied positionId.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/93aac21b1a16414effd9160f738

931d916b96cad

In the _decreasePosition function users can decrease their position size without realizing any of the positive or negative PnL for their position. This way a user can decrease their size to a trivial amount if they do not immediately start to profit.

When users decrease their position size account for a proportional amount of their current PnL being realized.

PariFi Team: Fixed https://github.com/Parifi/parifi-contracts-internal/pull/66

When the position's PnL is obtained in the liquidate function with (uint256 pnlInCollateral,) = getProfitOrLossInCollateral(_positionId, executionPrice) The protocol assumes that the pnlInCollateral is always a loss, uint256 lossInCollateral = pnlInCollateral + feesInCollateral.

As a result, a position with a large profit will be treated as if it is in a large loss and can be liquidated.

Require that the isProfit returned from the getProfitOrLossInCollateral function is false.

PariFi Team: Fixed https://github.com/Parifi/parifi-contracts-internal/pull/66

When the pnlInCollateral is greater in magnitude than the collateralAmount and isProfit is false the entire pnlInCollateral amount is transferred to the feeManager and distributed to the protocol and the LPers.

This steals the delta (|pnlInCollateral| - collateralAmount) from the collateral of other positions in the OrderManager, otherwise if that collateral amount isn't in the OrderManager contract from other positions the tx will simply revert with a balance underflow.

In the event where the pnlInCollateral is greater than the available collateral and isProfit is false, only send the available collateral from the position to the feeManager.

PariFi Team: Fixed https://github.com/Parifi/parifi-contracts-internal/pull/66

In the calculateLeverage function the executionPrice is hardcoded to use the OPEN_NEW_POSITION order type, however the calculateLeverage function is used for other order types such as DECREASE_POSITION where the resulting executionPrice ought to be using the less favorable price for decreases.

Allow the calculateLeverage function to take in an orderType and supply the appropriate orderType when validating leverage for each orderType.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/0ac134531f450367cefcfc07a20

57a1f38478464

A potential problem arises when price is updated according to the market's deviationPoints. The update only considers whether the user is long or short, if the user opens a long they receive a superior execution.

If the user closes a long position, the user receives a less favorable execution. The liquidity curve is intended to imitate widening or narrowing market spreads. If spreads are tight, execution should be favorable both when buying and selling. However, that is not the behavior displayed.

Take into consideration whether the orderType being executed is an increase or decrease orderType when adjusting the updatedPrice by the deviationPoints.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/commit/5831ec02198999cedc60727b6310f8505

d590664

The _deductFeesFromPosition function reverts if the remaining position collateral after deducting fees is less than the configured minimum collateral for the market.

However this _deductFeesFromPosition function is called during _closePosition execution, therefore positions that have accumulated enough fees to be put under the minimum collateral amount cannot be closed.

Do not validate the minimum collateral amount in the _deductFeesFromPosition when closing a position.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/c664351fe551b7afeda89b0825

9d7e2037409b65

The addNewMarket function allows a market to be added with the specified _marketId, however the _newMarket.marketId is not validated to be the same as the provided _marketId.

Additionally, the Market struct should not store a marketId as it is unnecessary and can always be accessed from an order or position object.

Remove the marketId attribute on the Market struct as it is unnecessary and leads to misconfiguration.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/commit/cd8522381bd390e66c2d13b2085f18867

4e41ea6

The updatedAvgPrice computed in the _increasePosition function is always rounded down, therefore it is possible for users with long positions to increase their position at an executionPrice that is higher than their average price and see no change in their position’s average price.

For example, an increase order with the following characteristics will not change the average price of the position.

• positionSize = 1e18
• avgPrice = 100e8
• orderSize = 1e10
• executionPrice = 101e8

Therefore, the updatedAvgPrice = (1e18 * 100e8 + 1e10 * 101e8) / (1e18 + 1e10) = 100e8.

For some tokens this rounding can yield opportunities for traders to manipulate their average price and make risk free profits.

Use roundUp division when calculating the updatedAvgPrice for longs and round down division when calculating the updatedAvgPrice for shorts.

PariFi Team: https://github.com/Parifi/parifi-contracts-internal/pull/58/commits/604f9bc35eb2943c858ddb61e7

5155ddffa2e6d1

Although the OrderManager validates that the position does not increase the open interest past the maximumOi, LPers are still exposed to great risk as there is no validation on the size of the position relative to the funds available in the market.

With a large enough order which is still within OI bounds, a user in profit can drain the entirety of the MarketVault’s reserves and leave LPer’s insolvent. Furthermore, any traders in profit would not be able to claim their profit. This would break a core function of the protocol.

Add validation checks to ensure the position size may be only up to a specific percentage of funds available in the market.

PariFi Team: We use fractional reserves and use maximumOi to keep the risks in check.

Whenever the Aggregate vaultOpenInterest is increased or decreased, it is done so at the current value of the index token:

• User opens a position with size 10 ETH @ $2,000 / ETH
• vaultOpenInterest is increased from 0 to $20,000
• The user then closes their position after ETH drops to $1,000 / ETH
• vaultOpenInterest is decreased from $20,000 → $10,000
• $10,000 vaultOpenInterest remains even though there are no open positions

This inaccuracy leads to cases where the vaultOpenInterest is massively inflated or massively reduced depending on whether user’s close their positions at a higher or lower index token price than when they opened.

Over the lifecycle of the vault this validation will become entirely detached from the actual open interest of position’s using the vault as backing, perturbing the purpose of the validation and preventing actions from occurring when the vaultOpenInterest is inflated.

Use the open interest that is tracked on a per market basis and value the open interest of each market in aggregate based on the current price of the each index token during validation.

This solution requires an O(n) approach where n is the number of markets configured for a single vault. This ought to be fine as long as the number of markets for a single vault is limited to a reasonable amount and the aggregate vaultOpenInterest is computed once per transaction.

PariFi Team: We removed the open interest validation check, and will stick to the original solution of using maximumOi to control the open interest for markets.

The gasSent is validated to be greater than 64/63 * the transaction.minGas, however the gasSent is recorded at the beginning of the execute function which has a non-trivial amount of logic, that will expend gas, before executing the external call.

Therefore the remaining gas that is forwarded to the external call may be less than the transaction.minGas.

Add a buffer to the gasSent validation that comfortably covers any expenditure that would occur before the external call.

PariFi Team: Agreed to explain in documentation about minGas being the gas needed for the entire execute function.

The admin can configure any maxVaultOpenInterest value with the setMaximumVaultOpenInterest function. This poses a risk as the admin may accidentally configure a maxVaultOpenInterest that is below the current vaultOpenInterest.

Consider the following scenario:

• maxVaultOpenInterest is 15
• vaultOpenInterest is 10
• maxVaultOpenInterest is set to 5
• A liquidation that would reduce the vaultOpenInterest by 3 cannot execute as it would fail the maxVaultOpenInterest validation.

Take care when using the setMaximumVaultOpenInterest function. Consider adding validation such that the maxVaultOpenInterest cannot be set below the current vaultOpenInterest.

PariFi Team: We removed the open interest validation check, and will stick to the original solution of using maximumOi to control the open interest for markets.

Limit orders are traditionally defined to execute at or better than the limit price. However, the current limit price check disallows execution when the market price is equivalent to the expected price. This may lead to unexpectedly failed entries for users who expected their order to be executed once price reached their limit price.

if (userOrder.isLimitOrder) { if ( (userOrder.triggerAbove && executionPrice <= expectedPrice) || (!userOrder.triggerAbove && executionPrice >= expectedPrice) ) { revert LibError.PriceMismatch(executionPrice, expectedPrice); } }

Allow limit orders to execute when they are at or better than the configured trigger price.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/1f65980275eb081c4132ab7625

e6e6cf0433acc9

For slippage calculations, _settleOrder calculates a percentage above the expectedPrice and a percentage below the expectedPrice.

However, slippage should not apply to both the upper and lower prices. Traditionally, when a user submits a long, slippage is measured against the best offer price. If a user submits a short, slippage is measured against the best bid price. PariFi is comparing against both a lowerLimit and upperLimit regardless of position direction.

A user who is long would like to buy the asset at a price lower than the lowerLimit. A user who is short would like to sell the asset at a price higher than the upperLimit.

Only compare the price against the upperLimit for longs and the lowerLimit for shorts.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/4a536c84448de1dea1327ce892

f1b93863c4e975

Upon creating a new position the maxLeverage is not validated. The maxLeverage validation will occur when you make a new order, however fees, price, and the maxLeverage itself may change in between the time you create your order and when it gets executed.

Therefore it is possible to bypass the configured maxLeverage for a market.

Validate the maxLeverage after the order has been executed.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/c36999eaa38b9712243799cd4e

68abbc3df66805

The addNewMarket function lacks several key validations on new markets being added such as validating the ranges for:

• openingFee
• closingFee
• liquidationFee
• minCollateral
• liquidationThreshold

Add validation on the configured values for each of the above.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/commit/d508fc32430f00040161e8910f22a64ca3

84502f

The updateExistingMarket function lacks several key validations on new markets being updated such as validating the ranges for:

• openingFee
• closingFee
• liquidationFee
• minCollateral
• liquidationThreshold

Add validation on the configured values for each of the above.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/commit/d508fc32430f00040161e8910f22a64ca3

84502f

The _withdrawalFee is not validated to be within the WITHDRAWAL_FEE_CAP in the MarketVault constructor.

Validate that the _withdrawalFee is within the WITHDRAWAL_FEE_CAP in the MarketVault constructor.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/bff4ead032203b5b39b987e58f8

888f63ea75f38

priceFeed.updatePythPrice(priceUpdateData) is not used to update the pyth price before cancelPendingOrder or createNewPosition.

To be entirely accurate prices should be updated before computing the fees in these functions using those pyth prices.

Update Pyth prices with priceFeed.updatePythPrice(priceUpdateData) for entirely up to date pricing when computing fees in the cancelPendingOrder and createNewPosition functions.

PariFi Team: This is by design that we use the TWAP EMA price for non-critical purposes to prevent pushing prices when the action is initiated from the user.

Currently there is no remuneration for the keeper executing orders with the settleOrder function. It may be prudent to cover the gas costs for the keeper by introducing an executionFee that must be sent in when creating an order to cover the keeper’s gas expenditure.

Such an executionFee would dissuade from potential keeper griefing as currently there are no fees charged upon cancelling decrease orders with the cancelPendingOrder function. Malicious actors could see the keeper submit a settleOrder transaction and front-run it to cancel their order and avoid paying any fees. Though such an attack is dubious at best on the Arbitrum network.

Consider implementing an executionFee to remunerate the keeper’s gas expenditure when settling orders. Additionally consider charging a percentage of this executionFee upon the cancellation of an order, especially decrease orders as they currently have no fees applied on cancellation.

PariFi Team: We are discussing internally to have a mechanism to have the execution fee as a configurable value so that we can subsidize the execution fee for a certain promotion period initially and later turn it on

In the updateExistingMarket function the admin may update the market and provide an _updatedMarket with a non-paused status e.g. isLive = true, however this would avoid the dataFabric.unpauseMarket call as the market has been toggled to a live status without calling the toggleMarketStatus function.

Either require that markets are created with an isLive of false or invoke the dataFabric.unpauseMarket function in the updateExistingMarket when the _updatedMarket.isLive == true.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/commit/7897962fdbcbf47bb69c464d1c0360e43c

956469#diff-c53abbb6be77f3b58b0c08e20fe48e64998cbc34900f06d64155a7b5345730c2R500

A position's leverage is calculated using an EMA price, however this is a lagging indicator and will not be entirely accurate to current prices.

Therefore users will be able to open positions that, at current prices, would be above the maxLeverage. However by the EMA are not above maxLeverage.

Consider if this is the desired behavior, if not, use current prices to calculate the position’s leverage.

PariFi Team: This is by design that we use the TWAP EMA price for non-critical purposes to prevent pushing prices when the action is initiated from the user.

The max leverage check relies on the index token price relative to the collateral token price, if the index token price moves relative to the collateral token price, a position’s leverage changes. This means the leverage of user’s position can change even if the price of their collateral token does not move and they don’t make any orders.

The leverage of a position will change with the price of the index token. However this is unexpected behavior as often the leverage of a perpetual position is a ratio of the collateral deposited and the initial size of the position, which does not change with the index price.

Determine if this ought to be the expected behavior and if so ensure it is well documented for users.

The fees on lines 524 and 651 are calculated based on an EMA price which is a lagging indicator and will not be entirely accurate to the current token price.

Consider if this is desired. If not, use the convertMarketToToken function to compute the value of these fees.

In the _closePosition function the position is deleted at the end of the function call, however for tokens with callbacks it would be safer to delete the position and update the market data before the transfer, it may also be wise to delete the position before the distributeFees external call as well, even though this is supposedly a trusted external call.

Ensure state is modified before initiating token transfers to protect against potential read-only reentrancies with ERC-777 tokens.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/24be4a247f94cc8844692ea0a5

d830074b757704

When executing a limit order the expectedPrice is validated for the first time, however this validation should be carried out when the order is first created. Otherwise user’s may submit many orders that will always fail this validation in order to waste the keeper’s gas upon execution of the settleOrder function.

Validate the expectedPrice for the limitOrder when first creating the order.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/62/commits/c8300e718976eb3f7c3ce98f4a

bd49ecef4b3bb4

Throughout the OrderManager contract, fees are calculated using round down division. Therefore it is possible for users to avoid paying fees by using order sizes small enough such that they round down to 0.

With sponsored order creation through the PariFiForwarder this can be a viable method to avoid fees.

Employing such a strategy would cost a significant amount of gas and is unlikely to ever be profitable, however it may be preferred to use round up division when computing these fees.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/58/commits/604f9bc35eb2943c858ddb61e7

5155ddffa2e6d1

When creating a new position, the Pyth price feed is not updated. The fees calculation and the leverage validation are at risk of utilizing a stale price. This is because the convertMarketToTokenSecondary function is called which uses the unsafe EMA price point without validating publish time.

PythStructs.Price memory pythPrice = pyth.getEmaPriceUnsafe(priceId); (priceUsd, priceTimestamp) = _calculatePythPrice(pythPrice, false);

According to Pyth documentation the getEmaPriceUnsafe function, “may return a price from arbitrarily far in the past. It is the caller's responsibility to check the returned publishTime to ensure that the update is recent enough for their use case.”

Consider validating the publish time or updating the price feed when creating a new position.

PariFi Team: This is by design that we use the TWAP EMA price for non-critical purposes to prevent pushing prices when the action is initiated from the user.

To reduce bytecode and favor DRY the dead address referenced multiple times can instead be a constant in the MarketVault contract.

Declare the dead address as a constant variable in the MarketVault contract.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/commit/a70deb0c3aceccbef250fdeca62586a6f6

8527de

The userPosition variable is declared as a storage reference variable, however it would be much more efficient to declare it as a memory variable.

Declare the userPosition variable as a memory variable.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/203b6273564acb4c5240776f1e

eb11b4783a542e

The userOrder variable is declared as a storage reference variable, however it would be much more efficient to declare it as a memory variable.

Declare the userOrder variable as a memory variable.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/54/commits/203b6273564acb4c5240776f1e

eb11b4783a542e

When computing the leverage of a position in the calculateLeverage function round down division is used. The result of the rounding is that the position appearing as if it has slightly lower leverage than it actually does.

Though this rounding will have a minimal impact it may be worthwhile to use roundUp division to avoid position’s being technically above the allowed leverage yet rounded to within the allowed range.

PariFi Team: Fixed

https://github.com/Parifi/parifi-contracts-internal/pull/58/commits/604f9bc35eb2943c858ddb61e7

5155ddffa2e6d1

In the event that isProfit is false and the pnlInCollateral is greater in magnitude than the collateralAmount for the position an alternative leverage calculation is used to avoid divide by zero reverts. However any position where the collateral is exactly equal to the pnlInCollateral or insufficient to cover the pnlInCollateral technically has infinite leverage and should therefore always fail the maxLeverage validation.

Simply revert with a maxLeverage validation failure in the event that the pnlInCollateral >= _collateralAmount.

PariFi Team: The calculateLeverage function will also be used off-chain by UI and other components, hence we are not reverting inside the function, but reverting later inside the _validateLeverage function.

When computing the profitOrLoss round down division is always used whether or not this variable represents profit or loss.

This way user’s are able to avoid a fraction of their losses that are lost from truncation. This amount will almost always be negligible, however the loss amount should use `roundUp` division to act in the protocol’s favor.

Use roundUp division when the profitOrLoss variable represents a loss amount.

PariFi Team: https://github.com/Parifi/parifi-contracts-internal/pull/59/commits/778092a296f766a9cb01a5131d

c5a2c0f968a30a

The orderToPositionId is unnecessary and inefficient as each order can simply have a positionId as an attribute, for open orders this id can be ignored.

Remove the orderToPositionId mapping and introduce an attribute on the order struct to store the positionId the order is meant for.

PariFi Team: Implementing this change would result in a need for additional validations in the contract and other changes in the UI, so we’ve decided to continue using this mapping for now.