Redacted Cartel contest - Ruhum's results

Lines of code

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

Vulnerability details

Impact

The ERC4626 contract's previewWithdraw() function is supposed to round the value up: https://eips.ethereum.org/EIPS/eip-4626#security-considerations

Finally, EIP-4626 Vault implementers should be aware of the need for specific, opposing rounding directions across the different mutable and view methods, as it is considered most secure to favor the Vault itself during calculations over its users: If (1) it’s calculating how many shares to issue to a user for a certain amount of the underlying tokens they provide or (2) it’s determining the amount of the underlying tokens to transfer to them for returning a certain amount of shares, it should round down. If (1) it’s calculating the amount of shares a user has to supply to receive a given amount of the underlying tokens or (2) it’s calculating the amount of underlying tokens a user has to provide to receive a certain amount of shares, it should round up.

But, the overwritten implementation of AutoPxGlp and AutoPxGmx round down. Deviating from the specified standard can cause issues for other parties integrating these two vaults.

Also, the withdraw() function in the standard implementation doesn't verify that the previewWithdraw() function doesn't return 0 shares because it expects the function to round up. The AutoPxGlp and AutoPxGmx contracts don't have that check added which could result in the function sending assets to the caller while burning no shares.

Proof of Concept

The overwritten previewWithdraw() function rounds down:

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

In the withdraw() function it doesn't verify that shares != 0. It still has the comment that previewWithdraw() is expected to round up: https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L323

    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.

        // ....
    }

See https://github.com/transmissions11/solmate/blob/main/src/mixins/ERC4626.sol for the standard implementation.

Tools Used

none

Recommended Mitigation Steps

previewWithdraw() should round up.

Lines of code

https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L268-L278 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L227 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L321 https://github.com/code-423n4/2022-11-redactedcartel/blob/main/src/vaults/AutoPxGmx.sol#L345

Vulnerability details

Impact

The AutoPxGmx contract swaps the rewarded WETH to GMX every time a user deposits or withdraws from the vault without slippage protection. With the current state of MEV, you can be sure that your swaps will be sandwiched. With every swap, you will receive less GMX than you should. That's a direct loss of funds for the users since it reduces the actual yield users receive.

I think this issue warrants a HIGH classification because of the prevalence of MEV. About 90% of all blocks are produced using MEV-Boost. So your transactions will most likely land in a block where the majority of searchers are also present. Since sandwiching is one of the core strategies you can be sure that your transaction will also be picked up. It won't be a rare occurrence. The majority, if not all, of your swaps, will be sandwiched. As specified in the judging criteria, I believe this classifies as an indirect attack without "hand-wavy hypotheticals":

High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals)

https://docs.code4rena.com/awarding/judging-criteria

Proof of Concept

For each swap, the amountOutMinimum value is set to the parameter passed to the compound() function:

    function compound(
        uint24 fee,
        uint256 amountOutMinimum,
        uint160 sqrtPriceLimitX96,
        bool optOutIncentive
    )
        public
        returns (
            uint256 gmxBaseRewardAmountIn,
            uint256 gmxAmountOut,
            uint256 pxGmxMintAmount,
            uint256 totalFee,
            uint256 incentive
        )
    {
        // ...

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

In depositGmx() the compound() function is triggered through beforeDeposit() where amountOutMinimum is set to 1:

    function beforeDeposit(
        address,
        uint256,
        uint256
    ) internal override {
        compound(poolFee, 1, 0, true);
    }

In withdraw() and redeem() compound() is triggered directly also with amountOutMinimum set to 1:

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


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

Tools Used

none

Recommended Mitigation Steps

You have to get the price from an on-chain oracle and then use that to compute an actual min value and use that. Make sure to add that computation to the compound() function itself. compound() is a public function that can be called by anyone directly. If you do it outside of it and pass the result to it, you still have the risk of an attacker triggering compound() without slippage protection.