First depositor can inflate share price

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/vaults/AutoPxGmx.sol#L397
https://github.com/code-423n4/2022-11-redactedcartel/blob/684627b7889e34ba7799e50074d138361f0f532b/src/vaults/AutoPxGlp.sol#L314

Vulnerability details

Impact

A well-known vulnerability for ERC4626 vaults is the inflation of the share price on the first deposit. Because AutoPxGlp and AutoPxGmx use the balance of the underlying asset for totalAssets() and do not have an initial minimum deposit amount, they are also suspectible to this attack. This will cause subsequent depositors to lose significant parts of their deposits due to rounding errors.

Proof Of Concept

Bob deposits a very small amount of GMX such that he gets 1 wei of pxGMX. Afterwards, he transfers 1e18 pxGMX to the vault address. totalAssets() is defined like that:

		/**
        @notice Get the pxGMX custodied by the AutoPxGmx contract
        @return uint256  Amount of pxGMX custodied by the autocompounder
     */
    function totalAssets() public view override returns (uint256) {
        return asset.balanceOf(address(this));
    }

It will therefore now return 1e18 + 1.

When Alice now deposits 2e18 pxGMX, the following calculation will be performed:

assets.mulDivDown(supply, totalAssets() - assets)
= 2e18 * 1 / (1e18 + 1) = 1

Therefore, she receives only 1 share and ~1e18 pxGMX are lost due to rounding.

Recommended Mitigation Steps

There are multiple mitigations (minimium initial deposit, internal total assets, creating dead shares), see the OpenZeppelin issue for an extensive discussion: OpenZeppelin/openzeppelin-contracts#3706

[c4-judge]

Picodes marked the issue as duplicate of #407

[c4-judge]

Picodes marked the issue as satisfactory

[C4-Staff]

JeeberC4 marked the issue as duplicate of #275