diff --git a/packages/protocol/contracts/L2/CrossChainOwned.sol b/packages/protocol/contracts/L2/CrossChainOwned.sol index 132cf651fd3..461f276fb36 100644 --- a/packages/protocol/contracts/L2/CrossChainOwned.sol +++ b/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(); diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol index f6d0e477d98..8c4e0b0bc9d 100644 --- a/packages/protocol/contracts/bridge/Bridge.sol +++ b/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(); diff --git a/packages/protocol/contracts/bridge/IBridge.sol b/packages/protocol/contracts/bridge/IBridge.sol index db509f1e4e8..ab2f5e535fe 100644 --- a/packages/protocol/contracts/bridge/IBridge.sol +++ b/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; +} diff --git a/packages/protocol/contracts/common/EssentialContract.sol b/packages/protocol/contracts/common/EssentialContract.sol index beacb412704..f522369da51 100644 --- a/packages/protocol/contracts/common/EssentialContract.sol +++ b/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)); } diff --git a/packages/protocol/contracts/tokenvault/BaseVault.sol b/packages/protocol/contracts/tokenvault/BaseVault.sol index 38dba2c7c99..8d58739f181 100644 --- a/packages/protocol/contracts/tokenvault/BaseVault.sol +++ b/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() { diff --git a/packages/protocol/contracts/tokenvault/ERC1155Vault.sol b/packages/protocol/contracts/tokenvault/ERC1155Vault.sol index 83e09bb4c62..a6c600d2069 100644 --- a/packages/protocol/contracts/tokenvault/ERC1155Vault.sol +++ b/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. diff --git a/packages/protocol/contracts/tokenvault/ERC20Vault.sol b/packages/protocol/contracts/tokenvault/ERC20Vault.sol index 567b709c5d2..5290977a012 100644 --- a/packages/protocol/contracts/tokenvault/ERC20Vault.sol +++ b/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. diff --git a/packages/protocol/contracts/tokenvault/ERC721Vault.sol b/packages/protocol/contracts/tokenvault/ERC721Vault.sol index 0c3151f27b9..d4d9b3ebeba 100644 --- a/packages/protocol/contracts/tokenvault/ERC721Vault.sol +++ b/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. diff --git a/packages/protocol/test/HelperContracts.sol b/packages/protocol/test/HelperContracts.sol index 2fc9a4663ea..b8f28a73146 100644 --- a/packages/protocol/test/HelperContracts.sol +++ b/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); } } diff --git a/packages/protocol/test/bridge/Bridge.t.sol b/packages/protocol/test/bridge/Bridge.t.sol index d9a11499e5f..418654eec54 100644 --- a/packages/protocol/test/bridge/Bridge.t.sol +++ b/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 diff --git a/packages/protocol/test/tokenvault/ERC1155Vault.t.sol b/packages/protocol/test/tokenvault/ERC1155Vault.t.sol index f029a0ed369..d4704d7c0f2 100644 --- a/packages/protocol/test/tokenvault/ERC1155Vault.t.sol +++ b/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); diff --git a/packages/protocol/test/tokenvault/ERC20Vault.t.sol b/packages/protocol/test/tokenvault/ERC20Vault.t.sol index 0d9f07988bf..68af505f05a 100644 --- a/packages/protocol/test/tokenvault/ERC20Vault.t.sol +++ b/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); diff --git a/packages/protocol/test/tokenvault/ERC721Vault.t.sol b/packages/protocol/test/tokenvault/ERC721Vault.t.sol index 4329a72e6ed..570904d0236 100644 --- a/packages/protocol/test/tokenvault/ERC721Vault.t.sol +++ b/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);