Liquidity Management

Description:

After a user calls PerpetualVault::deposit function while the vault has an open GMX position, a new MarketIncrease order request will be sent to GMX, then later executed by a GMX keeper. When the GMX keeper calls OrderHandler::executeOrder function, the execution process begins. At the end of the execution process, the GMX order handler performs a callback and calls GmxProxy::afterOrderExecution, which then calls PerpetualVault::afterOrderExecution, and this part of the function gets triggered:

The issue with the part of the code that I marked, is that increased is being adjusted wrongly in both cases, because when priceImpact is positive, it means the order executed at a better price (beneficial to Gamma's whole position), but in this case, Gamma subtracts the position fee AND price impact, punishing the user.

When priceImpact is negative, it means the order executed at a worse price (detrimental to Gamma's whole position), but in this case Gamma subtracts the position fee, but adds the price impact, meaning the user gets rewarded with more shares as they cause harm to Gamma's position.

This incorrect logic should be reversed, because in the current implementation users get rewarded when their deposited funds get added to the position at a worse price, and they get punished when their deposited funds get added to the position at a better price.

Statement from GMX github regarding entry/exit price being adjusted due to price impact:

For the price impact on position increases / decreases, if negative price impact is deducted as collateral from the position, this could lead to the position having a different leverage from what the user intended, so instead of deducting collateral the position's entry / exit price is adjusted based on the price impact

Source: https://github.com/gmx-io/gmx-synthetics/blob/main/README.md#price-impact

Impact:

Incorrect increased value gets passed to the _mint function and users receive a number of shares that does not reflect the current market situation. These minor share inaccuracies will accumulate over time, leading to larger consequences for the protocol.

Likelihood:

This issue will always happen when a user deposits into the vault when there is an open GMX position.

Proof of Concept:

1. Add the test provided below to PerpetualVault.t.sol

2. Run the test with this command: forge test --mt test_PriceImpact_IncorrectSigns --via-ir --rpc-url <YOUR_RPC_URL_HERE> -vv

3. In terminal you should see output like this:

Logs:
total amount before alice's position: 10000000000
total amount after alice's position: 9979999682
total amount after bob's position: 19959999363
total amount after charlie's position: 29939999044
Alice shares: 1000000000000000000
Bob shares: 1004507125020617174
Charlie shares: 1005628666727134279
charlie's USDC balance after withdraw: 10002260843
total amount after charlie's withdrawal: 19928571215
bob's USDC balance after withdraw: 9986690498
total amount after bob's withdrawal: 9932755498
alice's USDC balance after withdraw: 9923584145
total amount after alice's withdrawal: 0

1. Read the comments in the provided test to understand what's happening

2. Switch price impact signs in PerpetualVault::AfterOrderExecution function and run the test again to get the correct results

Recommended Mitigation:

Simply switch the signs in the lines where increased gets calculated: