Lack of Transaction ID Validation Allows Suspension and Restoration of Non-Existent Transactions

Summary

The suspendBridgeTransaction and restoreBridgeTransaction functions do not validate if the transaction ID is valid and corresponds to an existing transaction. This can lead to suspension and restoration of non-existent transactions.

Vulnerability Detail

Both suspendBridgeTransaction and restoreBridgeTransaction functions allow operations on transactions without checking if the transaction ID corresponds to an existing transaction. As the enum BridgeTransactionStatus.RECEIVED is equal to zero, the validations on suspendBridgeTransaction can by bypassed with an invalid ID.

Impact

This vulnerability can lead to suspension and restoration of non-existent transactions. This violates a basic invariant of the system. Although it does not disrupt with the normal working of the system.

Code Snippets

suspendBridgeTransaction function:

function suspendBridgeTransaction(uint256 transactionId) internal {
    BridgeStorage.Layout storage bridgeLayout = BridgeStorage.layout();
    BridgeTransaction storage bridgeTransaction = bridgeLayout.bridgeTransactions[transactionId];

    require(bridgeTransaction.status == BridgeTransactionStatus.RECEIVED, "BridgeFacet: Invalid status");
    bridgeTransaction.status = BridgeTransactionStatus.SUSPENDED;
}

restoreBridgeTransaction function:

function restoreBridgeTransaction(uint256 transactionId, uint256 validAmount) internal {
    BridgeStorage.Layout storage bridgeLayout = BridgeStorage.layout();
    BridgeTransaction storage bridgeTransaction = bridgeLayout.bridgeTransactions[transactionId];

    require(bridgeTransaction.status == BridgeTransactionStatus.SUSPENDED, "BridgeFacet: Invalid status");
    require(bridgeLayout.invalidBridgedAmountsPool != address(0), "BridgeFacet: Zero address");

    AccountStorage.layout().balances[bridgeLayout.invalidBridgedAmountsPool] += (bridgeTransaction.amount - validAmount);
    bridgeTransaction.status = BridgeTransactionStatus.RECEIVED;
    bridgeTransaction.amount = validAmount;
}

In the above functions, there is no check to ensure that the transactionId corresponds to a valid and existing transaction.

Tool used

Manual Review

Recommendation

Add validation checks to ensure that the transactionId corresponds to an existing transaction before performing any operations on it. For example:

function suspendBridgeTransaction(uint256 transactionId) internal {
    BridgeStorage.Layout storage bridgeLayout = BridgeStorage.layout();
    require(transactionId <= bridgeLayout.lastId, "BridgeFacet: Invalid transactionId");
    BridgeTransaction storage bridgeTransaction = bridgeLayout.bridgeTransactions[transactionId];

    require(bridgeTransaction.status == BridgeTransactionStatus.RECEIVED, "BridgeFacet: Invalid status");
    bridgeTransaction.status = BridgeTransactionStatus.SUSPENDED;
}

It is not required to add any validation for the restoreBridgeTransaction as the enum validation of BridgeTransactionStatus.SUSPENDED can not be bypassed if suspendBridgeTransaction is controlled appropriately.

sherlock-admin3 commented 3 months ago

1 comment(s) were left on this issue during the judging contest.

Hash01011122 commented:

Invalid

sherlock-admin2 commented 3 months ago

The protocol team fixed this issue in the following PRs/commits: https://github.com/SYMM-IO/protocol-core/pull/52