sherlock-audit / 2022-10-astaria-judging

6 stars 1 forks source link

__141345__ - Steal deposit fund in ERC4626 vault by exchange rate manipulation #260

Closed sherlock-admin closed 2 years ago

sherlock-admin commented 2 years ago

141345

medium

Steal deposit fund in ERC4626 vault by exchange rate manipulation

Summary

Although the ERC4626 contract has check for require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");, the rounding down error can still be used to steal new user deposit. Part of the new deposit could be stolen. The attacker may monitor the pool activities to catch the steal opportunities.

The mitigation is send a small share to address(0) for new vault.

Vulnerability Detail

An attacker will be the following:

  1. watch the mempool to monitor new ERC4626 vault deployment.
  2. make sure to be the first to deposit() in the pool.
  3. when a new user tries to call deposit(), the attacker will front run it, transfer some fund into the vault, not through deposit() function in vault, but direct transfer with ERC20(underlying).transfer(address(vault), amount), in this way the totalAssets / shares ratio will be inflated.
  4. after the manipulation, the new user will only get 1 wei of share, but the fund corresponding to the share will be half of the deposit amount.
  5. attacker call withdraw()/redeem(), steal part of the user's deposit fund.

Assume the asset is DAI, the numbers for each steps would be:

  1. watch mempool
  2. attacker being the first deposit: 99 wei DAI, 99 wei share
  3. user tries to deposit 1,000 DAI (1e21), so call deposit(1e21). The attacker will front run to transfer (500 * 99) DAI into the vault directly. Now the vault has (495e20 + 99) DAI as totalAssets, 99 share, 1 share inflated to (5e20 + 1) DAI.
  4. assets per share will be inflated. The call to previewDeposit(1e21) -> convertToShares(1e21) will calculate assets.mulDivDown(supply, totalAssets()), the result is 1e21 * 1 / (5e20 + 1), just less than 2, so round down to 1. So the new user gets 1 share, the pool has (505e20 + 99) DAI, almost 50,500 DAI.
  5. attacker withdraws the 99 share for about 49,995 DAI. The user end up with around 505 DAI, almost half is lost.

If the attacker does not have 49,500 DAI upfront, the steal still works, just the profit a little lower.

Although the condition of empty vault for this pattern seems not easy meet, if do it on purpose, the malicious user can still abuse the system in certain circumstances, in which the vector conditions are not so hard to meet. For instance in the following ways:

Impact

Part of the deposit fund from the 2nd user will be stolen by the attacker for new vault. Potentially the attacker can intentionally monitor the pool activities to find steal conditions.

Code Snippet

The totalAssets can be artificially influenced by direct transfer to inflate the balance, then manipulate the assets per shares deposit.

https://github.com/sherlock-audit/2022-10-astaria/blob/main/src/WithdrawProxy.sol#L38-L40

https://github.com/sherlock-audit/2022-10-astaria/blob/main/lib/astaria-gpl/src/ERC4626-Cloned.sol#L305-L322

https://github.com/sherlock-audit/2022-10-astaria/blob/main/lib/astaria-gpl/src/ERC4626-Cloned.sol#L414-L421

https://github.com/sherlock-audit/2022-10-astaria/blob/main/lib/astaria-gpl/src/ERC4626-Cloned.sol#L392-L401

Tool used

Manual Review

Recommendation

When first mint() in the vault, sent a small amount to address(0), then the attacker won't be able to benefit from this behavior.

Duplicate of #143