A malicious early user can play with the underlying assets' unit share price to get an unfair share of future users' deposits

[code423n4]

Lines of code

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

Vulnerability details

Lines of code

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

Vulnerability details

Impact

Unit price share manipulation of the first user can lead to fund loss of the later depositors in yield-bearing tokens such as ERC4626.

Proof of Concept

ERC4626, an extension of ERC20, is a standard that is mostly used in yield-bearing tokens. The contract of an ERC4626 token itself is an ERC20 which serves as a "shares" token and has an underlying asset which is another ERC20. The idea behind this is in such a way that the users deposit their assets and get corresponding shares. The function convertToShares() is responsible to return the corresponding share of a user with respect to his deposited assets.

    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());
    }

One of the main problems with these tokens lies in the fact that the first depositor can manipulate the unit share price in a bad way. The ratio of $\large \dfrac{totalSupply}{totalAssets}$ determines the corresponding share of a deposited asset. As it is clear from the ratio if the first user deposits 1 Wei, will get 1 Wei worth of share. Now consider a scenario with the preceding steps:

1 - The same bad guy deposits $\large 10000 \times 10 ^ {18} - 1 $ amount of underlying assets. 2 - As a consequence, the abovementioned ratio skyrockets and won't be a 1:1 pair anymore. $$\large \dfrac{10000 \times 10 ^ {18} - 1 + 1}{1} = 10 ^ {22}$$ 3 - The later users who deposit large amounts of assets (e.g. $\large 19999 \times 10 ^ {18} - 1 $) will get 1 Wei worth of shares. 4 - Thus, if they call redeem() after depositing, they will immediately lose $\large 9999 \times 10 ^ {18} - 1 $ (nearly half of) the assets.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter to prevent such manipulations

Assessed type

Math

[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 not a duplicate

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-judge]

trust1995 marked the issue as primary issue

[c4-judge]

trust1995 changed the severity to 3 (High Risk)

[c4-judge]

trust1995 changed the severity to 2 (Med Risk)

[c4-sponsor]

0xLightt marked the issue as sponsor disputed

[0xLightt]

In the only implementation of that contract in ERC4626PartnerManager, totalAssets is equal to totalSupply. So this is not being handled in this specific contract, but in its implementation.

[trust1995]

ERC4626.sol is indeed vulnerable, but it is clear sponsor has thought of this case when overriding from it. Therefore, I believe QA to be appropriate.

[c4-judge]

trust1995 changed the severity to QA (Quality Assurance)

[c4-judge]

trust1995 marked the issue as grade-c