Medium Risk
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
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
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.
As you can see in every time when the minimum market slippage amount is calculated pnl factor for withdrawals will be used:
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;
}
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
.
Change the isDeposit to true argument passed in the following functions: GMXDeposit::deposit
and GMXRebalance::rebalanceAdd
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;
}