kutugu - The liquidator can set any stale price data by setSymbolsPrice and others can't override it

[sherlock-admin]

kutugu

high

The liquidator can set any stale price data by setSymbolsPrice and others can't override it

Summary

setSymbolsPrice not checking whether the price is stale, the liquidator can set the stale price to liquidate.

Vulnerability Detail

function verifyPrices(PriceSig memory priceSig, address partyA) internal view {
    MuonStorage.Layout storage muonLayout = MuonStorage.layout();
    require(priceSig.prices.length == priceSig.symbolIds.length, "LibMuon: Invalid length");
    bytes32 hash = keccak256(
        abi.encodePacked(
            muonLayout.muonAppId,
            priceSig.reqId,
            address(this),
            partyA,
            priceSig.upnl,
            priceSig.totalUnrealizedLoss,
            priceSig.symbolIds,
            priceSig.prices,
            priceSig.timestamp,
            getChainId()
        )
    );
    verifyTSSAndGateway(hash, priceSig.sigs, priceSig.gatewaySignature);
}

function setSymbolsPrice(address partyA, PriceSig memory priceSig) internal {
        MAStorage.Layout storage maLayout = MAStorage.layout();
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();

        // @audit Here not check priceSig.timestamp
        LibMuon.verifyPrices(priceSig, partyA);
        require(maLayout.liquidationStatus[partyA], "LiquidationFacet: PartyA is solvent");
        // @audit Here only check whether priceSig is younger than the liquidation expiration time, not whether it is older than the liquidation setting time
        require(
            priceSig.timestamp <=
                maLayout.liquidationTimestamp[partyA] + maLayout.liquidationTimeout,
            "LiquidationFacet: Expired signature"
        );
        for (uint256 index = 0; index < priceSig.symbolIds.length; index++) {
            accountLayout.symbolsPrices[partyA][priceSig.symbolIds[index]] = Price(
                priceSig.prices[index],
                maLayout.liquidationTimestamp[partyA]
            );
        }

        int256 availableBalance = LibAccount.partyAAvailableBalanceForLiquidation(
            priceSig.upnl,
            partyA
        );
        // @audit only first time
        if (accountLayout.liquidationDetails[partyA].liquidationType == LiquidationType.NONE) {
            accountLayout.liquidationDetails[partyA] = LiquidationDetail({
                liquidationType: LiquidationType.NONE,
                upnl: priceSig.upnl,
                totalUnrealizedLoss: priceSig.totalUnrealizedLoss,
                deficit: 0,
                liquidationFee: 0
            });
            if (availableBalance >= 0) {
                uint256 remainingLf = accountLayout.lockedBalances[partyA].lf;
                accountLayout.liquidationDetails[partyA].liquidationType = LiquidationType.NORMAL;
                accountLayout.liquidationDetails[partyA].liquidationFee = remainingLf;
            } else if (uint256(-availableBalance) < accountLayout.lockedBalances[partyA].lf) {
                uint256 remainingLf = accountLayout.lockedBalances[partyA].lf -
                    uint256(-availableBalance);
                accountLayout.liquidationDetails[partyA].liquidationType = LiquidationType.NORMAL;
                accountLayout.liquidationDetails[partyA].liquidationFee = remainingLf;
            } else if (
                uint256(-availableBalance) <=
                accountLayout.lockedBalances[partyA].lf + accountLayout.lockedBalances[partyA].cva
            ) {
                uint256 deficit = uint256(-availableBalance) -
                    accountLayout.lockedBalances[partyA].lf;
                accountLayout.liquidationDetails[partyA].liquidationType = LiquidationType.LATE;
                accountLayout.liquidationDetails[partyA].deficit = deficit;
            } else {
                uint256 deficit = uint256(-availableBalance) -
                    accountLayout.lockedBalances[partyA].lf -
                    accountLayout.lockedBalances[partyA].cva;
                accountLayout.liquidationDetails[partyA].liquidationType = LiquidationType.OVERDUE;
                accountLayout.liquidationDetails[partyA].deficit = deficit;
            }
            AccountStorage.layout().liquidators[partyA].push(msg.sender);
        // @audit When invoke setSymbolsPrice again, the pnl and loss are required to be equal to the first time, it can be assumed that others cannot modify the price again
        } else {
            require(
                accountLayout.liquidationDetails[partyA].upnl == priceSig.upnl &&
                    accountLayout.liquidationDetails[partyA].totalUnrealizedLoss ==
                    priceSig.totalUnrealizedLoss,
                "LiquidationFacet: Invalid upnl sig"
            );
        }
    }

LibMuon.verifyPrices not check whether it is stale price and setSymbolsPrice only check that priceSig doesn't timeout.
So the liquidator can set a stale price data, and because setSymbolsPrice requires pnl and loss to be the same as the first time, it is difficult for others to override with the correct price. See my comment in the code for details.
Even though pnl and loss may equal, there is no time for others to overwrite because malicious liquidator can pack several liquidate txs into one block.
For comparison you can see that liquidatePositionsPartyB has two time checks:

        LibMuon.verifyQuotePrices(priceSig);
        require(
            priceSig.timestamp <=
                maLayout.partyBLiquidationTimestamp[partyB][partyA] + maLayout.liquidationTimeout,
            "LiquidationFacet: Expired signature"
        );
        require(
            maLayout.partyBLiquidationStatus[partyB][partyA],
            "LiquidationFacet: PartyB is solvent"
        );
        require(
            block.timestamp <= priceSig.timestamp + maLayout.liquidationTimeout,
            "LiquidationFacet: Expired price sig"
        );

Impact

The liquidator can use the stale price to liquidate. In particular, given that one side of the party may be the liquidator, they can use this price to arbitrage.

Code Snippet

• https://github.com/sherlock-audit/2023-06-symmetrical/blob/6d2b64b6732fcfbd07c8217897dd233dbb6cd1f5/symmio-core/contracts/facets/liquidation/LiquidationFacetImpl.sol#L40-L44

Tool used

Manual Review

Recommendation

Added freshness check for price data

Duplicate of #113