Redacted Cartel contest - HE1M's results

Lines of code

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

Vulnerability details

Impact

There is a possibility to prevent any user from redeeming in PirexGmx.sol.

Please note that this attack uses two other contracts that are not in the scope, but the execution and logic of the attack depends on the in-scope contracts.

Proof of Concept

A user can deposit ETH or GMX-whitelisted token into PirexGmx.sol to get minted pxGlp.

function depositGlp(
        address token,
        uint256 tokenAmount,
        uint256 minUsdg,
        uint256 minGlp,
        address receiver
    )
        external
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {
        if (token == address(0)) revert ZeroAddress();
        if (!gmxVault.whitelistedTokens(token)) revert InvalidToken(token);

        return _depositGlp(token, tokenAmount, minUsdg, minGlp, receiver);
    }

function depositGlpETH(
        uint256 minUsdg,
        uint256 minGlp,
        address receiver
    )
        external
        payable
        whenNotPaused
        nonReentrant
        returns (
            uint256,
            uint256,
            uint256
        )
    {
        return _depositGlp(address(0), msg.value, minUsdg, minGlp, receiver);
    }

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

Suppose a user deposits some amount of a whitelisted token. Then, this amount of token will be approved to be consumed by the glpManager. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L507

Moreover, the function mintAndStakeGlp will be called in gmxRewardRouterV2, in which it calls the glpManager to add liquidity for PirexGmx. https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L510 https://snowtrace.io/address/0x82147c5a7e850ea4e28155df107f2590fd4ba327#code#F1#L132

When adding liquidity in the glpManager, a mapping lastAddedAt[PirexGmx address] = block.timestamp will be set. Please note that address of PirexGmx will be tracked in this mapping, not the address of user. https://snowtrace.io/address/0xe1ae4d4b06a5fe1fc288f6b4cd72f9f8323b107f#code#F1#L176 https://snowtrace.io/address/0x82147c5a7e850ea4e28155df107f2590fd4ba327#code#F1#L131

When calling the functions redeemPxGlp or redeemPxGlpETH, the same flow will be executed again: redeemPxGlp in PirexGmx.sol ==> unstakeAndRedeemGlp in gmxRewardRouterV2 ==> removeLiquidityForAccount in glpManager

In the glpManager, it is checked that the cooldown duration is passed or not through: require(lastAddedAt[PirexGmx address].add(cooldownDuration) <= block.timestamp, "GlpManager: cooldown duration not yet passed"); https://snowtrace.io/address/0xe1ae4d4b06a5fe1fc288f6b4cd72f9f8323b107f#code#F1#L185

In which, cooldownDuration is equal to 900 seconds (15 minutes).

This provides an attack surface that whenever a user would like to redeem in PirexGmx.sol, an attacker quickly before the user deposits very small amount in PirexGmx.sol so that lastAddedAt will be updated in the glpManager, and the user's transaction will be reverted. So, the user should wait for 15 more minutes to again call redeem function.

Please note that the mapping lastAddedAt[account] tracks the time the PirexGmx address deposits some amount, not the user's address, because the deposited amount is transferred from the user to PirexGmx and then from PirexGmx to glpManager.

Tools Used

Recommended Mitigation Steps

There should be a limitation on the minimum amount that can be deposited, so that the attacker must pay higher to apply this attack.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L397

Vulnerability details

Impact

An attacker can prevent any user from depositing into PirexGmx contract, as a result this contract will have no use.

Proof of Concept

Suppose the AutoPxGmx.sol has just deployed, so it will have the following initial conditions:

• totalAssets() = 0: this shows the balance of pxGMX custodied by the AutoPxGmx contract
• totalSupply = 0: this tracks the number of share tokens minted apxGMX

Then, Bob (malicious user) deposits 1 token GMX as the first user by calling depositGmx in AutoPxGmx.sol:

function depositGmx(uint256 amount, address receiver)
        external
        nonReentrant
        returns (uint256 shares)
    {
        if (amount == 0) revert ZeroAmount();
        if (receiver == address(0)) revert ZeroAddress();

        // Handle compounding of rewards before deposit (arguments are not used by `beforeDeposit` hook)
        if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);

        // Intake sender GMX
        gmx.safeTransferFrom(msg.sender, address(this), amount);

        // Convert sender GMX into pxGMX and get the post-fee amount (i.e. assets)
        (, uint256 assets, ) = PirexGmx(platform).depositGmx(
            amount,
            address(this)
        );

        // NOTE: Modified `convertToShares` logic to consider assets already being in the vault
        // and handle it by deducting the recently-deposited assets from the total
        uint256 supply = totalSupply;

        if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);
    }

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L370

As a result, this 1 token GMX will be converted to pxGMX, and 1 share token will be minted to Bob, so we will have:

• totalAssets() = 1
• totalSupply = 1

Now, suppose Alice (honest user) would like to deposit 100 token GMX in AutoPxGmx contract by calling depositGmx. Bob notices Alice's transaction, and front-runs her by transferring 100 token pxGMX directly to AutoPxGmx contract not through depositGmx function. Since Bob's transfer is not through the function depositGmx, no share token will be minted to Bob. So we will have:

• totalAssets() = 1 + 100 = 101
• totalSupply = 1

