When maintaining a position in PerpMarketV1, users are not allowed to deposit extra margin to prevent their position from being liquidated.

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/perp/PerpMarketV1.sol#L128

Vulnerability details

Impact

When maintaining a position in PerpMarketV1, there "extra" margin is automatically withdrawed and transferred to users. This means users are not allowed to deposit extra margin to prevent their position from being liquidated.

Bug Description

During the predyTradeAfterCallback of PerpMarketV1, the required margin is calculated by _calculateInitialMargin. If the value is negative, this means there is extra margin in the vault. However, the extra margin is always withdrawn and transferred back to users by _predyPool.take(true, callbackData.trader, uint256(-marginAmountUpdate));.

This means no extra margin is allowed in PerpMarketV1, with the lowest leverage being leverage == 1. Let's discuss both the long and short case:

Long scenario.

1. A user longs N baseToken with leverage == 2 at price X. The _calculateInitialMargin() (which returns the initial deposit amount of margin) is calculated as (netValue / leverage).toInt256() - _calculatePositionValue(vault, sqrtPrice), which is NX/2 - 0 == NX/2. After deposit, the vault.margin is NX/2, and the liquidation price is X/2.
2. The price rises to 2X. Now, if the user toggles with the position again, the _calculateInitialMargin() would return 2NX/2 - (NX + NX/2) == -NX/2. The NX is PositionCalculator.calculateValue(sqrtPrice, PositionCalculator.getPosition(vault.openPosition)) and the NX/2 is the vault.margin. This means an amount of NX/2 margin is returned to the user, and this would raise the liquidation price, which is unexpected.

The issue here is the position is not allowed to store extra margin, and when the price raises, margin would be withdrawn, and the liquidation price would rise.

Short scenario

1. A user shorts N baseToken with leverage == 2 at price X. Similar to the long scenario, the initial deposited amount of margin would be NX/2, and the liquidation price would be 1.5X.
2. The price falls to 0.5X. Now, if the user toggles with the position again, the _calculateInitialMargin() would return 0.5NX/2 - (0.5NX + NX/2) == -0.75NX. The 0.5NX is PositionCalculator.calculateValue(sqrtPrice, PositionCalculator.getPosition(vault.openPosition)) and the NX/2 is the vault.margin. This means an amount of 0.75NX margin is returned to the user, and this would lower the liquidation price, which is unexpected.

Also, for the short scenario, since the minimum leverage is 1, there is no way to create a short position where the price is X and the liquidation price is above 2X (we can't create a short position with leverage smaller than 1, e.g. 0.5).

PerpMarketV1.sol

    function predyTradeAfterCallback(
        IPredyPool.TradeParams memory tradeParams,
        IPredyPool.TradeResult memory tradeResult
    ) external override(BaseHookCallbackUpgradable) onlyPredyPool {
        CallbackData memory callbackData = abi.decode(tradeParams.extraData, (CallbackData));
        ERC20 quoteToken = ERC20(_getQuoteTokenAddress(tradeParams.pairId));

        if (callbackData.callbackSource == CallbackSource.QUOTE) {
            _revertTradeResult(tradeResult);
        } else if (callbackData.callbackSource == CallbackSource.QUOTE3) {
            _revertTradeResult(tradeResult);
        } else if (callbackData.callbackSource == CallbackSource.TRADE3) {
            int256 marginAmountUpdate =
                _calculateInitialMargin(tradeParams.vaultId, tradeParams.pairId, callbackData.leverage);

            uint256 cost = 0;

            if (marginAmountUpdate > 0) {
                cost = uint256(marginAmountUpdate);
            }

            _verifyOrderV3(callbackData.resolvedOrder, cost);

            if (marginAmountUpdate > 0) {
                quoteToken.safeTransfer(address(_predyPool), uint256(marginAmountUpdate));
            } else if (marginAmountUpdate < 0) {
>               _predyPool.take(true, callbackData.trader, uint256(-marginAmountUpdate));
            }

            ...
        }
    }

    function _calculateInitialMargin(uint256 vaultId, uint256 pairId, uint256 leverage)
        internal
        view
        returns (int256)
    {
        DataType.Vault memory vault = _predyPool.getVault(vaultId);

        uint256 sqrtPrice = _predyPool.getSqrtIndexPrice(pairId);

        uint256 price = Math.calSqrtPriceToPrice(sqrtPrice);

        uint256 netValue = _calculateNetValue(vault, price);

>       return (netValue / leverage).toInt256() - _calculatePositionValue(vault, sqrtPrice);
    }

    function _calculateNetValue(DataType.Vault memory vault, uint256 price) internal pure returns (uint256) {
        int256 positionAmount = vault.openPosition.perp.amount;

        return Math.abs(positionAmount) * price / Constants.Q96;
    }

    function _calculatePositionValue(DataType.Vault memory vault, uint256 sqrtPrice) internal pure returns (int256) {
        return PositionCalculator.calculateValue(sqrtPrice, PositionCalculator.getPosition(vault.openPosition))
            + vault.margin;
    }

Proof of Concept

Presented above.

Tools Used

Manual review

Recommended Mitigation Steps

Add a function to manually deposit and withdraw margin from the vault (the vault should be solvent after withdraw) instead of automatically withdrawing margin.

Assessed type

Other

[alex-ppg]

The submission appears to not understand what the code precisely executes, and was marked by the validators as unsatisfactory.

[c4-judge]

alex-ppg marked the issue as unsatisfactory:
Invalid

[pkqs90]

Hey @alex-ppg, I don't quite understand why this issue is stated as The submission appears to not understand what the code precisely executes. The submission shows how PerpMarketV1.sol#predyTradeAfterCallback() function does not allow user to deposit more margin than 1x leverage, and describes scenarios why users should be freely allowed to any amount of margin they want, to avoid being liquidated.

To make a comparison, GammaTradeMarket allows users to deposit margin freely https://github.com/code-423n4/2024-05-predy/blob/main/src/markets/gamma/GammaTradeMarket.sol#L115-L121. (The marginAmountUpdate is passed in by users).

I think freely depositing margin is a very common feature for perp trades, e.g. all the CEX allows this.

[alex-ppg]

This report lacked Sponsor input due to being invalidated during the validation round. I explicitly requested the Sponsor to provide input to this and several other exhibits so as to provide a fine-tuned judgment, and my earlier judgment was based on validation.

I will invite the Sponsor to review this submission, but I believe your latest response summarizes what this issue is about. You label this as a common feature, meaning that it is not a security vulnerability but rather a missing feature that would render it a QA recommendation.

[syuhei176]

It is PerpMarket's specification that users can only specify leverage. Therefore, there is no problem.