sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

WATCHPUG - Attacker can deposit many times to the Vault to earn keeper rewards from the Vault. #142

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

WATCHPUG

high

Attacker can deposit many times to the Vault to earn keeper rewards from the Vault.

Summary

Vulnerability Detail

Each time a user deposits or withdraws, it will incur a keeper fee (settlementFee), but the fee will be shared equally among all participants, regardless of the number of orders the user has created.

Checkpoint#_withoutKeeperLocal() will calculate and apply the keeperPer to the user's amount. However, one account will only be charged for 1/checkpoint.count of the entire keeper of that checkpoint, while the user may have created multiple orders.

For example, if Alice created 9 orders and Bob created 1 order, the checkpoint.count would be 10, but both Alice and Bob will only pay for 1/10 of the entire keeper in this checkpoint. The remaining 8/10 will be borne by the entire vault.

An attacker can exploit this by creating a lot of orders and acting as the keeper to harvest the keeperFee paid mostly by the other vault users.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/types/Checkpoint.sol#L181-L188

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/pyth/PythOracle.sol#L124-L128

https://github.com/sherlock-audit/2023-07-perennial/blob/main/root/contracts/attribute/Kept.sol#L40-L57

Tool used

Manual Review

Recommendation

Either charge for the share of the keeper fee from the local account based on the number of orders they've created, or make sure only 1 unit of settlement fee is needed for the entire vault, and it can then be evenly distributed among the users per.

Duplicate of #59

sherlock-admin commented 1 year ago

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

141345 commented:

d

panprog commented:

invalid because vault only allows 1 pending position per account, so user can't create 2 deposits from the same account for the same checkpoint