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:

it's not possible to update the address of PirexGmx in AutoPxGlp and AutoPxGmx if addresses in the protocol has been updated.
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:

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

code-423n4 / 2022-11-redactedcartel-findings