Oblivionis - Perennial accounts can be rebalanced multiple times

[sherlock-admin4]

Oblivionis

High

Perennial accounts can be rebalanced multiple times

Summary

An attacker can call rebalanceGroup multiple times in a single oracle version to earn multiple keeper fee.

Root Cause

In Controller.sol:223, all market in the group is settled before validation:

    function _rebalanceGroup(address owner, uint256 group) internal {
        // settles each markets, such that locals are up-to-date
        _settleMarkets(owner, group);

        // determine imbalances
        (, bool canRebalance, Fixed6[] memory imbalances) = checkGroup(owner, group);
        if (!canRebalance) revert ControllerGroupBalancedError();

        IAccount account = IAccount(getAccountAddress(owner));
        // pull collateral from markets with surplus collateral
        for (uint256 i; i < imbalances.length; i++) {
            IMarket market = groupToMarkets[owner][group][i];
            if (Fixed6.unwrap(imbalances[i]) < 0) account.marketTransfer(market, imbalances[i]);
        }

        // push collateral to markets with insufficient collateral
        for (uint256 i; i < imbalances.length; i++) {
            IMarket market = groupToMarkets[owner][group][i];
            if (Fixed6.unwrap(imbalances[i]) > 0) account.marketTransfer(market, imbalances[i]);
        }

        emit GroupRebalanced(owner, group);
    }

But, when an account is rebalancable, calling Controller.rebalanceGroup() will simply push an order to Market, and this order must wait next oracle version to get executed:
Market.sol:730

        while (
            context.local.currentId != context.local.latestId &&
            (nextOrder = _pendingOrders[context.account][context.local.latestId + 1].read()).ready(context.latestOracleVersion)
        ) _processOrderLocal(context, settlementContext, context.local.latestId + 1, nextOrder.timestamp, nextOrder);

If rebalance is called multiple times in an oracle version, Market.settle will simply skip the settlement of prior orders. So when Controller checks if the account is rebalancable, the state of current oracle version will always be used:

    function _queryMarketCollateral(address owner, uint256 group) private view returns (
        Fixed6[] memory actualCollateral,
        Fixed6 groupCollateral
    ) {
        actualCollateral = new Fixed6[](groupToMarkets[owner][group].length);
        for (uint256 i; i < actualCollateral.length; i++) {
            Fixed6 collateral = groupToMarkets[owner][group][i].locals(owner).collateral;
            actualCollateral[i] = collateral;
            groupCollateral = groupCollateral.add(collateral);
        }
    }

At this point, when the attacker tries to rebalance again, all validations will pass until marketParameter.maxPendingLocal is reached.

Internal pre-conditions

Victims use perennial account and set a valid rebalance group.

External pre-conditions

User's rebalance group is rebalancable -- which means one/some markets is outside the rebalance threshold:

        for (uint256 i; i < actualCollateral.length; i++) {
            IMarket market = groupToMarkets[owner][group][i];
            RebalanceConfig memory marketRebalanceConfig = _rebalanceConfigs[owner][group][address(market)];
            (bool canMarketRebalance, Fixed6 imbalance) =
                RebalanceLib.checkMarket(marketRebalanceConfig, groupCollateral, actualCollateral[i]);
            imbalances[i] = imbalance;
            canRebalance = canRebalance || canMarketRebalance;
        }

Attack Path

1. Attacker observe a rebalancable account.
2. Attacker call Controller.rebalanceGroup() many times, until maxPendingLocal is reached:

perennial-v2/packages/perennial/contracts/libs/InvariantLib.sol:86

        if (
            context.global.currentId > context.global.latestId + context.marketParameter.maxPendingGlobal ||
            context.local.currentId > context.local.latestId + context.marketParameter.maxPendingLocal
        ) revert IMarket.MarketExceedsPendingIdLimitError();

Impact

Attacker can receive many times of keeper fee for the rebalance, victims directly lose funds from perennial account. The loss depends on marketParameter.maxPendingLocal, which is a protocol-specific param.

PoC

No response

Mitigation

Controller.rebalanceGroup should introduce an oracle version check: an account can only be settled once in one oracle version.