sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

feelereth - Assets that are locked in the vault are still included in the totalAssets calculation, which can make the conversions between assets and shares inaccurate #51

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

feelereth

high

Assets that are locked in the vault are still included in the totalAssets calculation, which can make the conversions between assets and shares inaccurate

Summary

Assets that can't be transferred out of the vault are still included in total assets. This could make the conversion inaccurate if some assets are locked.

Vulnerability Detail

The totalAssets() function calculates the total assets by taking the assets from the latest checkpoint and adding any new deposits and subtracting any redemptions: Link1 and Link2 This does not account for any assets that are locked in the vault and unable to be transferred out. For example, if some assets are locked as collateral in a lending protocol. Those locked assets would still be included in the total assets, even though they cannot be redeemed.

Impact

Share to asset conversion can be inaccurate:

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L111-L116 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L139-L143

Tool used

Manual Review

Recommendation

Have a separate variable to track the locked assets, and subtracting those from the total assets when doing conversions

sherlock-admin commented 1 year ago

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

panprog commented:

invalid because totalAssets are assets already redeemed and just waiting to be claimed, they're not locked.

n33k commented:

invalid, not convincing without PoC

darkart commented:

It show as part of total asset because it is