Synthetics Perpetuals Exchange, Round 2

Proof of concept: PoC

When creating an order with the function createNewPosition(), a user can send an executionFee to the feeReceiver. The feeReciever is different from the orderManager, meaning the executionFee will not be in the orderManager.

When cancelling an order the executionFee is returned to the user:

if (userOrder.executionFee != 0) {
  userBalance = userBalance + userOrder.executionFee;
}
...
if (userBalance != 0) {
  IERC20(market.depositToken).safeTransfer(userAddress, userBalance);
}

An attacker can create a order with a large execution fee and then cancel that order. By doing so, they can drain the orderManager as the orderManager is the one paying the refund, while the original funds are with the feeReceiver.

_closePosition(), _decreasePosition(), _increasePosition(),_liquidatePosition() will no longer fully work as all of these functions will eventually transfer the collateral either back to the user or to the vault. However, due to this attack the orderManager will not have sufficient funds to cover these transactions as it will have zero collateral.

Either do not refund the executionFee to the users or leave the executionFee in orderManager until settlement occurs. At that point, the keeper can pull the executionFee.

PariFi Team: The issue was resolved in commit d8dca2f. 10

A user’s position is created without taking the funds in the Vault into account. As a result, a trader may open a position when the Vault has zero funds, or not enough funds to support the market’s PnL.

Users will be unable to realize their profits and be stuck with their position. Furthermore, as soon as there are enough reserves in the Vault for withdrawal, the Vault’s funds will be drained and LP’s will lose their deposited funds.

Validate that the open interest does not exceed some percentage of the Vault’s funds. This would create a buffer and help avoid a scenario where the pool does not have enough liquidity to support user profits.

PariFi Team: Acknowledged

If a user creates an increase order and provides a deltaCollateral that is less than their openingFee, they cannot cancel this order and their funds will be stuck if the order cannot be executed.

Additionally, there is no opening fee charged when a decrease or close order is cancelled. This is in contradiction to the behavior of increase orders.

Refactor the way the opening fee is charged for increase, decrease and close orders. Consider requiring that the opening fee be provided up front in the modifyPosition function even for decrease or close orders.

Alternatively, consider only charging the opening fee when an order is executed and instead maintaining a fraction of the executionFee. It would then be prudent to ensure that the executionFee is above a 0 or trivial amount.

PariFi Team: The issue was resolved in commit d8dca2f.

Proof of concept: PoC

In ParifiVault, the cooldown function only checks that the balance of a sender is not 0. It does not check how many tokens the user has or the amount that could be withdrawn after the cooldown period. A user may:

• Prepare X addresses for which it sends 1 WEI of share tokens
• Call the ParifiVault.cooldown function from each address
• Rotate this system in order to also bypass the expiry window constraint
• Whenever a user wishes to withdraw any amount of LPs, they send all token shares to one of the pre-warmed addresses and withdraw reserves

Because the cooldown can be avoided, a depositor can view a profitable position but withdraw their liquidity without waiting. This is extremely detrimental to traders as they will be unable to withdraw their profits due to the lack of reserves.

Due to the bypassed cooldown, profit from the vault may also be extracted with the following steps:

• Flash-loan a large amount of tokens
• Deposit the tokens into the vault
• Create a new position that triggers fee distribution and increases the value per share
• Withdraw tokens using a pre-warmed address and profit

Note the balance of users that call the cooldown function and allow a maximum of that amount to be withdrawn in redeem and withdraw functions. Alternatively, reset the cooldown on shares transfer and deposit.

PariFi Team: The issue was resolved in commit 6c07f1a.

The current execution fee process only charges the user if the user-supplied order specifies a non-zero execution fee.

if (_order.executionFee != 0) {
  _chargeExecutionFee(orderId, market.depositToken, _order.executionFee,_order.userAddress);
}

Because there is no requirement that the execution fee must be non-zero, a user has no incentive to pass a non-zero execution fee and pay extra for their order.

