panprog - Invalid oracle version can cause the vault to open too large and risky position and get liquidated due to using unadjusted global current position

[sherlock-admin2]

panprog

medium

Invalid oracle version can cause the vault to open too large and risky position and get liquidated due to using unadjusted global current position

Summary

The fix to issue 49 of the main contest introduced new invalidation system, which stores invalidation accumulator for all positions. This means that market.pendingPosition() returns unadjusted global position which might be completely wrong.

The problem is that Vault (StrategyLib) uses market.pendingPosition(global.currentId) without adjusting it, which leads to incorrect current global position right after invalid oracle version (which creates different invalidation values for latest and current positions). This incorrect global position can lead to inflated position limits enforced in the market and vault opening too large risky position with very high leverage, which might liquidate the vault leading to loss of funds for vault users.

Vulnerability Detail

StrategyLib._loadContext for the market loads currentPosition as:

context.currentPosition = registration.market.pendingPosition(global.currentId);

However, this is unadjusted position, so its value is incorrect if invalid oracle version happens while this position is pending.

Later on, when calculating minimum and maxmium positions enforced by the vault in the market, they're calculated in _positionLimit:

function _positionLimit(MarketContext memory context) private pure returns (UFixed6, UFixed6) {
    return (
        // minimum position size before crossing the net position
        context.currentAccountPosition.maker.sub(
            context.currentPosition.maker
                .sub(context.currentPosition.net().min(context.currentPosition.maker))
                .min(context.currentAccountPosition.maker)
                .min(context.closable)
        ),
        // maximum position size before crossing the maker limit
        context.currentAccountPosition.maker.add(
            context.riskParameter.makerLimit
                .sub(context.currentPosition.maker.min(context.riskParameter.makerLimit))
        )
    );
}

And the target maker size for the market is set in allocate:

(targets[marketId].collateral, targets[marketId].position) = (
    Fixed6Lib.from(_locals.marketCollateral).sub(contexts[marketId].local.collateral),
    _locals.marketAssets
        .muldiv(registrations[marketId].leverage, contexts[marketId].latestPrice.abs())
        .min(_locals.maxPosition)
        .max(_locals.minPosition)
);

Since context.currentPosition is incorrect, it can happen that both _locals.minPosition and _locals.maxPosition are too high, the vault will open too large and risky position, breaking its risk limit and possibly getting liquidated, especially if it happens during high volatility.

Impact

If invalid oracle version happens, the vault might open too large and risky position in such market, potentially getting liquidated and vault users losing funds due to this liquidation.

Code Snippet

StrategyLib._loadContext loads current global position without adjusting it, meaning context.currentPosition is incorrect if invalid oracle version happens: https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/lib/StrategyLib.sol#L131

This leads to incorrect position limit calculations: https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/lib/StrategyLib.sol#L172-L187

This, in turn, leads to incorrect target vault's position calculation for the market: https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/lib/StrategyLib.sol#L93-L99

Tool used

Manual Review

Recommendation

Adjust global current position after loading it:

    context.currentPosition = registration.market.pendingPosition(global.currentId);
+   context.currentPosition.adjust(registration.market.position());

[kbrizzle]

Fixed in: https://github.com/equilibria-xyz/perennial-v2/pull/109.

Please note there were additional pending positions that required adjustment.

[panprog]

Fixed