Redacted Cartel contest - rvierdiiev's results

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L651-L662

Vulnerability details

Impact

Cooldown period of GlpManager that is 15 minutes now can restrict users from withdrawing their assets

Proof of Concept

Gmx GlpManager has cooldown period(15 minutes currently) that means that after you staked you can't unstake for some amount of time. https://arbiscan.io/address/0x321F653eED006AD1C29D174e17d96351BDe22649#code#L938

require(lastAddedAt[_account].add(cooldownDuration) <= block.timestamp, "GlpManager: cooldown duration not yet passed");

When users deposit directly through the PirexGmx or using AutoPxGlp or AutoPxGmx then their assets are transferred to PirexGmx and then this contract stakes to gmx system. That means that every time any user wants to deposit some assets, then PirexGmx will not be able to unstake during cooldown period. Note that max cooldown period is 48 hours. Then when someone want to withdraw it should burn his pxGmx tokens through PirexGmx which will call unstaling on gmx system. If cooldown period is not passed yet then users will not be able to withdraw their assets.

Tools Used

VsCode

Recommended Mitigation Steps

You can use pausing mechanism to make contract nor accept deposits when paused and allow people to withdraw when paused. That means that in case when people can't withdraw, because a lot of deposits you call pause and then people can't deposit. After cooldown period another people can withdraw, but you should remove whenNotPaused modifier from redeem functions.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L177-L195

Vulnerability details

Impact

AutoPxGlp.previewWithdraw doesn't round up shares count and allows to burn less shares than needed using withdraw function.

Proof of Concept

Per EIP 4626's Security Considerations (https://eips.ethereum.org/EIPS/eip-4626) previewWithdraw function should round up. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L177-L195

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

To calculate shares amount AutoPxGlp.previewWithdraw function uses convertToShares function which calculates shares with rounding down. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L156-L165

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

Later there is no any rounding up as you can see. Because of that in case when assets amount that user wants to withdraw is not equal to integer value of shares, then less amount of shares will be burnt and as result protocol will have loses.

The same thing exists in AutoPxGmx.previewWithdraw.

Also there is another thing that attacker can do here. If he call withdraw with small assets count such that shares amount will be < 1, then it will be rounded to 0 and in this case attacker don't need to have allowance(as he withdraw 0 shares) and he can take such small amounts from any account. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/PirexERC4626.sol#L99-L122

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

Tools Used

VsCode

Recommended Mitigation Steps

When calculate shares to burn, you need to round that amount up.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L304-L356 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L422-L465

Vulnerability details

Impact

Share price manipulation is possible for the first depositor of AutoPxGlp by depositing small amount first to AutoPxGlp and then transferring some pxGMX tokens to AutoPxGlp directly.

Proof of Concept

When users deposit into AutoPxGlp, new pxGMX tokens are minted for the AutoPxGlp. To know how many assets are controlled by AutoPxGlp it uses totalAssets function. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L142-L144

    function totalAssets() public view override returns (uint256) {
        return asset.balanceOf(address(this));
    }

When user provides tokens he wants to deposit they are sent to PirexGmx which then mints some amount of pxGMX tokens for AutoPxGlp. Later using that amount of minted tokens and totalAssets amount the amount of shares in AutoPxGlp is calculated. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L304-L322

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

Also you should note that it's possible for anyone to mint pxGMX tokens directly through the PirexGmx contract to have some pxGMX. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L422-L465

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


        // Transfer the caller's fsGLP (unstaked for the user, staked for this contract)
        stakedGlp.transferFrom(msg.sender, address(this), amount);


        // Get the pxGLP amounts for the receiver and the protocol (fees)
        (uint256 postFeeAmount, uint256 feeAmount) = _computeAssetAmounts(
            Fees.Deposit,
            amount
        );


        // Mint pxGLP for the receiver (excludes fees)
        pxGlp.mint(receiver, postFeeAmount);


        // Mint pxGLP for fee distribution contract
        if (feeAmount != 0) {
            pxGlp.mint(address(pirexFees), feeAmount);
        }


        emit DepositGlp(
            msg.sender,
            receiver,
            address(stakedGlp),
            0,
            0,
            0,
            amount,
            postFeeAmount,
            feeAmount
        );


        return (amount, postFeeAmount, feeAmount);
    }

This all allows first depositor to manipulate with share price. 1.Attacker deposit through PirexGmx.depositFsGlp(or any similar function) and mints huge amount of pxGMX tokens. 2.Attacker deposit(as first depositor) through AutoPxGlp.depositFsGlp(or any similar function) with minimum amount. 3.Attacker transfer his big amount of pxGMX tokens to AutoPxGlp. 4.Now because of big amount of totalAssets and small amount of totalSupply all next depositors will lose some tokens because of rounding while attacker will benefit as he will receive more tokens.

Tools Used

VsCode

Recommended Mitigation Steps

Restrict first depositor of AutoPxGlp to deposit big amount of assets to mint big amount of shares on start.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L921-L935

