First ERC4626 deposit exploit can break share calculation

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L370-L403

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.

depositGMX() function has 0 shares protect. But it hasn't few shares protect. For example; It's like getting 1 share after deposit with high token;

 if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();

Proof Of Concept

1 - A malicious early user can depositGMX() with 1 wei of asset token as the first depositor of the LToken, and get 1 wei of shares

2 - Then the attacker can send 10000e18 - 1 of asset tokens and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1)

3 - As a result, the future user who deposits 19999e18 will only receive 1 wei (from 19999e18 * 1 / 10000e18) of shares token

4 - They will immediately lose 9999e18 or half of their deposits if they redeem() right after the depositGMX()

The attacker can profit from future users' deposits. While the late users will lose part of their funds to the attacker.

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the Redacted MultiSig so that the pricePerShare can be more resistant to manipulation

function depositGmx(uint256 amount, address receiver)
        external
        nonReentrant
        returns (uint256 shares)
    {
        if (amount == 0) revert ZeroAmount();
        if (receiver == address(0)) revert ZeroAddress();

        // Handle compounding of rewards before deposit (arguments are not used by `beforeDeposit` hook)
        if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);

        // Intake sender GMX
        gmx.safeTransferFrom(msg.sender, address(this), amount);

        // Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
        (, uint256 assets, ) = PirexGmx(platform).depositGmx(
            amount,
            address(this)
        );

        // NOTE: Modified `convertToShares` logic to consider assets already being in the vault
        // and handle it by deducting the recently-deposited assets from the total
        uint256 supply = totalSupply;

        if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();


+	/** for the first mint, we require the mint amount > (10 ** decimals) / 100 and send (10 ** decimals) /
+	1_000_000 of the initial supply as a reserve to Redacted MultiSig
+               */
+ if (totalSupply == 0 && decimals >= 6) {
+       require(shares > 10 ** (decimals - 2));
+      uint256 reserveShares = 10 ** (decimals - 6);
+     _mint(Redacted_MultiSig, reserveShares);
+       shares -= reserveShares;
+     }

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);
    }

[c4-judge]

Picodes marked the issue as duplicate of #407

[c4-judge]

Picodes marked the issue as satisfactory

[c4-judge]

Picodes changed the severity to 3 (High Risk)

[C4-Staff]

JeeberC4 marked the issue as duplicate of #275