safdie - Lack of proper input validation for `quoteId` could lead to unexpected behavior and DoS in `ForceActionsFacet.sol`

[sherlock-admin4]

safdie

Medium

Lack of proper input validation for quoteId could lead to unexpected behavior and DoS in ForceActionsFacet.sol

Summary

Lack of proper input validation for quoteId in ForceActionsFacet.sol contract could lead to unexpected behavior and DoS.

Root Cause

The ForceActionsFacet contract includes several functions that rely on a quoteId to identify quotes in the system (e.g., forceCancelQuote(), forceCancelCloseRequest(), forceClosePosition(), settleAndForceClosePosition()). https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/ForceActions/ForceActionsFacet.sol#L23 However, there is no validation to ensure that the quoteId being passed corresponds to a valid and existing quote. This vulnerability could lead to unexpected behavior and denial of service.

Internal pre-conditions

Assume there are only 10 quotes in the system (valid quoteIds range from 0 to 9). We will attempt to force-close a non-existent quote (e.g., quoteId = 9999).

External pre-conditions

No response

Attack Path

The following PoC below demonstrates how passing an invalid quoteId can trigger unexpected behavior in the forceClosePosition() function.

Attack execution:

1. Deploy the ForceActionsFacet contract.
2. Deploy the AttackForceClose contract with the address of the ForceActionsFacet contract.
3. Call attackInvalidForceClose() from the AttackForceClose contract.

Expected behavior: If forceClosePosition() does not validate the quoteId, this will attempt to access and modify a non-existent quote, potentially causing a revert or other unintended behavior.

Actual impact: The transaction could revert due to trying to access an invalid index in storage, preventing legitimate users from interacting with the system. If the contract doesn't handle the error properly, it might attempt to force-close the wrong quote, leading to financial loss for legitimate users.

Impact

1. If an invalid quoteId is passed, the function may behave unexpectedly, possibly reverting during execution. This can result in a DoS scenario, where valid users are unable to force-close or force-cancel their positions due to incorrect input validation.
2. The system could potentially allow force-cancellation or force-closure of non-existent or incorrect quotes, which could lead to financial loss for legitimate users. If the wrong quoteId is passed and handled improperly, the system might update or close the wrong quote or position, causing mismatches in state and funds.
3. Without proper validation, force actions could operate on invalid or unintended quotes, leading to incorrect state updates that cause inconsistencies in the system’s internal storage. This could result in unresolvable errors or financial imbalances.

PoC

pragma solidity ^0.8.18;

contract ForceActionsFacet {
    // Example force close function from ForceActionsFacet
    function forceClosePosition(uint256 quoteId) external {
        // Assume QuoteStorage.layout().quotes[quoteId] throws an error if quoteId doesn't exist
        QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
        Quote storage quote = quoteLayout.quotes[quoteId];

        // Force-close the position
        require(quote.exists, "Invalid quoteId");

        // Proceed with closing position logic (omitted for brevity)
    }
}

contract AttackForceClose {
    ForceActionsFacet forceActionsFacet;

    constructor(address _forceActionsFacet) {
        forceActionsFacet = ForceActionsFacet(_forceActionsFacet);
    }

    // Malicious function trying to force-close a non-existent quote
    function attackInvalidForceClose() external {
        uint256 invalidQuoteId = 9999;

        // Calling the vulnerable function with an invalid quoteId
        forceActionsFacet.forceClosePosition(invalidQuoteId);
    }
}

Mitigation

To mitigate this vulnerability, add proper input validation to ensure the quoteId is valid and exists in the system. Here’s an example of a validation check that should be implemented:

function forceClosePosition(uint256 quoteId) external {
    // Validate that the quote exists
    QuoteStorage.Layout storage quoteLayout = QuoteStorage.layout();
    require(quoteId < quoteLayout.quotes.length, "Invalid quoteId");

    Quote storage quote = quoteLayout.quotes[quoteId];
    require(quote.exists, "Quote does not exist");

    // Proceed with closing position logic
}

By adding validation checks, the contract ensures that quoteId refers to an actual, existing quote before proceeding with further logic, thus preventing potential DoS attacks and ensuring the integrity of the system.