sherlock-admin
commented
1 year ago 4 comment(s) were left on this issue during the judging contest.
panprog commented:
low because this seems to be admin mistake (market shouldn't use the same token for collateral and rewards). Might be medium if such tokens usage can be intended behavior, but I think it's not.
n33k commented:
invalid, reward token can never be the same as Vault's asset token, https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L122
darkart commented:
I dont see why it would send all the tokens to vault it goes through rewards only
polarzero commented:
Medium. Downgrading to medium as this should not cause significant enough loss/malfunction to be categorized as high.
tvdung94
high
Using Vault#claimReward() might accidentally transfer all of vault's current asset to vault factory's owner.
Summary
Using Vault#claimReward() might accidentally transfer all of vault's current asset to vault owner.
Vulnerability Detail
When claimReward() is called, it will go through each market, claim reward token and send it all to vault factory's owner. The problem arises when market reward token is same as vault's asset token. In this case, _registrations[marketId].read().market.reward().push(factory().owner()); will be equivalent to asset.push(factory().owner)), which will transfer all of the vault's current asset to the vault factory's owner.
Impact
Loss of funds and disrupting vault's accounting.
Code Snippet
Vault#claimReward() https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L209-L214
Vault's asset token https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L30
Market's reward token https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L17
Tool used
Manual Review
Recommendation
Revise claimReward(), need to keep track of reward amount.