As a result, keepers will not be properly remunerated for settling orders and liquidations. This can lead to griefing as the protocol must pay a fee each time the oracle price is updated, in addition to the gas needed for execution.

Create a state variable for the execution fee and validate that it matches the execution fee passed on the order.

PariFi Team: The issue was resolved in commit d8dca2f.

Proof of concept: PoC

In the _liquidatePosition function the PnlRealized event is emitted which includes the pnlInCollateral. This is calculated by taking the netPnl and subtracting feesInCollateral. However, the feesInCollateral may be greater than netPnl, causing an underflow revert.

emit PnlRealized(_positionId, false, netPnl - feesInCollateral, feesInCollateral,
executionPrice);

In the _getNetProfitOrLossIncludingFees function which is called in _liquidatePosition, if the position is in profit excluding fees it will enter the inner if-statement the isNetProfit will be set to false and feesInCollateral will be greater than netPnl.

At the end of the _liquidatePosition function when the PnlRealized event is emitted the transaction will revert, making any liquidation impossible until the position is completely underwater, leading to a loss of yield for the protocol and LP's.

Since netPnl and fees will be a loss for the position and go to the feeManager anyway, do not emit an event where netPnl - feesInCollateral is being calculated. Instead, have a separate event for liquidations where only the netPnl is being emitted.

PariFi Team: Resolved in commit 3b5f6b.

The validation to ensure the maximum open interest is not exceeded compares both trading sides in aggregate:

if (config.totalLongs + config.totalShorts > (2 * config.maximumOi)) revert LibError.MaxOI();

With the current open interest (OI) validation, longs are able to dictate how much in shorts can be opened and vice versa. For example, if traders establish 1800 ETH in long OI, only 200 ETH in short OI can be opened when the maximumOi is set to 1000 ETH. This inherently leads the market to be imbalanced.

Validate open interest per side rather than in aggregate.

PariFi Team: Resolved in commit e186c87.

Pyth provides price feeds for numerous US equities with significant dividends. A position on a share that pays a dividend will see its price adjusted to reflect the dividend payment. For example, with a dividend of $1 per share, a stock that was trading at $100 will drop to $99 on the ex-dividend date. Furthermore, stocks may go through splits and reverse stock splits, drastically changing the price of a share.

Consider the following scenario:

• A trader anticipates a stock split so they sell 1 share for $100
• A 2:1 stock split occurs and the new price is $50
• The trader closes their short, making a risk-free profit.

Exercise caution with which markets are supported for trading and carefully monitor for equity events as they are announced in advanced. In anticipation of an event, put the market in close-only mode and pause the market afterwards to prevent further trading. Ensure the market starts from a clean slate post-event.

PariFi Team: Acknowledged.

In the case of a steep price move (UST for example), the protocol needs to be able to perform liquidations to ensure the system remains solvent. During this volatility, the percentage difference between the lagging EMA and the current price may exceed the market.maxPriceDeviation and revert, causing liquidations to fail.

Consider simply fetching and utilizing the primaryPrice for liquidation. Because the price feed is updated prior to liquidation, the call priceFeed.getMarketPricePrimary() should not revert.

It would also be worth adding a confidence interval when fetching only the primaryPrice to mitigate any potential price manipulation.

PariFi Team: Resolved in commit 352ff02.

In createNewPosition there is a check that prevents an order from being created if the collateral is below a set minimum. This is in part to ensure that liquidations are profitable. However, a user can decrease the position so that the collateral is below the minimum, making liquidations not profitable.

In addition, normal users can unintentionally decrease the position to such a low level that they don't bother to close it. Because the position is not being closed and may end up being unprofitable to liquidate, these small positions will reduce the available OI until the keepers opt to liquidate the position which at that point they will be liquidating at a loss.

Add a minimum collateral check in modifyPosition to ensure the collateral remains above a desired minimum.

PariFi Team: Resolved in commit 352ff02.

Updating an existing market, by calling the updateExistingMarket function from the DataFabric contract, incorrectly also calculates market fees up to that point. This is because it also includes a call to the _updateCumulativeFees function, which is responsible for updating fees up to that point.

