Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cheerful Taffy Dolphin - Missing Execution Rights Validation in _cancelOrder() Enables Front-Running of Signed Cancel Orders and Market Manipulation #52

Open
sherlock-admin3 opened this issue Jan 31, 2025 · 0 comments

Comments

@sherlock-admin3
Copy link

Cheerful Taffy Dolphin

High

Missing Execution Rights Validation in _cancelOrder() Enables Front-Running of Signed Cancel Orders and Market Manipulation

Summary

The protocol implements two cancellation paths: direct cancellation and signature-based cancellation, utilizing EIP712 signatures for off-chain authorization. The Manager contract coordinates with an OrderVerifier for signature validation and a MarketFactory for operator permissions. A security review of this system reveals a significant authorization vulnerability in the order cancellation flow.

The issue stems from the dual authorization paths in the order cancellation system. In the Manager contract, direct cancellations flow through:

https://github.com/sherlock-audit/2025-01-perennial-v2-4-update/blob/main/perennial-v2/packages/periphery/contracts/TriggerOrders/Manager.sol#L112

function cancelOrder(IMarket market, uint256 orderId) external {
    _cancelOrder(market, msg.sender, orderId);
}

This path implicitly ensures authorization as the account parameter passed to _cancelOrder is msg.sender. However, in the signature-based path:

function cancelOrderWithSignature(CancelOrderAction calldata request, bytes calldata signature)
    external 
    keepAction(request.action, abi.encode(request, signature))
{
    verifier.verifyCancelOrder(request, signature);
    _cancelOrder(request.action.market, request.action.common.account, request.action.orderId);
}

it delegates authorization to the OrderVerifier contract, which validates through EIP712 signatures:

function _verifySignature(Action calldata action, bytes32 hash, bytes calldata signature) internal view {
    if (!SignatureChecker.isValidSignatureNow(
        action.common.signer,
        _hashTypedDataV4(hash),
        signature
    )) revert VerifierInvalidSignerError();
}

The authorization check in OrderVerifier only validates that the signer is authorized:

function _authorized(address account, address signer) internal view override returns (bool) {
    return super._authorized(account, signer) || marketFactory.signers(account, signer);
}

The critical vulnerability lies in _cancelOrder lacking execution authorization:

function _cancelOrder(IMarket market, address account, uint256 orderId) private {
    TriggerOrder memory order = _orders[market][account][orderId].read();
    if (order.isEmpty() || order.isSpent) revert ManagerCannotCancelError();
    order.isSpent = true;
    _orders[market][account][orderId].store(order);
    emit TriggerOrderCancelled(market, account, orderId);
}

This creates a security gap where anyone can execute a signed cancellation order. While the signature proves intent from an authorized signer, there's no validation of the executing party (msg.sender). This enables front-running attacks where malicious actors could execute signed cancellations before the intended executor.

Impact

The authorization vulnerability in _cancelOrder has severe implications for the trigger order system's integrity. Any party can execute a signed cancel order without execution rights, enabling front-running of cancellations in volatile market conditions. Adversaries could manipulate the timing of order cancellations to their advantage, particularly damaging in a DeFi context where order execution sequence directly impacts profit/loss outcomes. Given that the Manager contract handles trigger orders for positions in trading markets, unauthorized control over cancellation timing could be exploited to manipulate market positions and force unfavorable execution conditions.

Fix

The fix requires adding operator validation in _cancelOrder:

function _cancelOrder(IMarket market, address account, uint256 orderId) private {
    if (msg.sender != account && !marketFactory.operators(account, msg.sender)) 
        revert ManagerNotOperatorError();
        
    TriggerOrder memory order = _orders[market][account][orderId].read();
    if (order.isEmpty() || order.isSpent) revert ManagerCannotCancelError();
    order.isSpent = true;
    _orders[market][account][orderId].store(order);
    emit TriggerOrderCancelled(market, account, orderId);
}

This aligns with the contract's existing operator check pattern, seen in the claim function:

modifier onlyOperator(address account, address operator) {
    if (account != operator && !marketFactory.operators(account, operator)) 
        revert ManagerNotOperatorError();
    _;
}

The vulnerability has significant implications for market operations, particularly during volatile conditions where order cancellation timing is critical. The fix establishes proper dual-layer authorization: signature validation for intent and operator validation for execution rights.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant