KungFuPanda

Medium

LibSettlement::settleUpnl will not revert with a correct "LibSettlement: Invalid partyBUpnlIndex in signature" message at the right time, leading to external integrations problems

Summary

This check here is not correctly implemented:

            require(data.partyBUpnlIndex <= settleSig.upnlPartyBs.length, "LibSettlement: Invalid partyBUpnlIndex in signature");

in the LibSettlement::settleUpnl function.

Root Cause

The function settleUpnl was added in the current latest update, and it exists in the LibSettlement library to allow the party B to settle the unrealized PNL of party A:

contract SettlementFacet is Accessibility, Pausable, ISettlementFacet {
    /**
     * @notice Allows Party B to settle the upnl of party A position for the specified quotes.
     * @param settlementSig The data struct contains quoteIds and upnl of parties and market prices
     * @param updatedPrices New prices to be set as openedPrice for the specified quotes.
     * @param partyA Address of party A
     */
    function settleUpnl(
        SettlementSig memory settlementSig,
        uint256[] memory updatedPrices,
        address partyA
    ) external whenNotPartyBActionsPaused notLiquidatedPartyA(partyA) {
        uint256[] memory newPartyBsAllocatedBalances = SettlementFacetImpl.settleUpnl(settlementSig, updatedPrices, partyA);
        emit SettleUpnl(
            settlementSig.quotesSettlementsData,
            updatedPrices,
            partyA,
            AccountStorage.layout().allocatedBalances[partyA],
            newPartyBsAllocatedBalances
        );
    }
}

The function settleUpnl function from SettlementFacet calls the SettlementFacetImpl's settleUpnl function first, and then SettlementFacetImpl calls the LibSettlement.settleUpnl function as the final destination:

library SettlementFacetImpl {
    function settleUpnl(
        SettlementSig memory settleSig,
        uint256[] memory updatedPrices,
        address partyA
    ) internal returns (uint256[] memory newPartyBsAllocatedBalances) {
        LibMuonSettlement.verifySettlement(settleSig, partyA);
        return LibSettlement.settleUpnl(settleSig, updatedPrices, partyA, false);
    }
}

And the SettlementFacetImpl:settleUpnl function calls:

