neko_nyaa - checkGroup() does not take into account the position's PnL

[sherlock-admin2]

neko_nyaa

Medium

checkGroup() does not take into account the position's PnL

Summary

Vulnerability Detail

Per the docs' provided in the contest:

After the account owner has configured a rebalance group, keepers may call Controller.checkGroup offchain to determine if the group may be rebalanced. Assuming state does not change beforehand, the keeper may then call Controller.rebalanceGroup to perform a rebalance.

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial-account/README.md

checkGroup() calls into _queryMarketCollateral():

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

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial-account/contracts/Controller.sol#L99

_queryMarketCollateral() calls into Market.locals() to obtain the total collateral for each market.

    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; // @audit this
            actualCollateral[i] = collateral;
            groupCollateral = groupCollateral.add(collateral);
        }
    }

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial-account/contracts/Controller.sol#L254

which returns the current collateral amount of a user's account at the current state

    function locals(address account) external view returns (Local memory) {
        return _locals[account].read();
    }

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L375-L377

Any price swings would've caused the position's PnL to settle, which would change the position's collaterals. Then an account would not be rebalanced even in case the market swing causes imbalance to the user's portfolio. When the position is not timely rebalanced and the risky position is not de-risked, liquidations can happen due to a position having low collateral.

Impact

Keeper bots reliant on checkGroup() will not rebalance groups when needed. This could lead to liquidation due to the position not being timely de-risked with more collateral when needed.

Per the Sherlock rules:

In case any of these incorrect values returned by the view functions are used as a part of a larger function which would result in loss of funds then it would be a valid medium/high depending on the impact.

Code Snippet

https://github.com/sherlock-audit/2024-08-perennial-v2-update-3/blob/main/perennial-v2/packages/perennial-account/contracts/Controller.sol#L92

Tool used

Manual Review

Recommendation

Account for the PnL from the price differences as well when checking rebalancing possibility.