0xAadi - Lack of Mechanism to Track `transactionId` in `BridgeFacet` Contract Prevents Bridges from Seamlessly Withdrawing Incoming Transfers

[sherlock-admin3]

0xAadi

Medium

Lack of Mechanism to Track transactionId in BridgeFacet Contract Prevents Bridges from Seamlessly Withdrawing Incoming Transfers

Summary

The transferToBridge() function in the BridgeFacet contract is designed to transfer a specified amount to a designated bridge address. The bridges can then withdraw the received bridge value associated with a specific transaction ID using withdrawReceivedBridgeValue. However, since there is no mechanism in this contract to track the transaction ID of a RECEIVE transaction to a bridge, the withdrawal process becomes very complex.

Vulnerability Detail

The vulnerability lies in the BridgeFacet contract. The transferToBridge() function, which processes the transfer, calls the transferToBridge() function from the BridgeFacetImpl library. While the Layout structure in the BridgeStorage library stores the transfer information, it is not publicly accessible because the layout() function within the library is marked as internal and there is no functions to view the layout data in BridgeFacet contract.

File: contracts/facets/Bridge/BridgeFacet.sol

    function transferToBridge(uint256 amount, address bridgeAddress) external whenNotAccountingPaused notSuspended(msg.sender) {
        BridgeFacetImpl.transferToBridge(msg.sender, amount, bridgeAddress);
        emit TransferToBridge(msg.sender, amount, bridgeAddress);
    }

File: contracts/facets/Bridge/BridgeFacetImpl.sol

    function transferToBridge(address user, uint256 amount, address bridge) internal {
        GlobalAppStorage.Layout storage appLayout = GlobalAppStorage.layout();
        BridgeStorage.Layout storage bridgeLayout = BridgeStorage.layout();

        require(bridgeLayout.bridges[bridge], "BridgeFacet: Invalid bridge");
        require(bridge != user, "BridgeFacet: Bridge and user can't be the same");

        uint256 amountWith18Decimals = (amount * 1e18) / (10 ** IERC20Metadata(appLayout.collateral).decimals());
        require(AccountStorage.layout().balances[user] >= amountWith18Decimals, "BridgeFacet: Insufficient balance");

        uint256 currentId = ++bridgeLayout.lastId;
        BridgeTransaction memory bridgeTransaction = BridgeTransaction({
            id: currentId,
            amount: amount,
            user: user,
            bridge: bridge,
            timestamp: block.timestamp,
            status: BridgeTransactionStatus.RECEIVED
        });
        AccountStorage.layout().balances[user] -= amountWith18Decimals;
        bridgeLayout.bridgeTransactions[currentId] = bridgeTransaction;
    }

File: contracts/storages/BridgeStorage.sol

    struct Layout {
        mapping(address => bool) bridges;
        mapping(uint256 => BridgeTransaction) bridgeTransactions;
        uint256 lastId;
        address invalidBridgedAmountsPool;
    }

    function layout() internal pure returns (Layout storage l) {
        bytes32 slot = BRIDGE_STORAGE_SLOT;
        assembly {
            l.slot := slot
        }
    }

Additionally, both the above functions and the contracts fail to implement a tracking mechanism to track or log the transactionId. This causes a situation where, without knowing the transaction ID associated with the bridge, the receiving bridge has to try out different transaction IDs in the withdrawReceivedBridgeValue() function to claim their receiving amount.

Impact

Due to the lack of a tracking mechanism or log, the receiving bridge cannot identify their incoming transfers. This forces the receiving bridge to try random transaction IDs to claim their amount. As a result, most of the transactions will revert due to the use of incorrect transaction IDs associated with other bridges. The rate of reverts in withdrawReceivedBridgeValues() will be high without knowing the exact transaction IDs associated with that bridge.

Code Snippet

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/f5b76ca33f5f05b927a9c0f2f57938e919d6420b/protocol-core/contracts/facets/Bridge/BridgeFacet.sol#L14C1-L27C6

https://github.com/sherlock-audit/2024-06-symmetrical-update-2/blob/f5b76ca33f5f05b927a9c0f2f57938e919d6420b/protocol-core/contracts/facets/Bridge/BridgeFacetImpl.sol#L18C2-L39C3

Tool used

Manual Review

