sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

WATCHPUG - `settle(address(0))` can result in incorrect `assets` and `shares` due to a miscalculation that mistakenly treats the global account as a local account. #137

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

WATCHPUG

high

settle(address(0)) can result in incorrect assets and shares due to a miscalculation that mistakenly treats the global account as a local account.

Summary

Vulnerability Detail

settle() is a public function that can be called by anyone at any time.

When called with address(0) as the account at L438-439, the global account _accounts[address(0)] will be copied as context.global and context.local.

Then, context.global will first get settled to the latestIds at L326, and a checkpoint will be saved.

Later at L342, context.local will be updated in processLocal(). However, it's actually processing the global account (address(0)) but in a "local" way, i.e., only 1/checkpoint.count of the keeper fee will be applied.

Finally, in _saveContext(), the global copy and the local will be saved into storage at L447 and L448, where the local one will be the final one as it comes later.

Impact

When checkpoint.count > 1, the assets and shares of the global account will be wrong.

Code Snippet

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

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

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

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/types/Account.sol#L74-L87

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

Tool used

Manual Review

Recommendation

Local account settlement should be skipped when settle(address(0)).

Duplicate of #62

sherlock-admin commented 1 year ago

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

141345 commented:

d