Redacted Cartel contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L436-L444 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L177-L195 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315-L337 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L199-L217 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156-L165 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L99-L122

Vulnerability details

Impact

Contracts AutoPxGlp and AutoPxGmx extends PirexERC4626 and function withdraw() and previewWithdraw() has been overridden in those contracts. withdraw() uses function previewWithdraw() to calculate number of shares need to burn for corresponding amount of assets and it assumes and requires previewWithdraw() to perform rounds up when calculating amount of shares. but previewWithdraw() in AutoPxGlp and AutoPxGmx don't round up when calculating shares (in PirexERC4626 function previewWithdraw() correctly rounds up, but when the function was overridden the behavior has been changed), this can cause inconsistency and also fund lose because of rounding error in the division. This is wrong implementation of PirexERC4626 in the child contracts.

Proof of Concept

This is withdraw() code in PirexERC4626 and AutoPxGlp and AutoPxGmx contracts:

   // PirexERC4626
    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public virtual returns (uint256 shares) {
        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;
        }

        beforeWithdraw(owner, assets, shares);

        _burn(owner, shares);

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

        asset.safeTransfer(receiver, assets);

        afterWithdraw(owner, assets, shares);
    }

    /** AutoPxGlp
        @notice Override the withdrawal method to make sure compound is called before withdrawing
     */
    function withdraw(
        uint256 assets,
        address receiver,
        address owner
    ) public override returns (uint256 shares) {
        compound(1, 1, true);

        shares = PirexERC4626.withdraw(assets, receiver, owner);
    }

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

As you can see all of the calls previewWithdraw() to calculates amount of share and they assume that there is no need to check for rounding error, because previewWithdraw() rounds up. This is previewWithdraw() code in PirexERC4626 contract:

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

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

As you can see it uses mulDivUp() to calculate shares and it correctly rounds up. This is the overridden previewWithdraw() in AutoPxGlp and AutoPxGmx contracts:

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

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

As you can see they both uses convertToShares() to calculate amount of shares and convertToShares() is in PirexERC4626. This is the code of convertToShares():

    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 it uses mulDivDown() to calculate amount of shares and doesn't rounds up. so the override of previewWithdraw() in AutoPxGlp and AutoPxGmx contract changes the intended behavior of function that withdraw() was depending on it, this bug would cause withdraw() to work incorrectly and burn wrong amount of shares for user. the fund loss is depending on the round error and it is critical when the price per share (amount of assets for 1 share) is high, it's possible for hacker to change PPS by sending assets directly to contract address in early stage of deployment which make this bug to be more critical. any time any user withdraw some assets, contract burn less than equivalent amount of share for that user (by margin of PPS) and it's like other users lose funds. As comments in code define that previewWithdraw() should rounds up and implementation is wrong so This is a High issue(This is a well-known issue in ERC4626).

Tools Used

VIM

Recommended Mitigation Steps

correctly rounds up in the previewWithdraw() in AutoPxGlp and AutoPxGmx.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L60-L78 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L178-L185 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/PirexERC4626.sol#L156-L165

Vulnerability details

Impact

This is a well-known attack vector for new contracts that utilize pricePerShare for accounting. Attacker can cause totalAssets() / totalSupply() ratio to go as high as he wants and then because of rounding error in convertToShares() lower amount of share would be mint for users. if totalSupply() is 0 attacker can directly transfer 1000 e 18 tokens to contract address(even before contract deployment, because the address of contract can be calculated even before deployment) and then deposit 1 wei to contract so then totalAssets() would be very high and totalSupply() would be low and totalAssets() / totalSupply() could be as high as 1000 e 18 which would make rounding error as 1000 e 1e18 token and users would lose funds, for example if a user deposits 1999 * 1e18 tokens he would receive shares only based on 1000 * 1e18 of his tokens because of rounding error.

Proof of Concept

This is deposit() and previewDeposit() and convertToShares() code in PirexERC4626 contract:

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


    function previewDeposit(uint256 assets)
        public
        view
        virtual
        returns (uint256)
    {
        return convertToShares(assets);
    }

    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 amount of shares users received for depositing asset is calculated by convertToShares() function. imagine this scenario for AutoPxGmx contract which is extended PirexERC4626:

