First ERC4626 deposit exploit can break share calculation

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/erc-4626/ERC4626.sol#L106-L110

Vulnerability details

Impact

A well known attack vector for almost all shares based liquidity pool contracts, where 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.

Proof of Concept

Problems with the code:

1. Integer division negatively affect the user.
2. Can be manipulated to cause a large loss, specifically for first victim.

Consider the following situation:

1. Attacker deposits 1 wei of Token.
2. Next, Attacker transfers 100 Token to the contract.
3. Victim deposits 200 Token.
4. Attacker withdraws 1 share.

Have a look at this table to understand the complete PoC:

	Before	Before		After	After
Tx	totalSupply	balanceOf	sharesGiven	totalSupply	balanceOf
BeforeAttacker deposits 1 wei of Token.	0	0	1	1	1
Attacker transfers 100 Token to the contract.	1	1	N/A	1	1 + 100 x 10^18
Victim deposits 200 Token.	1	1 + 100 x 10^18	=1.99 = 1	2	1 + 300 x 10^18
Attacker withdraws 1 share.	2	1 + 300 x 10^18	N/A	1	1 + 150 x 10^18

File: ERC4626.sol

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

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

Link to Code

Tools Used

VS Code

Recommended Mitigation Steps

1. Need to Enforce a minimum deposit that can't be withdrawn.
2. So, add some of the initial amount to the zero address.

Assessed type

Other

[c4-judge]

trust1995 marked the issue as duplicate of #115

[c4-judge]

trust1995 marked the issue as satisfactory

[c4-judge]

trust1995 marked the issue as duplicate of #852

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)