IllIllI - Collateral tokens that cannot be automatically swapped to the PnL token, cannot have slippage applied to them

[sherlock-admin]

IllIllI

medium

Collateral tokens that cannot be automatically swapped to the PnL token, cannot have slippage applied to them

Summary

Collateral tokens that cannot be automatically swapped to the PnL token, cannot have slippage applied to them, since the minOutputAmount is in units of the output token, not the secondary token.

Vulnerability Detail

If a user's order uses the Order.DecreasePositionSwapType.SwapCollateralTokenToPnlToken flag, it's possible for the swap to fail (e.g. because the token is paused), and in such cases, the collateral token is sent back as-is, without being converted to the PnL token. In such cases, it's not possible for the code, as it is written, to support slippage in such scenarios, because there is only one order slippage argument, minOutputAmount, and it's in units of the PnL token, not the collateral token.

Impact

A user that has a resting order open with the flag set, so that they can take profit at the appropriate time, will be forced to incur any price impact slippage present, even if they had specified a valid minOutputAmount that would otherwise have prevented the sub-optimal execution.

Code Snippet

If the swap goes through, the secondaryOutputAmount is cleared and added to the outputAmount, but if the swap fails, it's kept as the values.output.secondaryOutputAmount:

// File: gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol : DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken()   #1

383                try params.contracts.swapHandler.swap(
...
396                ) returns (address tokenOut, uint256 swapOutputAmount) {
397                    if (tokenOut != values.output.secondaryOutputToken) {
398                        revert InvalidOutputToken(tokenOut, values.output.secondaryOutputToken);
399                    }
400                    // combine the values into outputToken and outputAmount
401                    values.output.outputToken = tokenOut;
402 @>                 values.output.outputAmount = values.output.secondaryOutputAmount + swapOutputAmount;
403                    values.output.secondaryOutputAmount = 0;
404                } catch Error(string memory reason) {
405 @>                 emit SwapUtils.SwapReverted(reason, "");
406                } catch (bytes memory reasonBytes) {
407                    (string memory reason, /* bool hasRevertMessage */) = ErrorUtils.getRevertMessage(reasonBytes);
408 @>                 emit SwapUtils.SwapReverted(reason, reasonBytes);
409                }
410            }
411    
412            return values;
413:       }

https://github.com/sherlock-audit/2023-02-gmx/blob/main/gmx-synthetics/contracts/position/DecreasePositionCollateralUtils.sol#L383-L413

And is sent separately, with no slippage checks.

Tool used

Manual Review

Recommendation

Convert the USD value of secondaryOutputAmount to outputAmount, and ensure that the slippage checks against that total

[xvi10]

Fix in https://github.com/gmx-io/gmx-synthetics/pull/108