The updateExistingMarket function cannot be called if the market is not paused, but pausing the market does not fast forward feeLastUpdatedTimestamp, only unpausing does. For the time since the market was paused and until it was updated by the admin, the market will incorrectly deduct fees from participants.

Delete the call to the _updateCumulativeFees function since a call to it is already done in the pause function.

PariFi Team: Resolved in commit 62c8818.

Liquidating a position, by calling the function liquidatePosition from the OrderManager contract, does not check if the market in which the order was placed is currently paused or not. Any already existing positions can be liquidated but users cannot cancel or add to them during pause by design.

Another issue that appears when a liquidation is done on an order in a paused market is triggering market fee payment. This takes place as the function updateCumulativeFees from the DataFabric will get called.

Add a call to the _validateMarket function at the beginning of the _liquidatePosition function in the OrderManager contract.

PariFi Team: Resolved in commit e186c8.

In _increasePosition and _decreasePosition the user’s position is essentially recreated with a modified positionSize and/or positionCollateral. When the position is recreated, the userPosition.avgPrice is set to the updatedAvgPrice, which is based on the deltaSize and calculated as follows.

uint256 updatedAvgPrice = _verifyAndUpdatePrice(
  userPosition.marketId, userPosition.isLong, false, OrderDS.OrderType.OPEN_NEW_POSITION,
userOrder.deltaSize
);

When the updatedAvgPrice is calculated, it will include the negative impact from the increase or decrease delta change. Therefore, the remaining position immediately has negative PnL as the avgPrice assigned is automatically worse than market price.

Calculate the average price so that the updated size is valued at the current price, as this was the price the PnL was settled at.

avgPrice = marketPrice * (sizeRemaining/totalNewSize) + updatedPrice *
(sizeDelta/totalNewSize)

PariFi Team: Acknowledged as intended behavior for the pricing curve.

In the _liquidatePosition function, the netPnl is capped to the userPosition.positionCollateral amount before the liquidationThreshold validation.

Therefore if the user’s position has 0 collateral, the netPnl will be assigned to 0 and subsequently fail the validation on line 574 as the liquidationThreshold is also 0.

There is no straightforward path to getting a position with 0 collateral, however the liquidation logic should be refactored as certainly any position with 0 collateral must be liquidated.

Additionally, in the case that the netPnl is capped to 0, the safeTransfer and fee distribution on lines 593 and 594 should not occur as certain tokens may revert on 0 transfers.

Cap the netPnl to the userPosition.positionCollateral after the liquidationThreshold is validated. Additionally, do not execute the fee distribution logic if the netPnl is 0.

PariFi Team: Resolved in commit 3b5f6b.

Changing market settings, by calling the setMaximumOi or setBorrowingCurveConfig functions from the DataFabric contract, creates an issue regarding fee updates. Fees are updated and deducted whenever any protocol operation is settled with the current value applied for the entire time since the last fee update. Neither of the two functions call the _updateCumulativeFees function to update fees up to that point before influencing the fees.

Consider a situation when there are no operations for 3 hours, in which, after 2 hours the maximum OI was decreased. The next operation will commit all fees during those 3 hours with the new OI taken into consideration for fee calculation. This results in a higher than intended fee being paid by users since only 1 hrs of those 3 hrs was spent in the market with the new, higher fees.

• Consider allowing a grace period when configuring these market values so that users have a time limit to modify/cancel their current positions
• Call _updateCumulativeFees before setting new configuration value as to not impact fees since the last checkpoint. _updateCumulativeFees must not be called in a paused market as to not accumulate fees for users if paused.

PariFi Team: Resolved in commit 62c8818.

When a market is in closeOnly mode, users are able to add collateral to an existing position. When adding collateral to an existing position, the _increasePosition function is called, which, in turns, calls DataFabric::updateMarketData.

