safdie - Replay attack on `withdrawReceivedBridgeValue` and `withdrawReceivedBridgeValues` will allow attacker to withdraw more than they are entitled to by replaying or reusing transaction IDs in `BridgeFacet.sol`

[sherlock-admin4]

safdie

High

Replay attack on withdrawReceivedBridgeValue and withdrawReceivedBridgeValues will allow attacker to withdraw more than they are entitled to by replaying or reusing transaction IDs in BridgeFacet.sol

Summary

Replay attack on withdrawReceivedBridgeValue and withdrawReceivedBridgeValues in BridgeFacet.sol will allow attacker to withdraw more than they are entitled to by replaying or reusing transaction IDs.

Root Cause

Replay attacks occur when a malicious actor reuses valid data, such as a transaction ID, to perform the same operation multiple times. In this case, the functions withdrawReceivedBridgeValue https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Bridge/BridgeFacet.sol#L23-L26 and withdrawReceivedBridgeValues https://github.com/sherlock-audit/2024-09-symmio-v0-8-4-update-contest/blob/main/protocol-core/contracts/facets/Bridge/BridgeFacet.sol#L30-L33 use transactionId to identify bridge transactions for withdrawal, but they do not check if a transactionId has already been processed.

Internal pre-conditions

No response

External pre-conditions

No response

Attack Path

Consider the following scenario:

1. Initial Withdrawal: Alice initiates a bridge transaction (transactionId = 1234) and can withdraw the received value by calling:

   bridgeFacet.withdrawReceivedBridgeValue(1234);

   This successfully transfers the bridge value associated with transactionId 1234 to Alice.

2. Replay Attack: An attacker (or even Alice herself) can repeatedly call the withdrawReceivedBridgeValue function with the same transactionId = 1234 because there is no check to mark the transactionId as "processed."

   // Repeat the same withdrawal process using the same transaction ID
   bridgeFacet.withdrawReceivedBridgeValue(1234); // This will succeed again

   This could be repeated indefinitely or until the funds are drained, leading to the attacker receiving more funds than intended. Also, withdrawReceivedBridgeValue and withdrawReceivedBridgeValues in BridgeFacetImpl.sol functions in BridgeFacetImpl.sol allow users to withdraw based on transaction IDs. There is a check to ensure the status of the transaction is RECEIVED and not already withdrawn. However, if there are no additional safeguards such as nonce tracking or a mechanism that ensures the transaction can only be processed once, an attacker may be able to replay the withdrawal under certain conditions.

Impact

The impact of a replay attack in this case can be severe, as the same transactionId can be used multiple times to drain funds from the contract. The severity depends on the amount of value being transferred via the bridge.

1. Attackers can exploit this vulnerability to withdraw funds repeatedly using the same transactionId, leading to an over-withdrawal of bridge funds. The contract may experience significant financial losses, especially if large amounts are associated with each transactionId.
2. If a malicious actor uses this vulnerability in a targeted way, it could lead to a denial of service where legitimate users are unable to withdraw their bridge values due to funds being depleted.
3. The integrity of the bridge system could be questioned if users or attackers are able to exploit the contract and withdraw more than they should. This can erode trust in the protocol and result in users losing confidence in the system.

PoC

No response

Mitigation

To prevent replay attacks, the contract should track the status of each transactionId and ensure that each transaction can only be processed once. This can be done by marking the transactionId as "processed" once it has been successfully withdrawn. You can maintain a mapping to track processed transactions:

mapping(uint256 => bool) private processedTransactions;

function withdrawReceivedBridgeValue(uint256 transactionId) external whenNotAccountingPaused notSuspended(msg.sender) {
    require(!processedTransactions[transactionId], "Transaction already processed");

    BridgeFacetImpl.withdrawReceivedBridgeValue(transactionId);
    processedTransactions[transactionId] = true; // Mark the transaction as processed

    emit WithdrawReceivedBridgeValue(transactionId);
}

Similarly, for withdrawReceivedBridgeValues:

function withdrawReceivedBridgeValues(uint256[] memory transactionIds) external whenNotAccountingPaused notSuspended(msg.sender) {
    for (uint256 i = 0; i < transactionIds.length; i++) {
        require(!processedTransactions[transactionIds[i]], "Transaction already processed");

        BridgeFacetImpl.withdrawReceivedBridgeValue(transactionIds[i]);
        processedTransactions[transactionIds[i]] = true; // Mark the transaction as processed
    }

    emit WithdrawReceivedBridgeValues(transactionIds);
}

This fix ensures that each transactionId is processed only once, preventing replay attacks.