0xGoodess - short side of getReservedUsd does not work for market that has the same collateral token

[sherlock-admin]

0xGoodess

medium

short side of getReservedUsd does not work for market that has the same collateral token

Summary

short side of getReservedUsd does not work for market that has the same collateral token

Vulnerability Detail

Consider the case of ETH / USD market with both long and short collateral token as ETH.

the available amount to be reserved (ETH) would CHANGE with the price of ETH.

    function getReservedUsd(
        DataStore dataStore,
        Market.Props memory market,
        MarketPrices memory prices,
        bool isLong
    ) internal view returns (uint256) {
        uint256 reservedUsd;
        if (isLong) {
            // for longs calculate the reserved USD based on the open interest and current indexTokenPrice
            // this works well for e.g. an ETH / USD market with long collateral token as WETH
            // the available amount to be reserved would scale with the price of ETH
            // this also works for e.g. a SOL / USD market with long collateral token as WETH
            // if the price of SOL increases more than the price of ETH, additional amounts would be
            // automatically reserved
            uint256 openInterestInTokens = getOpenInterestInTokens(dataStore, market, isLong);
            reservedUsd = openInterestInTokens * prices.indexTokenPrice.max;
        } else {
            // for shorts use the open interest as the reserved USD value
            // this works well for e.g. an ETH / USD market with short collateral token as USDC
            // the available amount to be reserved would not change with the price of ETH
            reservedUsd = getOpenInterest(dataStore, market, isLong);
        }

        return reservedUsd;
    }

Impact

reservedUsd does not work when long and short collateral tokens are the same.

Code Snippet

https://github.com/sherlock-audit/2023-04-gmx/blob/main/gmx-synthetics/contracts/market/MarketUtils.sol#L1415-L1439

Tool used

Manual Review

Recommendation

Consider apply both long and short calculations of reserveUsd with relation to the indexTokenPrice.

[IllIllI000]

leaning towards invalid, but will let sponsor verify

[xvi10]

it would not be advisable to use non-stablecoins to back short positions

in case a non-stablecoin is used to back short positions, the amount to be reserved may not need to be changed since the reserve ratio should still validate if the total open interest is a reasonable ratio of the pool's USD value

[IllIllI000]

@xvi10 inadvisable, and the stipulation that open interest be a 'reasonable ratio' seem to indication that this bug is possible, and should therefore remain open - am I misunderstanding what you've said

[xvi10]

the issue does not seem valid to me and the current contract code seems reasonable, an example:

1. there is $50m worth of ETH in the pool and the reserve factor is set to 0.25
2. a max of $12.5m shorts can be opened
3. when validating the reserve we check that the max short open interest does not exceed this
4. if the price of ETH decreases and is now worth $40m, the validation would be that the max open interest does not exceed $10m

the pending pnl of the short positions would increase if the price of ETH decreases, which is a problem with choosing to use a non-stablecoin to back short positions rather than an issue with the validation

in that case the cap of trader pnl and ADL could help to reduce the risk of the market becoming insolvent, but it would be better to avoid the situation by not using a non-stablecoin to back short positions

[IllIllI000]

which is a problem with choosing to use a non-stablecoin to back short positions rather than an issue with the validation

I can see that argument, but you seem to be indicating that users are allowed to do it anyway, and markets becoming insolvent seems like a situation that should be prevented. I'll let Sherlock decide

[xvi10]

markets have a risk of becoming insolvent, capping trader pnl and ADL helps, reducing the risk of insolvency is left up to the market creator to configure the backing tokens and parameters

[hrishibhat]

Considering this issue as a valid medium based on the above comments that there is a possibility of market insolvency