First ERC4626 deposit can be exploited to break share calculation

[code423n4]

Lines of code

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

Vulnerability details

Impact

This is a common attack vector involving shares based liquidity pool contracts. An early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the rather large value of price per share.

Apparently, the first depositor of an ERC4626 vault can maliciously manipulate the share price by depositing the lowest possible amount with 1 wei of liquidity and then artificially inflating ERC4626.totalAssets.

This can inflate the base share price to as high as 1:1e18, forcing all subsequent deposits to be dictated by this share price as a base. Additionally, due to rounding down, if this malicious initial deposit were to front-run someone else depositing, this depositor would end up receiving 0 shares and losing his deposited assets.

ERC4626 vault share price can be maliciously inflated on the initial deposit, leading to the next depositor losing assets due to precision issues.

Proof of Concept

Here is a typical exploit scenario where:

A vault is using DAI as its underlying asset.

1. Bob, the attacker, deposits an initial liquidity of 1 wei of DAI via deposit().
2. Bob receives 1e18 (1 wei) vault of shares.
3. Bob transfers 1 ether of DAI to the vault to artificially inflate the asset balance without minting any new shares.
4. The asset balance is now 1 ether + 1 wei of DAI, i.e. the vault share price is now very high which is equivalent to 1000000000000000000001 wei or 1000 * 1e18.
5. Next, Alice, the victim, deposits 100 ether of DAI.
6. Alice receives 0 shares due to a precision issue.
7. Alice's deposited funds are deemed lost.

Note: As denoted in the code block below, shares are determined via its return statement, supply == 0 ? assets : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down). In the event of a very high share price, due to totalAssets() very much larger than assets * supply, shares will be returned 0.

File: Vault.sol#L294-L301

    function convertToShares(uint256 assets) public view returns (uint256) {
        uint256 supply = totalSupply(); // Saves an extra SLOAD if totalSupply is non-zero.

        return
            supply == 0
                ? assets
                : assets.mulDiv(supply, totalAssets(), Math.Rounding.Down);
    }

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider sending the first 1000 shares to address 0, a mitigation approach adopted by the Uniswap V2 protocol when supply == 0.

Additionally, the protocol could look into implementing slippage protection to further mitigate the situations.

[c4-judge]

dmvt marked the issue as duplicate of #15

[c4-sponsor]

RedVeil marked the issue as sponsor confirmed

[c4-judge]

dmvt marked the issue as partial-50

[c4-judge]

dmvt changed the severity to 3 (High Risk)

[c4-judge]

dmvt marked the issue as satisfactory