Recommendation

Implement a proper tracking mechanism to track transactionIds associated with any bridge, so that the bridge can fetch the incoming transaction IDs associated with them and withdraw the amount.

One method to use array to track induvidual bridges incoming transaction ids

Example:

struct Layout {
        mapping(address => bool) bridges;
        mapping(uint256 => BridgeTransaction) bridgeTransactions;
+               mapping(address => uint256[]) bridgesTransactionIds;
        uint256 lastId;
        address invalidBridgedAmountsPool;
    }

Push transactionId to this array while perform transfer and pop while performing withdraw.

[sherlock-admin2]

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

Hash01011122 commented:

Low/info vulnerability, No loss of funds or brick in core functionality of contract. Issue just suggests adding a mapping function of transactionId as an improvement to contracts

[sherlock-admin2]

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

[0xAadi]

Escalate

I believe this issue is valid medium.

The core issue is that the contract fails to expose transactionIds.

The transactionId is the pivote entity in this contract, which is generated in the transferToBridge() function, and used by the following core functions:

1. withdrawReceivedBridgeValue()
2. withdrawReceivedBridgeValues()
3. suspendBridgeTransaction()
4. restoreBridgeTransaction()

Without exposing the transactionId either in event or in a public function, none of the user cant able to use the above core functions.

For example, if there are few receive transactions, bridges can detect incoming transactions by reading the TransferToBridge event. However, without the specific transactionId, they cannot execute withdrawReceivedBridgeValue() or restoreBridgeTransaction() to withdraw funds, leading to locked funds. Inorder to withdraw the funds they need to try random transactionIds.

Similarly, without the exact transactionId, executing suspendBridgeTransaction() and restoreBridgeTransaction() becomes problematic. There is a higher risk of using an incorrect transactionId and mistakenly suspending legitimate transactions.

In my opinion, if the transactionId is not correctly logged or made publicly visible, there is a higher chance of a bridge missing the incoming transactionId, resulting in locked funds until the correct transactionId is used.

[sherlock-admin3]

Escalate

I believe this issue is valid medium.

The core issue is that the contract fails to expose transactionIds.

The transactionId is the pivote entity in this contract, which is generated in the transferToBridge() function, and used by the following core functions:

1. withdrawReceivedBridgeValue()
2. withdrawReceivedBridgeValues()
3. suspendBridgeTransaction()
4. restoreBridgeTransaction()

Without exposing the transactionId either in event or in a public function, none of the user cant able to use the above core functions.

For example, if there are few receive transactions, bridges can detect incoming transactions by reading the TransferToBridge event. However, without the specific transactionId, they cannot execute withdrawReceivedBridgeValue() or restoreBridgeTransaction() to withdraw funds, leading to locked funds. Inorder to withdraw the funds they need to try random transactionIds.

Similarly, without the exact transactionId, executing suspendBridgeTransaction() and restoreBridgeTransaction() becomes problematic. There is a higher risk of using an incorrect transactionId and mistakenly suspending legitimate transactions.

In my opinion, if the transactionId is not correctly logged or made publicly visible, there is a higher chance of a bridge missing the incoming transactionId, resulting in locked funds until the correct transactionId is used.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[0xAadi]

I will be grateful if someone can help me to escalate this issue.

[sherlock-admin3]

Escalate

Escalating on behalf of this comment: https://github.com/sherlock-audit/2024-06-symmetrical-update-2-judging/issues/1#issuecomment-2193709467

You've deleted an escalation for this issue.

[Autosaida]

The protocol includes a getBridgeTransaction function to retrieve all bridge transactions, allowing to find the corresponding transactionId based on the bridge. Although this might be somewhat cumbersome, it is clearly not a valid issue as you pointed out.

    function getBridgeTransaction(uint256 transactionId) external view returns (BridgeTransaction memory) {
        return BridgeStorage.layout().bridgeTransactions[transactionId];
    }

They only need to search one by one and find the bridge transaction belong to them. And this function is a view function, so there is not esist loss of fund. According to the sherlock rules, i think it just belong to Design decision.

Design decisions are not valid issues. Even if the design is suboptimal, but doesn't imply any loss of funds, these issues are considered informational.

[0xAadi]

Agree, the only impact is offline logic might be complex.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.