Bauchibred - DOS to MarginAccount::isLiquidatable() or any other function that calls this

[sherlock-admin]

Bauchibred

medium

DOS to MarginAccount::isLiquidatable() or any other function that calls this

Summary

The isLiquidatable() function contains multiple calls to other contracts, i.e theClearingHouse.sol and AMM.sol note that asides the gas hefty execution of the isLiquidatable()function itself there exist an unrestricted iteration over the AMMs with the help of getTotalNotionalPositionAndUnrealizedPnl() , which could potentially lead to gas exhaustion and render the system not being able to know if the user is liquidatable or not. Do note that protocol seems to know about the issue of how gas hefty the calls could be, meaning that the getNotionalPositionAndMargin() should instead be used in isLiquidatable() since it uses precompile (bibliophile)

Vulnerability Detail

The isLiquidatable function in MarginAccount.sol calls the getTotalNotionalPositionAndUnrealizedPnl() function of ClearingHouse.sol, which, in turn, iterates over all the AMM contracts stored in the amms array. For each AMM contract, it calls the getOptimalPnl() function. However, there are no limits imposed on the number of AMMs a trader can have positions in, making the iteration process computationally expensive and prone to gas exhaustion, note that this could even be worse, if say this loop occured after a call to the liquidateExactRepay() function, i.e a gas has been spent on updating positions already and then a call to isLiquidate() is made

Impact

An attacker could exploit this vulnerability by creating a large number of positions across multiple AMMs. As a result, the iteration process within the isLiquidatable function could consume excessive gas, leading to an "out of gas" error, essentially meaning that the user can side step being liquidatable

Code Snippet

MarginAccount.sol#L251-L311 ClearingHouse.sol#L374-L388

// Relevant portion of the `isLiquidatable` function in `MarginAccount.sol`
function isLiquidatable(address trader, bool includeFunding)
    override
    public
    view
    returns(IMarginAccount.LiquidationStatus _isLiquidatable, uint repayAmount, uint incentivePerDollar)
{
    // ... (omitted code for brevity) ...

    (uint256 notionalPosition,) = clearingHouse.getTotalNotionalPositionAndUnrealizedPnl(trader, 0, IClearingHouse.Mode.Min_Allowable_Margin);

    // ... (omitted code for brevity) ...
}

function getTotalNotionalPositionAndUnrealizedPnl(address trader, int256 margin, Mode mode)
    override
    public
    view
    returns (uint256 notionalPosition, int256 unrealizedPnl)
{
    uint256 _notionalPosition;
    int256 _unrealizedPnl;
    uint numAmms = amms.length;
    for (uint i; i < numAmms; ++i) {
        (_notionalPosition, _unrealizedPnl) = amms[i].getOptimalPnl(trader, margin, mode);
        notionalPosition += _notionalPosition;
        unrealizedPnl += _unrealizedPnl;
    }
}

function getOptimalPnl(address trader, int256 margin, IClearingHouse.Mode mode)
    override
    external
    view
    returns (uint notionalPosition, int256 unrealizedPnl)
{
    Position memory position = positions[trader];
    if (position.size == 0) {
        return (0, 0);
    }

    // based on last price
    int256 lastPriceBasedMF;
    (notionalPosition, unrealizedPnl, lastPriceBasedMF) = getPositionMetadata(
        lastPrice(),
        position.openNotional,
        position.size,
        margin
    );

    // based on oracle price
    (uint oracleBasedNotional, int256 oracleBasedUnrealizedPnl, int256 oracleBasedMF) = getPositionMetadata(
        oracle.getUnderlyingPrice(underlyingAsset).toUint256(),
        position.openNotional,
        position.size,
        margin
    );

    // while evaluating margin for liquidation, we give the best deal to the user
    if (
        (mode == IClearingHouse.Mode.Maintenance_Margin && oracleBasedMF > lastPriceBasedMF) ||
        (mode == IClearingHouse.Mode.Min_Allowable_Margin && oracleBasedMF < lastPriceBasedMF)
    ) {
        return (oracleBasedNotional, oracleBasedUnrealizedPnl);
    }
}

Tool used

Manual Audit

Recommendation

Simple fix, since there will not be an out of gas issue precompile is used, use getNotionalPositionAndMargin() in isLiquidatable() instead of getTotalNotionalPositionAndUnrealizedPnl()