Cheerful Taffy Dolphin - Suboptimal Position Size Rounding in MakerVault Reduces Protocol Fee Generation

[sherlock-admin2]

Cheerful Taffy Dolphin

Medium

Suboptimal Position Size Rounding in MakerVault Reduces Protocol Fee Generation

Summary

In MakerVault's implementation of _strategy(), maker position sizes are being calculated with overly conservative rounding that reduces vault revenue without providing additional safety benefits:

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

// in MakerVault.sol
function _strategy(
    Context memory context,
    UFixed6 deposit,
    UFixed6 withdrawal,
    UFixed6 ineligible
) internal override view returns (Target[] memory targets) {
    return MakerStrategyLib.allocate(context.registrations, deposit, withdrawal, ineligible);
}

The issue manifests in MakerStrategyLib's position size calculations, where rounding down is used despite safety checks already being enforced elsewhere. This reduces the vault's fee-generating capacity without providing additional risk protection.

The vault's fundamental safety checks happen early in the allocation process. The allocate() function first validates that there's sufficient collateral and assets:

// In allocate():
if (collateral.lt(context.totalMargin)) revert MakerStrategyInsufficientCollateralError();
if (assets.lt(context.minAssets)) revert MakerStrategyInsufficientAssetsError();

After these core validations pass, the vault calculates market-specific allocations in _allocateMarket(). Here, the available assets for each market are strictly bounded by both the weight-based allocation and a leveraged collateral limit:

marketAssets = assets
    .mul(marketContext.registration.weight)
    .min(marketCollateral.mul(LEVERAGE_BUFFER));

The LEVERAGE_BUFFER of 1.2x provides an additional safety margin on the collateral allocation. Only after these conservative limits are applied does the vault calculate the final maker position size:

newMaker = marketAssets
    .muldiv(marketContext.registration.leverage, marketContext.latestPrice.abs())
    .max(marketContext.minPosition)
    .min(marketContext.maxPosition);

This calculation currently rounds down, but by this point in the execution flow, we're already operating within comprehensively checked safety bounds. The rounding down creates maker positions slightly smaller than what the vault's capital could safely support. Since maker positions generate fees for the vault, this conservative rounding actually reduces revenue potential without providing additional safety, as all key risk constraints were enforced upstream through explicit checks and bounds.

The impact compounds because the vault typically operates across multiple markets and rebalances regularly. Each time this calculation runs, the rounding down slightly reduces position sizes below what's safely achievable within the already-enforced risk parameters. Rounding up the maker position calculation would allow the vault to capture this lost fee generation opportunity while still respecting all safety constraints, since the core protections are already handled by the upstream checks and bounds.

Recommended Fix

The fix is to use this instead:

// In MakerStrategyLib._allocateMarket()
function _allocateMarket(
    MarketMakerStrategyContext memory marketContext,
    UFixed6 totalMargin,
    UFixed6 collateral,
    UFixed6 assets
) private pure returns (Target memory target, UFixed6 marketCollateral) {
    // ... existing code ...

    // Use muldivOut instead of muldiv for maker position calculation to maximize fee generation
    // while staying within pre-validated safety bounds
    UFixed6 newMaker = marketAssets
        .muldivOut(marketContext.registration.leverage, marketContext.latestPrice.abs()) // Changed to muldivOut
        .max(marketContext.minPosition)
        .min(marketContext.maxPosition);

    target.maker = Fixed6Lib.from(newMaker).sub(Fixed6Lib.from(marketContext.currentAccountPosition.maker));
    
    // ... rest of the function ...
}

This modification ensures the vault maximizes its fee-generating capacity while maintaining all existing safety guarantees provided by the upstream checks and bounds.