Oblivionis - Perennial account users with rebalance group may suffer a donation attack

[sherlock-admin4]

Oblivionis

High

Perennial account users with rebalance group may suffer a donation attack

Summary

The checks in checkMarket only consider proportions and not values, users with 0 collateral in a rebalance group may get attacked to drain all DSU in their perennial accounts.

Root Cause

This vulnerability has two predicate facts:

1. Attacker can donate any value to any account.
   InvariantLib.sol:78-82

        if (
            !updateContext.signer &&                                            // sender is relaying the account's signed intention
            !updateContext.operator &&                                          // sender is operator approved for account
            !(newOrder.isEmpty() && newOrder.collateral.gte(Fixed6Lib.ZERO))    // sender is depositing zero or more into account, without position change
        ) revert IMarket.MarketOperatorNotAllowedError();

Users can sign an order if: 1. He is an signer or 2. He is an operator or 3. He is trying to deposit some value to the account without position change.

2. Consider a group with multiple markets, only one market has minimal collateral (1e-6 DSU, the minimum precision of Fixed6) and other markets have no collateral. Such group can be rebalanced infinitely.

Controller.sol:223

    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);
    }

Controller.sol:92

    function checkGroup(address owner, uint256 group) public view returns (
        Fixed6 groupCollateral,
        bool canRebalance,
        Fixed6[] memory imbalances
    ) {
        // query owner's collateral in each market and calculate sum
        Fixed6[] memory actualCollateral;
        (actualCollateral, groupCollateral) = _queryMarketCollateral(owner, group);
        imbalances = new Fixed6[](actualCollateral.length);

        // determine if anything 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;
        }
    }

RebalanceLib.sol:18

    function checkMarket(
        RebalanceConfig memory marketConfig,
        Fixed6 groupCollateral,
        Fixed6 marketCollateral
    ) external pure returns (bool canRebalance, Fixed6 imbalance) {
        // determine how much collateral the market should have
        Fixed6 targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));

        // if market is empty, prevent divide-by-zero condition
        if (marketCollateral.eq(Fixed6Lib.ZERO)) return (false, targetCollateral);
        // calculate percentage difference between target and actual collateral
        Fixed6 pctFromTarget = Fixed6Lib.ONE.sub(targetCollateral.div(marketCollateral));
        // if this percentage exceeds the configured threshold, the market may be rebelanced
        canRebalance = pctFromTarget.abs().gt(marketConfig.threshold);

        // return negative number for surplus, positive number for deficit
        imbalance = targetCollateral.sub(marketCollateral);
    }

In Controller.checkGroup():

groupCollateral = 1e-6, actualCollateral = 1e-6 for one market, = 0 for other markets.

After passed into RebalanceLib, for all markets,

targetCollateral = groupCollateral.mul(Fixed6Lib.from(marketConfig.target));

Since marketConfig.target < Fixed6.ONE(It is the percentage of a single market), targetCollateral will be less than the precision of Fixed6, so it round down to 0.

For the market with collateral, targetCollateral = 0 but marketCollateral = 1e-6.

So pctFromTarget = 1 - 0/1e-6 = 1 = 100%.

So canRebalance = pctFromTarget.abs().gt(marketConfig.threshold) = 1.

For the market without collateral, targetCollateral = 0 and marketCollateral = 0. canRebalance = 0 but it does not matter.

Now we have proven such group can always get rebalanced. Next we will show that each rebalance does not change the market allocation:

imbalance = targetCollateral.sub(marketCollateral);

For the market with collateral, imbalance = 0- 1e-6 = -1e-6.
For markets without collateral, imbalance = 0 - 0 = 0.

When Controller tries to perform the market transfer, the 1e-6 collateral will be transfered back to victim`s perennial account. Now we reached the initial state: all markets in the group have no fund in it.

Internal pre-conditions

1. Perennial account owner has activated a valid group.
2. All markets in the group reach a state where all marketCollateral = 0. This can happen in many situations:
   a. The owner withdraw from all these markets.
   b. The owner was liquidated in these markets and no margin left. (This is possible due to high leverage).
   c. The owner just activated the group and haven't had a chance to put money in it yet.
3. The perennial account has some fund in it.

External pre-conditions

N/A

Attack Path

1. Attacker donate 1e-6 DSU as collateral to one of victim's market in the group.
2. Attacker call Controller_Incentivized.rebalanceGroup() to perform the attack and resume group state.
3. Attacker repeat step1 and 2 to drain the whole DSU and USDC balance in victim's account.

Impact

Victim's account balance can get drained when they have an empty group.

PoC

No response

Mitigation

There should be a minimum rebalance value check to prevent this issue and prevent users pay more keeper fee than the rebalanced margin when margin is tiny.

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
equilibria-xyz/perennial-v2#450