berndartmueller - Decreasing a position without a swap path is susceptible to slippage

[sherlock-admin]

berndartmueller

medium

Decreasing a position without a swap path is susceptible to slippage

Summary

The DecreaseOrderUtils.processOrder function does not check the output amount against the minimum output amount if the order does not have a swap path defined. This could lead to a situation where the user receives fewer tokens than expected, and the provided slippage protection parameter is not respected.

Vulnerability Detail

Decreasing a position and specifying the order swap type SwapCollateralTokenToPnlToken swaps the withdrawn collateral to the PnL token. This swap is performed in DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken while minOutputAmount is set to 0, which means that the swap is not protected against slippage.

Even though users are able to provide a slippage protection parameter in the order (order.minOutputAmount()), this parameter is not used in the DecreaseOrderUtils.processOrder function if the order does not have a swap path (order.swapPath()) defined. This means that users safely assume that their order is protected against slippage, while in this case, it is not.

Impact

Users receive fewer tokens than expected due to slippage even though they provided a slippage protection parameter.

Code Snippet

contracts/order/DecreaseOrderUtils.processOrder(..) - L76-L81

If the order does not have a swap path, the output token is immediately transferred to the receiver. However, the output amount (result.outputAmount) is not checked against the minimum output amount (order.minOutputAmount()) specified in the order. This could lead to a situation where the receiver receives fewer tokens than expected.

020: function processOrder(BaseOrderUtils.ExecuteOrderParams memory params) external {
...      // [...]
037:
038:     DecreasePositionUtils.DecreasePositionResult memory result = DecreasePositionUtils.decreasePosition(
039:         PositionUtils.UpdatePositionParams(
040:             params.contracts,
041:             params.market,
042:             order,
043:             params.key,
044:             position,
045:             positionKey
046:         )
047:     );
048:
...      // [...]
074:
075:     if (order.swapPath().length == 0) {
076:         MarketToken(payable(order.market())).transferOut( // @audit-info Missing slippage protection
077:             result.outputToken,
078:             order.receiver(),
079:             result.outputAmount,
080:             order.shouldUnwrapNativeToken()
081:         );
082:     } else {
083:         try params.contracts.swapHandler.swap(
084:             SwapUtils.SwapParams(
085:                 params.contracts.dataStore,
086:                 params.contracts.eventEmitter,
087:                 params.contracts.oracle,
088:                 Bank(payable(order.market())),
089:                 result.outputToken,
090:                 result.outputAmount,
091:                 params.swapPathMarkets,
092:                 order.minOutputAmount(),
093:                 order.receiver(),
094:                 order.shouldUnwrapNativeToken()
095:             )
...      // [...]
114: }

contracts/position/DecreasePositionUtils.decreasePosition(..) - L281

As the last step of the DecreasePositionUtils.decreasePosition function, the DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken function is called in line 281 to swap the withdrawn collateral tokens to the PnL token. The swap result is returned.

068: function decreasePosition(
069:     PositionUtils.UpdatePositionParams memory params
070: ) external returns (DecreasePositionResult memory) {
...      // [...]
280:
281:     values = DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken(params, values);
282:
283:     return DecreasePositionResult(
284:         values.output.outputToken,
285:         values.output.outputAmount,
286:         values.output.secondaryOutputToken,
287:         values.output.secondaryOutputAmount
288:     );
289: }

contracts/position/DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken(..) - L392

The DecreasePositionCollateralUtils.swapWithdrawnCollateralToPnlToken function swaps the withdrawn collateral tokens to the PnL token without specifying a minOutputAmount in line 392.

375: function swapWithdrawnCollateralToPnlToken(
376:     PositionUtils.UpdatePositionParams memory params,
377:     PositionUtils.DecreasePositionCollateralValues memory values
378: ) external returns (PositionUtils.DecreasePositionCollateralValues memory) {
379:     if (params.order.decreasePositionSwapType() == Order.DecreasePositionSwapType.SwapCollateralTokenToPnlToken) {
380:         Market.Props[] memory swapPathMarkets = new Market.Props[](1);
381:         swapPathMarkets[0] = params.market;
382:
383:         try params.contracts.swapHandler.swap(
384:             SwapUtils.SwapParams(
385:                 params.contracts.dataStore,
386:                 params.contracts.eventEmitter,
387:                 params.contracts.oracle,
388:                 Bank(payable(params.market.marketToken)),
389:                 params.position.collateralToken(), // tokenIn
390:                 values.output.outputAmount, // amountIn
391:                 swapPathMarkets, // markets
392:                 0, // minOutputAmount // @audit-info no slippage protection
393:                 params.market.marketToken, // receiver
394:                 false // shouldUnwrapNativeToken
395:             )
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: }

Tool used

Manual Review

Recommendation

Consider adding a check in the DecreaseOrderUtils.processOrder function in line 76 to ensure result.outputAmount > order.minOutputAmount().

Duplicate of #138