0xloophole - Lack of Slippage Protection in MultiInvoker's _update Function

[sherlock-admin2]

0xloophole

Medium

Lack of Slippage Protection in MultiInvoker's _update Function

Summary

The MultiInvoker contract in the Perennial protocol is designed to handle various operations, including updating user positions in the market. The _update function is a critical component responsible for these position updates, directly affecting user balances and market exposure.

Issue Description

The _update function lacks slippage protection mechanisms. This omission exposes users to potential losses due to price movements between the initiation of an update and its execution on the blockchain.

Code Snippet

function _update(
    address account,
    IMarket market,
    UFixed6 newMaker,
    UFixed6 newLong,
    UFixed6 newShort,
    Fixed6 collateral,
    bool wrap,
    InterfaceFee memory interfaceFee1,
    InterfaceFee memory interfaceFee2
) internal isMarketInstance(market) {
    Fixed18 balanceBefore =  Fixed18Lib.from(DSU.balanceOf());

    if (collateral.sign() == 1) _deposit(account, collateral.abs(), wrap);

    market.update(
        account,
        newMaker,
        newLong,
        newShort,
        collateral,
        false,
        interfaceFee1.receiver == address(0) ? interfaceFee2.receiver : interfaceFee1.receiver
    );

    Fixed6 withdrawAmount = Fixed6Lib.from(Fixed18Lib.from(DSU.balanceOf()).sub(balanceBefore));
    if (!withdrawAmount.isZero()) _withdraw(account, withdrawAmount.abs(), wrap);

    _chargeFee(account, market, interfaceFee1);
    _chargeFee(account, market, interfaceFee2);
}

Impact

1. Unexpected Losses: Users may incur larger than anticipated losses due to unfavorable price movements during transaction confirmation.
2. Lack of Risk Control: Users cannot set their risk tolerance for position updates, potentially exposing them to unacceptable levels of slippage.
3. Vulnerability to Front-running: The absence of slippage protection makes transactions susceptible to front-running attacks, where malicious actors could manipulate prices before the user's transaction is confirmed.
4. Reduced User Trust: Unexpected outcomes in position updates could lead to a loss of user confidence in the protocol.

Scenario

Alice wants to increase her long position in a volatile market. She submits a transaction to update her position, but due to network congestion, it takes longer than expected to be mined. In the meantime, the market price moves unfavorably. When Alice's transaction is finally executed, she receives a significantly worse price than expected, with no bounds on how bad the slippage could be.

Proposed Fix

Implement slippage protection by adding a maxSlippage parameter to the _update function and perform a check before executing the market update:

function _update(
    address account,
    IMarket market,
    UFixed6 newMaker,
    UFixed6 newLong,
    UFixed6 newShort,
    Fixed6 collateral,
    bool wrap,
    InterfaceFee memory interfaceFee1,
    InterfaceFee memory interfaceFee2,
    UFixed6 maxSlippage
) internal isMarketInstance(market) {
    Fixed18 balanceBefore =  Fixed18Lib.from(DSU.balanceOf());

    if (collateral.sign() == 1) _deposit(account, collateral.abs(), wrap);

    // Get the current market price
    UFixed6 currentPrice = market.getPrice();

    // Calculate the maximum and minimum acceptable prices
    UFixed6 maxPrice = currentPrice.mul(UFixed6Lib.ONE.add(maxSlippage));
    UFixed6 minPrice = currentPrice.mul(UFixed6Lib.ONE.sub(maxSlippage));

    // Execute the update only if the current price is within the acceptable range
    require(currentPrice.gte(minPrice) && currentPrice.lte(maxPrice), "Slippage exceeded");

    market.update(
        account,
        newMaker,
        newLong,
        newShort,
        collateral,
        false,
        interfaceFee1.receiver == address(0) ? interfaceFee2.receiver : interfaceFee1.receiver
    );

    Fixed6 withdrawAmount = Fixed6Lib.from(Fixed18Lib.from(DSU.balanceOf()).sub(balanceBefore));
    if (!withdrawAmount.isZero()) _withdraw(account, withdrawAmount.abs(), wrap);

    _chargeFee(account, market, interfaceFee1);
    _chargeFee(account, market, interfaceFee2);
}