# GLP Finding

## ДИСКЛЕЙМЕР ДЛЯ НАС

У меня затык с классификацией уровня тяжести - между High и Critical
Ради денег хочется конечно завысить.
За критикал у них написано 5млн баксов, за High 25 тысяч баксов.
https://immunefi.com/bounty/gmx/

Аргументы почему это критикал:
- всё развалится (критичный Severity), вся ликвидность рационально будет выведена
- а именно, Vault останется на балансе с депегнутым стейблом. Кто успеет вывести нормальные активы, те не пострадают. Оставшиеся LP вынуждены будут выводить депегнутый стейбл и признавать потерю в долларе на величину депега. Еще и не все токены смогут вывести.
- не вижу это нигде в known issues

Аргументы почему это не критикал:
- депег стейблкоина это ситуация не вызываемая насильно, её можно трактовать как "маловероятную". В их случае это USDC должен слегка депегнуться чтобы началась жара, так как он в их AUM занимает 40%. Но и поойдет депег DAI (4% от AUM), правда надо чтобы просел на 50%. Посвятил этому мини-раздел в самом конце.
- я не нашел как внешние лица могут заработать на этом, это не атака
- сама уязвимость относится к дизайну, экономике. Я использую в качестве аргументов "мотивацию LP", предполагаю что они будут поступать рационально (что например не будут выбирать потерю 4% если можно вывести не теряя). Это все нетипиная аргументация для аудитов. Но все логично, за уши не притягиваю.
- возможно достанут очередной козырь из рукава в духе "ну мы поймаем такую ситуацию, вольем руками в рынок, компенсируем потери из контракта Y". Возможно есть что-то сильно out-of-scope что может работать с такой ситуацией. Я такого не нашел.
- у них есть 3 упомянания про кривость AUM, но все нерелевантные.
1) `GlpManager.getAum may return a slightly higher value until a liquidation occurs`
Ответ - это очень slightly, конкретно про не-стейблы, и вообще про другое.
2) `GlpManager.getAum may return a slightly lower value when there are shorts in profit but the price movement is below the 1.5% threshold`
Ответ - это очень slightly, конкретно про не-стейблы, и вообще про другое.
3) `There will be some deviation of Vault.globalShortAveragePrices from the true average price if users increase their short position while the mark price is within 1.5% of their position’s average price, it is evaluated to not be economical for users to do this intentionally whether in combination with GLP minting or otherwise, GlpManager.setAumAdjustment can be used to correct this drift if required`
Ответ - просто совсем не об этом. Кстати GlpManager.setAumAdjustment() не будет работать, я про это тоже им написал, надеюсь прочитают.

## High-Critical - Deppeged stablecoins on GMX Vault make a significant gap between GLP minting and burning returns, pushing GMX Vault to the doom loop.

### In short

If a stablecoin in the Vault falls below `maxStrictPriceDeviation`, it would result in significant gap between Maximized getAum() and Minimized getAum().
The reason - choosing strict 1.00 PriceFeed in order to Maximize.
It will apply an automatic USD loss on GLP minters (for example, atomic [mint->burn] will result in additional % of additional loss, equal to % gap between Maximized getAum() and Minimized getAum()).
Small gap does always exists, but it turns unnaturally large if a depegged stablecoin remain on balances.
As the result:
- no motivation for new mints in any token (instant loss applied)
- no motivation to withdraw depegged stablecoins (additional loss applied)
Then a doom loop happens, when these broken motivations results in even more depegged stablecoins accumulated -> more share of stablecoin in AUM -> more gap between getAums()s -> more loss on minting -> and so on.
`GlpManager.setAumAdjustment()` is not a solution - because it increases/decreases both of getAum()s, not affecting the spread between them.
As the result, PriceFeed should not return 1.00 when market price is significantly below.

### Description

***Disclamer: all formulas/examples aim to illustrate the core principle, so they ignore fees and active long/short positions for simplicity. The problem in general remains the same.***

GLPManager.getAum() is used to calculate the USD value of funds on GMX Vault - for GLP minting and burning.
- https://github.com/gmx-io/gmx-contracts/blob/7ce789b40086eb2eda71d72ebbe6468f8c1c8f3b/contracts/core/GlpManager.sol#L136-L179

AUM iterates every token on GMX Vault and sums poolAmount(token)*price(token)
(directly for stablecoins, and applying some adjustments for non-stablecoins)

Formulas are following:

Minting:
GLPminted = tokensIn * PriceUSDminimized(tokenIn) / **AUMmaximized** * GLPSupply
Burning:
tokensOut = GLPburned / totalSupply * **AUMminimized** / PriceUSDmaximized(tokenOut)

Stablecoin price is maximized to a strict 1.00 USD, even if it is depegged.
- https://github.com/gmx-io/gmx-contracts/blob/98bf45dbeab04024f04aabfce482e9c73f8ef839/contracts/core/VaultPriceFeed.sol#L183-L199

Here is function flow with our comments, when function tries to maximize.
(we assume depeg to be above `maxStrictPriceDeviation`)

if (strictStableTokens[_token]) {
            uint256 delta = price > ONE_USD ? price.sub(ONE_USD) :
                                              ONE_USD.sub(price);
(ignored)   if (delta <= maxStrictPriceDeviation) {
                return ONE_USD;
            }
            // if _maximise and price is e.g. 1.02, return 1.02
(ignored)   if (_maximise && price > ONE_USD) {
                return price;
            }

            // if !_maximise and price is e.g. 0.98, return 0.98
(ignored)   if (!_maximise && price < ONE_USD) {
                return price;
            }

 (HERE)     return ONE_USD;
 

**PROBLEM:**
Code does not directly consider the cases and returns 1.00 by default:
- `if (!_maximise && price > ONE_USD)` - close to impossible, but January 2020
- `if (_maximise && price < ONE_USD)` - our main case, depeg down, some probability. Wrong here to return 1.00.

As the result:
- `(_maximise && price < ONE_USD)` : returns 1.00
- `(!_maximise && price < ONE_USD)`: returns market price

### Case study (Proof of Severity)

Imagine we have 40% of GMX Vault represented by USDC (as it is now).
https://app.gmx.io/#/dashboard

current GLPManager.getAum() maximized = 429 846 511 USD
current GLPManager.getAum() minimized = 429 708 508 USD

Let's stresstest some scenarios when `maxStrictPriceDeviation` is 1%.

#### Scenario 1 - USDC price falls 0.5%

PriceFeed for USDC would return 1.00, so AUM return remains the same:
GLPManager.getAum() maximized = 429 846 511 USD
GLPManager.getAum() minimized = 429 708 508 USD

The gap remain insignificant, so everything works ok.

#### Scenario 2 - USDC price falls 10%

As the deviation is above `maxStrictPriceDeviation`, a new PriceFeed logic would be applied - maximizing and minimizing.
But this 10% drop works only for minimizing.
**For maximizing USDC price remains 1.00**

So,
GLPManager.getAum() maximized = 429 846 511 USD (based on maximized price 1.00)
GLPManager.getAum() minimized = 421 708 508 USD (based on the real price)

As we can see - the gap is significant now - 4% of AUM.
It is 8 594 170 USD gap between two returns.

As the result,
Every minting+burning for amount X would lose 4% of X.
Even if other assets are used as tokenIn/tokens out. Like ETH.
100 USD value of inputed ETH would mint 96 USD value in GLP.
So, depegged tokens spoil the whole GMX Vault - breaking the whole motivation to mint GLP tokens.

**This issue tends to worsen.**

Is there any chance that these depegged stablecoins would leave the Vault?
No, the opposite is true:
GLP holders can choose `tokenOut`, but choosing depegged USDC would result in even more loss.
The formula for burning:
tokensOut = GLPburned / totalSupply * AUMminimized / **PriceUSDmaximized(tokenOut)**
PriceUSDmaximized(tokenOut) is above the market, so LPs would receive less.
So choosing any token is better than choosing USDC.
Therefore, no motivation of USDC leaving the Vault.

As the result,
1) No motivation provide liquidity (loss of 4% on minting) in any token
2) No motivation to withdraw a depegged stablecoin

So, it will be more and more depegged stablecoin, and less of other assets.
**It means more share of USDC in the Vault.**
Like from 40% to 60%.
It means more spread between maximized and minimized AUM.
And the situation worsens.

>To sum up,
>Depegged stablecoin once detected pushes GMX vault to doom loop:
>- spread between maximized and minimized AUM
>- then automatic %USD loss on GLP minting (in any token)
>- then destroying motivation for new inflows (in any token)
>- automatic %USD loss on GLP burning if tokenOut is a depegged stablecoin, pushing to chose any other assets for withdrawals
>- then larger share of depegged stablecoin in AUM and following larger spread between minimized and Maximized AUM
>- then even more automatic % loss on GLP minting
>- and so one
>
> Last GLP holder are left with only depegged stablecoins on the Vault and have to suffer depeg loss on withdrawal : e.g. if we have 80M USDC left on pool, all GLPs left can only withdraw 72 M USDC (~65M$ value).

### How stablecoins should depeg

For 2% AUM gap it is required:
- (OR) for USDC: 5% drop
- (OR) for DAI: 50% drop
- (OR) for FRAX: 100% drop

### Other vectors

The core problem of the finding is bad return from price feed.
We expect that it can lead to some even more dangerous scenarios.

### Recommendation
We recommend to remove a strict 1.00 USD as a return for stablecoin price feeds, and implement mechanisms to rely on only market driven prices.