# GMX Security Audit Report (Interim)

###### tags: `gmx`, `dex`

## 1. INTRODUCTION

### 1.1 Disclaimer
The audit makes no statements or warranties about utility of the code, safety of the code, suitability of the business model, investment advice, endorsement of the platform or its products, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug free status. The audit documentation is for discussion purposes only. The information presented in this report is confidential and privileged. If you are reading this report, you agree to keep it confidential, not to copy, disclose or disseminate without the agreement of Client. If you are not the intended recipient(s) of this document, please note that any disclosure, copying or dissemination of its content is strictly forbidden.

### 1.2 Security Assessment Methodology

A group of auditors are involved in the work on the audit. The security engineers check the provided source code independently of each other in accordance with the methodology described below:

#### 1. Project architecture review:

* Project documentation review.
* General code review.
* Reverse research and study of the project architecture on the source code alone.

##### Stage goals
* Build an independent view of the project's architecture.
* Identifying logical flaws.

#### 2. Checking the code in accordance with the vulnerabilities checklist:

* Manual code check for vulnerabilities listed on the Contractor's internal checklist. The Contractor's checklist is constantly updated based on the analysis of hacks, research, and audit of the clients' codes.
* Code check with the use of static analyzers (i.e Slither, Mythril, etc).

##### Stage goal
Eliminate typical vulnerabilities (e.g. reentrancy, gas limit, flash loan attacks etc.).

#### 3. Checking the code for compliance with the desired security model:

* Detailed study of the project documentation.
* Examination of contracts tests.
* Examination of comments in code.
* Comparison of the desired model obtained during the study with the reversed view obtained during the blind audit.
* Exploits PoC development with the use of such programs as Brownie and Hardhat.

##### Stage goal
Detect inconsistencies with the desired model.

#### 4. Consolidation of the auditors' interim reports into one:
* Cross check: each auditor reviews the reports of the others.
* Discussion of the issues found by the auditors.
* Issuance of an interim audit report.

##### Stage goals
* Double-check all the found issues to make sure they are relevant and the determined threat level is correct.
* Provide the Client with an interim report.

#### 5. Bug fixing & re-audit:
* The Client either fixes the issues or provides comments on the issues found by the auditors. Feedback from the Customer must be received on every issue/bug so that the Contractor can assign them a status (either "fixed" or "acknowledged").
* Upon completion of the bug fixing, the auditors double-check each fix and assign it a specific status, providing a proof link to the fix.
* A re-audited report is issued.

##### Stage goals
* Verify the fixed code version with all the recommendations and its statuses.
* Provide the Client with a re-audited report.

#### 6. Final code verification and issuance of a public audit report:
* The Customer deploys the re-audited source code on the mainnet.
* The Contractor verifies the deployed code with the re-audited version and checks them for compliance.
* If the versions of the code match, the Contractor issues a public audit report.

##### Stage goals
* Conduct the final check of the code deployed on the mainnet.
* Provide the Customer with a public audit report.

#### Finding Severity breakdown

All vulnerabilities discovered during the audit are classified based on their potential severity and have the following classification:

Severity | Description
--- | ---
Critical | Bugs leading to assets theft, fund access locking, or any other loss of funds.
High | Bugs that can trigger a contract failure. Further recovery is possible only by manual modification of the contract state or replacement.
Medium | Bugs that can break the intended contract logic or expose it to DoS attacks, but do not cause direct loss funds.
Low | Bugs that do not have a significant immediate impact and could be easily fixed.

Based on the feedback received from the Customer regarding the list of findings discovered by the Contractor, they are assigned the following statuses:

Status | Description
--- | ---
Fixed | Recommended fixes have been made to the project code and no longer affect its security.
Acknowledged | The Customer is aware of the finding. Recommendations for the finding are planned to be resolved in the future.

### 1.3 Project Overview

### 1.4 Project Dashboard

#### Project Summary

Title | Description
--- | ---
Client | GMX
Project name | DEX
Timeline | January 10 2022 - January 23 2022
Number of Auditors | 4

#### Project Log

Date | Commit Hash | Note
--- | --- | ---
10.01.2022 | 5f28277bdb4a8aeb4e288a11c3a740bf5787a312 | Commit for the audit