Vulnerability details

Impact

PirexGmx.initiateMigration can be blocked so contract will not be able to migrate his funds to another contract using gmx.

Proof of Concept

PirexGmx was designed with the thought that the current contract can be changed with another during migration. PirexGmx.initiateMigration is the first point in this long process. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/PirexGmx.sol#L921-L935

    function initiateMigration(address newContract)
        external
        whenPaused
        onlyOwner
    {
        if (newContract == address(0)) revert ZeroAddress();


        // Notify the reward router that the current/old contract is going to perform
        // full account transfer to the specified new contract
        gmxRewardRouterV2.signalTransfer(newContract);


        migratedTo = newContract;


        emit InitiateMigration(newContract);
    }

As you can see gmxRewardRouterV2.signalTransfer(newContract); is called to start migration. This is the code of signalTransfer function https://arbiscan.io/address/0xA906F338CB21815cBc4Bc87ace9e68c87eF8d8F1#code#F1#L282

    function signalTransfer(address _receiver) external nonReentrant {
        require(IERC20(gmxVester).balanceOf(msg.sender) == 0, "RewardRouter: sender has vested tokens");
        require(IERC20(glpVester).balanceOf(msg.sender) == 0, "RewardRouter: sender has vested tokens");

        _validateReceiver(_receiver);
        pendingReceivers[msg.sender] = _receiver;
    }

As you can see the main condition to start migration is that PirexGmx doesn't control any gmxVester and glpVester tokens. So attacker can deposit and receive such tokens and then just transfer tokens directly to PirexGmx. As a result migration will never be possible as there is no possibility for PirexGmx to burn those gmxVester and glpVester tokens.

Also in same way migration receiver can also be blocked.

Tools Used

VsCode

Recommended Mitigation Steps

Think about how to make contract to be sure that he doesn't control any gmxVester and glpVester tokens before migration.

Lines of code

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

Vulnerability details

Impact

AutoPxGmx.compound can be frontrunned and because no slippage is provided it's possible that the price for swap will be very bad and received amount will be small.

Proof of Concept

AutoPxGmx.compound function is called very often in the AutoPxGmx contract to update rewards that are earned by users. And all time the function is called like this compound(poolFee, 1, 0, true);. In such scenario the minimum swap amount is only 1.

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

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


        emit Compounded(
            msg.sender,
            fee,
            amountOutMinimum,
            sqrtPriceLimitX96,
            gmxBaseRewardAmountIn,
            gmxAmountOut,
            pxGmxMintAmount,
            totalFee,
            incentive
        );
    }

Later function use that amountOutMinimum == 1 param to make swap with uniswap. Also sqrtPriceLimitX96 param is set to 0 which also switch off price control.

That means that in case when price suddenly changes the swap can still be executed and gmx tokens will be swapped for a very small amount.

Also because the function compound is public, everyone can call it. That means that someone can call the function right after he changed the price in the pool to make protocol receive less rewards.

Also the same problem has AutoPxGlp contract.

Tools Used

VsCode

Recommended Mitigation Steps

Add some slippage protection, for example you can calculate how much you expect to receive and then make slippage 10%.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L152-L158 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L130-L136 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L97 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L87

Vulnerability details

Impact

AutoPxGmx.setPlatform and AutoPxGlp.setPlatform do not provide approve for new platform address. As a result both contracts will not be able to work normally as platform will not be able to transfer any funds from both contracts.

Proof of Concept

AutoPxGmx.setPlatform and AutoPxGlp.setPlatform allow for the owner to provide new platform address. However in this methods approve is not provided for new platform as it is done in constructors of both contracts. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L97

        gmx.safeApprove(_platform, type(uint256).max);

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGlp.sol#L87

        gmxBaseReward.safeApprove(address(_platform), type(uint256).max);

As result after the owner will change platform contracts will not work normally as platform will not be able to transfer tokens from contracts. Also there is no any other method that can grant allowance, that means that owner will not be able to do anything, except return to new platform address.

Tools Used

VsCode

Recommended Mitigation Steps

Grant allowance for the new platform in both methods.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L104-L110

Vulnerability details

Impact

AutoPxGmx.setPoolFee doesn't check that pool with such fee exists, as result compound function will not be working and deposits and withdraws as well.

Proof of Concept

Function AutoPxGmx.setPoolFee allows to set pool fee for uniswap. https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L104-L110

    function setPoolFee(uint24 _poolFee) external onlyOwner {
        if (_poolFee == 0) revert ZeroAmount();


        poolFee = _poolFee;


        emit PoolFeeUpdated(_poolFee);
    }

This poolFee is then used to make uniswap calls in compound function. However if uniswap can't finds pool with such fee then transaction will revert. As result user will not be able to make deposits and withdraws as they depend on compound function.

Tools Used

VsCode

Recommended Mitigation Steps

Check that such pool exists in uniswap when setting new poolFee.