Redacted Cartel contest - gzeon's results

Lines of code

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

Vulnerability details

Impact

According to scoping

- Does it use a side-chain? Yes
- If yes, is the sidechain evm-compatible? Yes, Avalanche

The address 0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45 is an EOA on Avalanche, which will lead to revert when called from the vault contract (empty return value).

Proof of Concept

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

    IV3SwapRouter public constant SWAP_ROUTER =
        IV3SwapRouter(0x68b3465833fb72A70ecDF485E0e4C7bD8665Fc45);

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

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

Recommended Mitigation Steps

Set the swap router address in constructor.

Lines of code

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

Vulnerability details

Impact

When calling deposit, withdraw, and redeem it default to use `amountOutMinimum:1 sqrtPriceLimitX96:0 which may lead to unexpected slippage. Although it is not currently subject to sandwich attack (assuming you trust the Arbitrum sequencer) since there are no mempool.

Proof of Concept

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

        compound(poolFee, 1, 0, true);

Low

Invalid maxDeposit

Per EIP-4626, maxDeposit returns the maximum amount of underlying assets that can be deposited in a single deposit call by the receiver. When balance > 0, you can only deposit type(uint256).max - balance amount of token.

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

    function maxDeposit(address) public view virtual returns (uint256) {
        return type(uint256).max;
    }

Invalid maxMint

Per EIP-4626, maxMint returns the maximum amount of underlying assets that can be deposited in a single deposit call by the receiver. When totalsupply > 0, you can only deposit type(uint256).max - totalsupply amount of token.

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

    function maxMint(address) public view virtual returns (uint256) {
        return type(uint256).max;
    }

Non-Critical

Unused constant

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

    uint256 public constant EXPANDED_DECIMALS = 1e30;

Inconsistant FEE_PERCENT_DENOMINATOR

Multiple value of FEE_PERCENT_DENOMINATOR is used across the codebase, this might easily lead to configuration mistake.

Do not use assert() in production

assert() will consume all gas, use require() instead

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

        assert(feeAmount + postFeeAmount == assets);

Since GMX is on Arbitrum, a L2 rollup, it is much more important to optimize for calldata when considering gas optimization. In fact, EVM gas optimizations have minimal effect when compared to calldata optimizations. For example at 9 gwei L1 gas price and 0.1 gwei Arbitrum gas price, each byte of calldata after compression cost ~640 calldata gas. Here are some suggestions to reduce the calldata size:

1. For all the function with a receiver parameter (e.g. depositGmx(uint256 amount, address receiver)), add a helper function which default it with msg.sender. This saves 20 bytes (or 32 bytes, but those 0 will mostly be compressed away) of calldata ~= 12800 gas.

2. For deposit and withdraw functions, allow the user to specify some magic value to deposit/withdraw max. This saves 32 bytes of calldata ~= 20480 gas.

3. On the UI level, use more compressible value (e.g. 1024(0x400) instead of 1000(0x3E8)) for amounts like minGlp

4. Consider integrate with ArbAddressTable for token addresses and other common addresses.

Resources

• https://developer.offchainlabs.com/
• https://l2fees.info/blog/rollup-calldata-compression