Cheerful Taffy Dolphin - Incorrect Profit Share Distribution Due to Asset-Share Ratio Manipulation in Checkpoint Calculations

[sherlock-admin2]

Cheerful Taffy Dolphin

Medium

Incorrect Profit Share Distribution Due to Asset-Share Ratio Manipulation in Checkpoint Calculations

Summary

The vault's checkpoint mechanism contains a sequence vulnerability in its profit share calculations that affects coordinator compensation. The vulnerability originates in the vault's settlement flow where _settle() triggers checkpoint completion and propagates through to profit distribution:

https://github.com/sherlock-audit/2025-01-perennial-v2-4-update/blob/main/perennial-v2/packages/vault/contracts/Vault.sol#L374

function _settle(Context memory context, address account) private {
    // ... settlement logic ...
    (context.mark, profitShares) = nextCheckpoint.complete(
        context.mark,
        context.parameter,
        _checkpointAtId(context, nextCheckpoint.timestamp)
    );
    context.global.shares = context.global.shares.add(profitShares);
    _credit(coordinator, profitShares);
}

The current implementation in the resulting checkpoint completion updates assets before calculating profit shares, but adds the profit shares to the total share count afterwards. This creates a calculation using inconsistent state - new assets but old shares - leading to skewed profit distributions. The issue is particularly significant because it directly impacts the economic incentives of the vault coordinator role, potentially over/under allocating profit shares during each checkpoint period based on mismatched asset-to-share ratios.

function complete(Checkpoint memory self, UFixed18 mark, VaultParameter memory parameter, PerennialCheckpoint memory marketCheckpoint) internal pure returns (UFixed18 newMark, UFixed6 profitShares) {
    // First adds new collateral to assets
    self.assets = self.assets.add(marketCheckpoint.collateral);

    // Then calculates profit share using updated assets but old shares
    (newMark, profitShares) = _calculateProfitShare(self, mark, parameter);

    // Finally updates shares after profit calculation
    self.shares = self.shares.add(profitShares);
}

function _calculateProfitShare(Checkpoint memory self, UFixed18 mark, VaultParameter memory parameter) private pure {
    // Uses current assets with old shares for calculation
    newMark = mark.max(UFixed18Lib.from(UFixed6Lib.unsafeFrom(self.assets)).div(UFixed18Lib.from(self.shares)));
    
    // Calculates profit based on potentially incorrect mark
    UFixed6 profitAssets = parameter.profitShare.mul(UFixed6Lib.from(newMark.sub(mark).mul(UFixed18Lib.from(self.shares))));
    
    // Calculates shares based on skewed ratio
    profitShares = profitAssets.mul(self.shares).div(UFixed6Lib.unsafeFrom(self.assets).sub(profitAssets));
}

// Account processing that interacts with checkpoint
function processGlobal(Account memory self, uint256 latestId, Checkpoint memory checkpoint, UFixed6 deposit, UFixed6 redemption) internal pure {
    self.latest = latestId;
    (self.assets, self.shares) = (
        self.assets.add(checkpoint.toAssetsGlobal(redemption)),
        self.shares.add(checkpoint.toSharesGlobal(deposit))
    );
    // ...
}

The critical issue lies in the calculation sequence where new assets are factored into profit calculations before the share total is updated. This creates a momentary skew in the assets-to-shares ratio that affects profit distribution. While this skew corrects itself in the next checkpoint, it means the coordinator's profit share for that period is calculated against an artificially inflated asset base without corresponding share adjustments.

The vulnerability is exacerbated by the interaction between checkpoint completion and account processing, where profit share calculations occur with inconsistent state before the account system can properly process and update share totals.

Impact

The improper sequencing in checkpoint profit share calculations has direct financial implications on vault economics. When the vault calculates coordinator profit shares using updated asset values but stale share counts, it skews the assets-to-shares ratio used for profit allocation.

For example, if a checkpoint adds 1000 USDC in new assets before calculating profits on an existing 10000 USDC / 10000 shares (1:1 ratio), but the new assets should have minted 1000 new shares, the calculation uses an incorrect 11000:10000 ratio instead of the proper 11000:11000. With a 20% profit share parameter, this inflated ratio results in the coordinator receiving more profit shares than intended.

This miscalculation compounds if the vault frequently adds large amounts of collateral relative to its size. While the impact is bounded by the checkpoint period and corrects in subsequent checkpoints, it creates unfair profit distributions and could be exploited by coordinators timing their actions around checkpoint boundaries to maximize profit share allocations.

The issue is worsened by the vault's account management system where checkpoint state influences share calculations in processGlobal. When assets are updated before profit calculation but shares are handled afterward, it creates a discrepancy that propagates through the account processing system, leading to incorrect share allocations at the account level.

Fix

First fix the ordering issue in complete:

function complete(
    Checkpoint memory self,
    UFixed18 mark,
    VaultParameter memory parameter,
    PerennialCheckpoint memory marketCheckpoint
) internal pure returns (UFixed18 newMark, UFixed6 profitShares) {
    // Store original state before any updates
    Fixed6 originalAssets = self.assets;

    // Update collateral/assets
    self.assets = self.assets.add(marketCheckpoint.collateral);

    // Calculate profit shares using original asset state
    (newMark, profitShares) = _calculateProfitShare(
        originalAssets,
        self.shares,
        self.assets,
        mark,
        parameter
    );

    // Add profit shares after calculation
    self.shares = self.shares.add(profitShares);

    // Add fees last
    self.tradeFee = marketCheckpoint.tradeFee;
    self.settlementFee = marketCheckpoint.settlementFee;
}

Update profit calculation to handle states correctly:

function _calculateProfitShare(
    Fixed6 originalAssets,
    UFixed6 currentShares,
    Fixed6 newAssets,
    UFixed18 mark,
    VaultParameter memory parameter
) private pure returns (UFixed18 newMark, UFixed6 profitShares) {
    // Skip calculation for fresh vaults
    if (currentShares.isZero()) return (UFixed18Lib.ONE, UFixed6Lib.ZERO);

    // Calculate new mark using original assets to avoid inflated mark
    newMark = mark.max(UFixed18Lib.from(UFixed6Lib.unsafeFrom(originalAssets)).div(UFixed18Lib.from(currentShares)));

    // Skip if no previous mark (migration case)
    if (mark.isZero()) return (newMark, UFixed6Lib.ZERO);

    // Calculate profit using state-consistent ratio
    UFixed6 profitAssets = parameter.profitShare
        .mul(UFixed6Lib.from(newMark.sub(mark).mul(UFixed18Lib.from(currentShares))));

    // Skip if no assets to allocate
    if (UFixed6Lib.unsafeFrom(newAssets).sub(profitAssets).isZero()) return (newMark, UFixed6Lib.ZERO);

    // Calculate shares using new assets
    profitShares = profitAssets.mul(currentShares).div(UFixed6Lib.unsafeFrom(newAssets).sub(profitAssets));
}