Lines of code
<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L339-L362&gt;
<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L242-L313&gt;

Vulnerability details

Impact

Calling the following AutoPxGmx.withdraw and AutoPxGmx.redeem functions would execute compound(poolFee, 1, 0, true), which uses the configured Uniswap pool fee as the fee input of the AutoPxGmx.compound function below to further call the SWAP_ROUTER.exactInputSingle function. Yet, when calling the AutoPxGmx.compound function directly, it is possible that a pool fee, which is not the configured pool fee, is used. Using a pool corresponding to a pool fee that is not the configured pool fee can cause the increment of the asset/share amount resulted from calling the AutoPxGmx.compound function to be lower than that when using a pool corresponding to the configured pool fee; as a result, the withdrawn pxGMX amount resulted from calling functions like AutoPxGmx.withdraw and AutoPxGmx.redeem after the AutoPxGmx.compound function using a non-configured pool fee is directly called can be lower than that resulted from calling functions like AutoPxGmx.withdraw and AutoPxGmx.redeem without the AutoPxGmx.compound function being directly called beforehand.

<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L315-L337&gt;

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

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

        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;
        }

        _burn(owner, shares);

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

        asset.safeTransfer(receiver, assets);
    }

<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L339-L362&gt;

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

<https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L242-L313&gt;

    function compound(
        uint24 fee,
        uint256 amountOutMinimum,
        uint160 sqrtPriceLimitX96,
        bool optOutIncentive
    )
        public
        returns (
            uint256 gmxBaseRewardAmountIn,
            uint256 gmxAmountOut,
            uint256 pxGmxMintAmount,
            uint256 totalFee,
            uint256 incentive
        )
    {
        if (fee == 0) revert InvalidParam();
        if (amountOutMinimum == 0) revert InvalidParam();

        uint256 assetsBeforeClaim = asset.balanceOf(address(this));

        PirexRewards(rewardsModule).claim(asset, address(this));

        // Swap entire reward balance for GMX
        gmxBaseRewardAmountIn = gmxBaseReward.balanceOf(address(this));

        if (gmxBaseRewardAmountIn != 0) {
            gmxAmountOut = SWAP_ROUTER.exactInputSingle(
                IV3SwapRouter.ExactInputSingleParams({
                    tokenIn: address(gmxBaseReward),
                    tokenOut: address(gmx),
                    fee: fee,
                    recipient: address(this),
                    amountIn: gmxBaseRewardAmountIn,
                    amountOutMinimum: amountOutMinimum,
                    sqrtPriceLimitX96: sqrtPriceLimitX96
                })
            );

            // Deposit entire GMX balance for pxGMX, increasing the asset/share amount
            (, pxGmxMintAmount, ) = PirexGmx(platform).depositGmx(
                gmx.balanceOf(address(this)),
                address(this)
            );
        }

        // Only distribute fees if the amount of vault assets increased
        if ((totalAssets() - assetsBeforeClaim) != 0) {
            totalFee =
                ((asset.balanceOf(address(this)) - assetsBeforeClaim) *
                    platformFee) /
                FEE_DENOMINATOR;
            incentive = optOutIncentive
                ? 0
                : (totalFee * compoundIncentive) / FEE_DENOMINATOR;

            if (incentive != 0) asset.safeTransfer(msg.sender, incentive);

            asset.safeTransfer(owner, totalFee - incentive);
        }

        ...
    }

Proof of Concept

Please add the following code in test\AutoPxGmx.t.sol. Both the testCompoundUsingPoolFeeConfig and testCompoundUsingPoolFeeThatIsNotConfig tests will pass to demonstrate the described scenario. Please see the comments in these tests for more details.

    uint96 gmxAmount_ = 1e18;
    uint32 secondsElapsed_ = 3600;

    uint256 wethAmtIn = 598927240182;
    uint256 gmxAmtOutUsingPoolFeeConfig = 17947773728826;
    uint256 pxGmxMintAmtUsingPoolFeeConfig = 17947773728826;

    function testCompoundUsingPoolFeeConfig() external {
        address[] memory receivers = new address[](1);
        receivers[0] = address(this);
        _provisionRewardState(gmxAmount_, receivers, secondsElapsed_);

        // Functions like AutoPxGmx.redeem executes compound(poolFee, 1, 0, true),
        //   which is same as the following compound function call.
        vm.prank(testAccounts[0]);
        (
            uint256 wethAmountIn,
            uint256 gmxAmountOut,
            uint256 pxGmxMintAmount,
            uint256 totalFee,
            uint256 incentive
        ) = autoPxGmx.compound(autoPxGmx.poolFee(), 1, 0, true);

        // Using the configured pool fee, the 598927240182 WETH reward amount is swapped to 17947773728826 GMX via Uniswap V3,
        //   and 17947773728826 pxGMX reward amount is minted via PirexGmx.
        assertEq(wethAmountIn, wethAmtIn);
        assertEq(gmxAmountOut, gmxAmtOutUsingPoolFeeConfig);
        assertEq(pxGmxMintAmount, pxGmxMintAmtUsingPoolFeeConfig);
    }

    function testCompoundUsingPoolFeeThatIsNotConfig() external {
        // the inputs used for calling the _provisionRewardState function are same as these used in the testCompoundUsingPoolFeeConfig test
        address[] memory receivers = new address[](1);
        receivers[0] = address(this);
        _provisionRewardState(gmxAmount_, receivers, secondsElapsed_);

        // when calling the AutoPxGmx.compound function directly, it is possible that a pool fee, which is not the configured pool fee, is used
        uint24 poolFee = 500;
        assertTrue(poolFee != autoPxGmx.poolFee());

        vm.prank(testAccounts[0]);
        (
            uint256 wethAmountIn,
            uint256 gmxAmountOut,
            uint256 pxGmxMintAmount,
            uint256 totalFee,
            uint256 incentive
        ) = autoPxGmx.compound(poolFee, 1, 0, true);

        // When using the pool fee that is not the configured pool fee,
        //   the 598927240182 WETH reward amount, which is the same WETH reward amount used in the testCompoundUsingPoolFeeConfig test,
        //   is swapped to a GMX amount that is less than the corresponding output GMX amount when the configured pool fee is used.
        // In this case, the minted pxGMX reward amount is also less than the corresponding minted pxGMX reward amount when the configured pool fee is used.
        assertEq(wethAmountIn, wethAmtIn);
        assertLt(gmxAmountOut, gmxAmtOutUsingPoolFeeConfig);
        assertLt(pxGmxMintAmount, pxGmxMintAmtUsingPoolFeeConfig);

        // Thus, the increment of the asset/share amount resulted from calling the AutoPxGmx.compound function using this pool fee that is not the configured pool fee
        //   is lower than that resulted from calling functions like AutoPxGmx.redeem without the AutoPxGmx.compound function being directly called beforehand.
        // In other words, the withdrawn pxGMX amount resulted from calling functions like AutoPxGmx.redeem after the AutoPxGmx.compound function using this non-configured pool fee is directly called
        //   would be lower than that resulted from calling functions like AutoPxGmx.redeem without the AutoPxGmx.compound function being directly called beforehand.
    }

Tools Used

VSCode

Recommended Mitigation Steps

The AutoPxGmx.compound function can be updated to always use the configured pool fee, which is poolFee, instead of using the fee input.

---

The text was updated successfully, but these errors were encountered:

All reactions