float-audits - Price impact parameters could lead to risk-free profit opportunities for users

[sherlock-admin]

float-audits

high

Price impact parameters could lead to risk-free profit opportunities for users

Summary

Incorrect relationship between two admin parameters could lead to risk-free profit opportunities for users. NB: the finding is not about specific values for admin parameters, but rather the relationship between two admin parameters that are set.

Vulnerability Detail

J_0 is for dataStore.getUint(Keys.positionImpactFactorKey(market, hasPositiveImpact=false))

J_1 is for dataStore.getUint(Keys.positionImpactFactorKey(market, hasPositiveImpact=true))

This graph shows that, if J_1 < J_0 then a user can do first: a long market increase order; and second: another long market increase order and get a guaranteed increase in execution price.

NB: see Desmos graphs for mathematical proof https://www.desmos.com/calculator/zgoe2nqbwe

Impact

A user has 100% guarantee of profit if going long if (J_0 - J_1) (defined above) is positive.

Hence a malicious actor could use very large positions to make a risk-free profit and drain funds from other users' balances.

Code Snippet

LoC: https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/pricing/PositionPricingUtils.sol#L228-L261

Tool used

Manual Review

Recommendation

Recommendation here would be to force J_0 < J_1

[IllIllI000]

@xvi10 Can you elaborate on the risk free profit? I had marked this one as invalid because increasing long is expected to increase the entry price due to the impact, and in order to have a profit you need to exit at a higher price, and I couldn't see how changing the entry price affects the exit price, but thought I'd raise it with you just in case.

[xvi10]

if the positivePriceImpact multiplier is larger than the negativePriceImpact multiplier, then opening + closing a position would lead to a profit assuming price does not change much

potentially positivePriceImpact multiplier < negativePriceImpact multiplier could be validated in the Config contract

[IllIllI000]

@xvi10 isn’t this issue concerned with the case where negative impact is larger than positive (J_1[hasPositiveImpact=true] < J_0[hasPositiveImpact=false])?

[xvi10]

yes I think you're right, the idea is correct but the sign is flipped in the report

[IllIllI000]

@xvi10 to me it looks like more than just a mistake in flipping the sign of the inequality – if you look at the Desmos graphs, they show two price increases after two order, and are claiming that the price increase is the issue, rather than showing an increase followed by an exit. I don’t see how two increases would be relevant as opposed to just one. Do you agree that the confirmed tag should be removed?

[xvi10]

ok, will dispute it since the flipped sign makes the report invalid