When product is closed, User can withdraw stagnant PnL from the product

Summary

A product can be closed and reopened through updateClosedby market owner in Product.sol.

When a product is closed, users can still deposit collaterals and withdraw collaterals and PnL from the product. This might be problematic because this allows stagnant PnL to be withdrawn regardless of the current asset price.

Vulnerability Detail

In the protocol, user's PnL changes with the current price and product market conditions. A user will accumulate gain or loss over time without changing their positions. It's intended behavior that depending on when user update their position or settleAccount, oracle difference at that time will be factored in global accumulated position values, which impact the user's accumulated PnL. This is implemented in _settle(),accumulate() ,_accumulatePosition().

//Product.sol-_settle()
        UFixed18 accumulatedFee = _accumulator.accumulate(
            boundedFundingFee,
            _position,
            latestOracleVersion,
            settleOracleVersion
        );

//VersionedAccumulator.sol-accumulate()
     Accumulator memory accumulatedPositionPnl = _accumulatePosition(
            latestPosition,
            latestOracleVersion,
            toOracleVersion
        );

//VersionedAccumulator.sol-_accumulatePosition()
    function _accumulatePosition(
        Position memory latestPosition,
        IOracleProvider.OracleVersion memory latestOracleVersion,
        IOracleProvider.OracleVersion memory toOracleVersion
    ) private view returns (Accumulator memory accumulatedPosition) {   
|>       if (_product().closed() || latestPosition.taker.isZero() || latestPosition.maker.isZero())
            return accumulatedPosition;
        Fixed18 oracleDelta = toOracleVersion.price.sub(latestOracleVersion.price);
|>      Fixed18 totalTakerDelta = oracleDelta.mul(Fixed18Lib.from(latestPosition.taker));
        Fixed18 socializedTakerDelta = totalTakerDelta.mul(Fixed18Lib.from(latestPosition.socializationFactor()));
        accumulatedPosition.maker = socializedTakerDelta.div(Fixed18Lib.from(latestPosition.maker)).mul(
            Fixed18Lib.NEG_ONE
        );
        accumulatedPosition.taker = socializedTakerDelta.div(Fixed18Lib.from(latestPosition.taker));
    }

However, when a product is closed, global accumulatedPosition values will stop updating which is implemented through if(_product().closed().... Now user's account PnL will become stagnant, not updating based on the current price oracle. Since user PnL is stagnant, it should be considered unfair for users to withdraw a stagnant value at any time they choose after product is closed, because the current market price at the time of withdrawal will not be factored in.

In Collateral. sol, withdrawal from stagnant PnL will be possible through withdrawFrom() without reverting when a product is closed. And since collateral deposited is co-mingled with PnL in the product optimisticLedger, the user can withdraw accumulated PnL directly.

//Collateral.sol
    function withdrawFrom(
        address account,
        address receiver,
        IProduct product,
        UFixed18 amount
    )
        public
        nonReentrant
        notPaused
        notZeroAddress(receiver)
        isProduct(product)
        onlyAccountOrMultiInvoker(account)
        settleForAccount(account, product) 
        collateralInvariant(account, product)
        maintenanceInvariant(account, product)
    {
        amount = amount.eq(UFixed18Lib.MAX) ? collateral(account, product) : amount;
  |>      _products[product].debitAccount(account, amount);
        token.push(receiver, amount);

        emit Withdrawal(account, product, amount);
    }

//OptimisticLedger.sol
    function debitAccount(OptimisticLedger storage self, address account, UFixed18 amount) internal {
|>        self.balances[account] = self.balances[account].sub(amount);
        self.total = self.total.sub(amount);
    }

Impact

Under normal conditions, users withdraw PnL with current oracle price factored in, however, when user is able to withdraw over a specific old price, this leads to unfair PnL gain for one side and unfair PnL loss for the other side.

Code Snippet

https://github.com/equilibria-xyz/perennial-mono/blob/b06d5145db62a312dd88dfcafef0f8e2166c5e32/packages/perennial/contracts/collateral/Collateral.sol#L84-L99

Tool used

Manual Review

Recommendation

(1) When a product is closed, consider disable deposit and withdraw for that product, preventing user depositing collaterals and withdrawing collateral and PnL until reopening. (2) OR consider separate account PnL storage from collateral storage, and when product is closed, pause PnL withdrawal.

sherlock-audit / 2023-05-perennial-judging

branch_indigo - When product is closed, User can withdraw stagnant PnL from the product #107