sherlock-admin
commented
1 year ago 4 comment(s) were left on this issue during the judging contest.
panprog commented:
invalid because the difference in MultiInvoker balance before and after vault.update can only come from the funds MultiInvoker sent to user, proof of concept is incorrect - vault will send 9 asset to MultiInvoker and MultiInvoker balance will change by 9, so it will send 9 assets to user.
n33k commented:
invalid, not convincing without PoC
darkart commented:
What do you mean by Vault may take fee ? please give more details and escalate if you think this is valid
polarzero commented:
High. Agree with the comments.
feelereth
high
Invalid assumption that the amount of assets claimed from the vault will equal the change in the msg.sender's DSU balance. This can lead to issues
Summary
There is an assumption that the amount of assets claimed from the vault will equal the change in the msg.sender's DSU balance. However, this may not always be true due to things like settlement fees charged by the vault.
Vulnerability Detail
The problem is that claimAmount is calculated based on the balance change rather than the actual claimAssets amount. This can be an issue because: • The vault may charge fees like settlement fees on withdrawal • Other operations in vault.update() like depositing could impact the balance • External transfers could impact the balance between the two readings So claimAmount may not equal the assets the user intended to claim.
A proof of concept exploit:
Impact
The user may end up with less assets transferred back than they expected. This could lead to unintended losses for the user if they assumed all their assets were returned.
Code Snippet
https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-extensions/contracts/MultiInvoker.sol#L209-L216
Tool used
Manual Review
Recommendation
The _vaultUpdate function should rely solely on the claimAssets amount returned by the vault itself rather than trying to infer it from the DSU balance change. The DSU balance should only be used as a sanity check rather than the source of truth.