1. A malicious early user can deposit() with 1 wei of PxGmx token as the first depositor of the AutoPxGmx, and get 1 wei of shares token.
2. Then the attacker can send 1000e18 - 1 of PxGmx to AutoPxGmx address and inflate the price per share from 1.0000 to an extreme value of 1.0000e21 ( from (1 + 10000e18 - 1) / 1). (attacker can send this amount even when contract didn't get deployed yet because the deployed address can be calculated)
3. As a result, the future user who deposits 1999e18 will only receive 1 wei (from 1999e18 * 1 / 1000e18 = 1) of shares token.
4. They will immediately lose 999e18 or half of their deposits if they redeem() right after the deposit().

The code only protect itself from division error when deposit amount is less than PPS. (by checking require((shares = previewDeposit(assets)) != 0, "ZERO_SHARES")) but division error for higher values of deposits is possible as high as PPS which the value is controllable by attacker. (This is sample report of this bug which is confirmed: https://code4rena.com/reports/2022-04-pooltogether/#h-01-a-malicious-early-userattacker-can-manipulate-the-vaults-pricepershare-to-take-an-unfair-share-of-future-users-deposits )

Tools Used

VIM

Recommended Mitigation Steps

Consider requiring a minimal amount of share tokens to be minted for the first minter, and send a port of the initial mints as a reserve to the DAO address so that the pricePerShare can be more resistant to manipulation or consider adding a static multiplier to improve the precision of the shares calculation

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L197-L296 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L230-L313

Vulnerability details

Impact

Function compound() in AutoPxGmx and AutoPxGlp contracts is for compounding pxGLP (and additionally pxGMX) rewards. it works by calling PirexGmx.claim(px*, this) to collect the rewards of the vault and then swap the received amount (to calculate the reward, contract save the balance of a contract in that reward token before and after the call to the claim() and by subtracting them finds the received reward amount) and deposit them in PirexGmx again for compounding and in doing so it calculates fee based on what it received and in AutoPxGlp case it calculates pxGMX rewards too based on the extra amount contract receives during the execution of claim(). but attacker can call PirexGmx.claim(px*, PirexGlp) directly and make PirexGmx contract to transfer (gmxBaseReward and pxGmx) rewards to AutoPxGlp and in this case the logics of fee calculation and reward calculation in compound() function won't get executed and contract won't get it's fee from rewards and users won't get their pxGmx reward. so this bug would cause fee loss in AutoPxGmx and AutoPxGlp for contract and pxGmx's reward loss for users in AutoPxGlp.

Proof of Concept

the bug in in AutoPxGmx is similar to AutoPxGlp, so we only give Proof of Concept for AutoPxGlp. This is compound() function code in AutoPxGlp contract:

    function compound(
        uint256 minUsdg,
        uint256 minGlp,
        bool optOutIncentive
    )
        public
        returns (
            uint256 gmxBaseRewardAmountIn,
            uint256 pxGmxAmountOut,
            uint256 pxGlpAmountOut,
            uint256 totalPxGlpFee,
            uint256 totalPxGmxFee,
            uint256 pxGlpIncentive,
            uint256 pxGmxIncentive
        )
    {
        if (minUsdg == 0) revert InvalidParam();
        if (minGlp == 0) revert InvalidParam();

        uint256 preClaimTotalAssets = asset.balanceOf(address(this));
        uint256 preClaimPxGmxAmount = pxGmx.balanceOf(address(this));

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

        // Track the amount of rewards received
        gmxBaseRewardAmountIn = gmxBaseReward.balanceOf(address(this));

        if (gmxBaseRewardAmountIn != 0) {
            // Deposit received rewards for pxGLP
            (, pxGlpAmountOut, ) = PirexGmx(platform).depositGlp(
                address(gmxBaseReward),
                gmxBaseRewardAmountIn,
                minUsdg,
                minGlp,
                address(this)
            );
        }

        // Distribute fees if the amount of vault assets increased
        uint256 newAssets = totalAssets() - preClaimTotalAssets;
        if (newAssets != 0) {
            totalPxGlpFee = (newAssets * platformFee) / FEE_DENOMINATOR;
            pxGlpIncentive = optOutIncentive
                ? 0
                : (totalPxGlpFee * compoundIncentive) / FEE_DENOMINATOR;

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

            asset.safeTransfer(owner, totalPxGlpFee - pxGlpIncentive);
        }

        // Track the amount of pxGMX received
        pxGmxAmountOut = pxGmx.balanceOf(address(this)) - preClaimPxGmxAmount;

        if (pxGmxAmountOut != 0) {
            // Calculate and distribute pxGMX fees if the amount of pxGMX increased
            totalPxGmxFee = (pxGmxAmountOut * platformFee) / FEE_DENOMINATOR;
            pxGmxIncentive = optOutIncentive
                ? 0
                : (totalPxGmxFee * compoundIncentive) / FEE_DENOMINATOR;

            if (pxGmxIncentive != 0)
                pxGmx.safeTransfer(msg.sender, pxGmxIncentive);

            pxGmx.safeTransfer(owner, totalPxGmxFee - pxGmxIncentive);

            // Update the pxGmx reward accrual
            _harvest(pxGmxAmountOut - totalPxGmxFee);
        } else {
            // Required to keep the globalState up-to-date
            _globalAccrue();
        }

        emit Compounded(
            msg.sender,
            minGlp,
            gmxBaseRewardAmountIn,
            pxGmxAmountOut,
            pxGlpAmountOut,
            totalPxGlpFee,
            totalPxGmxFee,
            pxGlpIncentive,
            pxGmxIncentive
        );
    }

As you can see contract collects rewards by calling PirexRewards.claim() and in the line uint256 newAssets = totalAssets() - preClaimTotalAssets; contract calculates the received amount of rewards(by subtracting the balance after and before reward claim) and then calculates fee based on this amount totalPxGlpFee = (newAssets * platformFee) / FEE_DENOMINATOR; and then sends the fee in the line asset.safeTransfer(owner, totalPxGlpFee - pxGlpIncentive) for owner. the logic for pxGmx rewards are the same. As you can see the calculation of fee is based on the rewards received, and there is no other logic in the contract to calculate and transfer the fee of protocol. so if AutoPxGpl receives rewards without compound() getting called then for those rewards fee won't be calculated and transferred and protocol would lose it's fee. In the line _harvest(pxGmxAmountOut - totalPxGmxFee) contract calls _harvest() function to update the pxGmx reward accrual and there is no call to _harvest() in any other place and this is the only place where pxGmx reward accrual gets updated. contract uses pxGmxAmountOut which is the amount of gmx contract received during the call (code calculates it by subtracting the balance after and before reward claim: pxGmxAmountOut = pxGmx.balanceOf(address(this)) - preClaimPxGmxAmount;) so contract only handles accrual rewards in this function call and if some pxGmx rewards claimed for contract without compund() logic execution then those rewards won't be used in _harvest() and _globalAccrue() calculation and users won't receive those rewards. As mentioned attacker can call PirexRewards.claim(pxGmx, AutoPxGpl) directly and make PirexRewads contract to transfer AutoPxGpl rewards. This is claim() code in PirexRewards:

    function claim(ERC20 producerToken, address user) external {
        if (address(producerToken) == address(0)) revert ZeroAddress();
        if (user == address(0)) revert ZeroAddress();

        harvest();
        userAccrue(producerToken, user);

        ProducerToken storage p = producerTokens[producerToken];
        uint256 globalRewards = p.globalState.rewards;
        uint256 userRewards = p.userStates[user].rewards;

        // Claim should be skipped and not reverted on zero global/user reward
        if (globalRewards != 0 && userRewards != 0) {
            ERC20[] memory rewardTokens = p.rewardTokens;
            uint256 rLen = rewardTokens.length;

            // Update global and user reward states to reflect the claim
            p.globalState.rewards = globalRewards - userRewards;
            p.userStates[user].rewards = 0;

            emit Claim(producerToken, user);

            // Transfer the proportionate reward token amounts to the recipient
            for (uint256 i; i < rLen; ++i) {
                ERC20 rewardToken = rewardTokens[i];
                address rewardRecipient = p.rewardRecipients[user][rewardToken];
                address recipient = rewardRecipient != address(0)
                    ? rewardRecipient
                    : user;
                uint256 rewardState = p.rewardStates[rewardToken];
                uint256 amount = (rewardState * userRewards) / globalRewards;

                if (amount != 0) {
                    // Update reward state (i.e. amount) to reflect reward tokens transferred out
                    p.rewardStates[rewardToken] = rewardState - amount;

                    producer.claimUserReward(
                        address(rewardToken),
                        amount,
                        recipient
                    );
                }
            }
        }
    }

As you can see it can be called by anyone for any user. so to perform this attack, attacker would perform this steps:

1. suppose AutoPxGpl has pending rewards, for example 100 pxGmx and 100 weth.
2. attacker would call PirexRewards.claim(pxGmx, AutoPxGpl) and PirexRewards.claim(pxGpl, AutoPxGpl) and PirexRewards contract would calculate and claim and transfer pxGmx rewards and weth rewards of AutoPxGpl address.
3. then AutoPxGpl has no pending rewards but the balance of pxGmx and weth of contract has been increased.
4. if anyone call AutoPxGpl.compound() because there is no pending rewards contract would receive no rewards and because contract only calculates fee and rewards based on received rewards during the call to compound() so contract wouldn't calculate any fee or reward accrual for those 1000 pxGmx and weth rewards.
5. owner of AutoPxGpl would lose his fee for those rewards and users of AutoPxGpl would lose their claims for those 1000 pxGmx rewards (because the calculation for them didn't happen).

This bug is because of the fact that the only logic handling rewards is in compound() function which is only handling receiving rewards by calling claim() during the call to compound() but it's possible to call claim() directly (PirexRewards contract allows this) and AutoPxGpl won't get notified about this new rewards and the related logics won't get executed.

Tools Used

VIM

Recommended Mitigation Steps

contract should keep track of it's previous balance when compound() get executed and update this balance in deposits and withdraws and claims so it can detect rewards that directly transferred to contract without call to compound().

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L197-L296 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L230-L313

Vulnerability details

Impact

Function compound() in the AutoPxGlp and AutoPxGmx contracts is claiming all the rewards of the pool then swaps them if required and then deposits them in the PirexGmx again to compound the reward. when swapping called, code uses the slippage tolerance values that user specified This creates an issue where a single user specify slippage for all the pool contract rewards which belongs to all of the users. attacker can use this to steal contract rewards by creating a smart contract that first manipulate the pool in Uniswap (or any pool that GMX or Pirex protocol uses in depositing) then call compound() with very high slippage allowance (set amountOutMinimum very low) and code would swap rewards token with very bad price and then attacker contract would perform the reverse transaction in Uniswap.

Proof of Concept

This is compound() code in AutoPxGmx contract:

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

As you can see contract swaps the gmxBaseReward balance of contract for gmx token by calling IV3SwapRouter and the slippage parameters for swaps amountOutMinimum and sqrtPriceLimitX96 are set by the values user provided. This would give attacker to perform on-chain sandwich attack against the pool. to perform this attack, attacker would create a smart contract would perform this steps:

1. get a very large number of gmxBaseReward token as flash loan.
2. manipulate the uniswap pool (or any pool that GMX or Pirex protocol uses when compounding or depositing) and creates a bad swap ratio by swapping very large number of gmxBaseReward tokens to gmx tokens.
3. call compound() function in AutoPxGmx with high slippage allowance and the code would tries to swap gmxBaseReward token for gmx token but because the liquidity pool has been manipulated AutoPxGmx would receive smaller amount of gmx token that it was supposed to.
4. perform the reverse of step 2 transaction, swap the gmx tokens for gmxBaseReward tokens and receive extra tokens which belonged to AutoPxGmx.

By performing this attacker was abled to steal AutoPxGmx rewards with on-chain sandwich-attack in one transaction. The situation for AutoPxGlp is similar to this. the problem is that contract allows a user to chose slippage allowance for all the rewards of contract which belongs to all the users.

Tools Used

VIM

Recommended Mitigation Steps

add some slippage allowance for contract which is controllable by DAO

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L126-L136 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L148-L158

Vulnerability details

Impact

Function setPlatform() in AutoPxGlp and AutoPxGmx contracts update the PirexGmx address but because it doesn't set token(gmxBaseReward token for AutoPxGlp and gmx token for AutoPxGmx) spending approval for new address it would cause AutoPxGlp and AutoPxGmx to be in a broken state which depositing interaction with PirexGmx wouldn't be possible. This would cause compound() function to revert and because all the logics call compound() so all the logics of AutoPxGlp and AutoPxGmx to fail. so if owner calls setPlatform() then all the funds in the contract would be inaccessible and owner should call setPlatform() again and set the old address of PirexGmx and if this old address was broken then funds would be locked forever. so the impact is:

1. it's not possible to update the address of PirexGmx in AutoPxGlp and AutoPxGmx if addresses in the protocol has been updated.
2. funds lock forever if the PirexGmx was deployed in new address and the contract in the old address of was broken.

Proof of Concept

This is the setPlatform() code in AutoPxGlp and AutoPxGmx contracts:

    /**
        @notice Set the platform
        @param  _platform  address  Platform
     */
    function setPlatform(address _platform) external onlyOwner {
        if (_platform == address(0)) revert ZeroAddress();

        platform = _platform;

        emit PlatformUpdated(_platform);
    }

As you can see it sets the new address but don't give spending approval for the new address and don't set the approval for the old address as zero. This is the constructors() code in AutoPxGmx:

    constructor(
        address _gmxBaseReward,
        address _gmx,
        address _asset,
        string memory _name,
        string memory _symbol,
        address _platform,
        address _rewardsModule
    ) Owned(msg.sender) PirexERC4626(ERC20(_asset), _name, _symbol) {
        if (_gmxBaseReward == address(0)) revert ZeroAddress();
        if (_gmx == address(0)) revert ZeroAddress();
        if (_asset == address(0)) revert ZeroAddress();
        if (bytes(_name).length == 0) revert InvalidAssetParam();
        if (bytes(_symbol).length == 0) revert InvalidAssetParam();
        if (_platform == address(0)) revert ZeroAddress();
        if (_rewardsModule == address(0)) revert ZeroAddress();

        gmxBaseReward = ERC20(_gmxBaseReward);
        gmx = ERC20(_gmx);
        platform = _platform;
        rewardsModule = _rewardsModule;

        // Approve the Uniswap V3 router to manage our base reward (inbound swap token)
        gmxBaseReward.safeApprove(address(SWAP_ROUTER), type(uint256).max);
        gmx.safeApprove(_platform, type(uint256).max);
    }

As you can see it sets the unlimited spending approval of token gmx for platform address and this approval is required for contract logics to work correctly and if it wasn't there contract can't work with PirexGmx and function compound() would fail and all other function would fail because they call compound() function. This is compound() code in AutoPxGmx:

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

As you can see it deposits gmx balance of contract in platform and if the spending approval was not their the code would revert. The situation for AutoPxGlp contract is similar, the constructor() give unlimited spending approval of token gmxBaseReward for platform (meaning PirexGmx contract address) address which is necessary to interact with PirexGmx and without it compound() wouldn't work and all the other function that call compound() (which is all the important functionality of contract) would fail too. As you saw Function setPlatform() doesn't set proper approvals(like constructor()) for new platform address so if admin can't use this function to update PirexGmx addresses in AutoPxGlp and AutoPxGmx and if was required to update PirexGmx address(for example it was hacked or the contract was broken or contract has been updated and is in new addresses) then contracts AutoPxGlp and AutoPxGmx would be broken and even there is chance that old the funds would be lucked in the contract. imagine this scenario:

1. something happens to contract PirexGmx and team deploy new PirexGmx with new address.(the old PirexGmx is broken because of a hack or bug or it's just removed by team because they updated it in new address).
2. owner calls setPlatform() in AutoPxGlp and AutoPxGmx to update PirexGmx contract address in those contracts. and code would update the platform address but because of the bug don't give spending approval of necessary tokens for new address.
3. all function like withdraw(), redeem(), deposit(), mint() would be broken because they call compound() in their execution flow and compound() tries to deposit tokens in PirexGmx but the spending approval is zero.
4. all user's funds in AutoPxGlp and AutoPxGmx would be locked for ever because contracts can't work with new PirexGmx address and the contract in old address is broken or removed.

Tools Used

VIM

Recommended Mitigation Steps

in setPlatform() code should set approval for old address to zero and approval for new address to max.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L269-L293 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L346-L355

Vulnerability details

Impact

Function configureGmxState() of PirexGmx is for configuring GMX contract state but logic is using safeApprove() improperly, it won't reset approval amount for old stakedGmx address This would cause 4 problem:

1. different behavior in setContract() and configureGmxState() for handling stakeGmx address changes. in setContract() the logic reset approval for old address to zero but in configureGmxState() the logic don't reset old address GMX spending approval.
2. the call to this function would revert if stakeGmx address didn't changed but other addresses has been changed so owner can't use this to configure contract.
3. contract won't reset approval for old stakedGmx addresse which is a threat because contract in that address can steal all the GMX balance any time in the future if that old address had been compromised.
4. contract won't reset approval for old stakedGmx addresse, if owner use configureGmxState() to change the stakeGmx value then it won't be possible to set stakedGmx value to previous ones by using either configureGmxState() or setContract() and contract would be in broken state.

Proof of Concept

This is configureGmxState() code in PirexGmx:

    /**
        @notice Configure GMX contract state
     */
    function configureGmxState() external onlyOwner whenPaused {
        // Variables which can be assigned by reading previously-set GMX contracts
        rewardTrackerGmx = RewardTracker(gmxRewardRouterV2.feeGmxTracker());
        rewardTrackerGlp = RewardTracker(gmxRewardRouterV2.feeGlpTracker());
        feeStakedGlp = RewardTracker(gmxRewardRouterV2.stakedGlpTracker());
        stakedGmx = RewardTracker(gmxRewardRouterV2.stakedGmxTracker());
        glpManager = gmxRewardRouterV2.glpManager();
        gmxVault = IVault(IGlpManager(glpManager).vault());

        emit ConfigureGmxState(
            msg.sender,
            rewardTrackerGmx,
            rewardTrackerGlp,
            feeStakedGlp,
            stakedGmx,
            glpManager,
            gmxVault
        );

        // Approve GMX to enable staking
        gmx.safeApprove(address(stakedGmx), type(uint256).max);
    }

As you can see it just sets the approval for new stakeGmx address and don't do anything about spending approval of old stakeGmx address. This is part of the setContract() code that handels stakeGmx:

        if (c == Contracts.StakedGmx) {
            // Set the current stakedGmx (pending change) approval amount to 0
            gmx.safeApprove(address(stakedGmx), 0);

            stakedGmx = RewardTracker(contractAddress);

            // Approve the new stakedGmx contract address allowance to the max
            gmx.safeApprove(contractAddress, type(uint256).max);
            return;
        }

As you can see it resets the spending approval of old stakedGmx address to zero and then give unlimited spending approval for new address. So the impact #1 is obvious that the code for same logic in two different functions don't beehive similarly.

Function configureGmxState() is used for configuring GMX contract state but if owner uses this function one time then it won't be possible to call this function the second time if stakedGmx wasn't changed. for example in this scenario:

1. owner calls configureGmxState() to set values for GMX contract addresses.
2. then address of one of contract changes in GMX (stakedGmx stayed the same) and owner want's to call configureGmxState() to reset the values of variables to correct ones.
3. owner call to configureGmxState() would fail because stakedGmx address didn't changed and in the line gmx.safeApprove(address(stakedGmx), type(uint256).max); contract tries to set non zero approval for stakedGmx but it already has non zero spending allowance. (safeApprove() would revert if the current spending allowance is not zero and the new allowance is not zero either). so the impact #2 would happen and calls to this function in some scenarios would fail and it won't be functional.

Because function configureGmxState() don't reset the old stakeGmx address's GMX token spending approval to 0x0 so it would be possible to lose all GMX balance of PirexGmx contract if the old stakeGmx addresses are compromised. for example in this scenario:

1. GMX protocol get hacked(either by a bug or leaking some private keys) and stakeGmx contract control would be in hacker's hand.
2. GMX deploy new contracts and stakeGmx address changes in GMX. ()
3. owner of PirexGmx calls configureGmxState() to reset the values of GMX contracts addresses in PirexGmx.
4. function configureGmxState() logic would change the GMX contract addresses but it won't set GMX token spending approval for old stakeGmx address to zero.
5. hacker who control the old stakeGmx address would use his access to that address contract to withdraw GMX balance of PirexGmx. because PirexGmx won't set approval for old stakeGmx contract so it would be possible for that old stakeGmx address to transfer GMX balance of PirexGmx anytime in future. the bug in old stakeGmx contract or leakage of private keys of stakeGmx address(private key who can become the owner or admin of that contract) can be happen after migrating GMX and Pirex contracts too. This is impact #3 scenario.

in scenario #4 contract would stuck in unrecoverable state. the problem is that if configureGmxState() is get used more than once and stakeGmx variable's value has been changes more than once then it won't be possible to set stakeGmx value to old values either with configureGmxState() or setContract() and the contract won't be useful. the scenario is this: (safeApprove() won't allow to set non-zero approval for address that has already non-zero approval amount. see the OZ implementation)

1. GMX protocol changes its stakeGmx contract address from old address to new (for any reason, they got hacked or they are migrating or ....)
2. owner of PirexGmx calls configureGmxState() to update GMX protocol's contract address in PirexGmx contract and the logic would update the values of variables. (the GMX spending approval for old and new stakeGmx address would be max value).
3. GMX protocol changes stakeGmx contract address from new value to old value (for any reason, they decided to roll back to old address)
4. owner tries to call configureGmxState() to reupdate GMX protocol's address in PirexGmx but the call would revert because the code tries to call safeApprove() for address that has already non-zero allowance.
5. owner can't call setContract() to update value of stakeGmx variable because this function tries to call safeApprove() to set non-zero approval value for address that already has non-zero allowance. so in this state owner can't recover PirexGmx contract and because contract has wrong value for stakeGmx it won't be functional and it would stay in broken state.

Tools Used

VIM

Recommended Mitigation Steps

like setContract(), function configureGmxState() should set approval for old PirexGmx to zero first.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L733-L816 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L228-L267 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L332-L348

Vulnerability details

Impact

Function claimRewards() in PirexGmx claims WETH and esGMX rewards and multiplier points (MP) from GMX protocol. it uses _calculateRewards() to calculate the unclaimed reward token amounts produced for each token type. but because of the lack of checks, function _calculateRewards() would revert when RewardTracker.distributor.totalSupply() is zero so if any of 4 RewardTrackers has zero totalSupply() then function claimRewards() would revert too and function harvest() in PirexRewards contract would revert too because it calls PirexGmx.claimRewards(). harvest() is used in claim() function so claim() would not work too. This bug would harvesting rewards for PirexRewards contract and claiming reward for users from PirexRewards when supply of one of RewardTrackers contracts in GMX protocol is zero. Function claimRewards() is written based on GMX code, but the logic is not correctly copied because in GMX protocol contract checks for totalSupply() and it prevents this bug from happening.

Proof of Concept

This is function _calculateRewards()'s code in PirexGmx:

   function _calculateRewards(bool isBaseReward, bool useGmx)
        internal
        view
        returns (uint256)
    {
        RewardTracker r;

        if (isBaseReward) {
            r = useGmx ? rewardTrackerGmx : rewardTrackerGlp;
        } else {
            r = useGmx ? stakedGmx : feeStakedGlp;
        }

        address distributor = r.distributor();
        uint256 pendingRewards = IRewardDistributor(distributor)
            .pendingRewards();
        uint256 distributorBalance = (isBaseReward ? gmxBaseReward : esGmx)
            .balanceOf(distributor);
        uint256 blockReward = pendingRewards > distributorBalance
            ? distributorBalance
            : pendingRewards;
        uint256 precision = r.PRECISION();
        uint256 cumulativeRewardPerToken = r.cumulativeRewardPerToken() +
            ((blockReward * precision) / r.totalSupply());

        if (cumulativeRewardPerToken == 0) return 0;

        return
            r.claimableReward(address(this)) +
            ((r.stakedAmounts(address(this)) *
                (cumulativeRewardPerToken -
                    r.previousCumulatedRewardPerToken(address(this)))) /
                precision);
    }

As you can see in the line uint256 cumulativeRewardPerToken = r.cumulativeRewardPerToken() + ((blockReward * precision) / r.totalSupply()) if totalSupply() was zero then code would revert because of division by zero error. so if RewardTracker.distributor.totalSupply() was zero then function _calculateRewards would revert and won't work and other function using _calculateRewards() would be break too. This is part of function claimRewards()'s code in PirexGmx contract:

    function claimRewards()
        external
        onlyPirexRewards
        returns (
            ERC20[] memory producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {
        // Assign return values used by the PirexRewards contract
        producerTokens = new ERC20[](4);
        rewardTokens = new ERC20[](4);
        rewardAmounts = new uint256[](4);
        producerTokens[0] = pxGmx;
        producerTokens[1] = pxGlp;
        producerTokens[2] = pxGmx;
        producerTokens[3] = pxGlp;
        rewardTokens[0] = gmxBaseReward;
        rewardTokens[1] = gmxBaseReward;
        rewardTokens[2] = ERC20(pxGmx); // esGMX rewards distributed as pxGMX
        rewardTokens[3] = ERC20(pxGmx);

        // Get pre-reward claim reward token balances to calculate actual amount received
        uint256 baseRewardBeforeClaim = gmxBaseReward.balanceOf(address(this));
        uint256 esGmxBeforeClaim = stakedGmx.depositBalances(
            address(this),
            address(esGmx)
        );

        // Calculate the unclaimed reward token amounts produced for each token type
        uint256 gmxBaseRewards = _calculateRewards(true, true);
        uint256 glpBaseRewards = _calculateRewards(true, false);
        uint256 gmxEsGmxRewards = _calculateRewards(false, true);
        uint256 glpEsGmxRewards = _calculateRewards(false, false);

As you can see it calls _calculateRewards() to calculate the unclaimed reward token amounts produced for each token type in GMX protocol. so function claimRewards() would revert too when totalSupply() of one of these 4 RewardTracker's distributers was zero. This is part of functions harvest() and claim() code in PirexReward contract:

    function harvest()
        public
        returns (
            ERC20[] memory _producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {
        (_producerTokens, rewardTokens, rewardAmounts) = producer
            .claimRewards();
        uint256 pLen = _producerTokens.length;
.......
......
......


    function claim(ERC20 producerToken, address user) external {
        if (address(producerToken) == address(0)) revert ZeroAddress();
        if (user == address(0)) revert ZeroAddress();

        harvest();
        userAccrue(producerToken, user);
....
....
....

As you can see harvest() calls claimRewards() and claim() calls harvest() so these two function would revert and won't work when totalSupply() of one of these 4 RewardTracker's distributers in GMX protocol was zero. in that situation the protocol can't harvest and claim rewards from GMX because of this bug and users won't be able to claim their rewards from the protocol. the condition for this bug could happen from time to time as GMX decided to prevent it by checking the value of totalSupply(). This is function _updateRewards() code in RewardTracker in GMX protocol (https://github.com/gmx-io/gmx-contracts/blob/65e62b62aadea5baca48b8157acb9351249dbaf1/contracts/staking/RewardTracker.sol#L272-L286):

    function _updateRewards(address _account) private {
        uint256 blockReward = IRewardDistributor(distributor).distribute();

        uint256 supply = totalSupply;
        uint256 _cumulativeRewardPerToken = cumulativeRewardPerToken;
        if (supply > 0 && blockReward > 0) {
            _cumulativeRewardPerToken = _cumulativeRewardPerToken.add(blockReward.mul(PRECISION).div(supply));
            cumulativeRewardPerToken = _cumulativeRewardPerToken;
        }

        // cumulativeRewardPerToken can only increase
....
....

As you can see it checks that supply > 0 before using it as denominator in division. So GMX protocol handles the case when totalSupply() is zero and contract logics won't break when this case happens but function _calculateRewards() which tries to calculate GMX protocol rewards beforehand don't handle this case(the case where totalSupply() is zero) so the logics would break when this case happens and it would cause function harvest() and claim() to be unfunctional.

Tools Used

VIM

Recommended Mitigation Steps

check that totalSupply() is not zero before using it.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L373-L417 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L338-L366 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L739-L816

Vulnerability details

Impact

PirexRewards contract start tracking gmxBaseReward and pxGmx as rewardsToken for pxGmx and pxGlp as producerTokens from the begining of contract deployment because this values hardcoded in claimRewards() function of PirexGmx But those primary rewardTokens didn't get inserted to PirexRewards contract state from the beginning and owner should add them later. When rewardTokens state are wrong in PirexRewards contract (gmxBaseReward and pxGmx is not in list), users calling PirexReward.claim() would lose their rewards because contract set userStates[user].rewards = 0. This condition can happen in contract early stage or when owner tries to change or update rewardTokens. Those primary rewardTokens should be hardcoded and added to rewards token list when contract get initialized because their values are hardcoded in PirexGmx and they should be hardcoded in PirexRewards too.

Proof of Concept

This is function claim() code in PirexRewards contract:

    function claim(ERC20 producerToken, address user) external {
        if (address(producerToken) == address(0)) revert ZeroAddress();
        if (user == address(0)) revert ZeroAddress();

        harvest();
        userAccrue(producerToken, user);

        ProducerToken storage p = producerTokens[producerToken];
        uint256 globalRewards = p.globalState.rewards;
        uint256 userRewards = p.userStates[user].rewards;

        // Claim should be skipped and not reverted on zero global/user reward
        if (globalRewards != 0 && userRewards != 0) {
            ERC20[] memory rewardTokens = p.rewardTokens;
            uint256 rLen = rewardTokens.length;

            // Update global and user reward states to reflect the claim
            p.globalState.rewards = globalRewards - userRewards;
            p.userStates[user].rewards = 0;

            emit Claim(producerToken, user);

            // Transfer the proportionate reward token amounts to the recipient
            for (uint256 i; i < rLen; ++i) {
                ERC20 rewardToken = rewardTokens[i];
                address rewardRecipient = p.rewardRecipients[user][rewardToken];
                address recipient = rewardRecipient != address(0)
                    ? rewardRecipient
                    : user;
                uint256 rewardState = p.rewardStates[rewardToken];
                uint256 amount = (rewardState * userRewards) / globalRewards;

                if (amount != 0) {
                    // Update reward state (i.e. amount) to reflect reward tokens transferred out
                    p.rewardStates[rewardToken] = rewardState - amount;

                    producer.claimUserReward(
                        address(rewardToken),
                        amount,
                        recipient
                    );
                }
            }
        }
    }

As you can see the code transfers all the rewards of user for reward tokens registered for this producerToken based on user share of total rewards and also it sets the value of producerTokens[producerToken].userStates[user].rewards as zero(user share of rewards of this producerToken become zero) so if in the moment of calling the function claim() the rewards token list was wrong(some tokens were missing) then user would lose his right to those rewards. Function claim() is callable by others for any user so attacker can call this function for other users and make them lose access to their rewards. the rewards calculation for producer tokens pxGmx, pxGlp with rewards tokens gmxBaseReward, pxGmx are done from the beginning of the contract but adding those reward tokens should be done by owner dynamically so in contract early stage the reward list of those producer tokens are wrong and even in other times when owner tries to update the reward list(because of any reason like GMX protocol update or by mistake or ....) the list of rewards would be wrong and user would lose funds if attacker call claim() for them. there is no pause mechanism in the PirexRewards contract to prevent calling claim() in those states when rewards lists are wrong. This is harvest() code:

    function harvest()
        public
        returns (
            ERC20[] memory _producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {
        (_producerTokens, rewardTokens, rewardAmounts) = producer
            .claimRewards();
        uint256 pLen = _producerTokens.length;

        // Iterate over the producer tokens and update reward state
        for (uint256 i; i < pLen; ++i) {
            ERC20 p = _producerTokens[i];
            uint256 r = rewardAmounts[i];

            // Update global reward accrual state and associate with the update of reward state
            ProducerToken storage producerState = producerTokens[p];

            _globalAccrue(producerState.globalState, p);

            if (r != 0) {
                producerState.rewardStates[rewardTokens[i]] += r;
            }
        }

        emit Harvest(_producerTokens, rewardTokens, rewardAmounts);
    }

As you can see it update the state of producer and rewards tokens returned by PirexGmx.claimRewards(). This is some part of claimRewards() code in PirexGmx:

    function claimRewards()
        external
        onlyPirexRewards
        returns (
            ERC20[] memory producerTokens,
            ERC20[] memory rewardTokens,
            uint256[] memory rewardAmounts
        )
    {
        // Assign return values used by the PirexRewards contract
        producerTokens = new ERC20[](4);
        rewardTokens = new ERC20[](4);
        rewardAmounts = new uint256[](4);
        producerTokens[0] = pxGmx;
        producerTokens[1] = pxGlp;
        producerTokens[2] = pxGmx;
        producerTokens[3] = pxGlp;
        rewardTokens[0] = gmxBaseReward;
        rewardTokens[1] = gmxBaseReward;
        rewardTokens[2] = ERC20(pxGmx); // esGMX rewards distributed as pxGMX
        rewardTokens[3] = ERC20(pxGmx);
...
....

As you can see the function works all the time and producer tokens pxGmx, pxGlp with rewards tokens gmxBaseReward, pxGmx are hardcoded in the code so it always return them and harvest() would always calculate rewards for those tokens. so in this Scenario:

1. owner deployed contracts but rewards list are not yet updated and it is incorrect for producer tokens pxGmx, pxGlp. (or when owner tries to change or update reward tokens list and list is incorrect in the moment)
2. users deposit their tokens in PirexGmx and protocol stake them in GMX and start getting reward in GMX.
3. attacker would call PirexRewards.harvest() so PirexGmx.claimRewards() would get rewards from GMX and PirexRewards would calculate rewards for primary rewards tokens(gmxBaseReward, pxGmx)
4. attacker would call PirexRewards.claim(PxGmx/PxGLP, user) for users and contract would set users share of rewards of those producer tokens to zero but don't transfer any rewards to user because rewards list for those producer tokens are incorrect.

As you can see when contract is in some states the logics won't work correctly and users would lose their rewards. contract should protect itself from this type of bugs with on-chain mechanism.

Tools Used

VIM

Recommended Mitigation Steps

add producer tokens pxGmx, pxGlp with rewards tokens gmxBaseReward, pxGmx in PirexRewards when contract initialize or make sure claim() can't be called when reward tokens are not yet configured or in wrong states.

[[1]] When using safeApprove(), it's better to first set approve amount to zero and then set the new value, because if for any reason some spending dust has been left for that address then code would revert and contract would stuck in broken state. because contract assumes that other contract would spend all the allowance and make allowance zero again, but due to upgrades or .... other contract may not behave in this manner, so it's better to write the code in some way that the logic never breaks. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L503-L507 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L346-L347 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L391

[[2]] In function removeRewardToken() of PirexRewards contract, code don't check that removalIndex is less than array length, so if someone send incorrect values code would revert with wrong messages. for example if producerTokens[producerToken].rewardTokens was empty then code would revert with underflow error in the line uint256 lastIndex = rewardTokens.length - 1 and if removalIndex >= rewardTokens.length then code would revert with out of bound error message. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L174-L197

[[3]] Function getRewardRecipient() of PirexRewards contract, should return user as a recipient when user didn't set any recipient but instead it returns zero address. when the user didn't set any recipient then the value of producerTokens[producerToken].rewardRecipients[user][rewardToken] is 0x0 and function just returns this value which don't make sense because 0x0 address is not the real recipient, instead function could check for the value and if it's 0x0 it should return user address. like this:

    function getRewardRecipient(
        address user,
        ERC20 producerToken,
        ERC20 rewardToken
    ) external view returns (address) {
        address result = producerTokens[producerToken].rewardRecipients[user][rewardToken];
        if(result == address(0))
                     result  = user;
        return result;
    }

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexRewards.sol#L250-L264

[[4]] library SafeTransferLib imported two times in AutoPxGlp contract. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L6-L8

[[1]] function distributeFees() in PirexFees don't check the amount>0 that is going to get transferred but all other places in the code has this check for fee. contract send contributorsDistribution and treasuryDistribution amount of token without check that the amount != 0. this would cause a lot of unnecessary gas if balance of contract was zero or treasuryFeePercent was zero. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexFees.sol#L96-L116