Preventing other users from depositing GMX into AutoPxGmx contract Or from depositing GLP into AutoPxGlp contract

[code423n4]

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.

[c4-judge]

Picodes marked the issue as duplicate of #407

[c4-judge]

Picodes marked the issue as satisfactory

[C4-Staff]

JeeberC4 marked the issue as duplicate of #275