Popcorn contest - Qeew's results

Lines of code

https://github.com/code-423n4/2023-01-popcorn/blob/d95fc31449c260901811196d617366d6352258cd/src/vault/Vault.sol#L294

Vulnerability details

Impact

Detailed description of the impact of this finding.

The price of ERC4626 vault shares can be maliciously inflated during the first deposit, leading to the loss of assets for next depositors

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

Given a vault with DAI as the underlying asset:

Alice (attacker) deposits initial liquidity of 1 wei DAI via deposit() Alice receives 1e18 (1 wei) vault shares Alice transfers 1 ether of DAI via transfer() to the vault to artificially inflate the asset balance without minting new shares. The asset balance is now 1 ether + 1 wei DAI -> vault share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18) Bob (victim) deposits 100 ether DAI Bob receives 0 shares Bob receives 0 shares due to a precision issue. His deposited funds are lost.

The shares are calculated as following return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets()); In case of a very high share price, due to totalAssets() > assets * supply, shares will be 0.

Tools Used

Manual Review

Recommended Mitigation Steps

This is a well-known issue, Uniswap and other protocols had similar issues when supply == 0.

For the first deposit, mint a fixed amount of shares, e.g. 10**decimals()

if (supply == 0) { return 10**decimals; } else { return assets.mulDivDown(supply, totalAssets()); }