fix(protocol): mandate bridge message only calls onMessageInvocation (#…

…15996)

Co-authored-by: D <51912515+adaki2004@users.noreply.github.com>

packages/protocol/contracts/L2/CrossChainOwned.sol

@@ -24,23 +24,27 @@ import "../bridge/IBridge.sol";
 /// signals for transaction approval.
 /// @dev Notice that when sending the message on the owner chain, the gas limit of the message must
 /// not be zero, so on this chain, some EOA can help execute this transaction.
-abstract contract CrossChainOwned is EssentialContract {
+abstract contract CrossChainOwned is EssentialContract, IMessageInvocable {
     uint64 public ownerChainId; // slot 1
     uint64 public nextTxId;
     uint256[49] private __gap;
 
     event TransactionExecuted(uint64 indexed txId, bytes4 indexed selector);
 
-    error XCO_INVALID_TX_ID();
     error XCO_INVALID_OWNER_CHAINID();
+    error XCO_INVALID_TX_ID();
     error XCO_PERMISSION_DENIED();
     error XCO_TX_REVERTED();
 
-    function executeCrossChainTransaction(uint64 txId, bytes calldata txdata) external {
+    function onMessageInvocation(bytes calldata data)
+        external
+        payable
+        whenNotPaused
+        onlyFromNamed("bridge")
+    {
+        (uint64 txId, bytes memory txdata) = abi.decode(data, (uint64, bytes));
         if (txId != nextTxId) revert XCO_INVALID_TX_ID();
 
-        if (msg.sender != resolve("bridge", false)) revert XCO_PERMISSION_DENIED();
-
         IBridge.Context memory ctx = IBridge(msg.sender).context();
         if (ctx.srcChainId != ownerChainId || ctx.from != owner()) {
             revert XCO_PERMISSION_DENIED();

packages/protocol/contracts/bridge/Bridge.sol

@@ -14,6 +14,7 @@
 
 pragma solidity 0.8.24;
 
+import "@openzeppelin/contracts/utils/Address.sol";
 import "../common/EssentialContract.sol";
 import "../libs/LibAddress.sol";
 import "../signal/ISignalService.sol";
@@ -24,6 +25,7 @@ import "./IBridge.sol";
 /// @notice See the documentation for {IBridge}.
 /// @dev The code hash for the same address on L1 and L2 may be different.
 contract Bridge is EssentialContract, IBridge {
+    using Address for address;
     using LibAddress for address;
     using LibAddress for address payable;
 
@@ -528,8 +530,15 @@ contract Bridge is EssentialContract, IBridge {
 
         _storeContext({ msgHash: msgHash, from: message.from, srcChainId: message.srcChainId });
 
-        // Perform the message call and capture the success value
-        (success,) = message.to.call{ value: message.value, gas: gasLimit }(message.data);
+        if (
+            message.data.length >= 4 // msg can be empty
+                && bytes4(message.data) != IMessageInvocable.onMessageInvocation.selector
+                && message.to.isContract()
+        ) {
+            success = false;
+        } else {
+            (success,) = message.to.call{ value: message.value, gas: gasLimit }(message.data);
+        }
 
         // Reset the context after the message call
         _resetContext();

packages/protocol/contracts/bridge/IBridge.sol

@@ -80,3 +80,12 @@ interface IRecallableSender {
         external
         payable;
 }
+
+/// @title IMessageInvocable
+/// @notice An interface that all bridge message receiver shall implement
+interface IMessageInvocable {
+    /// @notice Called when this contract is the bridge target.
+    /// @param data The data for this contract to interpret.
+    /// @dev This method should be guarded with `onlyFromNamed("bridge")`.
+    function onMessageInvocation(bytes calldata data) external payable;
+}

packages/protocol/contracts/common/EssentialContract.sol

@@ -36,6 +36,7 @@ abstract contract EssentialContract is OwnerUUPSUpgradable, AddressResolver {
     }
 
     /// @notice Initializes the contract without an address manager.
+    // solhint-disable-next-line func-name-mixedcase
     function __Essential_init() internal virtual {
         __Essential_init(address(0));
     }

packages/protocol/contracts/tokenvault/BaseVault.sol

@@ -20,7 +20,12 @@ import "../common/EssentialContract.sol";
 import "../libs/LibAddress.sol";
 import "../libs/LibDeploy.sol";
 
-abstract contract BaseVault is EssentialContract, IRecallableSender, IERC165Upgradeable {
+abstract contract BaseVault is
+    EssentialContract,
+    IRecallableSender,
+    IMessageInvocable,
+    IERC165Upgradeable
+{
     error VAULT_PERMISSION_DENIED();
 
     modifier onlyFromBridge() {

packages/protocol/contracts/tokenvault/ERC1155Vault.sol

@@ -92,28 +92,20 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
         });
     }
 
-    /// @notice This function can only be called by the bridge contract while
-    /// invoking a message call. See sendToken, which sets the data to invoke
-    /// this function.
-    /// @param ctoken The canonical ERC1155 token which may or may not live on
-    /// this chain. If not, a BridgedERC1155 contract will be deployed.
-    /// @param from The source address.
-    /// @param to The destination address.
-    /// @param tokenIds The tokenIds to be sent.
-    /// @param amounts The amounts to be sent.
-    function receiveToken(
-        CanonicalNFT calldata ctoken,
-        address from,
-        address to,
-        uint256[] memory tokenIds,
-        uint256[] memory amounts
-    )
-        external
-        payable
-        nonReentrant
-        whenNotPaused
+    /// @inheritdoc IMessageInvocable
+    function onMessageInvocation(bytes calldata data) external payable nonReentrant whenNotPaused 
+    // onlyFromBridge
     {
+        (
+            CanonicalNFT memory ctoken,
+            address from,
+            address to,
+            uint256[] memory tokenIds,
+            uint256[] memory amounts
+        ) = abi.decode(data, (CanonicalNFT, address, address, uint256[], uint256[]));
+
         // Check context validity
+        // `onlyFromBridge` checked in checkProcessMessageContext
         IBridge.Context memory ctx = checkProcessMessageContext();
 
         // Don't allow sending to disallowed addresses.
@@ -146,11 +138,14 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
         override
         nonReentrant
         whenNotPaused
+    // onlyFromBridge
     {
+        // `onlyFromBridge` checked in checkRecallMessageContext
         checkRecallMessageContext();
 
+        (bytes memory _data) = abi.decode(message.data[4:], (bytes));
         (CanonicalNFT memory ctoken,,, uint256[] memory tokenIds, uint256[] memory amounts) =
-            abi.decode(message.data[4:], (CanonicalNFT, address, address, uint256[], uint256[]));
+            abi.decode(_data, (CanonicalNFT, address, address, uint256[], uint256[]));
 
         // Transfer the ETH and tokens back to the owner
         address token = _transferTokens(ctoken, message.srcOwner, tokenIds, amounts);
@@ -287,7 +282,9 @@ contract ERC1155Vault is BaseNFTVault, ERC1155ReceiverUpgradeable {
                 }
             }
         }
-        msgData = abi.encodeCall(this.receiveToken, (ctoken, user, op.to, op.tokenIds, op.amounts));
+        msgData = abi.encodeCall(
+            this.onMessageInvocation, abi.encode(ctoken, user, op.to, op.tokenIds, op.amounts)
+        );
     }
 
     /// @dev Retrieve or deploy a bridged ERC1155 token contract.

packages/protocol/contracts/tokenvault/ERC20Vault.sol

@@ -211,22 +211,14 @@ contract ERC20Vault is BaseVault {
         });
     }
 
-    /// @notice Receive bridged ERC20 tokens and Ether.
-    /// @param ctoken Canonical ERC20 data for the token being received.
-    /// @param from Source address.
-    /// @param to Destination address.
-    /// @param amount Amount of tokens being received.
-    function receiveToken(
-        CanonicalERC20 calldata ctoken,
-        address from,
-        address to,
-        uint256 amount
-    )
-        external
-        payable
-        nonReentrant
-        whenNotPaused
+    /// @inheritdoc IMessageInvocable
+    function onMessageInvocation(bytes calldata data) external payable nonReentrant whenNotPaused 
+    // onlyFromBridge
     {
+        (CanonicalERC20 memory ctoken, address from, address to, uint256 amount) =
+            abi.decode(data, (CanonicalERC20, address, address, uint256));
+
+        // `onlyFromBridge` checked in checkProcessMessageContext
         IBridge.Context memory ctx = checkProcessMessageContext();
 
         // Don't allow sending to disallowed addresses.
@@ -257,11 +249,14 @@ contract ERC20Vault is BaseVault {
         override
         nonReentrant
         whenNotPaused
+    // onlyFromBridge
     {
+        // `onlyFromBridge` checked in checkRecallMessageContext
         checkRecallMessageContext();
 
+        (bytes memory _data) = abi.decode(message.data[4:], (bytes));
         (CanonicalERC20 memory ctoken,,, uint256 amount) =
-            abi.decode(message.data[4:], (CanonicalERC20, address, address, uint256));
+            abi.decode(_data, (CanonicalERC20, address, address, uint256));
 
         // Transfer the ETH and tokens back to the owner
         address token = _transferTokens(ctoken, message.srcOwner, amount);
@@ -342,7 +337,8 @@ contract ERC20Vault is BaseVault {
             balanceChange = t.balanceOf(address(this)) - _balance;
         }
 
-        msgData = abi.encodeCall(this.receiveToken, (ctoken, user, to, balanceChange));
+        msgData =
+            abi.encodeCall(this.onMessageInvocation, abi.encode(ctoken, user, to, balanceChange));
     }
 
     /// @dev Retrieve or deploy a bridged ERC20 token contract.

packages/protocol/contracts/tokenvault/ERC721Vault.sol

@@ -80,22 +80,14 @@ contract ERC721Vault is BaseNFTVault, IERC721ReceiverUpgradeable {
         });
     }
 
-    /// @notice Receive bridged ERC721 tokens and handle them accordingly.
-    /// @param ctoken Canonical NFT data for the token being received.
-    /// @param from Source address.
-    /// @param to Destination address.
-    /// @param tokenIds Array of token IDs being received.
-    function receiveToken(
-        CanonicalNFT calldata ctoken,
-        address from,
-        address to,
-        uint256[] memory tokenIds
-    )
-        external
-        payable
-        nonReentrant
-        whenNotPaused
+    /// @inheritdoc IMessageInvocable
+    function onMessageInvocation(bytes calldata data) external payable nonReentrant whenNotPaused 
+    // onlyFromBridge
     {
+        (CanonicalNFT memory ctoken, address from, address to, uint256[] memory tokenIds) =
+            abi.decode(data, (CanonicalNFT, address, address, uint256[]));
+
+        // `onlyFromBridge` checked in checkProcessMessageContext
         IBridge.Context memory ctx = checkProcessMessageContext();
 
         // Don't allow sending to disallowed addresses.
@@ -128,11 +120,14 @@ contract ERC721Vault is BaseNFTVault, IERC721ReceiverUpgradeable {
         override
         nonReentrant
         whenNotPaused
+    // onlyFromBridge
     {
+        // `onlyFromBridge` checked in checkRecallMessageContext
         checkRecallMessageContext();
 
+        (bytes memory _data) = abi.decode(message.data[4:], (bytes));
         (CanonicalNFT memory ctoken,,, uint256[] memory tokenIds) =
-            abi.decode(message.data[4:], (CanonicalNFT, address, address, uint256[]));
+            abi.decode(_data, (CanonicalNFT, address, address, uint256[]));
 
         // Transfer the ETH and tokens back to the owner
         address token = _transferTokens(ctoken, message.srcOwner, tokenIds);
@@ -226,7 +221,8 @@ contract ERC721Vault is BaseNFTVault, IERC721ReceiverUpgradeable {
             }
         }
 
-        msgData = abi.encodeCall(this.receiveToken, (ctoken, user, op.to, op.tokenIds));
+        msgData =
+            abi.encodeCall(this.onMessageInvocation, abi.encode(ctoken, user, op.to, op.tokenIds));
     }
 
     /// @dev Retrieve or deploy a bridged ERC721 token contract.

packages/protocol/test/HelperContracts.sol

@@ -18,10 +18,11 @@ contract BadReceiver {
     }
 }
 
-contract GoodReceiver {
+contract GoodReceiver is IMessageInvocable {
     receive() external payable { }
 
-    function forward(address addr) public payable {
+    function onMessageInvocation(bytes calldata data) public payable {
+        address addr = abi.decode(data, (address));
         payable(addr).transfer(address(this).balance / 2);
     }
 }

packages/protocol/test/bridge/Bridge.t.sol

@@ -318,7 +318,7 @@ contract BridgeTest is TaikoTest {
             value: 1000,
             fee: 1000,
             gasLimit: 1_000_000,
-            data: abi.encodeCall(GoodReceiver.forward, (Carol)),
+            data: abi.encodeCall(GoodReceiver.onMessageInvocation, abi.encode(Carol)),
             memo: ""
         });
         // Mocking proof - but obviously it needs to be created in prod

packages/protocol/test/tokenvault/ERC1155Vault.t.sol

@@ -72,8 +72,8 @@ contract PrankDestBridge {
         // a contract
         // most probably due to some deployment address nonce issue. (Seems a
         // known issue).
-        destERC1155Vault.receiveToken{ value: mockLibInvokeMsgValue }(
-            ctoken, from, to, tokenIds, amounts
+        destERC1155Vault.onMessageInvocation{ value: mockLibInvokeMsgValue }(
+            abi.encode(ctoken, from, to, tokenIds, amounts)
         );
 
         ctx.sender = address(0);

packages/protocol/test/tokenvault/ERC20Vault.t.sol

@@ -51,8 +51,8 @@ contract PrankDestBridge {
         // The problem (with foundry) is that this way it is not able to deploy
         // a contract most probably due to some deployment address nonce issue. (Seems a known
         // issue).
-        destERC20Vault.receiveToken{ value: mockLibInvokeMsgValue }(
-            canonicalToken, from, to, amount
+        destERC20Vault.onMessageInvocation{ value: mockLibInvokeMsgValue }(
+            abi.encode(canonicalToken, from, to, amount)
         );
 
         ctx.sender = address(0);

packages/protocol/test/tokenvault/ERC721Vault.t.sol

@@ -87,8 +87,8 @@ contract PrankDestBridge {
         // a contract
         // most probably due to some deployment address nonce issue. (Seems a
         // known issue).
-        destERC721Vault.receiveToken{ value: mockLibInvokeMsgValue }(
-            canonicalToken, from, to, tokenIds
+        destERC721Vault.onMessageInvocation{ value: mockLibInvokeMsgValue }(
+            abi.encode(canonicalToken, from, to, tokenIds)
         );
 
         ctx.sender = address(0);