sherlock-admin
commented
1 year ago 4 comment(s) were left on this issue during the judging contest.
panprog commented:
invalid because transaction will revert entirely in this case (so transfer won't happen either, not just update reverts)
n33k commented:
invalid, not convincing without PoC
darkart commented:
How do you expect them to update vault before transfer this can break protocol as people can just update it and not deposit
polarzero commented:
High. Same as above.
feelereth
high
The current implementation could potentially put the user in a failed state if the vault update reverts after assets have already been pulled from the user
Summary
The depositAssets amount is pulled from the user before updating the vault. This means the user could get into a failed state if the vault update then reverts. It would be better to update the vault first and handle pulls after.
Vulnerability Detail
The key issue is that _deposit is called before vault.update. This pulls assets from the user into the contract before the vault update. If the vault update were to revert for any reason, the user's assets would remain locked in the contract.
Impact
The user would be left in a failed state with less assets than they started with.
Code Snippet
https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L198-L211 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L257-L264
Tool used
Manual Review
Recommendation
Update the vault first, and only pull assets from the user after the update succeeds. This ensures the user's assets remain in their control until after the vault update has succeeded. By doing the transfer last, the user is protected from a potential failed state if the update were to revert.