M-07. Wrong hardcoded PnL factor is used in all GMXVault add liquidity operations

Submitted by Giorgio, ro1sharkm, Drynooo, kz0213871, 0xanmol, SBSecurity, 0xAsen, Jeffauditor. Selected submission by: SBSecurity.

Relevant GitHub Links

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXDeposit.sol#L90-L101

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/strategy/gmx/GMXManager.sol#L148

https://github.com/Cyfrin/2023-10-SteadeFi/blob/0f909e2f0917cb9ad02986f631d622376510abec/contracts/oracles/GMXOracle.sol#L244-L248

https://github.com/gmx-io/gmx-synthetics/blob/3b37cfa213471aac86344c352aa1636c67f41ca8/contracts/market/MarketUtils.sol#L136C14-L136C33

Summary

Using a wrong hash when depositing into GMX Market will potentially stop all the deposits from GMXVaults, based on GMX’s deposit notes.

https://github.com/gmx-io/gmx-synthetics#deposit-notes

Which states:

• Deposits are not allowed above the MAX_PNL_FACTOR_FOR_DEPOSITS

Vulnerability Details

The vulnerability lies in the fact that when either GMXVault::deposit and GMXVault::rebalanceAdd are called wrong pnlFactor (MAX_PNL_FACTOR_FOR_WITHDRAWALS) will be passed to the oracle function GMXOracle::getLpTokenValue which is intended to fetch the price of the market token when deposit and withdrawal functions are called.

Impact

As you can see in every time when the minimum market slippage amount is calculated pnl factor for withdrawals will be used:

plain text
src: GMXDeposit.sol#L90-L101

if (dp.token == address(self.lpToken)) {
// If LP token deposited
_dc.depositValue = self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
false, //@audit when depositing this should be set to true
false
)
* dp.amt
/ SAFE_MULTIPLIER;
}

plain text
src: GMXOracle.sol#L234-L257

function getLpTokenValue(
address marketToken,
address indexToken,
address longToken,
address shortToken,
bool isDeposit,
bool maximize
) public view returns (uint256) {
bytes32 _pnlFactorType;

if (isDeposit) {
_pnlFactorType = keccak256(abi.encode("MAX_PNL_FACTOR_FOR_DEPOSITS"));
} else {
_pnlFactorType = keccak256(abi.encode("MAX_PNL_FACTOR_FOR_WITHDRAWALS"));
}

(int256 _marketTokenPrice,) = getMarketTokenInfo(
marketToken,
indexToken,
longToken,
shortToken,
_pnlFactorType,
maximize
);

Problem occurs when both MAX_PNL_FACTOR_FOR_DEPOSITS and MAX_PNL_FACTOR_FOR_WITHDRAWALS have different values.

There are 2 possible scenarios:

MAX_PNL_FACTOR_FOR_WITHDRAWALS is less than MAX_PNL_FACTOR_FOR_DEPOSITS

In this case, when the user wants to deposit the maximum allowed amount based on MAX_PNL_FACTOR_FOR_DEPOSITS transaction will most likely revert because there will be a different price of lpToken returned from the GMXOracle called with the pnlFactor = MAX_PNL_FACTOR_FOR_WITHDRAWALS, instead of the one for deposits.

MAX_PNL_FACTOR_FOR_WITHDRAWALS is more than MAX_PNL_FACTOR_FOR_DEPOSITS

In this case, GMXMarket’s Reader contract will return better price of the market token for the user, allowing him to deposit more than the actual value of MAX_PNL_FACTOR_FOR_DEPOSIT.

Tools Used

Recommendations

Change the isDeposit to true argument passed in the following functions: GMXDeposit::deposit and GMXRebalance::rebalanceAdd

diff
if (dp.token == address(self.lpToken)) {
// If LP token deposited
_dc.depositValue = self.gmxOracle.getLpTokenValue(
address(self.lpToken),
address(self.tokenA),
address(self.tokenA),
address(self.tokenB),
-       false,
+       true,
false
)
* dp.amt
/ SAFE_MULTIPLIER;
}