0xAadi - Lack of 18 Decimal Scaling in Balance Update in `restoreBridgeTransaction` Function in `BridgeFacetImpl` Library

[sherlock-admin4]

0xAadi

High

Lack of 18 Decimal Scaling in Balance Update in restoreBridgeTransaction Function in BridgeFacetImpl Library

Summary

The suspendBridgeTransaction() function in the BridgeFacet contract is designed to suspend a specific bridge transaction. A user with the DISPUTE_ROLE can restore the previously suspended bridge transaction and update the valid transaction amount through the restoreBridgeTransaction function by calling restoreBridgeTransaction from the BridgeFacetImpl library.

The issue is that the restoreBridgeTransaction function in the BridgeFacetImpl library does not perform 18 decimal scaling in the balance update when subtracting the validAmount from bridgeTransaction.amount. This is in contrast to the transferToBridge function, which correctly scales amounts to 18 decimals before updating balances.

Vulnerability Detail

The code in the restoreBridgeTransaction function that updates the balance does not scale the amounts to 18 decimals, leading to incorrect balance updates. This can result in an inconsistent state where the balances are not properly adjusted based on the amount being restored.

    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;
    }

Impact

The incorrect balance update can lead to inconsistencies in the token balances of the affected accounts. This could potentially result in loss of funds and incorrect accounting of token holdings.

Code Snippet

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

Tool used

Manual Review

Recommendation

Update the restoreBridgeTransaction() function to perform 18 decimal scaling when updating balances, similar to the transferToBridge function. This ensures consistency in balance updates and prevents potential issues related to incorrect token balances.

    function restoreBridgeTransaction(uint256 transactionId, uint256 validAmount) internal {
+       GlobalAppStorage.Layout storage appLayout = GlobalAppStorage.layout();
        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");

+       uint256 amountWith18Decimals = ((bridgeTransaction.amount - validAmount) * 1e18) / (10 ** IERC20Metadata(appLayout.collateral).decimals());
-       AccountStorage.layout().balances[bridgeLayout.invalidBridgedAmountsPool] += (bridgeTransaction.amount - validAmount);
+       AccountStorage.layout().balances[bridgeLayout.invalidBridgedAmountsPool] += amountWith18Decimals;
        bridgeTransaction.status = BridgeTransactionStatus.RECEIVED;
        bridgeTransaction.amount = validAmount;
    }

Duplicate of #5

[sherlock-admin2]

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

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.