Redacted Cartel contest - R2's results

Lines of code

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

Vulnerability details

Impact

In your AutoPxGlp and AutoPxGmx you calculate totalAssets() as current contract asset balance:

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

This value may be increased directly through token contract And this value affects shares calculation

So there is a way to do griefing attack by frontrunning users deposit() calls, and users will receive less shares than they expected So it will lead to users funds loss

Proof of Concept

See comments in test below There is a case when Bob instantly lost half of his deposit

function testFundsLosing() external {
        // 1. Alice deposits 10 pxGmx and receive 10 autoPxGmx
        uint256 aliceAssets = 10;
        address alice = address(this);

        _depositGmx(aliceAssets, alice);
        assertEq(pxGmx.totalSupply(), 10);

        pxGmx.approve(address(autoPxGmx), aliceAssets);
        autoPxGmx.deposit(aliceAssets, alice);
        assertEq(aliceAssets, autoPxGmx.totalAssets());
        assertEq(aliceAssets, autoPxGmx.balanceOf(alice));

        // She can withdraw her tokens at any moment
        uint256 redeemedAssets = autoPxGmx.previewRedeem(autoPxGmx.balanceOf(alice));
        assertEq(aliceAssets, redeemedAssets);
        
        // 2. Sid - is a blackhat and wants Bob to lose his money
        // He frontruns Bobs 'deposit' transaction
        // And transfer his pxGmx directrly to autoPxGmx contract
        uint256 sidAssets = 1000;
        address sid = testAccounts[2];
        _depositGmx(sidAssets, sid);
        vm.prank(sid);
        pxGmx.transfer(address(autoPxGmx), sidAssets);

        // 3. Bob wants to deposit 500 pxGmx
        uint256 bobAssets = 500;
        address bob = testAccounts[1];
        _depositGmx(bobAssets, bob);
        vm.prank(bob);
        pxGmx.approve(address(autoPxGmx), bobAssets);
        vm.prank(bob);
        autoPxGmx.deposit(bobAssets, bob);
        assertEq(bobAssets + aliceAssets + sidAssets, autoPxGmx.totalAssets());
        
        // Now Alice can redeem a lot of tokens
        redeemedAssets = autoPxGmx.previewRedeem(autoPxGmx.balanceOf(alice));
        assertEq(1046, redeemedAssets);

        // And Bob can redeem less tokens than he just deposited
        redeemedAssets = autoPxGmx.previewRedeem(autoPxGmx.balanceOf(bob));
        assertEq(419, redeemedAssets);
    }

Tools Used

vs code

Recommended Mitigation Steps

Now totalAssets() value may be increased outside your contract logic It may lead to many other unpredictable hacks, not that one I described above So it would be better, to calculate totalAssets by your own

1. Remove that function, store totalAssets in uint256 in PirexERC4626 and increase directly inside deposit(), mint(), withdraw(), redeem()

2. Add function to sweep tokens occasionally sent directly to your contract

Lines of code

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

Vulnerability details

Impact

In AutoPxGmx.compound() you useUniswapV3 router to change gmxBaseReward token (WETH/WAVAX) to gmx (GMX) token

But you use 1 as amountOutMinimum when call compound() from the next contract functions:

• AutoPxGmx.withdraw()
• AutoPxGmx.redeem()
• AutoPxGmx.beforeDeposit()

