sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

feelereth - The _socialize() function can penalize users who did not cause losses in the vault #52

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

feelereth

high

The _socialize() function can penalize users who did not cause losses in the vault

Summary

The _socialize() function socializes losses when assets drop below deposits. But it does this by reducing all withdrawals, which penalizes users who did not cause the losses.

Vulnerability Detail

The _socialize() function is called when processing a user's deposit, withdrawal or claim. It calculates the claimAmount that the user will actually receive after accounting for losses.

It does this by comparing the total collateral in the vault totalCollateral to the total assets context.global.assets. If assets have dropped below collateral, it will reduce the user's claimAmount proportionally: link This means all users claiming assets from the vault will have their claims reduced, even if they did not cause the losses.

For example:

User A deposits 100 assets User B deposits 100 assets Due to bad trades, the vault now only has 150 assets left but 200 collateral User A tries to withdraw 50 assets The _socialize() function will reduce User A's withdrawal to only 75% of 50 assets = 37.5 assets So User A is penalized for the losses, even though User B caused them

Impact

Users claiming assets from the vault will have their claims reduced, even if they did not cause the losses.

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L323

Tool used

Manual Review

Recommendation

A better approach would be to socialize losses only across users who caused them. For example: • Track which market positions led to the losses • Reduce withdrawals proportionally based on usage of those loss-causing positions • Allow withdrawals from non-loss positions to remain whole This requires additional accounting logic to track P&L by market. But it more fairly socializes losses to those responsible. The key is socializing based on causal positions, not evenly across all users.

sherlock-admin commented 1 year ago

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

panprog commented:

invalid because users don't trade and all of them share all vault profits and losses

n33k commented:

invalid, not convincing without PoC