In the case of adding collateral, userOrder.deltaSize equals 0, so the updateMarketData function will return in the first check and avoid the LibError.CloseOnlyMode() revert.

This can result in a position being kept longer in a market than intended, by continuously adding collateral when needed rather than closing it out.

Add a check to make sure the market is not in closeOnly mode:

if (size == 0 && !closeOnlyMode[marketId]) return;

Otherwise, if this functionality is indeed to be supported, clearly document this behavior.

PariFi Team: Resolved in commit 6b92dc.

The price returned by Pyth is checked to be non-zero, however the priceUsd may become 0 after decimal adjustment: priceUsd = SafeCast.toUint256(pythPrice.price) / (10 ** adjustedExpo);

For example, if the pythPrice.price = 1 and the pythPrice.expo = -9, then priceUsd = 1 / 10 = 0

Key protocol actions such as liquidations will fail because _verifyAndUpdatePrice calculates the price deviation between primary and secondary price with uint256 diffBps = (_getDiff(primaryPrice, secondaryPrice) * PRECISION_MULTIPLIER) / secondaryPrice; and there will be division by zero.

Carefully select which assets are supported for trading, as assets with a low price and a large, negative exponent are susceptible to this issue. Furthermore, validate the price is non-zero after conversion to FEED_DECIMALS.

PariFi Team: Resolved in commit 62c8818.

The vault does not conform to the ERC4626 standard which may break external integrations. Some examples of non-compliance include:

• maxWithdraw does not take into account whether the user is in their cooldown period and cannot withdraw. According to specification, maxWithdraw "MUST factor in both global and user-specific limits, like if withdrawals are entirely disabled (even temporarily) it MUST return 0."
• previewRedeem "MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees."
• previewWithdraw "MUST be inclusive of withdrawal fees. Integrators should be aware of the existence of withdrawal fees."

Consider adjusting the non-compliant functions to be in-line with ERC4626 standards.

PariFi Team: Resolved in commit 6c07f1a.

In the _getNetProfitOrLossIncludingFees function, a position’s isProfit status is determined by both the fees incurred and the position’s avgPrice compared to the current price of the asset. For liquidations these fees are liquidationFee and closingFee.

It is possible for a user to create a position that is immediately under water because fees are not considered when creating a position.

For example, if a user were to open a 100x position by supplying 100 USDC as collateral the user would pass the check in the validateLeverage function. However, with a liquidationFee of 1% and a closingFee of 0.1% the user would be immediately under water.

1% of 10000 = 100 0.1% of 10000 = 10 Total Fees: 110 Collateral: 100 The fees alone outweigh the users collateral leading to a complete liquidation.

Upon creating a position, calculate the net PnL with the liquidationFee and closingFee and ensure the liquidation threshold is not passed.

PariFi Team: Acknowledged.

During a paused market period, the protocol may choose to change opening, closing or liquidation fees. If this happens, any pending order, when settled, will use the new fee instead of the one that was at the time the order was created.

This affects:

• Canceling or settling a modify increase order
• Decreasing or closing an existing position
• Liquidating a position

Creating a pending order is the only operation that extracts fees exactly when it is executed.

Depending on the increase or decrease in fees, several unwanted scenarios may appear. Example, during a pause the opening fees were reduced and closing fees were increased. Afterwards:

• Any cancelled increase order will pay less fees then it was expecting and had agreed to by taking the initial trade, resulting in protocol funds losses
• Any settling of closing or decreasing orders, or liquidating a user will result more fees then user initial took into consideration when making his trade. Possibly the user would have not taken the trade with the new fee system

When creating pending orders also save a snapshot of the current market fees and use those when settling them.

PariFi Team: Acknowledged.

Function distributeFees implements a delay between fee distributions, where the delay is arbitrarily set by an admin.

However, the comment states that the function aims to // Distribute fees at regular intervals of every 1 hour.

Modify the comment to "Distribute fees at regular intervals of every delay period".

PariFi Team: Resolved in commit 62c8818.