And malicious user can use sandwich attack (buy GMX before you and then sell it after you). As a result, in the worst case you will buy just 1 GMX token for all your WETH/WAVAX, so you will lose you base rewards (Actually it will not be just 1 GMX because of AMM math, it's just an example)

Proof of Concept

Initial data:

• arbitrum network
• AutoPxGmx contract already has WETH on balance
• totalAssets() != 0 already, because of many transactions happened

Then:

1. Alice wants to deposit GMX though depositGmx() function There you have the next code:

if (totalAssets() != 0) beforeDeposit(address(0), 0, 0);

Because totalAssets() != 0, you call beforeDeposit(address(0), 0, 0)

2. In beforeDeposit() there is the next call:

compound(poolFee, 1, 0, true);

So you sent 1 as amountOutMinimum argument

3. In compound() there is the next call:

gmxAmountOut = SWAP_ROUTER.exactInputSingle(
                IV3SwapRouter.ExactInputSingleParams({
                    tokenIn: address(gmxBaseReward),
                    tokenOut: address(gmx),
                    fee: fee,
                    recipient: address(this),
                    amountIn: gmxBaseRewardAmountIn,
                    amountOutMinimum: amountOutMinimum,
                    sqrtPriceLimitX96: sqrtPriceLimitX96
                })
            );

So you use UniswapV3 router with 1 as amountOutMinimum

4. Bob sees your transaction in mempool and send his own transaction before Alice's one. He bought many GMT tokens and increase it's price

5. Alice bought just 1 GMX and spent all WETH tokens GMX price increased again

6. Bob sells all his GMX tokens and get incredible profit

(Actually it will not be just 1 GMX because of AMM math, it's just an example)

Tools Used

vs code

Recommended Mitigation Steps

Check WETH/GMX price and set correct amountOutMinimum E.g. last weighted price (use only TWAP prices, not instant price) - 2%

Lines of code

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

Vulnerability details

Impact

In PirexGmx and AutoPxGlp you have function depositGlp(), which accepts any ERC20 token from whitelist Now there are 9 tokens (see here: https://arbiscan.io/address/0x489ee077994b6658eafa855c308275ead8097c4a#readContract): WBTC, WETH, USDC, LINK, UNI, USDT, MIM, FRAX, DAI And the list may extend

So any user can deposit any of those tokens and receive pxGlp token:

    function testUSDTDepositGlp() external {
        // 0 USDT TOKENS
        address myAddress = address(this);
        assertEq(usdt.balanceOf(myAddress), 0);

        // The one with many USDT tokens
        vm.prank(0xB6CfcF89a7B22988bfC96632aC2A9D6daB60d641);
        uint256 amount = 100000;
        usdt.transfer(myAddress, amount);

        // amount USDT TOKENS
        assertEq(usdt.balanceOf(myAddress), amount);

        // DEPOSIT USDT TOKENS
        usdt.approve(address(pirexGmx), amount);
        pirexGmx.depositGlp(address(usdt), amount, 1, 1, address(this));
        
        // SUCCESSSFULLY DEPOSITED
        assertEq(usdt.balanceOf(address(this)), 0);
        assertEq(pxGlp.balanceOf(address(this)), 118890025839780442);
    }

But if of this tokens will start charge fee on transfers, the logic will be broken and calls to depositGlp() with suck token will fail

Because here you use the amount of tokens sent from user wallet: https://github.com/code-423n4/2022-11-redactedcartel/blob/03b71a8d395c02324cb9fdaf92401357da5b19d1/src/PirexGmx.sol#L512

            t.safeTransferFrom(msg.sender, address(this), tokenAmount);

            // Mint and stake GLP using ERC20 tokens
            deposited = gmxRewardRouterV2.mintAndStakeGlp(
                token,
                tokenAmount,
                minUsdg,
                minGlp
            );

And then gmxRewardRouterV2 tries to transfer tokens to his balance from your balance:

IERC20(_token).safeTransferFrom(_fundingAccount, address(vault), _amount);

(See https://arbiscan.io/address/0x321f653eed006ad1c29d174e17d96351bde22649#code - GlpManager and https://arbiscan.io/address/0xA906F338CB21815cBc4Bc87ace9e68c87eF8d8F1#code - RewardRouterV2)

But you received less than tokenAmount tokens because of fee. And transaction will fail

Proof of Concept

Let's imagine USDT in arbitrub started to charge fees 1% per transfer

Alice wants to deposit 100 USDT through PirexGmx.depositGlp() Then you do t.safeTransferFrom(Alice, address(this), 100); You will receive only 99 USDT

But in the next line you will try to send 100 USDT:

deposited = gmxRewardRouterV2.mintAndStakeGlp(
                token,
                tokenAmount,
                minUsdg,
                minGlp
            );

So transaction fails and Alice can't get pxGlp tokens

Tools Used

vs code

Recommended Mitigation Steps

USDT already has fees in other blockchains. Many of these tokens use proxy pattern (and USDT too). It's quite probably that in one day one of the tokens will start charge fees. Or you would like to add one more token to whitelist and the token will be with fees

Thats why I consider finding as medium severity

To avoid problems, use common pattern, when you check your balance before operation and balance after, like that:

            uint256 balanceBefore = t.balanceOf(address(this));
            t.safeTransferFrom(msg.sender, address(this), tokenAmount);
            uint256 balanceAfter = t.balanceOf(address(this));

            uint256 tokenAmount = balanceAfter - balanceBefore;

            // Mint and stake GLP using ERC20 tokens
            deposited = gmxRewardRouterV2.mintAndStakeGlp(
                token,
                tokenAmount,
                minUsdg,
                minGlp
            );

1. PirexGmx: rewards may be claimed even if protocol is paused

Impact

Even if you will find some bug/vulnerability and pause the protocol, anyone can claim rewards. Because there are not whenNotPaused modifiers in claimUserReward() and claimRewards() functions

Recommended Mitigation Steps

Add modifier whenNotPaused

2. PirexRewards.addRewardToken(): any token may be added

Impact

There are not checks in the PirexRewards.addRewardToken() function, so owner may add any token. But if it's not WETH/WAVAX or GMX - it will be ignored in PirexRewards.claimUserReward():

        if (token == address(pxGmx)) {
            // Mint pxGMX for the user - the analog for esGMX rewards
            pxGmx.mint(receiver, postFeeAmount);

            if (feeAmount != 0) pxGmx.mint(address(pirexFees), feeAmount);
        } else if (token == address(gmxBaseReward)) {
            // @audit-ok тут может не хватать баланса???
            // Как он пополняется, что это за токен вообще нахер
            gmxBaseReward.safeTransfer(receiver, postFeeAmount);

            if (feeAmount != 0)
                gmxBaseReward.safeTransfer(address(pirexFees), feeAmount);
        }

So if token doesn't match any clause, it will be ignored

It may lead to misunderstanding for users

Recommended Mitigation Steps

Do not allow to add any token except of WETH/WAVAX and GMX