#### Project Scope
The audit covered the following files:

File name | Link
--- | ---
contracts/core/Vault.sol | https://github.com/gmx-io/gmx-contracts/blob/5f28277bdb4a8aeb4e288a11c3a740bf5787a312/contracts/core/Vault.sol
contracts/core/Router.sol | https://github.com/gmx-io/gmx-contracts/blob/5f28277bdb4a8aeb4e288a11c3a740bf5787a312/contracts/core/Router.sol
contracts/core/VaultUtils.sol | https://github.com/gmx-io/gmx-contracts/blob/5f28277bdb4a8aeb4e288a11c3a740bf5787a312/contracts/core/VaultUtils.sol
contracts/core/VaultPriceFeed.sol | https://github.com/gmx-io/gmx-contracts/blob/5f28277bdb4a8aeb4e288a11c3a740bf5787a312/contracts/core/VaultPriceFeed.sol

### 1.5 Summary of findings

Severity | # of Findings
--- | ---
CRITICAL| 0
HIGH | 5
MEDIUM | 6
LOW | 12

### 1.6 Conclusion

## 2. FINDINGS REPORT

### 2.1 Critical

Not found

### 2.2 High

#### 1. Vault.upgradeVault() is a poor way to migrate to a new vault contract

##### Description

There is an `upgradeVault` function https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L437 which allows gov to transfer funds to a new `Vault`.
This is a too simplified way for vault upgrading in multiple reasons. Some of them are:
- the old vault still has LPs onboarded (USDG holders)
- the old vault keeps long/short positions opened
- if a new `Vault` would have similar logic to exisiting `Vault`, then those transfers wouldn't be tracked in `tokenBalances` mapping. If that new `Vault` wasn't set to manager mode, then an attacker would be able to call `buyUsdg` function and use those funds, because deposited `tokenAmount` here https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L457 is calcualted as a difference between `balanceOf(address(this))` and `tokenBalances` mapping.
- the function oveall grants too much control of funds to admin/governance (only one transaction to withdraw million dollars of users' funds)

##### Recommendation

We recommend applying more sophisticated procedures to upgrade Vault to a new address. In addition, it would be secure to set more protected procedures for withdrawing users' funds from the contract.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 2. Market prices for underlying tokens can make `USDG` under-collateralized. In this case some of LPs will not be able to withdraw.

##### Description

If someone will mint `USDG` using `buyUsdg` function and provide token (that is about to decrease in price), then some depositors could be left unable to withdraw their collateral. It could happen, because when user brings back his `USDG` to `sellUsdg` function it calculates token `redemptionAmount` assuming that 1 `USDG` costs one USD, so that if collateral token will decrease in price twice, then user will redeem twice more tokens from the pool. In this case last `USDG` sellers are likely to be left with an empty pool and not be able to get their tokens back.
In addition, Vault stores `reservedAmounts[]` and does not allow withdrawal for tokens below some amount.

##### Recommendation

Current USDG minting/burning logic suggests LPs being not equal in chances to withdraw. It will require significant logic adjustment to deal with this issue.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 3. In case of stablecoin depeg events, the Vault will not operate with actual market prices. There will be no motivation to get rid off such stablecoin from the Vault.

##### Description

If an underlying token is added as `strictStableTokens`, an additional price feed is included and it always returns 1.
- https://github.com/gmx-io/gmx-contracts/blob/98bf45dbeab04024f04aabfce482e9c73f8ef839/contracts/core/VaultPriceFeed.sol#L183-L199

VaultPriceFeed in the majority of cases tries to find either the lowest or the highest price value among the list of sources. Setting a strict 1 in the list of sources will return the price=1 USD for a stablecoin when the Vault is asking for a MaxPrice - even if the market price is far below 1 (e.g. in case of a stablecoin depeg).
If a stablecoin depegs, some of the Vault operations will apply non-market price:
- sellUSDG() - when a user receives this stablecoin burning USDG
- swap() - when a user buys this stablecoin selling anything

In this market situation, users will be able to sell stablecoin at lower price and buy at far much higher. As a result, healthy motivation to sell, but no motivation to buy => probable consolidation of a depressed asset.
Moreover, these devaluated stablecoins will not be used by LPs when withdrawing even if stablecoins have some USD value remained (LPs have other alternative assets to receive, priced correct). As the result, it worsens the problem of inequality among LPs - those LPs who withdraws in the last order will suffer losses (they will have to buy these stablecoins for 1 USD each).

##### Recommendation

We recommend considering the case of stablecoin depeg events and decide on the desired price return.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 4. Price feed from AMM V1 is too controllable

##### Description

If `useV2Pricing` is set `false`, one of the price feeds will be taken from the function `getPriceV1()`. It utilizes reserve devision from uni-like pools - the very dangerous pattern, allowing anyone to manipulate the return from this price feed.
An attacker can manipulate AMM pool price before someones attempt to sell USDG and redeem tokens from the pool. It will lead to a smaller amount of tokens redeemed, as the redemption price would be higher than the actual token price.
It is possible because of the use of `getMaxPrice` function inside `getRedemptionAmount` here:
- https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L770.

##### Recommendation

We recommend disallowing using V1 pricing for all operations on the code level or applying TWAP (as pools have sinificant liquidity). But it must be noted, that TWAP is controllable in case of poor liquidity.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 5. `Vault.usdgAmounts[token]` sets zero in case of decrease below zero.

##### Description

`usdgAmounts[token]` is increased by 100, when a user provided 20 tokens priced 5$ each. 100 USDG are minted.
As a result `usdgAmounts[token]` represents a historic USD value of tokens provided, in different timestamps + USDG minted for these tokens.
The same thing is for `usdgAmounts[token]` with one key difference: it sets 0 when numbers turn negative - so the sum of `usdgAmounts[token]` for all token can be nor equal the sum of USDG minted.
- https://github.com/gmx-io/gmx-contracts/blob/98bf45dbeab04024f04aabfce482e9c73f8ef839/contracts/core/Vault.sol#L1161-L1173

`usdgAmounts[token]` is used only in calculating fees. Fees depend on the difference between [`usdgAmounts[token]`] and [targetUsdgAmount]. TargetUsdgAmount = USDGTotalSupply * TragetTokenWeight / TotalWeights.
- https://github.com/gmx-io/gmx-contracts/blob/98bf45dbeab04024f04aabfce482e9c73f8ef839/contracts/core/Vault.sol#L985-L990

If we have some lost numbers in `usdgAmounts[token]` in comparison to targetUsdgAmount (that is based on UsdgTotalSupply), then we face up with the problem that tokens are generally do not reach the target. It will be not possible that all the tokens reach the target - there will be at least on not in target.

In general, this problem leads to less fees accumulated for the Vault.

##### Recommendation

We recommend reverting when usdgAmounts[token] is zero.

##### Status
**NEW**
##### Client's commentary
> Comment here

### 2.3 Medium

#### 1. The fees for large swap calculated inside `VaultUtils.getFeeBasisPoints` are smaller than a multiple small swaps if the `initialDiff` is high enough

##### Description

- https://github.com/gmx-io/gmx-contracts/blob/f1f38c5ba83e2c3a68155042229b04678d9425ce/contracts/core/VaultUtils.sol#L160

If the swap changes the position of `poolAmount` related to `targetAmount` (for example the `initialAmount` is below `targetAmount` and a `nextAmount` is above), and the swap reduces the difference (`nextDiff < initialDiff`), than it has smaller fees than the sequence of swaps with the following changes in pool amounts:

* `initialAmount` $\rightarrow$ `targetAmount`
* `targetAmount` $\rightarrow$ `nextAmount`

It happened because formula doesn't take into account the change of `nextDiff` related to `initialDiff` and the relative positions of `initialAmount` and `nextAmount` to `targetAmount`.
It is the break of the condition written in the comment to the function

8. a large swap should have similar fees as the same trade split into multiple smaller swaps

##### Recommendation

We recommend taking into account a difference of `nextDiff` and `initialDiff` into account, when calculating the `rebateBps` value.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 2. `Vault._validateBufferAmount()` is not performed for `sellUSDG()`

##### Description

`Vault._validateBufferAmount()` is used in `Vault.swap()` together with `Vault._decreasePoolAmount()`:
- https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L546-L556

`_validateBufferAmount()` helps pool to have some tokens for leverage trading.
But the contracts has quite similar function `Vault.sellUSDG()` - actually the swap from USDG to tokens. It utilize the identical `Vault._decreasePoolAmount()` but then does not check `_validateBufferAmount()`. It is likely that this condition is missed.

##### Recommendation

We recommend unifing the logic of transfers, storage writes and checks for two functions transferring tokens out - `Vault.swap()` and `Vault.sellUSDG()`.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 3. Oracle is always looking for maximums and minimums. It means the increased risk of outliers applied.

##### Description

VaultPriceFeed.getPrice() is configured to take a maximum or minimum feed from the list of sources. This design implies that in case of an outlier transmit (e.g. by mistake from FastPriceFeed/SecondaryPrice) it will be used as correct.
- https://github.com/gmx-io/gmx-contracts/blob/98bf45dbeab04024f04aabfce482e9c73f8ef839/contracts/core/VaultPriceFeed.sol#L148-L248

##### Recommendation

We recommend the design where maximum and minimum prices are chosen around the median price feed, so that VaultPriceFeed outliers far above/below this median price are ignored.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 4. Lack of two-step process for contract `gov` changes

##### Description

Function `setGov` exists in the majority of contracts and it allows setting `gov` to 0 address which will disable some crucial functions, like `withdrawFees()` (impact - fees lost in the contract forever).

- https://github.com/gmx-io/gmx-contracts/blob/65e62b62aadea5baca48b8157acb9351249dbaf1/contracts/core/Router.sol#L48
- https://github.com/gmx-io/gmx-contracts/blob/eee9b90ed2f2da73aea7c14e3a4101b84f498016/contracts/core/Vault.sol#L294
- https://github.com/gmx-io/gmx-contracts/blob/78b0361184ebee7ff2871a6bdc6e45afbb8d91e0/contracts/core/VaultPriceFeed.sol#L68

##### Recommendation

We recommend adding a two-step check of transferring governance rights, where governance is proposing a new address for `gov` rights, and the second step is accepting the `gov` rights from new address.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 5. Vault.directPoolDeposit() is a public function but is likely used by the goverance

##### Description

This function sends token to a pool but does not mint USDG in return. This is used by the governance to make the pool overcollaterized.
By the way, the function remain public, so users may expect that it is used for common deposits (as the name suggests).

##### Recommendation

We recommend additing onlyGov() modifier to this function

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 6. Two step transfering tokens to Vault can lead to sandwitch attack

##### Description

Transfering tokens to Vault is performed outside the contract. After that to count transfered tokens `_transferIn()` function is used:
- https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L1115

If a user works directly with Vault contract and transfers tokens to contract in separate from the main operation (e.g. from `Vault.swap()`) transaction, the attacker can put his own transaction between them. So the money will be considered as attacker's and will be stolen. Using of Router contract helps to reduce the danger of it, but direct calling of Vault's functions is still allowed.

##### Recommendation

Change tokens transfer mechanic in the Vault to `safeTransferFrom`.

##### Status
**NEW**
##### Client's commentary
> Comment here

### 2.4 Low

#### 1. `usdToTokenMin` during liquidation minimizes fees instead of maximizing

##### Description

There is a call https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L724 inside `liquidatePosition` function, which convert `marginFees` USD value to particular token quote. That function `usdToTokenMin` uses max token price inside, so that returned `feeTokens` value is minimized.

##### Recommendation

We recommend to use `usdToTokenMax` function to calculate tokens amount to maximize protocl fees (as it is done in all other functions)

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 2. Accessing errors mapping before initialization

##### Description

There is a function `setErrorController` https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L246, which sets address of new `errorController`, but inside that function it has `_onlyGov()` check, which uses `errors` mapping inside before it's initialization (`errors` can be initialized only by `errorController`)

##### Recommendation

We recommend not to use `errors` mapping during `errorController` initialization. It is better to introduce a simpler function to validate gov role

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 3. `clearTokenConfig` doesn't replace token address from `allWhitelistedTokens`

##### Descrption

There is a function `clearTokenConfig` https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L393 which removes configuration for previously added token. It removes all configurations, except value inside `allWhitelistedTokens`

##### Recommendation

We recommend to replace removed token address with zero address (and handle such values in future) or implement values shifting and maintaining array size in a separate variable

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 4. `clearTokenConfig` dissalows a token, but positions can remain open

##### Descrption

`clearTokenConfig` removes a token from whitelisted ones, so new operations with this token will revert. Otherwise, there are likely to be opened positions with this token with no way to be closed.

##### Recommendation

We recommend removing clearTokenConfig and using softer steps to remove a token.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 5. Vault methods will fail if the balance of token is not constant

##### Description

https://github.com/gmx-io/gmx-contracts/blob/eee9b90ed2f2da73aea7c14e3a4101b84f498016/contracts/core/Vault.sol#L1120

Tokens with not constant balance can break the logic of contract. For example tokens with balances, that can decrease over time can break `_transferIn` function.

##### Recommendation

We recommend taking into account, that some tokens can have a not constant balance, and not including them to whitelisted token list.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 6. Possibility to infinitely increase `USDG` amount for specific token

##### Description

There is an `_increaseUsdgAmount` function https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L1152 which is used to increase token backing by `USDG` tokens. It checks for upper bound only if `maxUsdgAmount` is not zero. But if it's value wasn't set by a mistake, then it is possible for anyone to increase token backing with `USDG` without any limits

##### Recommendation

We recommend checking a case when `maxUsdgAmount` is set 0 for a specific token, or check that value not equals zero on token configuration here - https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L360

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 7. Some Vault parameters can be set above their maximums

##### Description

For instance, `Vault.liquidationFeeUsd` can be above `MAX_LIQUIDATION_FEE_USD`
`liquidationFeeUsd` can be set in two functions:
- `setFees()` - it checks that `_liquidationFeeUsd <= MAX_LIQUIDATION_FEE_USD`
- `initialize()` - it checks nothing

So it is possible that `liquidationFeeUsd` is above `MAX_LIQUIDATION_FEE_USD`
The same thing is for:
- fundingRateFactor
- stableFundingRateFactor

##### Recommendation

We recommend additing neccesary check for initialize()

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 8. _validateGasPrice() is easily avoidable

##### Description

This function checks that `tx.gasprice` is below `maxGasPrice` set. It is applied during the `Vault.increasePosition()`.
This condition does not make sense, as transactions can be groupped in bundles, where the total fee is meaningful for a validator - so the transaction with the very low gasprice can be easlily included.

##### Recommendation

We recommend removing this check.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 9. No checks for zero addresses on `Vault` initialization

##### Description

There is an `initialize` function https://github.com/gmx-io/gmx-contracts/blob/96e516bc8d0979c51c7fdffee50622415d5c411d/contracts/core/Vault.sol#L220 which sets addresses for `router`, `USDG` and `priceFeed`. Those addresses are provided by gov, not checked for zero values. Some of them cannot be changed later.

##### Recommendation

We recommend to check addresses for zero value at the beginning of `initialize` function.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 10. VaultUtils is Governable but it's not used

##### Description

Contract `VaultUtils` is Governable but it acts almost like a library. It has only view methods and Governable is not used.

##### Recommendation

Remove Governable inheritance.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 11. Funding fee is not taken if the position exists less than funding interval

##### Description

Cumulative funding fee is calculated once per 8 hours. If the futures was opened and closed during the same interval, funding fee will not be taken.

##### Recommendation

Check the logic of taking funding fee.

##### Status
**NEW**
##### Client's commentary
> Comment here

#### 12. `Vault._reduceCollateral()` looks for already found position in `Vault.positions`

##### Description

`Vault._reduceCollateral()` is used only in `Vault._decreasePosition()`. Both functions calculate keccak256 and looks for corresponding element in `positions` mapping. It's possible to use the link to storage as a parameter of `Vault._reduceCollateral()` and save gas.

##### Recommendation

Use the link to position of `positions` as a parameter of `Vault._reduceCollateral()`.

##### Status
**NEW**
##### Client's commentary
> Comment here

## 3. ABOUT MIXBYTES
MixBytes is a team of blockchain developers, auditors and analysts keen on decentralized systems. We build opensource solutions, smart contracts and blockchain protocols, perform security audits, work on benchmarking and software testing solutions, do research and tech consultancy.