feelereth - The convertToShares and convertToAssets functions are vulnerable to manipulation of the total shares and assets values.

[sherlock-admin]

feelereth

high

The convertToShares and convertToAssets functions are vulnerable to manipulation of the total shares and assets values.

Summary

The convertToShares and convertToAssets functions assume the total shares and assets can't be manipulated. But an attacker could potentially manipulate checkpoints to change these values. The calculations should check for valid, immutable values.

Vulnerability Detail

The convertToShares and convertToAssets functions are vulnerable to manipulation of the total shares and assets values.

The key issue is that these functions rely on calling totalShares() and totalAssets() to get the current total shares and assets. However, an attacker could manipulate these values by manipulating the checkpoint data.

Specifically, an attacker could:

1. Deposit assets into the vault to increase totalAssets()
2. Withdraw all their shares to reduce totalShares()
3. Call convertToShares() to convert a small amount of assets into a large amount of shares. This works because totalAssets is high and totalShares is low.
4. Withdraw the newly minted shares to drain assets from the vault

   Impact

   Assets are drained

   Code Snippet

   https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L131-L132 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L140-L141

   Tool used

Manual Review

Recommendation

convertToShares and convertToAssets should rely on immutable values that can't be manipulated, like:

• The initial total supply of shares
• The total deposits into the vault
• The total withdrawals / redemptions from the vault

[sherlock-admin]

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

panprog commented:

invalid because convertToShares() is not callable by user

n33k commented:

invalid, not convincing without PoC

darkart commented:

The POC you gave dosen't show how you can manipulate checkpoints