In the cancelPendingOrder function, a user can cancel their order and will get the collateral they deposited back. However, the fees are already transferred to the feeManager so the user will not get those funds returned to them. This could be an issue when users are not aware of how fees are charged.

Clearly document that fees are always charged regardless if the order is settled or canceled.

PariFi Team: Acknowledged.

Both _createNewPosition and _increasePosition functions from the OrderManager contract validate markets by calling the _validateMarket function. This is redundant, since the two functions are only reached via _settleOrder which already validates the market in the same manner.

Remove the redundant call to the _validateMarket function from within the _increasePosition and _createNewPosition functions.

PariFi Team: Resolved with commit 352ff02.

If a user gets blacklisted by the depositToken, such as USDT or USDC, they will be unable to modify, close, or cancel their pending order, as the safeTransfer/safeTransferFrom method will revert. Also, the user position cannot be liquidated if remainingCollateral !=0.

Consider letting the user change their userAddress for a certain position.

PariFi Team: Acknowledged.

In the DataFabric contract, the liquidationThreshold has a maximum of 100%, meaning that a position cannot be liquidated until they are at a loss of greater than 100%. Consequently, the only time that liquidations will occur is when the protocol is at a loss, leading to a loss of yield for the protocol and the LP’s.

Ideally, check that the liquidationThreshold for any given market and ensure that it is less than PRECISION_MULTIPLIER. This can be done by changing:

if (_newMarket.liquidationThreshold < 5_000 || _newMarket.liquidationThreshold >
PRECISION_MULTIPLIER) {

to

if (_newMarket.liquidationThreshold < 5_000 || _newMarket.liquidationThreshold >=
PRECISION_MULTIPLIER) {

PariFi Team: Resolved in commit 62c8818.

In the getExpectedUtilization function in the DataFabric contract there is no validation that the size increase would remain under the maximum allowed OI.

The maximum OI validation occurs later on in the order execution, in the updateMarketData function, however adding the check in the getExpectedUtilization function would terminate execution earlier and save gas expenditure.

Additionally, any integrating system relying on the getExpectedUtilization function would not receive an invalid response when the OI exceeds the allowed maximum.

Consider implementing validation such that the maximum OI is validated in the getExpectedUtilization function.

PariFi Team: Resolved in commit e186c8.

The DEFAULT_ADMIN_ROLE role can change any other roles by default and is a security risk the team explicitly stated they do not want in their roles. This role is however granted to the initial multisig and a TODO mentioning it to be removed was forgotten in the code.

Remove lines 45-46 from the RBAC.sol file.

PariFi Team: Resolved in commit 62c8818.

The cooldown function from the ParifiVault contract is a user facing function. In case of a vault pause users may still call this function and, when unpause happens, have a direct withdraw executed.

Add the whenNotPaused modifier to the cooldown function.

PariFi Team: Resolved in commit e186c8.

There is no validation that pending orders settled by keepers on behalf of users are executed in the correct order, meaning in the order they were created by the user.

Consider the following scenario:

• User has a position that is close to being liquidated so he creates an order to add collateral
• Then user realizes he added too much collateral and send a new order to slightly reduce the collateral
• A keeper may, by mistake, execute the second order before the first, that would make the position liquidatable

Settling an order should have a mechanism to ensure that initial user order creation is respected. If the keeper role will be decentralized in the future, this is an issue that must be fixed before that point.

PariFi Team: Acknowledged.

In a standard 2-step role transfer, the current holder initiates the pending transfer and the new holder must accept it. As it is implemented in the RBAC contract, the old ADMIN role holder initiates the transfer by calling the proposeNewMultisig function and again the old holder then commits the change by calling the updateMultisig function.

Change so that the new ADMIN role holder must call the updateMultisig function.

PariFi Team: Resolved in commit 6b92dc9.

Tokens that revert on 0 transfers could cause a DoS in the distributeFees function if the lpAmount rounds to 0 or if the protocolFeeAmount is ever 0.

Consider only making the transfers if the amount to transfer is nonzero so that these will not fail.

PariFi Team: Resolved in commit 6b92dc9.

Usually, there is an incentive to have a partner/referral address. However, the person getting referred here has no incentive, as the feeInCollateral experienced by the msg.sender is not reduced.

Consider implementing an incentive for the user to use a partner.

PariFi Team: Acknowledged.

In the _getPriceWithDeviation function, the increasedPrice and reducedPrice are always computed, however only one of these is ever used depending on isLong.

Compute the increasePrice if isLong == true and the reducedPrice if isLong == false to save on gas.

PariFi Team: Resolved in commit 352ff02.

In the createNewPosition function, safeTransferFrom occurs at the end of the function, however, this is potentially handing over tx execution to an untrusted address when the system is in an invalid state.

The invalid state is the order having been saved to storage but the collateral not actually collected into the contract. The risk is low as this requires tokens with callbacks that execute before the transfer is made, however should be addressed if tokens with callbacks are to be supported.

Collect tokens at the beginning of the createNewPosition function.

PariFi Team: Resolved in commit 45d7f88.

After an order is settled, it still exists in the orderToPositionId mapping although it has been deleted from the pendingOrders mapping. This contrasts with the functionality in function cancelPendingOrder() where the order is deleted from both mappings.

Perform delete orderToPositionId[_orderId]; at the end of settlement.

PariFi Team: Resolved in commit 352ff02.

When withdrawing or redeeming from the PariFi Vault, a check is performed that the owner of the assets has passed the cooldown and if not, then it is considered that 0 assets can be withdrawn. Anyone can call functions withdraw or redeem for any depositor in the vault as long as they have the required allowance.

Withdrawals and redemptions can be called with a 0 input amount. Execution will pass without the need for an allowance and the cooldown for the owner will be reset as if the owner has withdrawn. This is possible because there is no 0 amount validation in the execution path, neither in the allowance check nor in the withdrawal itself.

An attacker can continuously call the withdraw or redeem functions for any other account with a 0 amount and delete their cooldown, effectively blocking that account from ever withdrawing their assets.

In the withdraw and redeem functions from the ParifiVault contract, if the requested amount is 0, then return 0 as the first operation.

PariFi Team: Resolved in commits: 86284efab84ee8f9e7994a0dc6f88d8bfa7c959b, 654e9f03a218ad8a014c0f8e90e1e8c1355d1b63.

The execution fee is charged whenever a position is modified from the existing position's collateral (excluding the creation of a new position).

This poses potential problems as the execution fee could exceed the collateral a position has.

Consider a user who wants to close their position because they are getting too close to the liquidation threshold, but now have to firstly increase the collateral of their position just to close their position. In both the increase and the close the user would be charged an execution fee.

This is even further problematic because the _increasePosition function will try to deduct the execution fee from the existing collateral, rather than the collateral after it has been increased by userOrder.deltaCollateral. Consequently, the user is stuck until they are liquidated.

Charge the execution fee after the collateral is increased. Furthermore, consider restricting the execution fee to be less than the market.minCollateral.

PariFi Team: Currently, execution fee is $2, while minCollateral is $50. So this risk is mitigated already.

Function getLiquidationNetPNLInCollateral calculates whether the position is liquidatable using canLiquidate = netProfitOrLoss > liquidationThreshold; However, it does not take into account whether it is indeed profit or loss. A user may be in a large profit and now be considered liquidatable from the Adapter’s perspective.

Take into consideration whether the position is in profit prior to setting canLiquidate.

PariFi Team: Acknowledged.

Function _getProfitOrLossInCollateral uses the latest tokenPrice in the OrderManager. In the Adapter, getProfitOrLossInCollateral uses the EMA tokenPrice. Because these two prices may differ, the net PnL and leverage calculations will provide inconsistent results. For example, a position may appear liquidatable from the Adapter when it is not.

Use the same token pricing as the OrderManager in the Adapter.

PariFi Team: Acknowledged.

When updating an existing market using the `updateExistingMarket` function, at the end of the function there is a check that if the new market argument bundle would set the market to true, meaning directly activate it, then to clear it since the market needs to be unpaused by the admin after update separately.

This validation is incorrectly checking if the market is already false, then it sets it to false again, meaning that if `_updatedMarket.isLive` is true, the market would be left as it is and incorrectly remain active after function execution:

`if (!_updatedMarket.isLive) availableMarkets[_marketId].isLive = false;`

Remove the ! operator from the if clause on line 598.

PariFi Team: Resolved in commit 05e94064760013b215b8443fd3074ef5a8fd171c.

While the Arbitrum sequencer is down it is possible for a users position to go from healthy to undercollateralized. During this time the average user will not be able to rescue their position as they will not be able to submit orders directly through Arbitrum.

However, liquidators will be able to submit liquidation transactions through the delayed inbox on L1. When the sequencer is back online the transactions submitted through the delay box will be executed first, meaning the position will be liquidated before the users have a chance to rescue their position.

Consider adding a grace period after outages to allow users some time to save their position when the sequencer is back online.

PariFi Team: Currently, liquidations can be triggered by the keeper role, which is Gelato address, and its not automated. Therefore, in this edge we might think of giving a grace period.

While the Arbitrum sequencer is down it is possible for a users position to go from healthy to undercollateralized. During this time the average user will not be able to rescue their position as they will not be able to submit orders directly through Arbitrum.

However, liquidators will be able to submit liquidation transactions through the delayed inbox on L1. When the sequencer is back online the transactions submitted through the delay box will be executed first, meaning the position will be liquidated before the users have a chance to rescue their position.

Ensure that a position cannot be liquidated during its creation. Implement a validation check in the createNewPosition function.

PariFi Team: Resolved with commit ba6341dec76dfaf1cceda2cc084f1a71d2f160ea.

For any keepers or users viewing a position’s status through the Adapter, it is important to document some of the differences between it's behavior and the behavior in OrderManager.

Some differences include but are not limited to:

• `_verifyAndUpdatePrice` reverts if the difference between the primary and secondary price is exceeded in the OrderManager. In the Adapter, the primary price is simply used.
• `getPriceWithDeviation` does not consider whether the order is an increase or decrease order for deviation and rounding because in the OrderManager it is called in the context of recreating a position regardless of the direction. In the Adapter, it can take into account order direction.

Clearly document the differences between Adapter and OrderManager functionalities and their reasons.

PariFi Team: We can’t use primary price with view functions, as the pyth price query reverts.

Many of the functions in the Adapter are copies of functions in the OrderManager or have slight variations e.g. getProfitOrLossInCollateral

Furthermore, functions such as getAvgPriceWithDeviation and getPriceWithDeviation in the Adapter perform the exact same calculations where the only difference is the deltaSize parameter.

Extract common components into a single library. This will help prevent any discrepancy between Adapter and OrderManager readings. Furthermore, any functions that are duplicative (e.g. getAvgPriceWithDeviation) should be calling helpers with common functionality extracted.

PariFi Team: Acknowledged.

When setting the cooldown for a user in the cooldown function from ParifiVault vault, the function caller is saved in a local variable user. This value it is used only once but _msgSender is subsequently called 2 more times only to retrieve the same value.

Reuse the existing user variable where needed.

PariFi Team: Resolved in commit 86284efab84ee8f9e7994a0dc6f88d8bfa7c959b.

Whenever execution fee is to be deducted, protocol checks that a execution fee is set but does not check if an execution fee receiver is set.

In this particular case, the fee would be sent to address(0) and lost.

Add the extra check that executionFeeReceiver is not address(0) before extracting it.

PariFi Team: Resolved in commit 86284efab84ee8f9e7994a0dc6f88d8bfa7c959b.

In ParifiForwarder there are several cases of debug logging via console.log.

Remove the calls to console.log as well as its import.

PariFi Team: Resolved in commit 86284efab84ee8f9e7994a0dc6f88d8bfa7c959b.