diff --git a/l1-contracts/contracts/bridge/L1ERC20Bridge.sol b/l1-contracts/contracts/bridge/L1ERC20Bridge.sol index da3098969..965bb34d1 100644 --- a/l1-contracts/contracts/bridge/L1ERC20Bridge.sol +++ b/l1-contracts/contracts/bridge/L1ERC20Bridge.sol @@ -11,6 +11,8 @@ import {IL1SharedBridge} from "./interfaces/IL1SharedBridge.sol"; import {L2ContractHelper} from "../common/libraries/L2ContractHelper.sol"; import {ReentrancyGuard} from "../common/ReentrancyGuard.sol"; +import {Unauthorized, EmptyDeposit, ValueMismatch, WithdrawalAlreadyFinalized} from "../common/L1ContractErrors.sol"; + /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev /// @notice Smart contract that allows depositing ERC20 tokens from Ethereum to hyperchains @@ -65,7 +67,9 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard { /// @dev transfer token to shared bridge as part of upgrade function transferTokenToSharedBridge(address _token) external { - require(msg.sender == address(SHARED_BRIDGE), "Not shared bridge"); + if (msg.sender != address(SHARED_BRIDGE)) { + revert Unauthorized(msg.sender); + } uint256 amount = IERC20(_token).balanceOf(address(this)); IERC20(_token).safeTransfer(address(SHARED_BRIDGE), amount); } @@ -148,9 +152,15 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard { uint256 _l2TxGasPerPubdataByte, address _refundRecipient ) public payable nonReentrant returns (bytes32 l2TxHash) { - require(_amount != 0, "0T"); // empty deposit + // empty deposit + if (_amount == 0) { + revert EmptyDeposit(); + } uint256 amount = _depositFundsToSharedBridge(msg.sender, IERC20(_l1Token), _amount); - require(amount == _amount, "3T"); // The token has non-standard transfer logic + // The token has non-standard transfer logic + if (amount != _amount) { + revert ValueMismatch(_amount, amount); + } l2TxHash = SHARED_BRIDGE.depositLegacyErc20Bridge{value: msg.value}({ _msgSender: msg.sender, @@ -194,7 +204,10 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard { bytes32[] calldata _merkleProof ) external nonReentrant { uint256 amount = depositAmount[_depositSender][_l1Token][_l2TxHash]; - require(amount != 0, "2T"); // empty deposit + // empty deposit + if (amount == 0) { + revert EmptyDeposit(); + } delete depositAmount[_depositSender][_l1Token][_l2TxHash]; SHARED_BRIDGE.claimFailedDepositLegacyErc20Bridge({ @@ -223,7 +236,9 @@ contract L1ERC20Bridge is IL1ERC20Bridge, ReentrancyGuard { bytes calldata _message, bytes32[] calldata _merkleProof ) external nonReentrant { - require(!isWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex], "pw"); + if (isWithdrawalFinalized[_l2BatchNumber][_l2MessageIndex]) { + revert WithdrawalAlreadyFinalized(); + } // We don't need to set finalizeWithdrawal here, as we set it in the shared bridge (address l1Receiver, address l1Token, uint256 amount) = SHARED_BRIDGE.finalizeWithdrawalLegacyErc20Bridge({ diff --git a/l1-contracts/contracts/bridge/L1SharedBridge.sol b/l1-contracts/contracts/bridge/L1SharedBridge.sol index 92cf5a6fa..82c5872be 100644 --- a/l1-contracts/contracts/bridge/L1SharedBridge.sol +++ b/l1-contracts/contracts/bridge/L1SharedBridge.sol @@ -22,6 +22,33 @@ import {ETH_TOKEN_ADDRESS, TWO_BRIDGES_MAGIC_VALUE} from "../common/Config.sol"; import {IBridgehub, L2TransactionRequestTwoBridgesInner, L2TransactionRequestDirect} from "../bridgehub/IBridgehub.sol"; import {IGetters} from "../state-transition/chain-interfaces/IGetters.sol"; import {L2_BASE_TOKEN_SYSTEM_CONTRACT_ADDR} from "../common/L2ContractAddresses.sol"; +import { + Unauthorized, + ZeroAddress, + SharedBridgeValueAlreadySet, + SharedBridgeKey, + NoFundsTransferred, + ZeroBalance, + ValueMismatch, + RevertWithMsg, + ERR_RECEIVE_ETH_BRIDGE, + NonEmptyMsgValue, + L2BridgeNotDeployed, + TokenNotSupported, + WithdrawIncorrectAmount, + EmptyDeposit, + DepositExists, + AddressAlreadyUsed, + InvalidProof, + DepositDNE, + InsufficientFunds, + DepositFailed, + ShareadBridgeValueNotSet, + WithdrawalAlreadyFinalized, + WithdrawFailed, + MalformedMessage, + InvalidSelector +} from "../common/L1ContractErrors.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev @@ -91,22 +118,25 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @notice Checks that the message sender is the bridgehub. modifier onlyBridgehub() { - require(msg.sender == address(BRIDGE_HUB), "ShB not BH"); + if (msg.sender != address(BRIDGE_HUB)) { + revert Unauthorized(msg.sender); + } _; } /// @notice Checks that the message sender is the bridgehub or zkSync Era Diamond Proxy. modifier onlyBridgehubOrEra(uint256 _chainId) { - require( - msg.sender == address(BRIDGE_HUB) || (_chainId == ERA_CHAIN_ID && msg.sender == ERA_DIAMOND_PROXY), - "L1SharedBridge: not bridgehub or era chain" - ); + if (msg.sender != address(BRIDGE_HUB) && (_chainId != ERA_CHAIN_ID || msg.sender != ERA_DIAMOND_PROXY)) { + revert Unauthorized(msg.sender); + } _; } /// @notice Checks that the message sender is the legacy bridge. modifier onlyLegacyBridge() { - require(msg.sender == address(legacyBridge), "ShB not legacy bridge"); + if (msg.sender != address(legacyBridge)) { + revert Unauthorized(msg.sender); + } _; } @@ -129,21 +159,27 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @param _owner Address which can change L2 token implementation and upgrade the bridge /// implementation. The owner is the Governor and separate from the ProxyAdmin from now on, so that the Governor can call the bridge. function initialize(address _owner) external reentrancyGuardInitializer initializer { - require(_owner != address(0), "ShB owner 0"); + if (_owner == address(0)) { + revert ZeroAddress(); + } _transferOwnership(_owner); } /// @dev This sets the first post diamond upgrade batch for era, used to check old eth withdrawals /// @param _eraPostDiamondUpgradeFirstBatch The first batch number on the zkSync Era Diamond Proxy that was settled after diamond proxy upgrade. function setEraPostDiamondUpgradeFirstBatch(uint256 _eraPostDiamondUpgradeFirstBatch) external onlyOwner { - require(eraPostDiamondUpgradeFirstBatch == 0, "ShB: eFPUB already set"); + if (eraPostDiamondUpgradeFirstBatch != 0) { + revert SharedBridgeValueAlreadySet(SharedBridgeKey.PostUpgradeFirstBatch); + } eraPostDiamondUpgradeFirstBatch = _eraPostDiamondUpgradeFirstBatch; } /// @dev This sets the first post upgrade batch for era, used to check old token withdrawals /// @param _eraPostLegacyBridgeUpgradeFirstBatch The first batch number on the zkSync Era Diamond Proxy that was settled after legacy bridge upgrade. function setEraPostLegacyBridgeUpgradeFirstBatch(uint256 _eraPostLegacyBridgeUpgradeFirstBatch) external onlyOwner { - require(eraPostLegacyBridgeUpgradeFirstBatch == 0, "ShB: eFPUB already set"); + if (eraPostLegacyBridgeUpgradeFirstBatch != 0) { + revert SharedBridgeValueAlreadySet(SharedBridgeKey.LegacyBridgeFirstBatch); + } eraPostLegacyBridgeUpgradeFirstBatch = _eraPostLegacyBridgeUpgradeFirstBatch; } @@ -154,8 +190,12 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade uint256 _eraLegacyBridgeLastDepositBatch, uint256 _eraLegacyBridgeLastDepositTxNumber ) external onlyOwner { - require(eraLegacyBridgeLastDepositBatch == 0, "ShB: eLOBDB already set"); - require(eraLegacyBridgeLastDepositTxNumber == 0, "ShB: eLOBDTN already set"); + if (eraLegacyBridgeLastDepositBatch != 0) { + revert SharedBridgeValueAlreadySet(SharedBridgeKey.LegacyBridgeLastDepositBatch); + } + if (eraLegacyBridgeLastDepositTxNumber != 0) { + revert SharedBridgeValueAlreadySet(SharedBridgeKey.LegacyBridgeLastDepositTxn); + } eraLegacyBridgeLastDepositBatch = _eraLegacyBridgeLastDepositBatch; eraLegacyBridgeLastDepositTxNumber = _eraLegacyBridgeLastDepositTxNumber; } @@ -166,7 +206,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade uint256 balanceBefore = address(this).balance; IMailbox(_target).transferEthToSharedBridge(); uint256 balanceAfter = address(this).balance; - require(balanceAfter > balanceBefore, "ShB: 0 eth transferred"); + if (balanceAfter <= balanceBefore) { + revert NoFundsTransferred(); + } chainBalance[_targetChainId][ETH_TOKEN_ADDRESS] = chainBalance[_targetChainId][ETH_TOKEN_ADDRESS] + balanceAfter - @@ -174,16 +216,22 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade } else { uint256 balanceBefore = IERC20(_token).balanceOf(address(this)); uint256 legacyBridgeBalance = IERC20(_token).balanceOf(address(legacyBridge)); - require(legacyBridgeBalance > 0, "ShB: 0 amount to transfer"); + if (legacyBridgeBalance == 0) { + revert ZeroBalance(); + } IL1ERC20Bridge(_target).transferTokenToSharedBridge(_token); uint256 balanceAfter = IERC20(_token).balanceOf(address(this)); - require(balanceAfter - balanceBefore == legacyBridgeBalance, "ShB: wrong amount transferred"); + if (balanceAfter - balanceBefore != legacyBridgeBalance) { + revert ValueMismatch(balanceAfter - balanceBefore, legacyBridgeBalance); + } chainBalance[_targetChainId][_token] = chainBalance[_targetChainId][_token] + legacyBridgeBalance; } } function receiveEth(uint256 _chainId) external payable { - require(BRIDGE_HUB.getHyperchain(_chainId) == msg.sender, "receiveEth not state transition"); + if (BRIDGE_HUB.getHyperchain(_chainId) != msg.sender) { + revert RevertWithMsg(ERR_RECEIVE_ETH_BRIDGE); + } } /// @dev Initializes the l2Bridge address by governance for a specific chain. @@ -200,13 +248,20 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade uint256 _amount ) external payable virtual onlyBridgehubOrEra(_chainId) whenNotPaused { if (_l1Token == ETH_TOKEN_ADDRESS) { - require(msg.value == _amount, "L1SharedBridge: msg.value not equal to amount"); + if (msg.value != _amount) { + revert ValueMismatch(_amount, msg.value); + } } else { // The Bridgehub also checks this, but we want to be sure - require(msg.value == 0, "ShB m.v > 0 b d.it"); + if (msg.value != 0) { + revert NonEmptyMsgValue(); + } uint256 amount = _depositFunds(_prevMsgSender, IERC20(_l1Token), _amount); // note if _prevMsgSender is this contract, this will return 0. This does not happen. - require(amount == _amount, "3T"); // The token has non-standard transfer logic + // The token has non-standard transfer logic + if (amount != _amount) { + revert ValueMismatch(_amount, amount); + } } if (!hyperbridgingEnabled[_chainId]) { @@ -242,27 +297,43 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade whenNotPaused returns (L2TransactionRequestTwoBridgesInner memory request) { - require(l2BridgeAddress[_chainId] != address(0), "ShB l2 bridge not deployed"); + if (l2BridgeAddress[_chainId] == address(0)) { + revert L2BridgeNotDeployed(_chainId); + } (address _l1Token, uint256 _depositAmount, address _l2Receiver) = abi.decode( _data, (address, uint256, address) ); - require(_l1Token != L1_WETH_TOKEN, "ShB: WETH deposit not supported"); - require(BRIDGE_HUB.baseToken(_chainId) != _l1Token, "ShB: baseToken deposit not supported"); + if (_l1Token == L1_WETH_TOKEN) { + revert TokenNotSupported(L1_WETH_TOKEN); + } + if (BRIDGE_HUB.baseToken(_chainId) == _l1Token) { + revert TokenNotSupported(_l1Token); + } uint256 amount; if (_l1Token == ETH_TOKEN_ADDRESS) { amount = msg.value; - require(_depositAmount == 0, "ShB wrong withdraw amount"); + if (_depositAmount != 0) { + revert WithdrawIncorrectAmount(); + } } else { - require(msg.value == 0, "ShB m.v > 0 for BH d.it 2"); + if (msg.value != 0) { + revert NonEmptyMsgValue(); + } amount = _depositAmount; uint256 withdrawAmount = _depositFunds(_prevMsgSender, IERC20(_l1Token), _depositAmount); - require(withdrawAmount == _depositAmount, "5T"); // The token has non-standard transfer logic + // The token has non-standard transfer logic + if (withdrawAmount != _depositAmount) { + revert WithdrawIncorrectAmount(); + } } - require(amount != 0, "6T"); // empty deposit amount + // empty deposit amount + if (amount == 0) { + revert EmptyDeposit(); + } bytes32 txDataHash = keccak256(abi.encode(_prevMsgSender, _l1Token, amount)); if (!hyperbridgingEnabled[_chainId]) { @@ -298,15 +369,21 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade bytes32 _txDataHash, bytes32 _txHash ) external override onlyBridgehub whenNotPaused { - require(depositHappened[_chainId][_txHash] == 0x00, "ShB tx hap"); + if (depositHappened[_chainId][_txHash] != 0x00) { + revert DepositExists(); + } depositHappened[_chainId][_txHash] = _txDataHash; emit BridgehubDepositFinalized(_chainId, _txDataHash, _txHash); } /// @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"); - require(_legacyBridge != address(0), "ShB: legacy bridge 0"); + if (address(legacyBridge) != address(0)) { + revert AddressAlreadyUsed(address(legacyBridge)); + } + if (_legacyBridge == address(0)) { + revert ZeroAddress(); + } legacyBridge = IL1ERC20Bridge(_legacyBridge); } @@ -393,9 +470,13 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _merkleProof: _merkleProof, _status: TxStatus.Failure }); - require(proofValid, "yn"); + if (!proofValid) { + revert InvalidProof(); + } + } + if (_amount == 0) { + revert NoFundsTransferred(); } - require(_amount > 0, "y1"); { bool notCheckedInLegacyBridgeOrWeCanCheckDeposit; @@ -410,14 +491,18 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade if (notCheckedInLegacyBridgeOrWeCanCheckDeposit) { bytes32 dataHash = depositHappened[_chainId][_l2TxHash]; bytes32 txDataHash = keccak256(abi.encode(_depositSender, _l1Token, _amount)); - require(dataHash == txDataHash, "ShB: d.it not hap"); + if (dataHash != txDataHash) { + revert DepositDNE(); + } delete depositHappened[_chainId][_l2TxHash]; } } if (!hyperbridgingEnabled[_chainId]) { // check that the chain has sufficient balance - require(chainBalance[_chainId][_l1Token] >= _amount, "ShB n funds"); + if (chainBalance[_chainId][_l1Token] < _amount) { + revert InsufficientFunds(); + } chainBalance[_chainId][_l1Token] -= _amount; } @@ -428,7 +513,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade assembly { callSuccess := call(gas(), _depositSender, _amount, 0, 0, 0, 0) } - require(callSuccess, "ShB: claimFailedDeposit failed"); + if (!callSuccess) { + revert DepositFailed(); + } } else { IERC20(_l1Token).safeTransfer(_depositSender, _amount); // Note we don't allow weth deposits anymore, but there might be legacy weth deposits. @@ -443,7 +530,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @param _l2BatchNumber The L2 batch number for the withdrawal. /// @return Whether withdrawal was initiated on zkSync Era before diamond proxy upgrade. function _isEraLegacyEthWithdrawal(uint256 _chainId, uint256 _l2BatchNumber) internal view returns (bool) { - require((_chainId != ERA_CHAIN_ID) || eraPostDiamondUpgradeFirstBatch != 0, "ShB: diamondUFB not set for Era"); + if ((_chainId == ERA_CHAIN_ID) && eraPostDiamondUpgradeFirstBatch == 0) { + revert ShareadBridgeValueNotSet(SharedBridgeKey.PostUpgradeFirstBatch); + } return (_chainId == ERA_CHAIN_ID) && (_l2BatchNumber < eraPostDiamondUpgradeFirstBatch); } @@ -452,10 +541,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade /// @param _l2BatchNumber The L2 batch number for the withdrawal. /// @return Whether withdrawal was initiated on zkSync Era before Legacy Bridge upgrade. function _isEraLegacyTokenWithdrawal(uint256 _chainId, uint256 _l2BatchNumber) internal view returns (bool) { - require( - (_chainId != ERA_CHAIN_ID) || eraPostLegacyBridgeUpgradeFirstBatch != 0, - "ShB: LegacyUFB not set for Era" - ); + if((_chainId == ERA_CHAIN_ID) && eraPostLegacyBridgeUpgradeFirstBatch == 0) { + revert ShareadBridgeValueNotSet(SharedBridgeKey.LegacyBridgeFirstBatch); + } return (_chainId == ERA_CHAIN_ID) && (_l2BatchNumber < eraPostLegacyBridgeUpgradeFirstBatch); } @@ -469,10 +557,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade uint256 _l2BatchNumber, uint256 _l2TxNumberInBatch ) internal view returns (bool) { - require( - (_chainId != ERA_CHAIN_ID) || (eraLegacyBridgeLastDepositBatch != 0), - "ShB: last deposit time not set for Era" - ); + if ((_chainId == ERA_CHAIN_ID) && (eraLegacyBridgeLastDepositBatch == 0)) { + revert ShareadBridgeValueNotSet(SharedBridgeKey.LegacyBridgeLastDepositBatch); + } return (_chainId == ERA_CHAIN_ID) && (_l2BatchNumber < eraLegacyBridgeLastDepositBatch || @@ -498,7 +585,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade // To avoid rewithdrawing txs that have already happened on the legacy bridge. // Note: new withdraws are all recorded here, so double withdrawing them is not possible. if (_isEraLegacyTokenWithdrawal(_chainId, _l2BatchNumber)) { - require(!legacyBridge.isWithdrawalFinalized(_l2BatchNumber, _l2MessageIndex), "ShB: legacy withdrawal"); + if (legacyBridge.isWithdrawalFinalized(_l2BatchNumber, _l2MessageIndex)) { + revert WithdrawalAlreadyFinalized(); + } } _finalizeWithdrawal({ _chainId: _chainId, @@ -526,7 +615,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade bytes calldata _message, bytes32[] calldata _merkleProof ) internal nonReentrant whenNotPaused returns (address l1Receiver, address l1Token, uint256 amount) { - require(!isWithdrawalFinalized[_chainId][_l2BatchNumber][_l2MessageIndex], "Withdrawal is already finalized"); + if (isWithdrawalFinalized[_chainId][_l2BatchNumber][_l2MessageIndex]) { + revert WithdrawalAlreadyFinalized(); + } isWithdrawalFinalized[_chainId][_l2BatchNumber][_l2MessageIndex] = true; // Handling special case for withdrawal from zkSync Era initiated before Shared Bridge. @@ -536,7 +627,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _l2BatchNumber, _l2MessageIndex ); - require(!alreadyFinalized, "Withdrawal is already finalized 2"); + if (alreadyFinalized) { + revert WithdrawalAlreadyFinalized(); + } } MessageParams memory messageParams = MessageParams({ @@ -548,7 +641,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade if (!hyperbridgingEnabled[_chainId]) { // Check that the chain has sufficient balance - require(chainBalance[_chainId][l1Token] >= amount, "ShB not enough funds 2"); // not enough funds + if (chainBalance[_chainId][l1Token] < amount) { + // not enough funds + revert InsufficientFunds(); + } chainBalance[_chainId][l1Token] -= amount; } @@ -558,7 +654,9 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade assembly { callSuccess := call(gas(), l1Receiver, amount, 0, 0, 0, 0) } - require(callSuccess, "ShB: withdraw failed"); + if (!callSuccess) { + revert WithdrawFailed(); + } } else { // Withdraw funds IERC20(l1Token).safeTransfer(l1Receiver, amount); @@ -593,7 +691,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade _message: l2ToL1Message, _proof: _merkleProof }); - require(success, "ShB withd w proof"); // withdrawal wrong proof + // withdrawal wrong proof + if (!success) { + revert InvalidProof(); + } } function _parseL2WithdrawalMessage( @@ -610,7 +711,10 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade // = 4 + 20 + 32 + 32 + _additionalData.length >= 68 (bytes). // So the data is expected to be at least 56 bytes long. - require(_l2ToL1message.length >= 56, "ShB wrong msg len"); // wrong message length + // wrong message length + if (_l2ToL1message.length < 56) { + revert MalformedMessage(); + } (uint32 functionSignature, uint256 offset) = UnsafeBytes.readUint32(_l2ToL1message, 0); if (bytes4(functionSignature) == IMailbox.finalizeEthWithdrawal.selector) { @@ -626,12 +730,14 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade // 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"); + if (_l2ToL1message.length != 76) { + revert MalformedMessage(); + } (l1Receiver, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset); (l1Token, offset) = UnsafeBytes.readAddress(_l2ToL1message, offset); (amount, offset) = UnsafeBytes.readUint256(_l2ToL1message, offset); } else { - revert("ShB Incorrect message function selector"); + revert InvalidSelector(bytes4(functionSignature)); } } @@ -672,8 +778,12 @@ contract L1SharedBridge is IL1SharedBridge, ReentrancyGuard, Ownable2StepUpgrade uint256 _l2TxGasPerPubdataByte, address _refundRecipient ) external payable override onlyLegacyBridge nonReentrant whenNotPaused returns (bytes32 l2TxHash) { - require(l2BridgeAddress[ERA_CHAIN_ID] != address(0), "ShB b. n dep"); - require(_l1Token != L1_WETH_TOKEN, "ShB: WETH deposit not supported 2"); + if (l2BridgeAddress[ERA_CHAIN_ID] == address(0)) { + revert L2BridgeNotDeployed(ERA_CHAIN_ID); + } + if (_l1Token == L1_WETH_TOKEN) { + revert TokenNotSupported(L1_WETH_TOKEN); + } // Note that funds have been transferred to this contract in the legacy ERC20 bridge. if (!hyperbridgingEnabled[ERA_CHAIN_ID]) { diff --git a/l1-contracts/contracts/bridgehub/Bridgehub.sol b/l1-contracts/contracts/bridgehub/Bridgehub.sol index c10cdd359..7260ef4cf 100644 --- a/l1-contracts/contracts/bridgehub/Bridgehub.sol +++ b/l1-contracts/contracts/bridgehub/Bridgehub.sol @@ -13,6 +13,20 @@ import {IZkSyncHyperchain} from "../state-transition/chain-interfaces/IZkSyncHyp import {ETH_TOKEN_ADDRESS, TWO_BRIDGES_MAGIC_VALUE, BRIDGEHUB_MIN_SECOND_BRIDGE_ADDRESS} from "../common/Config.sol"; import {BridgehubL2TransactionRequest, L2Message, L2Log, TxStatus} from "../common/Messaging.sol"; import {AddressAliasHelper} from "../vendor/AddressAliasHelper.sol"; +import { + Unauthorized, + STMAlreadyRegistered, + STMNotRegistered, + TokenAlreadyRegistered, + TokenNotRegistered, + InvalidChainId, + WethBridgeNotSet, + BridgeHubAlreadyRegistered, + ValueMismatch, + InsufficientFunds, + NonEmptyMsgValue, + AddressTooLow +} from "../common/L1ContractErrors.sol"; contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, PausableUpgradeable { /// @notice all the ether is held by the weth bridge @@ -44,7 +58,9 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus } modifier onlyOwnerOrAdmin() { - require(msg.sender == admin || msg.sender == owner(), "Bridgehub: not owner or admin"); + if (msg.sender != admin && msg.sender != owner()) { + revert Unauthorized(msg.sender); + } _; } @@ -62,7 +78,10 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus /// @inheritdoc IBridgehub function acceptAdmin() external { address currentPendingAdmin = pendingAdmin; - require(msg.sender == currentPendingAdmin, "n42"); // Only proposed by current admin address can claim the admin rights + // Only proposed by current admin address can claim the admin rights + if (msg.sender != currentPendingAdmin) { + revert Unauthorized(msg.sender); + } address previousAdmin = admin; admin = currentPendingAdmin; @@ -83,26 +102,26 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus /// @notice State Transition can be any contract with the appropriate interface/functionality function addStateTransitionManager(address _stateTransitionManager) external onlyOwner { - require( - !stateTransitionManagerIsRegistered[_stateTransitionManager], - "Bridgehub: state transition already registered" - ); + if (stateTransitionManagerIsRegistered[_stateTransitionManager]) { + revert STMAlreadyRegistered(); + } stateTransitionManagerIsRegistered[_stateTransitionManager] = true; } /// @notice State Transition can be any contract with the appropriate interface/functionality /// @notice this stops new Chains from using the STF, old chains are not affected function removeStateTransitionManager(address _stateTransitionManager) external onlyOwner { - require( - stateTransitionManagerIsRegistered[_stateTransitionManager], - "Bridgehub: state transition not registered yet" - ); + if (!stateTransitionManagerIsRegistered[_stateTransitionManager]) { + revert STMNotRegistered(); + } stateTransitionManagerIsRegistered[_stateTransitionManager] = false; } /// @notice token can be any contract with the appropriate interface/functionality function addToken(address _token) external onlyOwner { - require(!tokenIsRegistered[_token], "Bridgehub: token already registered"); + if (tokenIsRegistered[_token]) { + revert TokenAlreadyRegistered(_token); + } tokenIsRegistered[_token] = true; } @@ -123,17 +142,26 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus address _admin, bytes calldata _initData ) external onlyOwnerOrAdmin nonReentrant whenNotPaused returns (uint256) { - require(_chainId != 0, "Bridgehub: chainId cannot be 0"); - require(_chainId <= type(uint48).max, "Bridgehub: chainId too large"); + if (_chainId == 0) { + revert InvalidChainId(); + } + if (_chainId <= type(uint48).max) { + revert InvalidChainId(); + } - require( - stateTransitionManagerIsRegistered[_stateTransitionManager], - "Bridgehub: state transition not registered" - ); - require(tokenIsRegistered[_baseToken], "Bridgehub: token not registered"); - require(address(sharedBridge) != address(0), "Bridgehub: weth bridge not set"); + if (!stateTransitionManagerIsRegistered[_stateTransitionManager]) { + revert STMNotRegistered(); + } + if (!tokenIsRegistered[_baseToken]) { + revert TokenNotRegistered(_baseToken); + } + if (address(sharedBridge) == address(0)) { + revert WethBridgeNotSet(); + } - require(stateTransitionManager[_chainId] == address(0), "Bridgehub: chainId already registered"); + if (stateTransitionManager[_chainId] != address(0)) { + revert BridgeHubAlreadyRegistered(); + } stateTransitionManager[_chainId] = _stateTransitionManager; baseToken[_chainId] = _baseToken; @@ -219,9 +247,13 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus { address token = baseToken[_request.chainId]; if (token == ETH_TOKEN_ADDRESS) { - require(msg.value == _request.mintValue, "Bridgehub: msg.value mismatch 1"); + if (msg.value != _request.mintValue) { + revert InsufficientFunds(); + } } else { - require(msg.value == 0, "Bridgehub: non-eth bridge with msg.value"); + if (msg.value != 0) { + revert NonEmptyMsgValue(); + } } // slither-disable-next-line arbitrary-send-eth @@ -266,13 +298,14 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus address token = baseToken[_request.chainId]; uint256 baseTokenMsgValue; if (token == ETH_TOKEN_ADDRESS) { - require( - msg.value == _request.mintValue + _request.secondBridgeValue, - "Bridgehub: msg.value mismatch 2" - ); + if (msg.value != _request.mintValue + _request.secondBridgeValue) { + revert InsufficientFunds(); + } baseTokenMsgValue = _request.mintValue; } else { - require(msg.value == _request.secondBridgeValue, "Bridgehub: msg.value mismatch 3"); + if (msg.value != _request.secondBridgeValue) { + revert NonEmptyMsgValue(); + } baseTokenMsgValue = 0; } // slither-disable-next-line arbitrary-send-eth @@ -295,14 +328,16 @@ contract Bridgehub is IBridgehub, ReentrancyGuard, Ownable2StepUpgradeable, Paus _request.secondBridgeCalldata ); - require(outputRequest.magicValue == TWO_BRIDGES_MAGIC_VALUE, "Bridgehub: magic value mismatch"); + if (outputRequest.magicValue != TWO_BRIDGES_MAGIC_VALUE) { + revert ValueMismatch(uint256(TWO_BRIDGES_MAGIC_VALUE), uint256(outputRequest.magicValue)); + } address refundRecipient = AddressAliasHelper.actualRefundRecipient(_request.refundRecipient, msg.sender); - require( - _request.secondBridgeAddress > BRIDGEHUB_MIN_SECOND_BRIDGE_ADDRESS, - "Bridgehub: second bridge address too low" - ); // to avoid calls to precompiles + if (_request.secondBridgeAddress <= BRIDGEHUB_MIN_SECOND_BRIDGE_ADDRESS) { + revert AddressTooLow(_request.secondBridgeAddress); + } + // to avoid calls to precompiles canonicalTxHash = IZkSyncHyperchain(hyperchain).bridgehubRequestL2Transaction( BridgehubL2TransactionRequest({ sender: _request.secondBridgeAddress, diff --git a/l1-contracts/contracts/common/L1ContractErrors.sol b/l1-contracts/contracts/common/L1ContractErrors.sol new file mode 100644 index 000000000..24d86a101 --- /dev/null +++ b/l1-contracts/contracts/common/L1ContractErrors.sol @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.24; + +error Unauthorized(address caller); +error EmptyDeposit(); +error ValueMismatch(uint256 expected, uint256 actual); +error WithdrawalAlreadyFinalized(); +error ZeroAddress(); +error SharedBridgeValueAlreadySet(SharedBridgeKey); +error NoFundsTransferred(); +error ZeroBalance(); +error RevertWithMsg(string message); +error NonEmptyMsgValue(); +error L2BridgeNotDeployed(uint256 chainId); +error TokenNotSupported(address token); +error WithdrawIncorrectAmount(); +error DepositExists(); +error AddressAlreadyUsed(address addr); +error InvalidProof(); +error DepositDNE(); +error InsufficientFunds(); +error DepositFailed(); +error ShareadBridgeValueNotSet(SharedBridgeKey); +error WithdrawFailed(); +error MalformedMessage(); +error InvalidSelector(bytes4 func); +error STMAlreadyRegistered(); +error STMNotRegistered(); +error TokenAlreadyRegistered(address token); +error TokenNotRegistered(address token); +error InvalidChainId(); +error WethBridgeNotSet(); +error BridgeHubAlreadyRegistered(); +error AddressTooLow(address); +error SlotOccupied(); +error MalformedBytecode(BytecodeError); +error OperationShouldBeReady(); +error OperationShouldBePending(); +error OperationExists(); +error InvalidDelay(); +error PreviousOperationNotExecuted(); + +enum SharedBridgeKey { + PostUpgradeFirstBatch, + LegacyBridgeFirstBatch, + LegacyBridgeLastDepositBatch, + LegacyBridgeLastDepositTxn +} + +enum BytecodeError { + Version, + NumberOfWords, + Length, + WordsMustBeOdd +} + +string constant ERR_RECEIVE_ETH_BRIDGE = "receiveEth not state transition"; diff --git a/l1-contracts/contracts/common/ReentrancyGuard.sol b/l1-contracts/contracts/common/ReentrancyGuard.sol index a19020b77..7c13efa25 100644 --- a/l1-contracts/contracts/common/ReentrancyGuard.sol +++ b/l1-contracts/contracts/common/ReentrancyGuard.sol @@ -2,6 +2,8 @@ pragma solidity 0.8.24; +import {SlotOccupied} from "./L1ContractErrors.sol"; + /** * @custom:security-contact security@matterlabs.dev * @dev Contract module that helps prevent reentrant calls to a function. @@ -55,7 +57,9 @@ abstract contract ReentrancyGuard { } // Check that storage slot for reentrancy guard is empty to rule out possibility of slot conflict - require(lockSlotOldValue == 0, "1B"); + if (lockSlotOldValue != 0) { + revert SlotOccupied(); + } } /** @@ -72,7 +76,9 @@ abstract contract ReentrancyGuard { } // On the first call to nonReentrant, _notEntered will be true - require(_status == _NOT_ENTERED, "r1"); + if (_status != _NOT_ENTERED) { + revert SlotOccupied(); + } // Any calls to nonReentrant after this point will fail assembly { diff --git a/l1-contracts/contracts/common/libraries/L2ContractHelper.sol b/l1-contracts/contracts/common/libraries/L2ContractHelper.sol index ae1a64250..cb563b546 100644 --- a/l1-contracts/contracts/common/libraries/L2ContractHelper.sol +++ b/l1-contracts/contracts/common/libraries/L2ContractHelper.sol @@ -2,6 +2,8 @@ pragma solidity 0.8.24; +import {BytecodeError, MalformedBytecode} from "../L1ContractErrors.sol"; + /** * @author Matter Labs * @custom:security-contact security@matterlabs.dev @@ -20,11 +22,19 @@ library L2ContractHelper { /// - Bytecode words length is not odd function hashL2Bytecode(bytes memory _bytecode) internal pure returns (bytes32 hashedBytecode) { // Note that the length of the bytecode must be provided in 32-byte words. - require(_bytecode.length % 32 == 0, "pq"); + if (_bytecode.length % 32 != 0) { + revert MalformedBytecode(BytecodeError.Length); + } uint256 bytecodeLenInWords = _bytecode.length / 32; - require(bytecodeLenInWords < 2 ** 16, "pp"); // bytecode length must be less than 2^16 words - require(bytecodeLenInWords % 2 == 1, "ps"); // bytecode length in words must be odd + // bytecode length must be less than 2^16 words + if (bytecodeLenInWords >= 2 ** 16) { + revert MalformedBytecode(BytecodeError.NumberOfWords); + } + // bytecode length in words must be odd + if (bytecodeLenInWords % 2 == 0) { + revert MalformedBytecode(BytecodeError.WordsMustBeOdd); + } hashedBytecode = sha256(_bytecode) & 0x00000000FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF; // Setting the version of the hash hashedBytecode = (hashedBytecode | bytes32(uint256(1 << 248))); @@ -38,9 +48,15 @@ library L2ContractHelper { /// @param _bytecodeHash The hash of the bytecode to validate. function validateBytecodeHash(bytes32 _bytecodeHash) internal pure { uint8 version = uint8(_bytecodeHash[0]); - require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); // Incorrectly formatted bytecodeHash + // Incorrectly formatted bytecodeHash + if (version != 1 || _bytecodeHash[1] != bytes1(0)) { + revert MalformedBytecode(BytecodeError.Version); + } - require(bytecodeLen(_bytecodeHash) % 2 == 1, "uy"); // Code length in words must be odd + // Code length in words must be odd + if (bytecodeLen(_bytecodeHash) % 2 == 0) { + revert MalformedBytecode(BytecodeError.WordsMustBeOdd); + } } /// @notice Returns the length of the bytecode associated with the given hash. diff --git a/l1-contracts/contracts/governance/Governance.sol b/l1-contracts/contracts/governance/Governance.sol index 509422bd1..4107965db 100644 --- a/l1-contracts/contracts/governance/Governance.sol +++ b/l1-contracts/contracts/governance/Governance.sol @@ -4,6 +4,15 @@ pragma solidity 0.8.24; import {Ownable2Step} from "@openzeppelin/contracts/access/Ownable2Step.sol"; import {IGovernance} from "./IGovernance.sol"; +import { + ZeroAddress, + Unauthorized, + OperationShouldBeReady, + OperationShouldBePending, + OperationExists, + InvalidDelay, + PreviousOperationNotExecuted +} from "../common/L1ContractErrors.sol"; /// @author Matter Labs /// @custom:security-contact security@matterlabs.dev @@ -39,7 +48,9 @@ contract Governance is IGovernance, Ownable2Step { /// @param _securityCouncil The address to be assigned as the security council of the contract. /// @param _minDelay The initial minimum delay (in seconds) to be set for operations. constructor(address _admin, address _securityCouncil, uint256 _minDelay) { - require(_admin != address(0), "Admin should be non zero address"); + if (_admin == address(0)) { + revert ZeroAddress(); + } _transferOwnership(_admin); @@ -56,25 +67,25 @@ contract Governance is IGovernance, Ownable2Step { /// @notice Checks that the message sender is contract itself. modifier onlySelf() { - // solhint-disable-next-line reason-string - require(msg.sender == address(this), "Only governance contract itself is allowed to call this function"); + if (msg.sender != address(this)) { + revert Unauthorized(msg.sender); + } _; } /// @notice Checks that the message sender is an active security council. modifier onlySecurityCouncil() { - // solhint-disable-next-line reason-string - require(msg.sender == securityCouncil, "Only security council is allowed to call this function"); + if (msg.sender != securityCouncil) { + revert Unauthorized(msg.sender); + } _; } /// @notice Checks that the message sender is an active owner or an active security council. modifier onlyOwnerOrSecurityCouncil() { - // solhint-disable-next-line reason-string - require( - msg.sender == owner() || msg.sender == securityCouncil, - "Only the owner and security council are allowed to call this function" - ); + if (msg.sender != owner() && msg.sender != securityCouncil) { + revert Unauthorized(msg.sender); + } _; } @@ -155,7 +166,9 @@ contract Governance is IGovernance, Ownable2Step { /// @dev Only owner can call this function. /// @param _id Proposal id value (see `hashOperation`) function cancel(bytes32 _id) external onlyOwner { - require(isOperationPending(_id), "Operation must be pending"); + if (!isOperationPending(_id)) { + revert OperationShouldBePending(); + } delete timestamps[_id]; emit OperationCancelled(_id); } @@ -173,15 +186,17 @@ contract Governance is IGovernance, Ownable2Step { // Check if the predecessor operation is completed. _checkPredecessorDone(_operation.predecessor); // Ensure that the operation is ready to proceed. - // solhint-disable-next-line reason-string - require(isOperationReady(id), "Operation must be ready before execution"); + if (!isOperationReady(id)) { + revert OperationShouldBeReady(); + } // Execute operation. // slither-disable-next-line reentrancy-eth _execute(_operation.calls); // Reconfirming that the operation is still ready after execution. // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation. - // solhint-disable-next-line reason-string - require(isOperationReady(id), "Operation must be ready after execution"); + if (!isOperationReady(id)) { + revert OperationShouldBeReady(); + } // Set operation to be done timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP; emit OperationExecuted(id); @@ -196,15 +211,17 @@ contract Governance is IGovernance, Ownable2Step { // Check if the predecessor operation is completed. _checkPredecessorDone(_operation.predecessor); // Ensure that the operation is in a pending state before proceeding. - // solhint-disable-next-line reason-string - require(isOperationPending(id), "Operation must be pending before execution"); + if (!isOperationPending(id)) { + revert OperationShouldBePending(); + } // Execute operation. // slither-disable-next-line reentrancy-eth _execute(_operation.calls); // Reconfirming that the operation is still pending before execution. // This is needed to avoid unexpected reentrancy attacks of re-executing the same operation. - // solhint-disable-next-line reason-string - require(isOperationPending(id), "Operation must be pending after execution"); + if (!isOperationPending(id)) { + revert OperationShouldBePending(); + } // Set operation to be done timestamps[id] = EXECUTED_PROPOSAL_TIMESTAMP; emit OperationExecuted(id); @@ -224,8 +241,12 @@ contract Governance is IGovernance, Ownable2Step { /// @param _id The operation hash (see `hashOperation` function) /// @param _delay The delay time (in seconds) after which the proposed upgrade can be executed by the owner. function _schedule(bytes32 _id, uint256 _delay) internal { - require(!isOperation(_id), "Operation with this proposal id already exists"); - require(_delay >= minDelay, "Proposed delay is less than minimum delay"); + if (isOperation(_id)) { + revert OperationExists(); + } + if (_delay < minDelay) { + revert InvalidDelay(); + } timestamps[_id] = block.timestamp + _delay; } @@ -250,8 +271,9 @@ contract Governance is IGovernance, Ownable2Step { /// @param _predecessorId The hash of the operation that should be completed. /// @dev Doesn't check the operation to be complete if the input is zero. function _checkPredecessorDone(bytes32 _predecessorId) internal view { - // solhint-disable-next-line reason-string - require(_predecessorId == bytes32(0) || isOperationDone(_predecessorId), "Predecessor operation not completed"); + if (_predecessorId != bytes32(0) && !isOperationDone(_predecessorId)) { + revert PreviousOperationNotExecuted(); + } } /*//////////////////////////////////////////////////////////////