joestakey - Variable subtracted where it should be assigned leads to `getAdjustedLongAndShortTokenAmounts()` reverting.

[sherlock-admin]

joestakey

medium

Variable subtracted where it should be assigned leads to getAdjustedLongAndShortTokenAmounts() reverting.

Summary

adjustedLongTokenAmount is subtracted where it should be initialized, leading to the call reverting with an underflow error, which means executeDeposit will revert in some cases.

Vulnerability Detail

The function calculates the adjusted token amounts that would minimize the price impact. In the poolLongTokenAmount >= poolShortTokenAmount block, there is a subtraction where an assignment should be line 406

File: contracts/deposit/ExecuteDepositUtils.sol
389:       uint256 adjustedLongTokenAmount;
390:         uint256 adjustedShortTokenAmount;
391: 
392:         if (poolLongTokenAmount < poolShortTokenAmount) { 
393:             uint256 diff = poolLongTokenAmount - poolShortTokenAmount;
394: 
395:             if (diff < poolLongTokenAmount) {
396:                 adjustedLongTokenAmount = diff + (longTokenAmount - diff) / 2;
397:                 adjustedShortTokenAmount = longTokenAmount - adjustedLongTokenAmount;
398:             } else {
399:                 adjustedLongTokenAmount = longTokenAmount;
400:             }
401:         } else {
402:             uint256 diff = poolShortTokenAmount - poolLongTokenAmount;
403: 
404:             if (diff < poolShortTokenAmount) {
405:                 adjustedShortTokenAmount = diff + (longTokenAmount - diff) / 2;
406:                 adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount;//@audit should be = 
407:             } else {
408:                 adjustedLongTokenAmount = 0;
409:                 adjustedShortTokenAmount = longTokenAmount;
410:             }
411:         }

This line below will always revert, as adjustedLongTokenAmount is not initialized, meaning adjustedLongTokenAmount - longTokenAmount will underflow.

 adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount;

Note: currently the call reverts beforehand because of another error (handling of the poolLongTokenAmount < poolShortTokenAmount cases), which is detailed in another report. Once you have fixed it, the call will revert line 406 because of the problem described in this report.

Impact

getAdjustedLongAndShortTokenAmounts() always reverts in the case where poolLongTokenAmount >= poolShortTokenAmount for markets where market.longToken == market.shortToken.

Code Snippet

https://github.com/gmx-io/gmx-synthetics/blob/7be3ef2d119d9e84473e1a49f346bcdc06fd57a3/contracts/deposit/ExecuteDepositUtils.sol#L392-L402

Tool used

Manual Review

Recommendation

-406:    adjustedLongTokenAmount - longTokenAmount - adjustedShortTokenAmount;
+406:    adjustedLongTokenAmount = longTokenAmount - adjustedShortTokenAmount;

Duplicate of #164