From 7d7af53d3e6e8b373145015f8cc9c842bab433f1 Mon Sep 17 00:00:00 2001 From: Stanislav Bezkorovainyi Date: Tue, 18 Jun 2024 10:38:48 +0200 Subject: [PATCH 01/14] Fix incorrect clause (#535) --- .gitignore | 1 + system-contracts/SystemContractsHashes.json | 4 ++-- system-contracts/contracts/ContractDeployer.sol | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 63f96063b..1897dead9 100644 --- a/.gitignore +++ b/.gitignore @@ -26,3 +26,4 @@ l1-contracts/broadcast/* l1-contracts/script-config/* l1-contracts/script-out/* !l1-contracts/script-out/.gitkeep +*.timestamp diff --git a/system-contracts/SystemContractsHashes.json b/system-contracts/SystemContractsHashes.json index 5c927c10f..f45ca6844 100644 --- a/system-contracts/SystemContractsHashes.json +++ b/system-contracts/SystemContractsHashes.json @@ -31,8 +31,8 @@ "contractName": "ContractDeployer", "bytecodePath": "artifacts-zk/contracts-preprocessed/ContractDeployer.sol/ContractDeployer.json", "sourceCodePath": "contracts-preprocessed/ContractDeployer.sol", - "bytecodeHash": "0x010004e5219e70c32d67ed1df0c46983a83a382e482abb5c4310ad554f55f738", - "sourceCodeHash": "0x28208c532ed8851077a9bb0a87636215840ce964397ab0767692f956a0fd11b3" + "bytecodeHash": "0x010004e5b06e78c8ffbdf6c578ce9d101d8d852e62e71f79ff8f845e7ada7fc9", + "sourceCodeHash": "0xdc95cb75554260ef8793c7899dbbc681165a441109458b3d058b03a0857d00e3" }, { "contractName": "Create2Factory", diff --git a/system-contracts/contracts/ContractDeployer.sol b/system-contracts/contracts/ContractDeployer.sol index 30d5e3930..3148de8ff 100644 --- a/system-contracts/contracts/ContractDeployer.sol +++ b/system-contracts/contracts/ContractDeployer.sol @@ -78,7 +78,7 @@ contract ContractDeployer is IContractDeployer, ISystemContract { AccountInfo memory currentInfo = accountInfo[msg.sender]; if ( - _nonceOrdering != AccountNonceOrdering.Arbitrary && + _nonceOrdering != AccountNonceOrdering.Arbitrary || currentInfo.nonceOrdering != AccountNonceOrdering.Sequential ) { revert InvalidNonceOrderingChange(); From 100b3bfea3444767a04dbeab68c7f0ee6b277d82 Mon Sep 17 00:00:00 2001 From: Vlad Bochok <41153528+vladbochok@users.noreply.github.com> Date: Tue, 18 Jun 2024 15:01:14 +0200 Subject: [PATCH 02/14] Adding doc comments to the chain contracts (#530) Signed-off-by: Danil Co-authored-by: Lyova Potyomkin Co-authored-by: kelemeno Co-authored-by: Stanislav Bezkorovainyi Co-authored-by: Raid Ateir Co-authored-by: kelemeno <34402761+kelemeno@users.noreply.github.com> Co-authored-by: Jmunoz Co-authored-by: Bence Haromi Co-authored-by: Danil --- .../contracts/bridge/L1SharedBridge.sol | 21 +++++++++++++-- .../contracts/bridgehub/Bridgehub.sol | 26 +++++++++++++++++++ .../chain-deps/facets/Mailbox.sol | 2 +- .../contracts/bridge/L2SharedBridge.sol | 4 +-- 4 files changed, 48 insertions(+), 5 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 541b0e8eb..442f15e8a 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -156,7 +156,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } /// @dev This sets the first post upgrade batch for era, used to check old withdrawals - /// @param _eraLegacyBridgeLastDepositBatch The the zkSync Era batch number that processes the last deposit tx initiated by the legacy bridge + /// @param _eraLegacyBridgeLastDepositBatch The the zkSync Era batch number that processes the last deposit tx initiated by the legacy bridge /// @param _eraLegacyBridgeLastDepositTxNumber The tx number in the _eraLegacyBridgeLastDepositBatch of the last deposit tx initiated by the legacy bridge function setEraLegacyBridgeLastDepositTime( uint256 _eraLegacyBridgeLastDepositBatch, @@ -168,7 +168,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade eraLegacyBridgeLastDepositTxNumber = _eraLegacyBridgeLastDepositTxNumber; } - /// @dev transfer tokens from legacy erc20 bridge or mailbox and set chainBalance as part of migration process + /// @dev Transfer tokens from legacy erc20 bridge or mailbox and set chainBalance as part of migration process. + /// @param _token The address of token to be transferred (address(1) for ether and contract address for ERC20). + /// @param _target The hyperchain or bridge contract address from where to transfer funds. + /// @param _targetChainId The chain ID of the corresponding hyperchain. function transferFundsFromLegacy(address _token, address _target, uint256 _targetChainId) external onlySelf { if (_token == ETH_TOKEN_ADDRESS) { uint256 balanceBefore = address(this).balance; @@ -204,6 +207,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } } + /// @dev Accepts ether only from the hyperchain associated with the specified chain ID. + /// @param _chainId The chain ID corresponding to the hyperchain allowed to send ether. function receiveEth(uint256 _chainId) external payable { require(BRIDGE_HUB.getHyperchain(_chainId) == msg.sender, "receiveEth not state transition"); } @@ -215,6 +220,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @notice Allows bridgehub to acquire mintValue for L1->L2 transactions. /// @dev If the corresponding L2 transaction fails, refunds are issued to a refund recipient on L2. + /// @param _chainId The chain ID of the hyperchain to which deposit. + /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one. + /// @param _l1Token The L1 token address which is deposited. + /// @param _amount The total amount of tokens to be bridged. function bridgehubDepositBaseToken( uint256 _chainId, address _prevMsgSender, @@ -250,6 +259,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } /// @notice Initiates a deposit transaction within Bridgehub, used by `requestL2TransactionTwoBridges`. + /// @param _chainId The chain ID of the hyperchain to which deposit. + /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one. + /// @param _l2Value The L2 `msg.value` from the L1 -> L2 deposit transaction. + /// @param _data The calldata for the second bridge deposit. function bridgehubDeposit( uint256 _chainId, address _prevMsgSender, @@ -315,6 +328,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @notice Confirms the acceptance of a transaction by the Mailbox, as part of the L2 transaction process within Bridgehub. /// This function is utilized by `requestL2TransactionTwoBridges` to validate the execution of a transaction. + /// @param _chainId The chain ID of the hyperchain to which confirm the deposit. + /// @param _txDataHash The keccak256 hash of abi.encode(msgSender, l1Token, amount) + /// @param _txHash The hash of the L1->L2 transaction to confirm the deposit. function bridgehubConfirmL2Transaction( uint256 _chainId, bytes32 _txDataHash, @@ -665,6 +681,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// of processing an L2 transaction where tokens would be minted. /// @dev If the token is bridged for the first time, the L2 token contract will be deployed. Note however, that the /// newly-deployed token does not support any custom logic, i.e. rebase tokens' functionality is not supported. + /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one. /// @param _l2Receiver The account address that should receive funds on L2 /// @param _l1Token The L1 token address which is deposited /// @param _amount The total amount of tokens to be bridged diff --git a/l1-contracts/contracts/bridgehub/Bridgehub.sol b/l1-contracts/contracts/bridgehub/Bridgehub.sol index 8a1498fec..51e3af1eb 100644 --- a/l1-contracts/contracts/bridgehub/Bridgehub.sol +++ b/l1-contracts/contracts/bridgehub/Bridgehub.sol @@ -16,6 +16,11 @@ import {ETH_TOKEN_ADDRESS, TWO_BRIDGES_MAGIC_VALUE, BRIDGEHUB_MIN_SECOND_BRIDGE_ import {BridgehubL2TransactionRequest, L2Message, L2Log, TxStatus} from "../common/Messaging.sol"; import {AddressAliasHelper} from "../vendor/AddressAliasHelper.sol"; +/// @author Matter Labs +/// @custom:security-contact security@matterlabs.dev +/// @dev The Bridgehub contract serves as the primary entry point for L1<->L2 communication, +/// facilitating interactions between end user and bridges. +/// It also manages state transition managers, base tokens, and chain registrations. contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, PausableUpgradeable { /// @notice all the ether is held by the weth bridge IL1SharedBridge public sharedBridge; @@ -155,6 +160,12 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus //// Mailbox forwarder /// @notice forwards function call to Mailbox based on ChainId + /// @param _chainId The chain ID of the hyperchain where to prove L2 message inclusion. + /// @param _batchNumber The executed L2 batch number in which the message appeared + /// @param _index The position in the L2 logs Merkle tree of the l2Log that was sent with the message + /// @param _message Information about the sent message: sender address, the message itself, tx index in the L2 batch where the message was sent + /// @param _proof Merkle proof for inclusion of L2 log that was sent with the message + /// @return Whether the proof is valid function proveL2MessageInclusion( uint256 _chainId, uint256 _batchNumber, @@ -167,6 +178,12 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus } /// @notice forwards function call to Mailbox based on ChainId + /// @param _chainId The chain ID of the hyperchain where to prove L2 log inclusion. + /// @param _batchNumber The executed L2 batch number in which the log appeared + /// @param _index The position of the l2log in the L2 logs Merkle tree + /// @param _log Information about the sent log + /// @param _proof Merkle proof for inclusion of the L2 log + /// @return Whether the proof is correct and L2 log is included in batch function proveL2LogInclusion( uint256 _chainId, uint256 _batchNumber, @@ -179,6 +196,15 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus } /// @notice forwards function call to Mailbox based on ChainId + /// @param _chainId The chain ID of the hyperchain where to prove L1->L2 tx status. + /// @param _l2TxHash The L2 canonical transaction hash + /// @param _l2BatchNumber The L2 batch number where the transaction was processed + /// @param _l2MessageIndex The position in the L2 logs Merkle tree of the l2Log that was sent with the message + /// @param _l2TxNumberInBatch The L2 transaction number in the batch, in which the log was sent + /// @param _merkleProof The Merkle proof of the processing L1 -> L2 transaction + /// @param _status The execution status of the L1 -> L2 transaction (true - success & 0 - fail) + /// @return Whether the proof is correct and the transaction was actually executed with provided status + /// NOTE: It may return `false` for incorrect proof, but it doesn't mean that the L1 -> L2 transaction has an opposite status! function proveL1ToL2TransactionStatus( uint256 _chainId, bytes32 _l2TxHash, diff --git a/l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol b/l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol index a65a47b55..4c38ef4a2 100644 --- a/l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol +++ b/l1-contracts/contracts/state-transition/chain-deps/facets/Mailbox.sol @@ -36,7 +36,7 @@ contract MailboxFacet is ZkSyncHyperchainBase, IMailbox { string public constant override getName = "MailboxFacet"; /// @dev Era's chainID - uint256 public immutable ERA_CHAIN_ID; + uint256 internal immutable ERA_CHAIN_ID; constructor(uint256 _eraChainId) { ERA_CHAIN_ID = _eraChainId; diff --git a/l2-contracts/contracts/bridge/L2SharedBridge.sol b/l2-contracts/contracts/bridge/L2SharedBridge.sol index 2a0fe1903..a96b45252 100644 --- a/l2-contracts/contracts/bridge/L2SharedBridge.sol +++ b/l2-contracts/contracts/bridge/L2SharedBridge.sol @@ -39,10 +39,10 @@ contract L2SharedBridge is IL2SharedBridge, Initializable { /// This is non-zero only on Era, and should not be renamed for backward compatibility with the SDKs. address public override l1Bridge; - uint256 public immutable ERA_CHAIN_ID; - /// @dev Contract is expected to be used as proxy implementation. /// @dev Disable the initialization to prevent Parity hack. + uint256 public immutable ERA_CHAIN_ID; + constructor(uint256 _eraChainId) { ERA_CHAIN_ID = _eraChainId; _disableInitializers(); From 125d66e3ef0dd3a02b57fd95d9100c6696e085f4 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Wed, 19 Jun 2024 15:44:37 +0400 Subject: [PATCH 03/14] (fix): ADT could be set to virtual address --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 865f62c1f..9a6dc6aaf 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -187,7 +187,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade address sender = msg.sender == address(nativeTokenVault) ? NATIVE_TOKEN_VAULT_VIRTUAL_ADDRESS : msg.sender; bytes32 assetId = keccak256(abi.encode(uint256(block.chainid), sender, _additionalData)); assetHandlerAddress[assetId] = _assetHandlerAddress; - assetDeploymentTracker[assetId] = sender; + assetDeploymentTracker[assetId] = msg.sender; emit AssetHandlerRegisteredInitial(assetId, _assetHandlerAddress, _additionalData, sender); } From 439190ffe991860f838afabd9efef392e455fc92 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Wed, 19 Jun 2024 16:05:46 +0400 Subject: [PATCH 04/14] (fix known issues): remove unnecessary call to _getAssetProperties --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 9a6dc6aaf..cb2027d3b 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -337,7 +337,6 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade require(BRIDGE_HUB.baseTokenAssetId(_chainId) != _assetId, "ShB: baseToken deposit not supported"); - (, _assetId) = _getAssetProperties(_assetId); // Handles the non-legacy case bytes memory bridgeMintCalldata = _burn({ _chainId: _chainId, _l2Value: _l2Value, From 005441d6a1d405e724958a3003891abb93c2671b Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Wed, 19 Jun 2024 16:58:51 +0400 Subject: [PATCH 05/14] (fix): doc --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 17b50cf29..a04216ba2 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -225,8 +225,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @notice Allows bridgehub to acquire mintValue for L1->L2 transactions. /// @dev If the corresponding L2 transaction fails, refunds are issued to a refund recipient on L2. /// @param _chainId The chain ID of the hyperchain to which deposit. + /// @param _assetId The deposited asset ID. /// @param _prevMsgSender The `msg.sender` address from the external call that initiated current one. - /// @param _l1Token The L1 token address which is deposited. /// @param _amount The total amount of tokens to be bridged. function bridgehubDepositBaseToken( uint256 _chainId, From 0ded81c8960fdb399ccf656238c16f6046565d1e Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Wed, 19 Jun 2024 19:47:16 +0400 Subject: [PATCH 06/14] (feat): update _parseL2WithdrawalMessage to support both legacy and new finalizeWithdrawal encodings --- .../contracts/bridge/L1SharedBridge.sol | 17 ++++++++++++++--- .../L1SharedBridge/L1SharedBridgeBase.t.sol | 10 +++++----- .../L1SharedBridgeHyperEnabled.t.sol | 10 +++++----- .../L1SharedBridge/L1SharedBridgeLegacy.t.sol | 4 ++-- .../unit_tests/l1_shared_bridge_test.spec.ts | 2 +- .../test/unit_tests/legacy_era_test.spec.ts | 6 +++--- .../contracts/bridge/L2SharedBridge.sol | 3 ++- 7 files changed, 32 insertions(+), 20 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index a04216ba2..c31cbd838 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -671,12 +671,23 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade transferData = abi.encode(amount, l1Receiver); } else if (bytes4(functionSignature) == IL1ERC20Bridge.finalizeWithdrawal.selector) { // We use the IL1ERC20Bridge for backward compatibility with old withdrawals. - + address l1Token; // this message is a token withdrawal - (assetId, offset) = UnsafeBytes.readBytes32(_l2ToL1message, offset); - transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset); + + // Check that the message length is correct. + // It should be equal to the length of the function signature + address + address + uint256 = 4 + 20 + 20 + 32 = + // 76 (bytes). + require(_l2ToL1message.length == 76, "ShB wrong msg len 2"); + (l1Receiver, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset); + (l1Token, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset); + (amount, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset); + + assetId = keccak256(abi.encode(block.chainid, NATIVE_TOKEN_VAULT_VIRTUAL_ADDRESS, l1Token)); + transferData = abi.encode(amount, l1Receiver); } else if (bytes4(functionSignature) == this.finalizeWithdrawal.selector) { //todo + (assetId, offset) = UnsafeBytes.readBytes32(_l2ToL1message, offset); + transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset); } else { revert("ShB Incorrect message function selector"); } diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol index 32b96aea1..edd63bf45 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol @@ -7,7 +7,7 @@ import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; -import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; +import {IL1SharedBridge} from "contracts/bridge/interfaces/IL1SharedBridge.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; import {IGetters} from "contracts/state-transition/chain-interfaces/IGetters.sol"; import {L1NativeTokenVault} from "contracts/bridge/L1NativeTokenVault.sol"; @@ -199,7 +199,7 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { function test_finalizeWithdrawal_ErcOnEth() public { _setNativeTokenVaultChainBalance(chainId, address(token), amount); bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); @@ -244,7 +244,7 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { vm.prank(bridgehubAddress); bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, ETH_TOKEN_ASSET_ID, abi.encode(amount, alice) ); @@ -286,7 +286,7 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { vm.prank(bridgehubAddress); bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); @@ -325,7 +325,7 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { function test_finalizeWithdrawal_NonBaseErcOnErc() public { bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeHyperEnabled.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeHyperEnabled.t.sol index 051e3a56c..5b6d7a5d8 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeHyperEnabled.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeHyperEnabled.t.sol @@ -7,7 +7,7 @@ import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; -import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; +import {IL1SharedBridge} from "contracts/bridge/interfaces/IL1SharedBridge.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; // note, this should be the same as where hyper is disabled @@ -217,7 +217,7 @@ contract L1SharedBridgeHyperEnabledTest is L1SharedBridgeTest { _setBaseTokenAssetId(ETH_TOKEN_ASSET_ID); bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); @@ -258,7 +258,7 @@ contract L1SharedBridgeHyperEnabledTest is L1SharedBridgeTest { _setBaseTokenAssetId(tokenAssetId); bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, ETH_TOKEN_ASSET_ID, abi.encode(amount, alice) ); @@ -299,7 +299,7 @@ contract L1SharedBridgeHyperEnabledTest is L1SharedBridgeTest { _setBaseTokenAssetId(tokenAssetId); bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); @@ -338,7 +338,7 @@ contract L1SharedBridgeHyperEnabledTest is L1SharedBridgeTest { function test_finalizeWithdrawal_NonBaseErcOnErc2() public { bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol index 6873d8047..7869acc18 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol @@ -9,7 +9,7 @@ import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; -import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; +import {IL1SharedBridge} from "contracts/bridge/interfaces/IL1SharedBridge.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; contract L1SharedBridgeLegacyTest is L1SharedBridgeTest { @@ -99,7 +99,7 @@ contract L1SharedBridgeLegacyTest is L1SharedBridgeTest { // solhint-disable-next-line func-named-parameters bytes memory message = abi.encodePacked( - IL1ERC20Bridge.finalizeWithdrawal.selector, + IL1SharedBridge.finalizeWithdrawal.selector, tokenAssetId, abi.encode(amount, alice) ); diff --git a/l1-contracts/test/unit_tests/l1_shared_bridge_test.spec.ts b/l1-contracts/test/unit_tests/l1_shared_bridge_test.spec.ts index 4ea040db9..05e98e136 100644 --- a/l1-contracts/test/unit_tests/l1_shared_bridge_test.spec.ts +++ b/l1-contracts/test/unit_tests/l1_shared_bridge_test.spec.ts @@ -195,7 +195,7 @@ describe("Shared Bridge tests", () => { [ethers.constants.HashZero] ) ); - expect(revertReason).equal("ShB withd w proof"); + expect(revertReason).equal("ShB wrong msg len 2"); }); it("Should revert on finalizing a withdrawal with wrong function selector", async () => { diff --git a/l1-contracts/test/unit_tests/legacy_era_test.spec.ts b/l1-contracts/test/unit_tests/legacy_era_test.spec.ts index da771332b..11f011c5a 100644 --- a/l1-contracts/test/unit_tests/legacy_era_test.spec.ts +++ b/l1-contracts/test/unit_tests/legacy_era_test.spec.ts @@ -130,9 +130,9 @@ describe("Legacy Era tests", function () { const l1Receiver = await randomSigner.getAddress(); l2ToL1message = ethers.utils.hexConcat([ functionSignature, - "0x".concat(l1Receiver.slice(2).padStart(64, "0")), - "0x".concat(ethers.utils.parseUnits("800", 18).toHexString().slice(2).padStart(64, "0")), - "0x".concat(erc20TestToken.address.slice(2).padStart(64, "0")), + l1Receiver, + erc20TestToken.address, + ethers.constants.HashZero, ]); }); diff --git a/l2-contracts/contracts/bridge/L2SharedBridge.sol b/l2-contracts/contracts/bridge/L2SharedBridge.sol index 4c0c6c7fa..2beb326ba 100644 --- a/l2-contracts/contracts/bridge/L2SharedBridge.sol +++ b/l2-contracts/contracts/bridge/L2SharedBridge.sol @@ -6,6 +6,7 @@ import {Initializable} from "@openzeppelin/contracts/proxy/utils/Initializable.s import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; import {IL1ERC20Bridge} from "./interfaces/IL1ERC20Bridge.sol"; +import {IL1SharedBridge} from "./interfaces/IL1SharedBridge.sol"; import {IL2SharedBridge} from "./interfaces/IL2SharedBridge.sol"; import {ILegacyL2SharedBridge} from "./interfaces/ILegacyL2SharedBridge.sol"; import {IL2AssetHandler} from "./interfaces/IL2AssetHandler.sol"; @@ -143,7 +144,7 @@ contract L2SharedBridge is IL2SharedBridge, ILegacyL2SharedBridge, Initializable // note we use the IL1ERC20Bridge.finalizeWithdrawal function selector to specify the selector for L1<>L2 messages, // and we use this interface so that when the switch happened the old messages could be processed // solhint-disable-next-line func-named-parameters - return abi.encodePacked(IL1ERC20Bridge.finalizeWithdrawal.selector, _assetId, _bridgeMintData); + return abi.encodePacked(IL1SharedBridge.finalizeWithdrawal.selector, _assetId, _bridgeMintData); } /// @dev Used to set the assedAddress for a given assetId. From a83590ccf6e7eed21fe0936e81c92fffa2db09e2 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Fri, 21 Jun 2024 13:46:46 +0400 Subject: [PATCH 07/14] (fix): improve foundry test coverage for key contracts in PR --- .../contracts/bridge/L1NativeTokenVault.sol | 4 +- .../contracts/bridge/L1SharedBridge.sol | 9 +- .../L1SharedBridge/L1SharedBridgeBase.t.sol | 142 +++++++ .../L1SharedBridge/L1SharedBridgeFails.t.sol | 353 +++++++++++++++++- .../L1SharedBridge/L1SharedBridgeLegacy.t.sol | 9 +- .../_L1SharedBridge_Shared.t.sol | 17 +- 6 files changed, 504 insertions(+), 30 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol index 90a756cc3..6ac7e3df9 100644 --- a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol @@ -195,7 +195,7 @@ contract L1NativeTokenVault is assembly { callSuccess := call(gas(), _l1Receiver, amount, 0, 0, 0, 0) } - require(callSuccess, "NTV: withdraw failed"); + require(callSuccess, "NTV: withdrawal failed, no funds or cannot transfer to receiver"); } else { // Withdraw funds IERC20(l1Token).safeTransfer(_l1Receiver, amount); @@ -221,7 +221,7 @@ contract L1NativeTokenVault is assembly { callSuccess := call(gas(), _depositSender, _amount, 0, 0, 0, 0) } - require(callSuccess, "NTV: claimFailedDeposit failed"); + require(callSuccess, "NTV: claimFailedDeposit failed, no funds or cannot transfer to receiver"); } else { IERC20(l1Token).safeTransfer(_depositSender, _amount); // Note we don't allow weth deposits anymore, but there might be legacy weth deposits. diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index c31cbd838..6176c582a 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -116,7 +116,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade modifier onlyBridgehubOrEra(uint256 _chainId) { require( msg.sender == address(BRIDGE_HUB) || (_chainId == ERA_CHAIN_ID && msg.sender == ERA_DIAMOND_PROXY), - "L1SharedBridge: msg.value not equal to amount bridgehub or era chain" + "L1SharedBridge: msg.sender not equal to bridgehub or era chain" ); _; } @@ -172,8 +172,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @dev Sets the L1ERC20Bridge contract address. Should be called only once. function setNativeTokenVault(IL1NativeTokenVault _nativeTokenVault) external onlyOwner { - require(address(nativeTokenVault) == address(0), "ShB: legacy bridge already set"); - require(address(_nativeTokenVault) != address(0), "ShB: legacy bridge 0"); + require(address(nativeTokenVault) == address(0), "ShB: native token vault already set"); + require(address(_nativeTokenVault) != address(0), "ShB: native token vault 0"); nativeTokenVault = _nativeTokenVault; } @@ -260,7 +260,8 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade : _assetId; l1AssetHandler = assetHandlerAddress[_assetId]; // Check if no asset handler is set - if (l1AssetHandler == address(0) && uint256(_assetId) <= type(uint160).max) { + if (l1AssetHandler == address(0)) { + require(uint256(_assetId) <= type(uint160).max, "ShB: only address can be registered"); l1AssetHandler = address(nativeTokenVault); nativeTokenVault.registerToken(address(uint160(uint256(_assetId)))); } diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol index edd63bf45..436f50b67 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol @@ -8,11 +8,49 @@ import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; import {IL1SharedBridge} from "contracts/bridge/interfaces/IL1SharedBridge.sol"; +import {IL1NativeTokenVault} from "contracts/bridge/interfaces/IL1NativeTokenVault.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; import {IGetters} from "contracts/state-transition/chain-interfaces/IGetters.sol"; import {L1NativeTokenVault} from "contracts/bridge/L1NativeTokenVault.sol"; +import {StdStorage, stdStorage} from "forge-std/Test.sol"; contract L1SharedBridgeTestBase is L1SharedBridgeTest { + using stdStorage for StdStorage; + + function test_bridgehubPause() public { + vm.prank(owner); + sharedBridge.pause(); + assertEq(sharedBridge.paused(), true, "Shared Bridge Not Paused"); + } + + function test_bridgehubUnpause() public { + vm.prank(owner); + sharedBridge.pause(); + assertEq(sharedBridge.paused(), true, "Shared Bridge Not Paused"); + vm.prank(owner); + sharedBridge.unpause(); + assertEq(sharedBridge.paused(), false, "Shared Bridge Remains Paused"); + } + + function test_setAssetHandlerAddressOnCounterPart() public payable { + uint256 l2TxGasLimit = 100000; + uint256 l2TxGasPerPubdataByte = 100; + uint256 mintValue = 1; + address refundRecipient = address(0); + + vm.deal(owner, amount); + vm.prank(owner); + sharedBridge.setAssetHandlerAddressOnCounterPart{value: 1}( + eraChainId, + mintValue, + l2TxGasLimit, + l2TxGasPerPubdataByte, + refundRecipient, + tokenAssetId, + address(token) + ); + } + function test_bridgehubDepositBaseToken_Eth() public { vm.prank(bridgehubAddress); // solhint-disable-next-line func-named-parameters @@ -21,6 +59,21 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { sharedBridge.bridgehubDepositBaseToken{value: amount}(chainId, ETH_TOKEN_ASSET_ID, alice, amount); } + function test_bridgehubDepositBaseToken_Eth_Token_NotRegistered() public { + stdstore + .target(address(sharedBridge)) + .sig("assetHandlerAddress(bytes32)") + .with_key(ETH_TOKEN_ASSET_ID) + .checked_write(address(0)); + vm.prank(bridgehubAddress); + sharedBridge.bridgehubDepositBaseToken{value: amount}( + chainId, + bytes32(uint256(uint160(ETH_TOKEN_ADDRESS))), + alice, + amount + ); + } + function test_bridgehubDepositBaseToken_Erc() public { vm.prank(bridgehubAddress); // solhint-disable-next-line func-named-parameters @@ -29,6 +82,16 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { sharedBridge.bridgehubDepositBaseToken(chainId, tokenAssetId, alice, amount); } + function test_bridgehubDepositBaseToken_Erc_NoApproval() public { + vm.prank(alice); + token.approve(address(nativeTokenVault), 0); + vm.prank(bridgehubAddress); + // solhint-disable-next-line func-named-parameters + vm.expectEmit(true, true, true, true, address(sharedBridge)); + emit BridgehubDepositBaseTokenInitiated(chainId, alice, tokenAssetId, amount); + sharedBridge.bridgehubDepositBaseToken(chainId, tokenAssetId, alice, amount); + } + function test_bridgehubDeposit_Eth() public { _setBaseTokenAssetId(tokenAssetId); @@ -53,6 +116,31 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { sharedBridge.bridgehubDeposit{value: amount}(chainId, alice, 0, abi.encode(ETH_TOKEN_ADDRESS, amount, bob)); } + function test_bridgehubDeposit_Eth_NewEncoding() public { + _setBaseTokenAssetId(tokenAssetId); + + bytes memory transferData = abi.encode(amount, bob); + bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ASSET_ID, transferData)); + bytes memory mintCalldata = abi.encode( + amount, + alice, + bob, + nativeTokenVault.getERC20Getters(address(ETH_TOKEN_ADDRESS)), + address(ETH_TOKEN_ADDRESS) + ); + // solhint-disable-next-line func-named-parameters + vm.expectEmit(true, true, true, true, address(sharedBridge)); + vm.prank(bridgehubAddress); + emit BridgehubDepositInitiated({ + chainId: chainId, + txDataHash: txDataHash, + from: alice, + assetId: ETH_TOKEN_ASSET_ID, + bridgeMintCalldata: mintCalldata + }); + sharedBridge.bridgehubDeposit{value: amount}(chainId, alice, 0, abi.encode(ETH_TOKEN_ASSET_ID, transferData)); + } + function test_bridgehubDeposit_Erc() public { vm.prank(bridgehubAddress); // solhint-disable-next-line func-named-parameters @@ -68,6 +156,17 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { sharedBridge.bridgehubDeposit(chainId, alice, 0, abi.encode(address(token), amount, bob)); } + function test_bridgehubDeposit_Erc_CustomAssetHandler() public { + // ToDo: remove the mock call and register custom asset handler? + vm.mockCall( + address(nativeTokenVault), + abi.encodeWithSelector(IL1NativeTokenVault.tokenAddress.selector, tokenAssetId), + abi.encode(address(0)) + ); + vm.prank(bridgehubAddress); + sharedBridge.bridgehubDeposit(chainId, alice, 0, abi.encode(address(token), amount, bob)); + } + function test_bridgehubConfirmL2Transaction() public { // solhint-disable-next-line func-named-parameters vm.expectEmit(true, true, true, false, address(sharedBridge)); @@ -161,6 +260,49 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { }); } + function test_bridgeRecoverFailedTransfer_Eth() public { + bytes memory transferData = abi.encode(amount, alice); + bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); + _setSharedBridgeDepositHappened(chainId, txHash, txDataHash); + require(sharedBridge.depositHappened(chainId, txHash) == txDataHash, "Deposit not set"); + + vm.mockCall( + bridgehubAddress, + // solhint-disable-next-line func-named-parameters + abi.encodeWithSelector( + IBridgehub.proveL1ToL2TransactionStatus.selector, + chainId, + txHash, + l2BatchNumber, + l2MessageIndex, + l2TxNumberInBatch, + merkleProof, + TxStatus.Failure + ), + abi.encode(true) + ); + + // solhint-disable-next-line func-named-parameters + vm.expectEmit(true, true, true, false, address(sharedBridge)); + emit ClaimedFailedDepositSharedBridge({ + chainId: chainId, + to: alice, + assetId: ETH_TOKEN_ASSET_ID, + assetDataHash: bytes32(0) + }); + sharedBridge.bridgeRecoverFailedTransfer({ + _chainId: chainId, + _depositSender: alice, + _assetId: ETH_TOKEN_ASSET_ID, + _assetData: transferData, + _l2TxHash: txHash, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _merkleProof: merkleProof + }); + } + function test_finalizeWithdrawal_EthOnEth() public { bytes memory message = abi.encodePacked(IMailbox.finalizeEthWithdrawal.selector, alice, amount); L2Message memory l2ToL1Message = L2Message({ diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol index 37b7e3302..89397bc9e 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol @@ -9,17 +9,22 @@ import {TransparentUpgradeableProxy} from "@openzeppelin/contracts/proxy/transpa import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; import {L1SharedBridge} from "contracts/bridge/L1SharedBridge.sol"; +import {L1NativeTokenVault} from "contracts/bridge/L1NativeTokenVault.sol"; import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; +import {IL1NativeTokenVault} from "contracts/bridge/interfaces/IL1NativeTokenVault.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; import {IGetters} from "contracts/state-transition/chain-interfaces/IGetters.sol"; +import {StdStorage, stdStorage} from "forge-std/Test.sol"; /// We are testing all the specified revert and require cases. contract L1SharedBridgeFailTest is L1SharedBridgeTest { - function test_initialize_wrongOwner() public { + using stdStorage for StdStorage; + + function test_initialize_WrongOwner() public { vm.expectRevert("ShB owner 0"); new TransparentUpgradeableProxy( address(sharedBridgeImpl), @@ -36,19 +41,113 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { ); } - function test_bridgehubDepositBaseToken_EthwrongMsgValue() public { + function test_initialize_wrongOwnerNTV() public { + vm.expectRevert("NTV owner 0"); + new TransparentUpgradeableProxy( + address(nativeTokenVaultImpl), + admin, + // solhint-disable-next-line func-named-parameters + abi.encodeWithSelector(L1NativeTokenVault.initialize.selector, address(0)) + ); + } + + function test_registerToken_noCode() public { + vm.expectRevert("NTV: empty token"); + nativeTokenVault.registerToken(address(0)); + } + + function test_setL1Erc20Bridge_alreadySet() public { + vm.prank(owner); + vm.expectRevert("ShB: legacy bridge already set"); + sharedBridge.setL1Erc20Bridge(address(0)); + } + + function test_setL1Erc20Bridge_emptyAddressProvided() public { + stdstore.target(address(sharedBridge)).sig(sharedBridge.legacyBridge.selector).checked_write(address(0)); + vm.prank(owner); + vm.expectRevert("ShB: legacy bridge 0"); + sharedBridge.setL1Erc20Bridge(address(0)); + } + + function test_setNativeTokenVault_alreadySet() public { + vm.prank(owner); + vm.expectRevert("ShB: native token vault already set"); + sharedBridge.setNativeTokenVault(IL1NativeTokenVault(address(0))); + } + + function test_setNativeTokenVault_emptyAddressProvided() public { + stdstore.target(address(sharedBridge)).sig(sharedBridge.nativeTokenVault.selector).checked_write(address(0)); + vm.prank(owner); + vm.expectRevert("ShB: native token vault 0"); + sharedBridge.setNativeTokenVault(IL1NativeTokenVault(address(0))); + } + + function test_setAssetHandlerAddressOnCounterPart_notOwnerOrADT() public { + uint256 l2TxGasLimit = 100000; + uint256 l2TxGasPerPubdataByte = 100; + address refundRecipient = address(0); + + vm.prank(alice); + vm.expectRevert("ShB: only ADT or owner"); + sharedBridge.setAssetHandlerAddressOnCounterPart( + eraChainId, + mintValue, + l2TxGasLimit, + l2TxGasPerPubdataByte, + refundRecipient, + tokenAssetId, + address(token) + ); + } + + function test_setAssetHandlerAddressOnCounterPart_chainNotAddedToSharedBridge() public { + uint256 l2TxGasLimit = 100000; + uint256 l2TxGasPerPubdataByte = 100; + address refundRecipient = address(0); + + vm.prank(owner); + vm.expectRevert("ShB: chain governance not initialized"); + sharedBridge.setAssetHandlerAddressOnCounterPart( + randomChainId, + mintValue, + l2TxGasLimit, + l2TxGasPerPubdataByte, + refundRecipient, + tokenAssetId, + address(token) + ); + } + + function test_bridgehubDepositBaseToken_Eth_Token_notRegisteredTokenID() public { + // ToDo: Shall we do it properly instead of mocking? + stdstore + .target(address(sharedBridge)) + .sig("assetHandlerAddress(bytes32)") + .with_key(ETH_TOKEN_ASSET_ID) + .checked_write(address(0)); + vm.prank(bridgehubAddress); + vm.expectRevert("ShB: only address can be registered"); + sharedBridge.bridgehubDepositBaseToken{value: amount}(chainId, ETH_TOKEN_ASSET_ID, alice, amount); + } + + function test_bridgehubDepositBaseToken_Eth_Token_incorrectSender() public { + vm.expectRevert("L1SharedBridge: msg.sender not equal to bridgehub or era chain"); + sharedBridge.bridgehubDepositBaseToken{value: amount}(chainId, ETH_TOKEN_ASSET_ID, alice, amount); + } + + function test_bridgehubDepositBaseToken_ethwrongMsgValue() public { vm.prank(bridgehubAddress); vm.expectRevert("L1NTV: msg.value not equal to amount"); sharedBridge.bridgehubDepositBaseToken(chainId, ETH_TOKEN_ASSET_ID, alice, amount); } - function test_bridgehubDepositBaseToken_ErcWrongMsgValue() public { + function test_bridgehubDepositBaseToken_ercWrongMsgValue() public { vm.prank(bridgehubAddress); vm.expectRevert("NTV m.v > 0 b d.it"); sharedBridge.bridgehubDepositBaseToken{value: amount}(chainId, tokenAssetId, alice, amount); } - function test_bridgehubDepositBaseToken_ErcWrongErcDepositAmount() public { + function test_bridgehubDepositBaseToken_ercWrongErcDepositAmount() public { vm.mockCall(address(token), abi.encodeWithSelector(IERC20.balanceOf.selector), abi.encode(10)); bytes memory message = bytes("5T"); @@ -126,6 +225,154 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { sharedBridge.bridgehubConfirmL2Transaction(chainId, txDataHash, txHash); } + function test_finalizeWithdrawal_EthOnEth_withdrawalFailed() public { + vm.deal(address(nativeTokenVault), 0); + bytes memory message = abi.encodePacked(IMailbox.finalizeEthWithdrawal.selector, alice, amount); + L2Message memory l2ToL1Message = L2Message({ + txNumberInBatch: l2TxNumberInBatch, + sender: L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR, + data: message + }); + + vm.mockCall( + bridgehubAddress, + // solhint-disable-next-line func-named-parameters + abi.encodeWithSelector( + IBridgehub.proveL2MessageInclusion.selector, + chainId, + l2BatchNumber, + l2MessageIndex, + l2ToL1Message, + merkleProof + ), + abi.encode(true) + ); + + vm.expectRevert("NTV: withdrawal failed, no funds or cannot transfer to receiver"); + sharedBridge.finalizeWithdrawal({ + _chainId: chainId, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _message: message, + _merkleProof: merkleProof + }); + } + + function test_bridgeRecoverFailedTransfer_Eth_claimFailedDepositFailed() public { + vm.deal(address(nativeTokenVault), 0); + bytes memory transferData = abi.encode(amount, alice); + bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); + _setSharedBridgeDepositHappened(chainId, txHash, txDataHash); + require(sharedBridge.depositHappened(chainId, txHash) == txDataHash, "Deposit not set"); + + vm.mockCall( + bridgehubAddress, + // solhint-disable-next-line func-named-parameters + abi.encodeWithSelector( + IBridgehub.proveL1ToL2TransactionStatus.selector, + chainId, + txHash, + l2BatchNumber, + l2MessageIndex, + l2TxNumberInBatch, + merkleProof, + TxStatus.Failure + ), + abi.encode(true) + ); + + vm.expectRevert("NTV: claimFailedDeposit failed, no funds or cannot transfer to receiver"); + sharedBridge.bridgeRecoverFailedTransfer({ + _chainId: chainId, + _depositSender: alice, + _assetId: ETH_TOKEN_ASSET_ID, + _assetData: transferData, + _l2TxHash: txHash, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _merkleProof: merkleProof + }); + } + + function test_bridgeRecoverFailedTransfer_invalidChainID() public { + vm.store(address(sharedBridge), bytes32(isWithdrawalFinalizedStorageLocation - 5), bytes32(uint256(0))); + + bytes memory transferData = abi.encode(amount, alice); + bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); + _setSharedBridgeDepositHappened(chainId, txHash, txDataHash); + require(sharedBridge.depositHappened(chainId, txHash) == txDataHash, "Deposit not set"); + + vm.mockCall( + bridgehubAddress, + // solhint-disable-next-line func-named-parameters + abi.encodeWithSelector( + IBridgehub.proveL1ToL2TransactionStatus.selector, + eraChainId, + txHash, + l2BatchNumber, + l2MessageIndex, + l2TxNumberInBatch, + merkleProof, + TxStatus.Failure + ), + abi.encode(true) + ); + + vm.expectRevert("ShB: last deposit time not set for Era"); + sharedBridge.bridgeRecoverFailedTransfer({ + _chainId: eraChainId, + _depositSender: alice, + _assetId: ETH_TOKEN_ASSET_ID, + _assetData: transferData, + _l2TxHash: txHash, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _merkleProof: merkleProof + }); + } + + function test_bridgeRecoverFailedTransfer_eraLegacyDeposit() public { + vm.store(address(sharedBridge), bytes32(isWithdrawalFinalizedStorageLocation - 5), bytes32(uint256(2))); + + uint256 l2BatchNumber = 1; + bytes memory transferData = abi.encode(amount, alice); + bytes32 txDataHash = keccak256(abi.encode(alice, ETH_TOKEN_ADDRESS, amount)); + _setSharedBridgeDepositHappened(chainId, txHash, txDataHash); + require(sharedBridge.depositHappened(chainId, txHash) == txDataHash, "Deposit not set"); + + vm.mockCall( + bridgehubAddress, + // solhint-disable-next-line func-named-parameters + abi.encodeWithSelector( + IBridgehub.proveL1ToL2TransactionStatus.selector, + eraChainId, + txHash, + l2BatchNumber, + l2MessageIndex, + l2TxNumberInBatch, + merkleProof, + TxStatus.Failure + ), + abi.encode(true) + ); + + vm.expectRevert("ShB: legacy cFD"); + sharedBridge.bridgeRecoverFailedTransfer({ + _chainId: eraChainId, + _depositSender: alice, + _assetId: ETH_TOKEN_ASSET_ID, + _assetData: transferData, + _l2TxHash: txHash, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _merkleProof: merkleProof + }); + } + function test_claimFailedDeposit_proofInvalid() public { vm.mockCall( bridgehubAddress, @@ -252,7 +499,7 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { }); } - function test_finalizeWithdrawal_EthOnEth_LegacyTxFinalizedInERC20Bridge() public { + function test_finalizeWithdrawal_EthOnEth_legacyTxFinalizedInERC20Bridge() public { vm.deal(address(sharedBridge), amount); uint256 legacyBatchNumber = 0; @@ -280,7 +527,7 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { }); } - function test_finalizeWithdrawal_EthOnEth_LegacyTxFinalizedInSharedBridge() public { + function test_finalizeWithdrawal_EthOnEth_legacyTxFinalizedInSharedBridge() public { vm.deal(address(sharedBridge), amount); uint256 legacyBatchNumber = 0; @@ -324,21 +571,53 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { }); } - function test_finalizeWithdrawal_EthOnEth_LegacyTxFinalizedInDiamondProxy() public { + function test_finalizeWithdrawal_EthOnEth_legacyTxFinalizedInDiamondProxy() public { vm.deal(address(sharedBridge), amount); uint256 legacyBatchNumber = 0; - vm.mockCall( - l1ERC20BridgeAddress, - abi.encodeWithSelector(IL1ERC20Bridge.isWithdrawalFinalized.selector), - abi.encode(false) + bytes memory message = abi.encodePacked( + IL1ERC20Bridge.finalizeWithdrawal.selector, + alice, + address(token), + amount ); + vm.expectRevert("ShB: legacy eth withdrawal"); - vm.mockCall( - eraDiamondProxy, - abi.encodeWithSelector(IGetters.isEthWithdrawalFinalized.selector), - abi.encode(true) + sharedBridge.finalizeWithdrawal({ + _chainId: eraChainId, + _l2BatchNumber: legacyBatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _message: message, + _merkleProof: merkleProof + }); + } + + function test_finalizeWithdrawal_EthOnEth_diamondUpgradeFirstBatchNotSet() public { + vm.store(address(sharedBridge), bytes32(isWithdrawalFinalizedStorageLocation - 7), bytes32(uint256(0))); + vm.deal(address(sharedBridge), amount); + + bytes memory message = abi.encodePacked( + IL1ERC20Bridge.finalizeWithdrawal.selector, + alice, + address(token), + amount ); + vm.expectRevert("ShB: diamondUFB not set for Era"); + + sharedBridge.finalizeWithdrawal({ + _chainId: eraChainId, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _message: message, + _merkleProof: merkleProof + }); + } + + function test_finalizeWithdrawal_TokenOnEth_legacyTokenWithdrawal() public { + vm.store(address(sharedBridge), bytes32(isWithdrawalFinalizedStorageLocation - 6), bytes32(uint256(5))); + vm.deal(address(sharedBridge), amount); bytes memory message = abi.encodePacked( IL1ERC20Bridge.finalizeWithdrawal.selector, @@ -346,11 +625,33 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { address(token), amount ); - vm.expectRevert("ShB: legacy eth withdrawal"); + vm.expectRevert("ShB: legacy token withdrawal"); sharedBridge.finalizeWithdrawal({ _chainId: eraChainId, - _l2BatchNumber: legacyBatchNumber, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _message: message, + _merkleProof: merkleProof + }); + } + + function test_finalizeWithdrawal_TokenOnEth_legacyUpgradeFirstBatchNotSet() public { + vm.store(address(sharedBridge), bytes32(isWithdrawalFinalizedStorageLocation - 6), bytes32(uint256(0))); + vm.deal(address(sharedBridge), amount); + + bytes memory message = abi.encodePacked( + IL1ERC20Bridge.finalizeWithdrawal.selector, + alice, + address(token), + amount + ); + vm.expectRevert("ShB: LegacyUFB not set for Era"); + + sharedBridge.finalizeWithdrawal({ + _chainId: eraChainId, + _l2BatchNumber: l2BatchNumber, _l2MessageIndex: l2MessageIndex, _l2TxNumberInBatch: l2TxNumberInBatch, _message: message, @@ -427,7 +728,7 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { }); } - function test_parseL2WithdrawalMessage_WrongMsgLength() public { + function test_parseL2WithdrawalMessage_wrongMsgLength() public { bytes memory message = abi.encodePacked(IMailbox.finalizeEthWithdrawal.selector); vm.expectRevert("ShB wrong msg len"); @@ -441,7 +742,21 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { }); } - function test_parseL2WithdrawalMessage_WrongSelector() public { + function test_parseL2WithdrawalMessage_wrongMsgLength2() public { + bytes memory message = abi.encodePacked(IL1ERC20Bridge.finalizeWithdrawal.selector, abi.encode(amount, token)); + + vm.expectRevert("ShB wrong msg len 2"); + sharedBridge.finalizeWithdrawal({ + _chainId: chainId, + _l2BatchNumber: l2BatchNumber, + _l2MessageIndex: l2MessageIndex, + _l2TxNumberInBatch: l2TxNumberInBatch, + _message: message, + _merkleProof: merkleProof + }); + } + + function test_parseL2WithdrawalMessage_wrongSelector() public { // notice that the selector is wrong bytes memory message = abi.encodePacked(IMailbox.proveL2LogInclusion.selector, alice, amount); diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol index 7869acc18..d82e4c6e3 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeLegacy.t.sol @@ -9,7 +9,7 @@ import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; import {IBridgehub} from "contracts/bridgehub/IBridgehub.sol"; import {L2Message, TxStatus} from "contracts/common/Messaging.sol"; import {IMailbox} from "contracts/state-transition/chain-interfaces/IMailbox.sol"; -import {IL1SharedBridge} from "contracts/bridge/interfaces/IL1SharedBridge.sol"; +import {IL1ERC20Bridge} from "contracts/bridge/interfaces/IL1ERC20Bridge.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "contracts/common/L2ContractAddresses.sol"; contract L1SharedBridgeLegacyTest is L1SharedBridgeTest { @@ -99,9 +99,10 @@ contract L1SharedBridgeLegacyTest is L1SharedBridgeTest { // solhint-disable-next-line func-named-parameters bytes memory message = abi.encodePacked( - IL1SharedBridge.finalizeWithdrawal.selector, - tokenAssetId, - abi.encode(amount, alice) + IL1ERC20Bridge.finalizeWithdrawal.selector, + alice, + address(token), + amount ); L2Message memory l2ToL1Message = L2Message({ txNumberInBatch: l2TxNumberInBatch, diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol index 2031f3bf1..226e7b386 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol @@ -64,6 +64,7 @@ contract L1SharedBridgeTest is Test { L1SharedBridge sharedBridgeImpl; L1SharedBridge sharedBridge; + L1NativeTokenVault nativeTokenVaultImpl; L1NativeTokenVault nativeTokenVault; address bridgehubAddress; address l1ERC20BridgeAddress; @@ -80,9 +81,11 @@ contract L1SharedBridgeTest is Test { address bob; uint256 chainId; uint256 amount = 100; + uint256 mintValue = 1; bytes32 txHash; uint256 eraChainId; + uint256 randomChainId; address eraDiamondProxy; address eraErc20BridgeAddress; @@ -118,6 +121,7 @@ contract L1SharedBridgeTest is Test { chainId = 1; eraChainId = 9; + randomChainId = 999; eraDiamondProxy = makeAddr("eraDiamondProxy"); eraErc20BridgeAddress = makeAddr("eraErc20BridgeAddress"); @@ -134,11 +138,17 @@ contract L1SharedBridgeTest is Test { abi.encodeWithSelector(L1SharedBridge.initialize.selector, owner, 1, 1, 1, 0) ); sharedBridge = L1SharedBridge(payable(sharedBridgeProxy)); - nativeTokenVault = new L1NativeTokenVault({ + nativeTokenVaultImpl = new L1NativeTokenVault({ _l1WethAddress: l1WethAddress, _l1SharedBridge: IL1SharedBridge(address(sharedBridge)), _eraChainId: eraChainId }); + TransparentUpgradeableProxy nativeTokenVaultProxy = new TransparentUpgradeableProxy( + address(nativeTokenVaultImpl), + admin, + abi.encodeWithSelector(L1NativeTokenVault.initialize.selector, owner) + ); + nativeTokenVault = L1NativeTokenVault(payable(nativeTokenVaultProxy)); vm.prank(owner); sharedBridge.setL1Erc20Bridge(l1ERC20BridgeAddress); tokenAssetId = nativeTokenVault.getAssetId(address(token)); @@ -175,6 +185,11 @@ contract L1SharedBridgeTest is Test { abi.encodeWithSelector(IBridgehub.baseTokenAssetId.selector, chainId), abi.encode(ETH_TOKEN_ASSET_ID) ); + vm.mockCall( + bridgehubAddress, + abi.encodeWithSelector(IBridgehub.requestL2TransactionDirect.selector), + abi.encode(txHash) + ); // vm.mockCall( // address(bridgehubAddress), // abi.encodeWithSelector(IBridgehub.baseTokenAssetId.selector, address(token)), From dc7bd136fca7f7bd5e0e9557b8959d0f325fb890 Mon Sep 17 00:00:00 2001 From: Vlad Bochok <41153528+vladbochok@users.noreply.github.com> Date: Fri, 21 Jun 2024 16:55:51 +0200 Subject: [PATCH 08/14] Add CI coverage (reopenned to dev) (#543) --- .github/workflows/l1-contracts-ci.yaml | 70 ++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/.github/workflows/l1-contracts-ci.yaml b/.github/workflows/l1-contracts-ci.yaml index 2123b47cb..d761b63ed 100644 --- a/.github/workflows/l1-contracts-ci.yaml +++ b/.github/workflows/l1-contracts-ci.yaml @@ -145,3 +145,73 @@ jobs: - name: Compare run: diff tools/data/Verifier.sol l1-contracts/contracts/state-transition/Verifier.sol + + coverage: + defaults: + run: + working-directory: l1-contracts + needs: [build, lint] + runs-on: ubuntu-latest + + steps: + - name: Checkout the repository + uses: actions/checkout@v4 + with: + submodules: recursive + + - name: Use Foundry + uses: foundry-rs/foundry-toolchain@v1 + + - name: Use Node.js + uses: actions/setup-node@v3 + with: + node-version: 18.18.0 + cache: yarn + + - name: Install dependencies + run: yarn + + - name: Restore artifacts cache + uses: actions/cache/restore@v3 + with: + fail-on-cache-miss: true + key: artifacts-l1-${{ github.sha }} + path: | + l1-contracts/artifacts + l1-contracts/cache + l1-contracts/typechain + + - name: Run coverage + run: FOUNDRY_PROFILE=default yarn test:foundry && FOUNDRY_PROFILE=default yarn coverage:foundry --report summary --report lcov + + # To ignore coverage for certain directories modify the paths in this step as needed. The + # below default ignores coverage results for the test and script directories. Alternatively, + # to include coverage in all directories, comment out this step. Note that because this + # filtering applies to the lcov file, the summary table generated in the previous step will + # still include all files and directories. + # The `--rc lcov_branch_coverage=1` part keeps branch info in the filtered report, since lcov + # defaults to removing branch info. + - name: Filter directories + run: | + sudo apt update && sudo apt install -y lcov + lcov --remove lcov.info 'test/*' 'contracts/dev-contracts/*' 'lib/*' --output-file lcov.info --rc lcov_branch_coverage=1 + + # This step posts a detailed coverage report as a comment and deletes previous comments on + # each push. The below step is used to fail coverage if the specified coverage threshold is + # not met. The below step can post a comment (when it's `github-token` is specified) but it's + # not as useful, and this action cannot fail CI based on a minimum coverage threshold, which + # is why we use both in this way. + - name: Post coverage report + if: github.event_name == 'pull_request' # This action fails when ran outside of a pull request. + uses: romeovs/lcov-reporter-action@v0.3.1 + with: + delete-old-comments: true + lcov-file: ./l1-contracts/lcov.info + github-token: ${{ secrets.GITHUB_TOKEN }} # Adds a coverage summary comment to the PR. + + - name: Verify minimum coverage + uses: zgosalvez/github-actions-report-lcov@v2 + with: + coverage-files: ./l1-contracts/lcov.info + working-directory: l1-contracts + minimum-coverage: 85 # Set coverage threshold. From 9ca06a1873f6cfb51972d7f4ab75875216e7cc55 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Mon, 24 Jun 2024 19:47:04 +0400 Subject: [PATCH 09/14] (feat): migrate tokens to NTV Option 1 --- .../contracts/bridge/L1NativeTokenVault.sol | 48 +++++++++++++++++++ .../contracts/bridge/L1SharedBridge.sol | 26 ++++++++++ .../bridge/interfaces/IL1SharedBridge.sol | 4 ++ .../L1SharedBridge/L1SharedBridgeBase.t.sol | 21 ++++++++ .../_L1SharedBridge_Shared.t.sol | 11 +++++ 5 files changed, 110 insertions(+) diff --git a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol index 6ac7e3df9..628267469 100644 --- a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol @@ -64,6 +64,12 @@ contract L1NativeTokenVault is _; } + /// @notice Checks that the message sender is the shared bridge itself. + modifier onlySelf() { + require(msg.sender == address(this), "NTV only"); + _; + } + /// @dev Contract is expected to be used as proxy implementation. /// @dev Initialize the implementation to prevent Parity hack. constructor( @@ -85,6 +91,48 @@ contract L1NativeTokenVault is _transferOwnership(_owner); } + /// @dev Accepts ether only from the Shared Bridge. + receive() external payable { + require(address(L1_SHARED_BRIDGE) == msg.sender, "NTV: ETH only accepted from Shared Bridge"); + } + + /// @dev Transfer tokens from legacy erc20 bridge or mailbox and set chainBalance as part of migration process. + /// @param _token The address of token to be transferred (address(1) for ether and contract address for ERC20). + /// @param _targetChainId The chain ID of the corresponding hyperchain. + function transferFundsFromSharedBridge(address _token, uint256 _targetChainId) external onlySelf { + if (_token == ETH_TOKEN_ADDRESS) { + uint256 balanceBefore = address(this).balance; + L1_SHARED_BRIDGE.transferEthToNTV(_targetChainId); + uint256 balanceAfter = address(this).balance; + require(balanceAfter > balanceBefore, "NTV: 0 eth transferred"); + chainBalance[_targetChainId][ETH_TOKEN_ADDRESS] = + chainBalance[_targetChainId][ETH_TOKEN_ADDRESS] + + balanceAfter - + balanceBefore; + } else { + uint256 balanceBefore = IERC20(_token).balanceOf(address(this)); + uint256 sharedBridgeChainBalance = chainBalance[_targetChainId][_token]; + require(sharedBridgeChainBalance > 0, "NTV: 0 amount to transfer"); + L1_SHARED_BRIDGE.transferTokenToNTV(_targetChainId, _token); + uint256 balanceAfter = IERC20(_token).balanceOf(address(this)); + require(balanceAfter - balanceBefore >= sharedBridgeChainBalance, "NTV: wrong amount transferred"); + chainBalance[_targetChainId][_token] = chainBalance[_targetChainId][_token] + sharedBridgeChainBalance; + } + } + + /// @dev transfer tokens from legacy erc20 bridge or mailbox and set chainBalance as part of migration process. + /// @dev Unlike `transferFundsFromLegacy` is provides a concrete limit on the gas used for the transfer and even if it will fail, it will not revert the whole transaction. + function safeTransferFundsFromSharedBridge( + address _token, + uint256 _targetChainId, + uint256 _gasPerToken + ) external { + try this.transferFundsFromSharedBridge{gas: _gasPerToken}(_token, _targetChainId) {} catch { + // A reasonable amount of gas will be provided to transfer the token. + // If the transfer fails, we don't want to revert the whole transaction. + } + } + /// @dev We want to be able to bridge native tokens automatically, this means registering them on the fly /// @notice Allows the bridge to register a token address for the vault. /// @notice No access control is ok, since the bridging of tokens should be permissionless. This requires permissionless registration. diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 6176c582a..5941c1849 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -87,6 +87,11 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade // slither-disable-next-line uninitialized-state mapping(uint256 chainId => bool enabled) public hyperbridgingEnabled; + /// @dev Maps token balances for each chain to prevent unauthorized spending across hyperchains. + /// This serves as a security measure until hyperbridging is implemented. + /// NOTE: this function may be removed in the future, don't rely on it! + mapping(uint256 chainId => mapping(address l1Token => uint256 balance)) public chainBalance; + /// @dev A mapping assetId => assetHandlerAddress /// @dev Tracks the address of Asset Handler contracts, where bridged funds are locked for each asset /// @dev P.S. this liquidity was locked directly in SharedBridge before @@ -163,6 +168,27 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } } + /// @dev transfer token to shared bridge as part of upgrade + function transferEthToNTV(uint256 _chainId) external { + require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); + uint256 amount = chainBalance[_chainId][ETH_TOKEN_ADDRESS]; + bool callSuccess; + address ntvAddress = address(nativeTokenVault); + // Low-level assembly call, to avoid any memory copying (save gas) + assembly { + callSuccess := call(gas(), ntvAddress, amount, 0, 0, 0, 0) + } + chainBalance[_chainId][ETH_TOKEN_ADDRESS] = chainBalance[_chainId][ETH_TOKEN_ADDRESS] - amount; + } + + /// @dev transfer token to shared bridge as part of upgrade + function transferTokenToNTV(uint256 _chainId, address _token) external { + require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); + uint256 amount = chainBalance[_chainId][_token]; + IERC20(_token).safeTransfer(address(nativeTokenVault), amount); + chainBalance[_chainId][_token] = chainBalance[_chainId][_token] - amount; + } + /// @dev Sets the L1ERC20Bridge contract address. Should be called only once. function setL1Erc20Bridge(address _legacyBridge) external onlyOwner { require(address(legacyBridge) == address(0), "ShB: legacy bridge already set"); diff --git a/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol b/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol index dc51517da..fc89caa0f 100644 --- a/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol @@ -173,4 +173,8 @@ interface IL1SharedBridge { uint16 _l2TxNumberInBatch, bytes32[] calldata _merkleProof ) external; + + function transferEthToNTV(uint256 _chainId) external; + + function transferTokenToNTV(uint256 _chainId, address _token) external; } diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol index 436f50b67..7564b4f38 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.24; +import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol"; + import {L1SharedBridgeTest} from "./_L1SharedBridge_Shared.t.sol"; import {ETH_TOKEN_ADDRESS} from "contracts/common/Config.sol"; @@ -509,4 +511,23 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { _merkleProof: merkleProof }); } + + function test_safeTransferFundsFromSharedBridge_Erc() public { + uint256 maxGas = 30_000_000; + // solhint-disable-next-line func-named-parameters + vm.expectEmit(true, true, false, true, address(token)); + emit IERC20.Transfer(address(sharedBridge), address(nativeTokenVault), amount); + nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(address(token), chainId, maxGas); + } + + function test_safeTransferFundsFromSharedBridge_Eth() public { + uint256 maxGas = 30_000_000; + uint256 startBalanceShb = sharedBridge.chainBalance(chainId, ETH_TOKEN_ADDRESS); + uint256 startBalanceNtv = nativeTokenVault.chainBalance(chainId, ETH_TOKEN_ADDRESS); + nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, chainId, maxGas); + uint256 endBalanceShb = sharedBridge.chainBalance(chainId, ETH_TOKEN_ADDRESS); + uint256 endBalanceNtv = nativeTokenVault.chainBalance(chainId, ETH_TOKEN_ADDRESS); + assertEq(endBalanceNtv - startBalanceNtv, amount); + assertEq(startBalanceShb - endBalanceShb, amount); + } } diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol index 226e7b386..45d8bec5d 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol @@ -202,6 +202,8 @@ contract L1SharedBridgeTest is Test { _setNativeTokenVaultChainBalance(chainId, address(token), 1000 * amount); _setNativeTokenVaultChainBalance(chainId, ETH_TOKEN_ADDRESS, amount); // console.log("chainBalance %s, %s", address(token), nativeTokenVault.chainBalance(chainId, address(token))); + _setSharedBridgeChainBalance(chainId, address(token), amount); + _setSharedBridgeChainBalance(chainId, ETH_TOKEN_ADDRESS, amount); vm.deal(bridgehubAddress, amount); vm.deal(address(sharedBridge), amount); @@ -247,6 +249,15 @@ contract L1SharedBridgeTest is Test { .checked_write(_value); } + function _setSharedBridgeChainBalance(uint256 _chainId, address _token, uint256 _value) internal { + stdstore + .target(address(sharedBridge)) + .sig(sharedBridge.chainBalance.selector) + .with_key(_chainId) + .with_key(_token) + .checked_write(_value); + } + function _setBaseTokenAssetId(bytes32 _assetId) internal { // vm.prank(bridgehubAddress); vm.mockCall( From 1ad7078c1b95884777a52aad725fcd7722fd8d04 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Mon, 24 Jun 2024 20:16:53 +0400 Subject: [PATCH 10/14] (feat): migrate tokens to NTV Option 2 --- .../contracts/bridge/L1NativeTokenVault.sol | 50 +++++++++++++------ .../contracts/bridge/L1SharedBridge.sol | 10 ++-- .../bridge/interfaces/IL1SharedBridge.sol | 6 ++- .../L1SharedBridge/L1SharedBridgeBase.t.sol | 15 ++++-- 4 files changed, 53 insertions(+), 28 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol index 628267469..572610ac3 100644 --- a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol @@ -49,6 +49,10 @@ contract L1NativeTokenVault is /// NOTE: this function may be removed in the future, don't rely on it! mapping(uint256 chainId => mapping(address l1Token => uint256 balance)) public chainBalance; + /// @dev Maps token balances for each chain to verify that chain balance was migrated from shared bridge. + /// This serves only for migration purposes. + mapping(uint256 chainId => mapping(address l1Token => bool migrated)) public chainBalanceMigrated; + /// @dev A mapping assetId => tokenAddress mapping(bytes32 assetId => address tokenAddress) public tokenAddress; @@ -96,38 +100,54 @@ contract L1NativeTokenVault is require(address(L1_SHARED_BRIDGE) == msg.sender, "NTV: ETH only accepted from Shared Bridge"); } - /// @dev Transfer tokens from legacy erc20 bridge or mailbox and set chainBalance as part of migration process. + /// @dev Transfer tokens from shared bridge as part of migration process. /// @param _token The address of token to be transferred (address(1) for ether and contract address for ERC20). - /// @param _targetChainId The chain ID of the corresponding hyperchain. - function transferFundsFromSharedBridge(address _token, uint256 _targetChainId) external onlySelf { + function transferFundsFromSharedBridge(address _token) external onlySelf { if (_token == ETH_TOKEN_ADDRESS) { uint256 balanceBefore = address(this).balance; - L1_SHARED_BRIDGE.transferEthToNTV(_targetChainId); + L1_SHARED_BRIDGE.transferEthToNTV(); uint256 balanceAfter = address(this).balance; require(balanceAfter > balanceBefore, "NTV: 0 eth transferred"); - chainBalance[_targetChainId][ETH_TOKEN_ADDRESS] = - chainBalance[_targetChainId][ETH_TOKEN_ADDRESS] + - balanceAfter - - balanceBefore; } else { uint256 balanceBefore = IERC20(_token).balanceOf(address(this)); - uint256 sharedBridgeChainBalance = chainBalance[_targetChainId][_token]; + uint256 sharedBridgeChainBalance = IERC20(_token).balanceOf(address(L1_SHARED_BRIDGE)); require(sharedBridgeChainBalance > 0, "NTV: 0 amount to transfer"); - L1_SHARED_BRIDGE.transferTokenToNTV(_targetChainId, _token); + L1_SHARED_BRIDGE.transferTokenToNTV(_token); uint256 balanceAfter = IERC20(_token).balanceOf(address(this)); require(balanceAfter - balanceBefore >= sharedBridgeChainBalance, "NTV: wrong amount transferred"); - chainBalance[_targetChainId][_token] = chainBalance[_targetChainId][_token] + sharedBridgeChainBalance; } } - /// @dev transfer tokens from legacy erc20 bridge or mailbox and set chainBalance as part of migration process. - /// @dev Unlike `transferFundsFromLegacy` is provides a concrete limit on the gas used for the transfer and even if it will fail, it will not revert the whole transaction. - function safeTransferFundsFromSharedBridge( + /// @dev Set chain token balance as part of migration process. + /// @param _token The address of token to be transferred (address(1) for ether and contract address for ERC20). + /// @param _targetChainId The chain ID of the corresponding hyperchain. + function transferBalancesFromSharedBridge(address _token, uint256 _targetChainId) external onlySelf { + require( + chainBalanceMigrated[_targetChainId][_token] == false, + "NTV: chain balance for the token already migrated" + ); + uint256 sharedBridgeChainBalance = L1_SHARED_BRIDGE.chainBalance(_targetChainId, _token); + chainBalance[_targetChainId][_token] = chainBalance[_targetChainId][_token] + sharedBridgeChainBalance; + chainBalanceMigrated[_targetChainId][_token] == true; + } + + /// @dev Transfer tokens from shared bridge as part of migration process. + /// @dev Unlike `transferFundsFromSharedBridge` is provides a concrete limit on the gas used for the transfer and even if it will fail, it will not revert the whole transaction. + function safeTransferFundsFromSharedBridge(address _token, uint256 _gasPerToken) external { + try this.transferFundsFromSharedBridge{gas: _gasPerToken}(_token) {} catch { + // A reasonable amount of gas will be provided to transfer the token. + // If the transfer fails, we don't want to revert the whole transaction. + } + } + + /// @dev Set chain token balance as part of migration process. + /// @dev Unlike `transferBalancesFromSharedBridge` is provides a concrete limit on the gas used for the transfer and even if it will fail, it will not revert the whole transaction. + function safeTransferBalancesFromSharedBridge( address _token, uint256 _targetChainId, uint256 _gasPerToken ) external { - try this.transferFundsFromSharedBridge{gas: _gasPerToken}(_token, _targetChainId) {} catch { + try this.transferBalancesFromSharedBridge{gas: _gasPerToken}(_token, _targetChainId) {} catch { // A reasonable amount of gas will be provided to transfer the token. // If the transfer fails, we don't want to revert the whole transaction. } diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 5941c1849..cd0ae8d77 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -169,24 +169,22 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } /// @dev transfer token to shared bridge as part of upgrade - function transferEthToNTV(uint256 _chainId) external { + function transferEthToNTV() external { require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); - uint256 amount = chainBalance[_chainId][ETH_TOKEN_ADDRESS]; + uint256 amount = address(this).balance; bool callSuccess; address ntvAddress = address(nativeTokenVault); // Low-level assembly call, to avoid any memory copying (save gas) assembly { callSuccess := call(gas(), ntvAddress, amount, 0, 0, 0, 0) } - chainBalance[_chainId][ETH_TOKEN_ADDRESS] = chainBalance[_chainId][ETH_TOKEN_ADDRESS] - amount; } /// @dev transfer token to shared bridge as part of upgrade - function transferTokenToNTV(uint256 _chainId, address _token) external { + function transferTokenToNTV(address _token) external { require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); - uint256 amount = chainBalance[_chainId][_token]; + uint256 amount = IERC20(_token).balanceOf(address(this)); IERC20(_token).safeTransfer(address(nativeTokenVault), amount); - chainBalance[_chainId][_token] = chainBalance[_chainId][_token] - amount; } /// @dev Sets the L1ERC20Bridge contract address. Should be called only once. diff --git a/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol b/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol index fc89caa0f..f49a0f0fd 100644 --- a/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/interfaces/IL1SharedBridge.sol @@ -174,7 +174,9 @@ interface IL1SharedBridge { bytes32[] calldata _merkleProof ) external; - function transferEthToNTV(uint256 _chainId) external; + function chainBalance(uint256 _chainId, address _token) external view returns (uint256); - function transferTokenToNTV(uint256 _chainId, address _token) external; + function transferEthToNTV() external; + + function transferTokenToNTV(address _token) external; } diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol index 7564b4f38..ed9b2de89 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol @@ -514,20 +514,25 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { function test_safeTransferFundsFromSharedBridge_Erc() public { uint256 maxGas = 30_000_000; + uint256 startBalanceNtv = nativeTokenVault.chainBalance(chainId, address(token)); // solhint-disable-next-line func-named-parameters vm.expectEmit(true, true, false, true, address(token)); emit IERC20.Transfer(address(sharedBridge), address(nativeTokenVault), amount); - nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(address(token), chainId, maxGas); + nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(address(token), maxGas); + nativeTokenVault.safeTransferBalancesFromSharedBridge{gas: maxGas}(address(token), chainId, maxGas); + uint256 endBalanceNtv = nativeTokenVault.chainBalance(chainId, address(token)); + assertEq(endBalanceNtv - startBalanceNtv, amount); } function test_safeTransferFundsFromSharedBridge_Eth() public { uint256 maxGas = 30_000_000; - uint256 startBalanceShb = sharedBridge.chainBalance(chainId, ETH_TOKEN_ADDRESS); + uint256 startEthBalanceNtv = address(nativeTokenVault).balance; uint256 startBalanceNtv = nativeTokenVault.chainBalance(chainId, ETH_TOKEN_ADDRESS); - nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, chainId, maxGas); - uint256 endBalanceShb = sharedBridge.chainBalance(chainId, ETH_TOKEN_ADDRESS); + nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, maxGas); + nativeTokenVault.safeTransferBalancesFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, chainId, maxGas); uint256 endBalanceNtv = nativeTokenVault.chainBalance(chainId, ETH_TOKEN_ADDRESS); + uint256 endEthBalanceNtv = address(nativeTokenVault).balance; assertEq(endBalanceNtv - startBalanceNtv, amount); - assertEq(startBalanceShb - endBalanceShb, amount); + assertEq(endEthBalanceNtv - startEthBalanceNtv, amount); } } From 82e6c770271b34d9883aa010c547374c60146ff3 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Mon, 24 Jun 2024 21:21:06 +0400 Subject: [PATCH 11/14] (feat): increase coverage --- .../contracts/bridge/L1NativeTokenVault.sol | 2 +- .../L1SharedBridge/L1SharedBridgeBase.t.sol | 2 - .../L1SharedBridge/L1SharedBridgeFails.t.sol | 41 +++++++++++++++++++ .../_L1SharedBridge_Shared.t.sol | 2 + 4 files changed, 44 insertions(+), 3 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol index 572610ac3..2e8a712be 100644 --- a/l1-contracts/contracts/bridge/L1NativeTokenVault.sol +++ b/l1-contracts/contracts/bridge/L1NativeTokenVault.sol @@ -128,7 +128,7 @@ contract L1NativeTokenVault is ); uint256 sharedBridgeChainBalance = L1_SHARED_BRIDGE.chainBalance(_targetChainId, _token); chainBalance[_targetChainId][_token] = chainBalance[_targetChainId][_token] + sharedBridgeChainBalance; - chainBalanceMigrated[_targetChainId][_token] == true; + chainBalanceMigrated[_targetChainId][_token] = true; } /// @dev Transfer tokens from shared bridge as part of migration process. diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol index ed9b2de89..99e5ad499 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeBase.t.sol @@ -513,7 +513,6 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { } function test_safeTransferFundsFromSharedBridge_Erc() public { - uint256 maxGas = 30_000_000; uint256 startBalanceNtv = nativeTokenVault.chainBalance(chainId, address(token)); // solhint-disable-next-line func-named-parameters vm.expectEmit(true, true, false, true, address(token)); @@ -525,7 +524,6 @@ contract L1SharedBridgeTestBase is L1SharedBridgeTest { } function test_safeTransferFundsFromSharedBridge_Eth() public { - uint256 maxGas = 30_000_000; uint256 startEthBalanceNtv = address(nativeTokenVault).balance; uint256 startBalanceNtv = nativeTokenVault.chainBalance(chainId, ETH_TOKEN_ADDRESS); nativeTokenVault.safeTransferFundsFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, maxGas); diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol index 89397bc9e..7646e206a 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/L1SharedBridgeFails.t.sol @@ -51,6 +51,16 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { ); } + function test_transferEthToNTV_wrongCaller() public { + vm.expectRevert("ShB: not NTV"); + sharedBridge.transferEthToNTV(); + } + + function test_transferTokenToNTV_wrongCaller() public { + vm.expectRevert("ShB: not NTV"); + sharedBridge.transferTokenToNTV(address(token)); + } + function test_registerToken_noCode() public { vm.expectRevert("NTV: empty token"); nativeTokenVault.registerToken(address(0)); @@ -118,6 +128,37 @@ contract L1SharedBridgeFailTest is L1SharedBridgeTest { ); } + function test_transferFundsToSharedBridge_Eth_0_AmountTransferred() public { + uint256 gas = 1_000_000; + vm.deal(address(sharedBridge), 0); + vm.prank(address(nativeTokenVault)); + vm.expectRevert("NTV: 0 eth transferred"); + nativeTokenVault.transferFundsFromSharedBridge(ETH_TOKEN_ADDRESS); + } + + function test_transferFundsToSharedBridge_Erc_0_AmountTransferred() public { + vm.prank(address(sharedBridge)); + token.transfer(address(1), amount); + vm.prank(address(nativeTokenVault)); + vm.expectRevert("NTV: 0 amount to transfer"); + nativeTokenVault.transferFundsFromSharedBridge(address(token)); + } + + function test_transferFundsToSharedBridge_Erc_WrongAmountTransferred() public { + vm.mockCall(address(token), abi.encodeWithSelector(IERC20.balanceOf.selector), abi.encode(10)); + vm.prank(address(nativeTokenVault)); + vm.expectRevert("NTV: wrong amount transferred"); + nativeTokenVault.transferFundsFromSharedBridge(address(token)); + } + + function test_safeTransferFundsFromSharedBridge_ShouldNotBeCalledTwice() public { + nativeTokenVault.safeTransferBalancesFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, chainId, maxGas); + assertEq(nativeTokenVault.chainBalanceMigrated(chainId, ETH_TOKEN_ADDRESS), true); + vm.prank(address(nativeTokenVault)); + vm.expectRevert("NTV: chain balance for the token already migrated"); + nativeTokenVault.transferBalancesFromSharedBridge{gas: maxGas}(ETH_TOKEN_ADDRESS, chainId); + } + function test_bridgehubDepositBaseToken_Eth_Token_notRegisteredTokenID() public { // ToDo: Shall we do it properly instead of mocking? stdstore diff --git a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol index 45d8bec5d..e0b7114ed 100644 --- a/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol +++ b/l1-contracts/test/foundry/unit/concrete/Bridges/L1SharedBridge/_L1SharedBridge_Shared.t.sol @@ -83,6 +83,8 @@ contract L1SharedBridgeTest is Test { uint256 amount = 100; uint256 mintValue = 1; bytes32 txHash; + uint256 gas = 1_000_000; + uint256 maxGas = 30_000_000; uint256 eraChainId; uint256 randomChainId; From 91d7dc2f71abb4a9a9e3651c370e91f128b062f1 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Mon, 24 Jun 2024 22:35:03 +0400 Subject: [PATCH 12/14] (fix): deploy was failing with safeTransfer --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index cd0ae8d77..de0ba876f 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -184,7 +184,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade function transferTokenToNTV(address _token) external { require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); uint256 amount = IERC20(_token).balanceOf(address(this)); - IERC20(_token).safeTransfer(address(nativeTokenVault), amount); + IERC20(_token).safeTransferFrom(address(this), address(nativeTokenVault), amount); } /// @dev Sets the L1ERC20Bridge contract address. Should be called only once. From b7ba3f2267b79630a6c30ede0b6cfc18d4853a2c Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Mon, 24 Jun 2024 22:55:32 +0400 Subject: [PATCH 13/14] (fix): hit contract size limit previously --- l1-contracts/contracts/bridge/L1SharedBridge.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index de0ba876f..dea167169 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -170,10 +170,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @dev transfer token to shared bridge as part of upgrade function transferEthToNTV() external { - require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); + address ntvAddress = address(nativeTokenVault); + require(msg.sender == ntvAddress, "ShB: not NTV"); uint256 amount = address(this).balance; bool callSuccess; - address ntvAddress = address(nativeTokenVault); // Low-level assembly call, to avoid any memory copying (save gas) assembly { callSuccess := call(gas(), ntvAddress, amount, 0, 0, 0, 0) @@ -183,8 +183,7 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @dev transfer token to shared bridge as part of upgrade function transferTokenToNTV(address _token) external { require(msg.sender == address(nativeTokenVault), "ShB: not NTV"); - uint256 amount = IERC20(_token).balanceOf(address(this)); - IERC20(_token).safeTransferFrom(address(this), address(nativeTokenVault), amount); + IERC20(_token).safeTransfer(address(nativeTokenVault), IERC20(_token).balanceOf(address(this))); } /// @dev Sets the L1ERC20Bridge contract address. Should be called only once. From 56236e7f66b0a37683af85b6efe5be511cb46ec6 Mon Sep 17 00:00:00 2001 From: Raid Ateir Date: Mon, 24 Jun 2024 23:05:58 +0400 Subject: [PATCH 14/14] (update): review prep md --- contracts-review-prep.md | 25 ++----------------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/contracts-review-prep.md b/contracts-review-prep.md index 850d5893e..c4e0ab196 100644 --- a/contracts-review-prep.md +++ b/contracts-review-prep.md @@ -46,17 +46,6 @@ We need to either: for Markdown's syntax, type some text into the left window and watch the results in the right. -### redundant call \_getAssetProperties - -> (, \_assetId) = \_getAssetProperties(\_assetId); // Handles the non-legacy case - -Most likely a redundant call: - -- for the new assets not needed -- for legacy tokens the handleLegacyData already provided the correct data. - -This one could be used in theory if the data has new format, but still uses the address instead of normal id. However, not sure we should support such cases, so it will be removed and tests updated. - ### not allowing legacy withdrawals > require(!\_isEraLegacyEthWithdrawal(\_chainId, \_l2BatchNumber), "ShB: legacy eth withdrawal"); @@ -64,16 +53,6 @@ This one could be used in theory if the data has new format, but still uses the No method to finalize an old withdrawal. We will manually finalize all legacy withdrawals before the upgrade, i.e. withdrawals that happened before the previous Bridgehub upgrade. -### empty branch when matching function signatures - -> (assetId, offset) = UnsafeBytes.readBytes32(\_l2ToL1message, offset); - - transferData = UnsafeBytes.readRemainingBytes(_l2ToL1message, offset); - } else if (bytes4(functionSignature) == this.finalizeWithdrawal.selector) { - //todo - -Currently, the support of old selector with ERC20 is completed, but the new format with chainId in the `finalizeWithdraw` selector hasn't been implemented in the `_parseL2WithdrawalMessage` function. - ### Custom Errors not implemented > require(expectedDepositAmount == \_depositAmount, "3T"); // The token has non-standard transfer logic @@ -82,5 +61,5 @@ Custom errors will be introduced for all contracts. ## Migration plan -- Bulkheads will need o be migrated -- Tokens will have to be transferred +- Bulkheads will need to be migrated (methods added) +- Tokens will have to be transferred (methods added)