library LibSettlement {
    function settleUpnl(
        SettlementSig memory settleSig,
        uint256[] memory updatedPrices,
        address partyA,
        bool isForceClose
    ) internal returns (uint256[] memory newPartyBsAllocatedBalances) {
        QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
        AccountStorage.Layout storage accountLayout = AccountStorage.layout();

        require(settleSig.quotesSettlementsData.length == updatedPrices.length, "LibSettlement: Invalid length");
        require(
            LibAccount.partyAAvailableBalanceForLiquidation(settleSig.upnlPartyA, accountLayout.allocatedBalances[partyA], partyA) >= 0,
            "LibSettlement: PartyA is insolvent"
        );

        require(
            isForceClose || quoteLayout.partyBOpenPositions[msg.sender][partyA].length > 0,
            "LibSettlement: Sender should have a position with partyA"
        );
        accountLayout.partyANonces[partyA] += 1;

        int256[] memory settleAmounts = new int256[](settleSig.upnlPartyBs.length);
        address[] memory partyBs = new address[](settleSig.upnlPartyBs.length);
        newPartyBsAllocatedBalances = new uint256[](settleSig.upnlPartyBs.length);

        for (uint8 i = 0; i < settleSig.quotesSettlementsData.length; i++) {
            QuoteSettlementData memory data = settleSig.quotesSettlementsData[i];
            Quote storage quote = quoteLayout.quotes[data.quoteId];
            require(quote.partyA == partyA, "LibSettlement: PartyA is invalid");
            require(
                quote.quoteStatus == QuoteStatus.OPENED ||
                    quote.quoteStatus == QuoteStatus.CLOSE_PENDING ||
                    quote.quoteStatus == QuoteStatus.CANCEL_CLOSE_PENDING,
                "LibSettlement: Invalid state"
            );
            require(data.partyBUpnlIndex <= settleSig.upnlPartyBs.length, "LibSettlement: Invalid partyBUpnlIndex in signature");
            require(

                partyBs[data.partyBUpnlIndex] == address(0) || partyBs[data.partyBUpnlIndex] == quote.partyB,
                "LibSettlement: Invalid upnlPartyBs list"
            );
            partyBs[data.partyBUpnlIndex] = quote.partyB;

                        // ........
}
}

The SettlementFacetImpl's settleUpnl function also calls the following function to verify whether the settlement data is all correct and the signatures are also verified in LibMuon then:

library LibMuonSettlement {
    function verifySettlement(SettlementSig memory settleSig, address partyA) internal view {
        MuonStorage.Layout storage muonLayout = MuonStorage.layout();
        // == SignatureCheck( ==
        require(block.timestamp <= settleSig.timestamp + muonLayout.upnlValidTime, "LibMuon: Expired signature");
        // == ) ==
        bytes memory encodedData;
        uint256[] memory nonces = new uint256[](settleSig.quotesSettlementsData.length);
        for (uint8 i = 0; i < settleSig.quotesSettlementsData.length; i++) {
            nonces[i] = AccountStorage.layout().partyBNonces[QuoteStorage.layout().quotes[settleSig.quotesSettlementsData[i].quoteId].partyB][partyA];
            encodedData = abi.encodePacked(
                encodedData,  // Append the previously encoded data
                settleSig.quotesSettlementsData[i].quoteId,
                settleSig.quotesSettlementsData[i].currentPrice,
                settleSig.quotesSettlementsData[i].partyBUpnlIndex
            );
        }
        bytes32 hash = keccak256(
            abi.encodePacked(
                muonLayout.muonAppId,
                settleSig.reqId,
                address(this),
                "verifySettlement",
                nonces,
                AccountStorage.layout().partyANonces[partyA],
                encodedData,
                settleSig.upnlPartyBs,
                settleSig.upnlPartyA,
                settleSig.timestamp,
                LibMuon.getChainId()
            )
        );
        LibMuon.verifyTSSAndGateway(hash, settleSig.sigs, settleSig.gatewaySignature);
    }
}

Where we can see that the:

encodedData = abi.encodePacked(
                encodedData,  // Append the previously encoded data
                settleSig.quotesSettlementsData[i].quoteId,
                settleSig.quotesSettlementsData[i].currentPrice,
                settleSig.quotesSettlementsData[i].partyBUpnlIndex
            );

encodedData will include the partyBUpnlIndex variable.

The problem is later that in reality the partyBUpnlIndex value should be less than the upnlPartyBs length, because after this check is done:

            require(data.partyBUpnlIndex <= settleSig.upnlPartyBs.length, "LibSettlement: Invalid partyBUpnlIndex in signature");

... we will see these lines:

            require(
                partyBs[data.partyBUpnlIndex] == address(0) || partyBs[data.partyBUpnlIndex] == quote.partyB,
                "LibSettlement: Invalid upnlPartyBs list"
            );
            partyBs[data.partyBUpnlIndex] = quote.partyB;

(all in LibSettlement:settleUpnl)

These lines prove that the dataPartyBUpnlIndex variable should be one of the partyBs available entry slots.

But the previous check require(data.partyBUpnlIndex <= settleSig.upnlPartyBs.length, "LibSettlement: Invalid partyBUpnlIndex in signature"); implies that partyBUpnlIndex here can be as big as the upnlPartyBs.length, which is not correct, because later accessing partyBs with a data.partyBUpnlIndex equal to its length (partyBs.length) will always revert due to that the partyBs array is a fixed-length array, and Solidity will always throw an Out-of-Bounds exception without any further explanations.

The main problem here is if a caller sets one of the data.partyBUpnlIndex values to the length of upnlPartyBs array, considering in Solidity arrays start count at 0, not 1, unlike humans, and if the caller is a contract trying to implement a try {} catch {} block and propagating a wrong data.partyBUpnlIndex == settleSig.upnlPartyBs.length value, they will never know the real reason of the revert, because the error will be unstyled and just a low-level kind of revert, instead of the intended message ""LibSettlement: Invalid partyBUpnlIndex in signature"".

Internal pre-conditions

None, as far as I can see.

External pre-conditions

The party B caller being an integrator will never get the real reason of the revert, and won't be able to appropriately handle the reverting settleUpnl call on his side either, because try {} catch(err) {} blocks allow catching specific errors that have their names thrown properly, as:

function rate(address token) public returns (uint value, bool success) {
        // Permanently disable the mechanism if there are
        // more than 10 errors.
        require(errorCount < 10);
        try feed.getData(token) returns (uint v) {
            return (v, true);
        } catch Error(string memory /*reason*/) {
            // This is executed in case
            // revert was called inside getData
            // and a reason string was provided.
            errorCount++;
            return (0, false);
        } catch Panic(uint /*errorCode*/) {
            // This is executed in case of a panic,
            // i.e. a serious error like division by zero
            // or overflow. The error code can be used
            // to determine the kind of error.
            errorCount++;
            return (0, false);
        } catch (bytes memory /*lowLevelData*/) {
            // This is executed in case revert() was used.
            errorCount++;
            return (0, false);
        }

(https://docs.soliditylang.org/en/latest/control-structures.html)

Attack Path

The problem lies in this line: https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/a975aafb06cc3dcb9e4bf9b56ceeb4a9f8163503/protocol-core/contracts/libraries/LibSettlement.sol#L50

But in this case, when the Party B wants to call settleUpnl for the unrelaized PNL of the party A, they won't be able to handle the named errors in their code or (if they are an EOA) --- they won't be able to query the failed functions events because there will not be any named string errors with "LibSettlement: Invalid partyBUpnlIndex in signature", but rather all of the errors thrown when the partyBUpnlIndex equals to the partyBs.length, there will only be Panic errors with `Array-Out-Of-Bounds Solidity low-level exception, and they won't be able to handle their corresponding logic appropriately due to this bug.

Impact

From this contest's README:

What properties/invariants do you want to hold even if breaking them has a low/unknown impact?

Yes, report potential issues, including broken assumptions about function behavior, if they pose future integration risks. Key properties that should hold include correctness (accurate returns), security (resistant to manipulation), consistency (uniform behavior on-chain and off-chain), and reliability (functioning correctly under all conditions).

This invariant is particularly absolutely violated here for an edge case when in LibSettlement.settleUpnl data.partyBUpnlIndex == settleSig.upnlPartyBs.length is propagated, because instead of reverting with a predefined error "LibSettlement: Invalid partyBUpnlIndex in signature");, the Panic: Array-Out-Of-Bounds error will be thrown:

Reliability: The function should function correctly under all conditions, including edge cases and unexpected inputs. For example, a function that reads from a data structure should handle cases where the requested data does not exist and return a predefined error or null value.

And the following criteria is matched:

Low severity issues falling in these categories would not be valid and issues falling in these categories would be valid only for future integrations of other protocols with Symm.

The issue is not Low severity, because the integrations directly depend on the correct Error being returned at the right time, with the right (predefined by the design) name.

PoC

Tailor the following test case:

    it("Should fail on invalid partyBUpnlIndex in signature", async function () {
        await expect(hedger.settleUpnl(await user.getAddress(), [decimal(5n, 17)], getDummySettlementSig(0n, [0n], [
            {
                quoteId: shortHedger1,
                currentPrice: 0n,
                partyBUpnlIndex: 3n
            } as QuoteSettlementDataStructOutput,
        ]))).to.be.revertedWith("LibSettlement: Invalid partyBUpnlIndex in signature")
    })

in Settlement.behavior.ts,

to have partyBUpnlIndex: 2n instead of partyBUpnlIndex: 3n.

The index 2 will never be correctly accessed, because the indexes available are literally: 0, 1, and the length is 2 here.

Mitigation

-           require(data.partyBUpnlIndex <= settleSig.upnlPartyBs.length, "LibSettlement: Invalid partyBUpnlIndex in signature");
+           require(data.partyBUpnlIndex < settleSig.upnlPartyBs.length, "LibSettlement: Invalid partyBUpnlIndex in signature");

This should be a sufficient fix, I believe.

sherlock-audit / 2024-09-symmio-v0-8-4-update-contest-judging

KungFuPanda - LibSettlement::settleUpnl will not revert with a correct "LibSettlement: Invalid partyBUpnlIndex in signature" message at the right time, leading to external integrations problems #55