GimelSec - The calculation of `feeAmount ` is incorrect in `Perp._placePerpOrder`

[github-actions[bot]]

GimelSec

medium

The calculation of feeAmount is incorrect in Perp._placePerpOrder

Summary

The calculation of feeAmount is incorrect in Perp._placePerpOrder. It uses the returned quoteAmount to calculate the fee paid in opening and closing positions. But the amount of the fee has already been removed from quoteAmount. Thus, the calculation based on quoteAmount is incorrect.

Vulnerability Detail

Perp._placePerpOrder calculates feeAmount based on quoteAmount.

    function _placePerpOrder(
        uint256 amount,
        bool isShort,
        bool amountIsInput,
        uint160 sqrtPriceLimit
    ) private returns (uint256, uint256) {
        …

        (uint256 baseAmount, uint256 quoteAmount) = clearingHouse.openPosition(
            params
        );
        uint256 feeAmount = _calculatePerpOrderFeeAmount(quoteAmount);
        totalFeesPaid += feeAmount;

        …
    }

But the fee has already been removed from quoteAmount https://github.com/perpetual-protocol/perp-curie-contract/blob/main/contracts/Exchange.sol#L534

    function _swap(SwapParams memory params) internal returns (InternalSwapResponse memory) {
        …

        return
            InternalSwapResponse({
                base: exchangedPositionSize,
                quote: exchangedPositionNotional.sub(replayResponse.fee.toInt256()), // the fee is removed.
                exchangedPositionSize: exchangedPositionSize,
                exchangedPositionNotional: exchangedPositionNotional,
                fee: replayResponse.fee,
                insuranceFundFee: replayResponse.insuranceFundFee,
                tick: replayResponse.tick
            });
    }

ClearingHouse._openPositionFor returns the correct amount of fee. But ClearingHouse.openPosition ignores the returned fee. https://github.com/perpetual-protocol/perp-curie-contract/blob/main/contracts/ClearingHouse.sol#L1009

    function _openPositionFor(address trader, OpenPositionParams memory params)
        internal
        returns (
            uint256 base,
            uint256 quote,
            uint256 fee
        )
    {
        …

        return (response.base, response.quote, response.fee);
    }

    function openPosition(OpenPositionParams memory params)
        external
        override
        whenNotPaused
        nonReentrant
        checkDeadline(params.deadline)
        returns (uint256 base, uint256 quote)
    {
        // openPosition() is already published, returned types remain the same (without fee)
        (base, quote, ) = _openPositionFor(_msgSender(), params);
        return (base, quote);
    }

Impact

The wrong calculation of feeAmount causes an accounting error. It won’t immediately harm the protocol. But it could lead to a potential threat in the future.

Code Snippet

https://github.com/sherlock-audit/2023-01-uxd/blob/main/contracts/integrations/perp/PerpDepository.sol#L370

Tool used

Manual Review

Recommendation

Fix the calculation wrongly based on quoteAmount. Or simply use ClearingHouse.openPositionFor instead of ClearingHouse.openPosition.

ClearingHouse.openPositionFor returns fee https://github.com/perpetual-protocol/perp-curie-contract/blob/main/contracts/ClearingHouse.sol#L396

    function openPositionFor(address trader, OpenPositionParams memory params)
        external
        override
        whenNotPaused
        nonReentrant
        checkDeadline(params.deadline)
        returns (
            uint256 base,
            uint256 quote,
            uint256 fee
        )
    {
        // CH_SHNAOPT: Sender Has No Approval to Open Position for Trader
        require(IDelegateApproval(_delegateApproval).canOpenPositionFor(trader, _msgSender()), "CH_SHNAOPT");

        return _openPositionFor(trader, params);
    }

Duplicate of #271