safdie - Inadequate handling of suspended transactions could allow suspended transactions to still be withdrawn in `BridgeFacetImpl.sol`

[sherlock-admin4]

safdie

High

Inadequate handling of suspended transactions could allow suspended transactions to still be withdrawn in BridgeFacetImpl.sol

Summary

Inadequate handling of suspended transactions in BridgeFacetImpl.sol could allow suspended transactions to still be withdrawn.

Root Cause

In the suspendBridgeTransaction function, a transaction's status is updated to SUSPENDED. However, there are no checks in other functions (e.g., withdrawReceivedBridgeValue) to prevent further actions (like withdrawals) on suspended transactions. This could allow suspended transactions to still be withdrawn, violating the suspension.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

We will simulate the following actions:

1. Create a bridge transaction.
2. Suspend that bridge transaction.
3. Attempt to withdraw the suspended transaction. The PoC demonstrates how a suspended transaction can still be withdrawn due to the lack of validation in withdrawReceivedBridgeValue.

Expected behavior: After suspending the transaction with suspendBridgeTransaction(transactionId), the withdrawal function withdrawReceivedBridgeValue(transactionId) should not allow the transaction to proceed. However, the current implementation does not check whether a transaction is suspended before allowing withdrawal. As a result, the suspended transaction is withdrawn successfully.

In the withdrawReceivedBridgeValue function, there is no check for the transaction's status being SUSPENDED: https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Bridge/BridgeFacetImpl.sol#L42-L55

Exploit explanation: After suspending a transaction, suspendBridgeTransaction sets the status to SUSPENDED. The withdrawReceivedBridgeValue function does not verify that the transaction status is not SUSPENDED. It only checks whether the status is RECEIVED. Therefore, a suspended transaction can still be withdrawn.

Impact

Users or attackers could withdraw suspended transactions, violating the suspension mechanism and allowing for unauthorized withdrawals. Suspended transactions are meant to prevent further action. If attackers can bypass this protection, they could exploit this flaw to withdraw funds or alter the state of the contract improperly.

PoC

// SPDX-License-Identifier: MIT
pragma solidity >=0.8.18;

import "./BridgeFacetImpl.sol";

contract BridgeFacetPoC {
    using SafeERC20 for IERC20;

    event BridgeTransactionSuspended(uint256 indexed transactionId);
    event BridgeTransactionWithdrawn(uint256 indexed transactionId);

    // Example token contract for collateral (mock)
    IERC20 public collateralToken;

    constructor(IERC20 _collateralToken) {
        collateralToken = _collateralToken;
    }

    function testSuspendedTransactionWithdrawal() external {
        address user = msg.sender;
        uint256 bridgeAmount = 1000 * 1e18; // Example amount
        address bridgeAddress = address(this); // Use contract as bridge for test

        // Step 1: Transfer funds to the bridge (this creates a bridge transaction)
        uint256 transactionId = BridgeFacetImpl.transferToBridge(user, bridgeAmount, bridgeAddress);
        emit BridgeTransactionWithdrawn(transactionId);

        // Step 2: Suspend the bridge transaction
        BridgeFacetImpl.suspendBridgeTransaction(transactionId);
        emit BridgeTransactionSuspended(transactionId);

        // Step 3: Attempt to withdraw the suspended transaction
        // This should NOT be allowed, but the current implementation doesn't prevent it.
        BridgeFacetImpl.withdrawReceivedBridgeValue(transactionId);

        // If the withdraw succeeds, it means the vulnerability was exploited.
        // Proper behavior would be to revert and not allow the withdrawal.
    }
}

Mitigation

To fix this issue, add a check for the SUSPENDED status in the withdrawReceivedBridgeValue function:

require(bridgeTransaction.status != BridgeTransactionStatus.SUSPENDED, "BridgeFacet: Transaction is suspended");

Updated withdrawReceivedBridgeValue with fix:

function withdrawReceivedBridgeValue(uint256 transactionId) internal {
    BridgeTransaction storage bridgeTransaction = bridgeLayout.bridgeTransactions[transactionId];

    // Prevent withdrawing suspended transactions
    require(bridgeTransaction.status != BridgeTransactionStatus.SUSPENDED, "BridgeFacet: Transaction is suspended");

    require(bridgeTransaction.status == BridgeTransactionStatus.RECEIVED, "BridgeFacet: Already withdrawn");
    require(block.timestamp >= MAStorage.layout().deallocateCooldown + bridgeTransaction.timestamp, "BridgeFacet: Cooldown hasn't reached");
    require(msg.sender == bridgeTransaction.bridge, "BridgeFacet: Sender is not the transaction's bridge");

    bridgeTransaction.status = BridgeTransactionStatus.WITHDRAWN;
    IERC20(appLayout.collateral).safeTransfer(bridgeTransaction.bridge, bridgeTransaction.amount);
}

By adding this check, the contract will properly enforce the suspension, and any attempt to withdraw a suspended transaction will be reverted.