Description

In the updateAccTotalPnl function, the total accumulated PNL is tracked and readjusted for the current oracle price from the feed:

function updateAccTotalPnl(
    int256 oraclePrice,
    uint256 openPrice,
    uint256 oiNotional,
    uint16 pairIndex,
    bool buy,
    bool open
) external {
    // [...]
    if (open) {
        accTotalPnl -= oiNotionalSigned * (int256(openPrice) - oraclePrice) / PRECISION_18;
    } else {
        accClosedPnl -= oiNotionalSigned * (int256(openPrice) - oraclePrice) / PRECISION_18;
    }

    if (lastTradePrice[pairIndex] != 0) {
        accTotalPnl += (oraclePrice - lastTradePrice[pairIndex]) * accNetOiUnits[pairIndex] / PRECISION_18;
    }
    lastTradePrice[pairIndex] = oraclePrice;
    // [...]
}

The price obtained from the oracle cannot be zero. However, of the four places this function is called, inside OstiumTradingCallbacks, two are during a trade open and two are during a trade close, and the ones that are during a trade close pass in a price-impact-adjusted price, instead of the direct oracle price, as the first parameter:

function openTradeMarketCallback( /* [...] */ ) {
    IOstiumOpenPnl(registry.getContractAddress('openPnl')).updateAccTotalPnl(
        int256(a.price),
        trade.openPrice,
        // [...]

function closeTradeMarketCallback( /* [...] */ ) {
    IOstiumOpenPnl(registry.getContractAddress('openPnl')).updateAccTotalPnl(
        int256(priceAfterImpact),
        t.openPrice, 
        // [...]

function executeAutomationOpenOrderCallback( /* [...] */ ) {
    IOstiumOpenPnl(registry.getContractAddress('openPnl')).updateAccTotalPnl(
        a.price,
        finalTrade.openPrice,
        // [...]

function executeAutomationCloseOrderCallback( /* [...] */ ) {
    IOstiumOpenPnl(registry.getContractAddress('openPnl')).updateAccTotalPnl(
        int256(priceAfterImpact),
        t.openPrice,
        // [...]

This means that it is possible that the price impact makes the first parameter actually zero. In this case, the lastTradePrice for the pair will be set to zero, and then the next call will fail to adjust the accTotalPnl to the new price.

Impact

In the event the price impact makes a trade execute at zero, accTotalPnl for that pair will become permanently offset by an amount.

Additionally, the accTotalPnl will fluctuate depending on whether the last-executed order was an open or close. This is concerning because it, through the averaging and the epochs, is what informs the OstiumVault about the funds to ensure collateralization. A malicious party could ensure that the last-executed order was a close with high price impact each time a snapshot is taken, manipulating the PNL seen by the OstiumVault.

Recommendations

We recommend just removing the conditional and executing the line inside the if statement unconditionally. It seems like the conditional is intended to treat the first time a pair is traded as a special case, since in that case, all the relevant storage variables are uninitialized and zero. However, for this special setup case, accNetOiUnits is also zero, so this line will not change accTotalPnl. The logic for the typical case works for this special case because there are zero net OI units outstanding in this special case, and so the last trade price does not matter.

For the second part, we recommend either passing the oracle price into this function so that the basis of accTotalPnl is always the oracle price, or calculating the total PNL in getOpenPnl so that would be determined on demand. The latter method greatly simplifies the business logic, removing a way for a bug to cause a permanent offset in the quantity.

Remediation

This issue has been acknowledged by Ostium Labs, and fixes were implemented in the following commits:

• 15810556↗
• 7ad7a691↗