hansfriese - `rebalanceLite` should provide a slippage protection

[github-actions[bot]]

hansfriese

medium

rebalanceLite should provide a slippage protection

Summary

Users can lose funds while rebalancing.

Vulnerability Detail

The protocol provides two kinds of rebalancing functions - rebalance() and rebalanceLite(). While the function rebalance() is protected from an unintended slippage because the caller can specify amountOutMinimum, rebalanceLite() does not have this protection. This makes the user vulnerable to unintended slippage due to various scenarios.

PerpDepository.sol
597:     function rebalanceLite(
598:         uint256 amount,
599:         int8 polarity,
600:         uint160 sqrtPriceLimitX96,
601:         address account
602:     ) external nonReentrant returns (uint256, uint256) {
603:         if (polarity == -1) {
604:             return
605:                 _rebalanceNegativePnlLite(amount, sqrtPriceLimitX96, account);
606:         } else if (polarity == 1) {
607:             // disable rebalancing positive PnL
608:             revert PositivePnlRebalanceDisabled(msg.sender);
609:             // return _rebalancePositivePnlLite(amount, sqrtPriceLimitX96, account);
610:         } else {
611:             revert InvalidRebalance(polarity);
612:         }
613:     }
614:
615:     function _rebalanceNegativePnlLite(
616:         uint256 amount,
617:         uint160 sqrtPriceLimitX96,
618:         address account
619:     ) private returns (uint256, uint256) {
620:         uint256 normalizedAmount = amount.fromDecimalToDecimal(
621:             ERC20(quoteToken).decimals(),
622:             18
623:         );
624:
625:         _checkNegativePnl(normalizedAmount);
626:         IERC20(quoteToken).transferFrom(account, address(this), amount);
627:         IERC20(quoteToken).approve(address(vault), amount);
628:         vault.deposit(quoteToken, amount);
629:
630:         bool isShort = false;
631:         bool amountIsInput = true;
632:         (uint256 baseAmount, uint256 quoteAmount) = _placePerpOrder(
633:             normalizedAmount,
634:             isShort,
635:             amountIsInput,
636:             sqrtPriceLimitX96
637:         );
638:         vault.withdraw(assetToken, baseAmount);
639:         IERC20(assetToken).transfer(account, baseAmount);
640:
641:         emit Rebalanced(baseAmount, quoteAmount, 0);
642:
643:         return (baseAmount, quoteAmount);
644:     }

Especially, according to the communication with the PERP dev team, it is possible for the Perp's ClearingHouse to fill the position partially when the price limit is specified (sqrtPriceLimitX96). It is also commented in the Perp contract comments here.

63:     /// @param sqrtPriceLimitX96 tx will fill until it reaches this price but WON'T REVERT
64:     struct InternalOpenPositionParams {
65:         address trader;
66:         address baseToken;
67:         bool isBaseToQuote;
68:         bool isExactInput;
69:         bool isClose;
70:         uint256 amount;
71:         uint160 sqrtPriceLimitX96;
72:     }

So it is possible that the order is not placed to the full amount. As we can see in the #L626~#L628, the UXD protocol grabs the quote token of amount and deposits to the Perp's vault. And the unused amount will remain in the Perp vault while this is supposed to be returned to the user who called this rebalance function.

Impact

Users can lose funds while lite rebalancing.

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L597

Tool used

Manual Review

Recommendation

Add a protection parameter to the function rebalanceLite() so that the user can specify the minimum out amount.

[WarTech9]

The caller can specify the target price using the sqrtPriceLimitX96 parameter to the rebalanceLite function. This offers slippage protection.

[hansfriese]

Escalate for 10 USDC

I suggest the judge and the sponsor read this issue carefully again. The key problem is that the Perp protocol can partially fill the position, especially when the sqrtPriceLimitX96 is specified. (This is related to how Uniswap works, check here) So it is possible that the order is not placed to the full amount and the remaining amount should be returned to the user. I admit that my explanation sounded vague because I mentioned slippage. I mean, the protocol should return the remaining value or allow the user to explicitly specify the minimum output amount. Please check the screenshot of the chat I had with the Perp team. They confirmed it is possible that the order is not filled to the full amount when the sqrtPriceLimitX96 is specified.

[sherlock-admin]

Escalate for 10 USDC

I suggest the judge and the sponsor read this issue carefully again. The key problem is that the Perp protocol can partially fill the position, especially when the sqrtPriceLimitX96 is specified. (This is related to how Uniswap works, check here) So it is possible that the order is not placed to the full amount and the remaining amount should be returned to the user. I admit that my explanation sounded vague because I mentioned slippage. I mean, the protocol should return the remaining value or allow the user to explicitly specify the minimum output amount. Please check the screenshot of the chat I had with the Perp team. They confirmed it is possible that the order is not filled to the full amount when the sqrtPriceLimitX96 is specified.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WarTech9]

With the updated comments I agree there is a valid issue here. If the requested amount is not filled completely in the Perp order, we should only transfer from the user the amount returned from the _placePerpOrder() call.

[hrishibhat]

Escalation accepted.

As pointed out in the Escalation that there is the possibility of partial order fills when sqrtPriceLimitX96 is specified.

[sherlock-admin]

Escalation accepted.

As pointed out in the Escalation that there is the possibility of partial order fills when sqrtPriceLimitX96 is specified.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

[WarTech9]

Fixed here by removing sqrtPriceLimitX96 in swap when rebalancing https://github.com/UXDProtocol/uxd-evm/pull/24

[IAm0x52]

Fixed in this PR

Replaces sqrtPriceLimitX96 with minAmountOut and enforces it as slippage protection