0xGoodess - short side of getReservedUsd does not work for market that has the same collateral token
Opened this issue · 7 comments
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
Tool used
Manual Review
Recommendation
Consider apply both long and short calculations of reserveUsd with relation to the indexTokenPrice.
leaning towards invalid, but will let sponsor verify
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
@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
the issue does not seem valid to me and the current contract code seems reasonable, an example:
- there is $50m worth of ETH in the pool and the reserve factor is set to 0.25
- a max of $12.5m shorts can be opened
- when validating the reserve we check that the max short open interest does not exceed this
- 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
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
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
Considering this issue as a valid medium based on the above comments that there is a possibility of market insolvency