Description

The function updateMargin allows users to deposit or withdraw USDC from their open positions. When the function is called to withdraw USDC, it first verifies if the new leverage lies in the correct range, and then, if it does, passes control to the fulfill function in PriceAggregator.

If the value of useBackupOnly is set to true in PriceAggregator, the value of price would be zero and it will be pushed to the answers array. This function would then call the callback function updateMarginCallback with the a.price value as zero.

function updateMarginCallback(
    AggregatorAnswer memory a
) external override onlyPriceAggregator {
    //...
        int profitP = _currentPercentProfit(_trade.openPrice,
            a.price, _trade.buy, _trade.leverage);
        int pnl = (int(_trade.initialPosToken) * profitP)
            / int(_PRECISION) / 100;
        if (pnl < 0) {
            pnl = (
                pnl * int(
                    aggregator.pairsStorage()
                    .lossProtectionMultiplier(_trade.pairIndex, o.tier)
                )
            ) / 100;
        }
        require((int(_trade.initialPosToken) + pnl) >
            (int(_trade.initialPosToken) *
                int(100 - _WITHDRAW_THRESHOLD_P))
            / 100, "WITHDRAW_THRES_BREACHED");
    }
    storageT.vaultManager().sendUSDCToTrader(_trade.trader, o.amount);
    //...
}

function _currentPercentProfit(
    uint openPrice,
    uint currentPrice,
    bool buy,
    uint leverage
) private returns (int p) {
    int diff = buy ?
        (int(currentPrice) - int(openPrice)) :
        (int(openPrice) - int(currentPrice));
    int minPnlP = int(_PRECISION) * (-100);
    int maxPnlP = int(_MAX_GAIN_P) * int(_PRECISION);
    p = (diff * 100 * int(_PRECISION.mul(leverage))) / int(openPrice);
    p = p < minPnlP ? minPnlP : p > maxPnlP ? maxPnlP : p;
}

If the original trade was a short trade, the returned value of profitP would be 100 * int(_PRECISION.mul(leverage)) (max capped to 900%). Therefore, even if the real price is higher (which means the short suffered a loss), the trader can still withdraw their tokens as if the price actually hit zero.

Impact

Users can withdraw much more tokens from their position than they should be allowed to.

Recommendations

The other callback functions delete the pending order and return without making any changes in the trades if the value of a.price is zero. The same check here would prevent this issue.

Alternatively, if Finding ref↗ is resolved by making cancelled orders revert, consider reverting in PriceAggregator instead of calling any callbacks when the price is set to zero because it is not valid.

Remediation

This issue has been acknowledged by Avantis Labs, Inc., and a fix was implemented in commit a445138c↗.