Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cheerful Taffy Dolphin - First Depositor Share Price Manipulation via Zero Minimum Deposit Enables Unfair Value Extraction From Subsequent Depositors #39

Open
sherlock-admin4 opened this issue Jan 31, 2025 · 0 comments
Labels
Sponsor Disputed The sponsor disputed this issue's validity

Comments

@sherlock-admin4
Copy link
Contributor

Cheerful Taffy Dolphin

Medium

First Depositor Share Price Manipulation via Zero Minimum Deposit Enables Unfair Value Extraction From Subsequent Depositors

Summary

A vulnerability exists in the Vault contract where an attacker can manipulate the initial share price ratio by exploiting the lack of minimum deposit enforcement during initialization. While the contract has a minDeposit parameter in its VaultParameter struct, the initialization explicitly sets this to zero, creating a window where an attacker can establish an unfavorable share ratio through a minimal initial deposit before any deposit limits are enforced.

The issue lies in the initialization sequence and how it affects early depositors.

  1. During initialization, the Vault sets parameters with zero minimum deposit:

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

function initialize(
    Token18 asset_,
    IMarket initialMarket,
    UFixed6 initialDeposit,
    string calldata name_
) external initializer(1) {
    // ...
    _updateParameter(VaultParameter(initialDeposit, UFixed6Lib.ZERO));
}
  1. The share calculation for deposits is handled in convertToShares():
function convertToShares(UFixed6 assets) external view returns (UFixed6) {
    (UFixed6 _totalAssets, UFixed6 _totalShares) =
        (UFixed6Lib.unsafeFrom(totalAssets()), totalShares());
    return _totalShares.isZero() ? assets : assets.muldiv(_totalShares, _totalAssets);
}
  1. When users deposit, the _update() function enforces minimum deposit checks:
function _update(
    Context memory context,
    address account,
    UFixed6 depositAssets,
    UFixed6 redeemShares,
    UFixed6 claimAssets
) private {
    // ...
    if (!depositAssets.isZero() && depositAssets.lt(context.parameter.minDeposit))
        revert VaultInsufficientMinimumError();
    // ...
}

vulnerability sequence:

  1. Contract is initialized with minDeposit = 0
  2. First depositor can deposit a tiny amount (e.g., 1 wei) since there's no minimum enforcement
  3. Their shares are minted 1:1 with assets since _totalShares.isZero()
  4. They can manipulate underlying asset value through market actions
  5. Subsequent depositors get fewer shares due to the manipulated asset/share ratio

Impact

The impact manifests in the share/asset ratio manipulation through the convertToShares() calculation:

function convertToShares(UFixed6 assets) external view returns (UFixed6) {
    (UFixed6 _totalAssets, UFixed6 _totalShares) = 
        (UFixed6Lib.unsafeFrom(totalAssets()), totalShares());
    return _totalShares.isZero() ? assets : assets.muldiv(_totalShares, _totalAssets);
}

When legitimate users deposit after the attack:

  1. Share Dilution: If attacker deposits 1 wei and manipulates _totalAssets to 100 ETH, a user depositing 50 ETH would receive shares calculated as: 50 ETH * (1 wei) / 100 ETH, resulting in dramatically fewer shares than their deposit proportion warrants.

  2. Value Extraction: The attacker can then extract value by redeeming their disproportionate shares. Their 1 wei initial deposit now controls a significant portion of the vault's shares, allowing them to claim an unfair percentage of all subsequent deposits when redeeming. The profit is realized as: (initial_share_price - manipulated_share_price) * attacker_shares.

This creates a systemic asymmetry where every new deposit inherently advantages the attacker's position due to the manipulated share/asset ratio established during initialization.

Recommendation:

Add mandatory minimum deposit enforcement in initialization:

function initialize(
    Token18 asset_,
    IMarket initialMarket,
    UFixed6 initialDeposit,
    UFixed6 minDeposit,  // New required parameter
    string calldata name_
) external initializer(1) {
    require(!minDeposit.isZero(), "Vault: minimum deposit must be non-zero");
    __Instance__initialize();
    asset = asset_;
    _name = name_;
    _register(initialMarket);
    _updateParameter(VaultParameter(initialDeposit, minDeposit));
}

This ensures there's always a meaningful minimum deposit requirement from the very first deposit, preventing share price manipulation through tiny initial deposits.

While the owner could theoretically call updateParameter() right after initialization to set a minimum, relying on external actions for security invariants is unsafe. The contract must enforce these constraints at the protocol level.

The fix makes the security guarantee explicit in code rather than depending on proper owner operation.

@sherlock-admin3 sherlock-admin3 added the Sponsor Disputed The sponsor disputed this issue's validity label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sponsor Disputed The sponsor disputed this issue's validity
Projects
None yet
Development

No branches or pull requests

2 participants