Now Alice's transaction will be executed. During the calculation for amount of share tokens to be minted to Alice, supply is equal to 1, so the second statement of if block will be executed: assets.mulDivDown(supply, totalAssets() - assets). This statement will be equal to 0, because 100 * 1 / (201 - 100) = 100 / 101 = 0. So, value of shares will be equal to zero, and Alice's transaction will be reverted ZeroShares().

if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();

So we will have still the same state as before:

• totalAssets() = 101
• totalSupply = 1

Now, Bob can call the function redeem(1, Bob, Bob):

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/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L339

This function will call the function previewRedeem(shares). In this function the amount of assets pxGMX which is 101 will be transferred to Bob, and 1 share token apxGMX will be burned. Moreover, Bob will not pay any withdrawal penalty, because _totalSupply - shares = 0.

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

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L173

As a summary, Bob can prevent any user from depositing into AutoPxGmx contract by depositing just 1 token GMX to have nonzero totalSupply and then front-running any user by depositing the same amount of the user's deposit. As a result, the contract AutoPxGmx will be useless.

Please note that Bob can prepare token pxGMX by depositing GMX in the contract PirexGmx.sol by calling depositGmx.

The same vulnerability also exists is in the contract AutoPxGlp during depositing Glp/FsGlp/GlpETH, so maybe it is not reasonable to repeat the same scenario explanation for that contract as well, or make another report.

function _deposit(uint256 assets, address receiver)
        internal
        returns (uint256 shares)
    {
        // Check for rounding error since we round down in previewDeposit.
        uint256 supply = totalSupply;

        if (
            (shares = supply == 0
                ? assets
                : assets.mulDivDown(supply, totalAssets() - assets)) == 0
        ) revert ZeroShares();

        _mint(receiver, shares);

        emit Deposit(msg.sender, receiver, assets, shares);

        afterDeposit(receiver, assets, shares);
    }

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGlp.sol#L314

Tools Used

Recommended Mitigation Steps

The solution is to mint a large amount of apxGMX into AutoPxGmx at the time of deploy. For example, if 100000 apxGMX is minted to this contract, the totalSupply will be qual to 100000. So, Bob to apply the same attack in the previous scenario, he should front-runs Alice and transfers directly 100000 pxGMX, so makes the attack almost impossible for Bob.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L275

Vulnerability details

Impact

It is possible that an attacker can prevent any user from calling the functions withdraw, redeem, or depositGmx in contract AutoPxGmx by just manipulating the balance of token gmxBaseReward, so that during the function compound the swap will be reverted.

Proof of Concept

Whenever a user calls the functions withdraw, redeem, or depositGmx in contract AutoPxGmx, the function compound is called: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L321 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L345 https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L379

The function compound claims token gmxBaseReward from rewardModule: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L262

Then, if the balance of the token gmxBaseReward in custodian of the contract AutoPxGmx is not zero, the token gmxBaseReward will be swapped to token GMX thrrough uniswap V3 by callind the function exactInputSingle. Then the total amount of token GMX in custodian of the contract AutoPxGmx will be deposited in the contract PirexGmx to receive token pxGMX:

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

Whenever the function compound is called inside the mentioned functions, the parameters are: compound(poolFee, 1, 0, true);

• fee = poolFee
• amountOutMinimum = 1
• sqrtPriceLimitX96 = 0
• optOutIncentive = true

The vulnerability is the parameter amountOutMinimum which is equal to 1. This provides an attack surface so that if the balance of token gmxBaseReward in AutoPxGmx is nonzero and small enough that does not worth 1 token GMX, the swap procedure will be reverted.

For example, if the balance of gmxBaseReward is equal to 1, then since the value of gmxBaseReward is lower than token GMX, the output amount of GMX after swapping gmxBaseReward will be zero, and as parameter amountOutMinimum is equal to 1, the swap will be reverted, and as a result compound function will be reverted.

Attack Scenario:

Suppose, recently the function compound was called, so the balance of token gmxBaseReward in contract AutoPxGmx is equal to zero. Later, Alice (honest user) would like to withdraw. So, she calls the function withdraw(...).

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/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/vaults/AutoPxGmx.sol#L315

In a normal situation, the function compound will be called, and since the balance of gmxBaseReward is zero, no swap will be executed in uniswap V3, and the rest of the code logic will be executed.

But in this scenario, before the Alice's transaction, Bob transfers 1 token gmxBaseReward directly to contract AutoPxGmx . So, when Alice's transaction is going to be executed, inside the function compound the swap function will be called (because the balance gmxBaseReward is equal to 1 now). In the function exactInputSingle in uniswap V3, there is a check: require(amountOut >= params.amountOutMinimum, 'Too little received'); https://etherscan.io/address/0xe592427a0aece92de3edee1f18e0157c05861564#code#F1#L128

This check will be reverted, because 1 token of gmxBaseReward is worth less than 1 token of GMX, so the amount out will be zero which is smaller than amountOutMinimum.

In summary, an attacker before user's deposit, checks the balance of token gmxBaseReward in AutoPxGmx. If this balance is equal to zero, the attacker transfers 1 token gmxBaseReward to contract AutoPxGmx, so the user's transaction will be reverted, and user should wait until this balance reaches to the amount that worth more than or equal to 1 toke GMX.

Tools Used

Recommended Mitigation Steps

The parameter amountOutMinimum should be equal to zero when the function compound is called. compound(poolFee, 0, 0, true);