diff --git a/packages/contracts-bedrock/interfaces/L1/IETHLockbox.sol b/packages/contracts-bedrock/interfaces/L1/IETHLockbox.sol index c7d2d5fdc2e..27df1600dd6 100644 --- a/packages/contracts-bedrock/interfaces/L1/IETHLockbox.sol +++ b/packages/contracts-bedrock/interfaces/L1/IETHLockbox.sol @@ -9,6 +9,7 @@ import { IOptimismPortal2 } from "interfaces/L1/IOptimismPortal2.sol"; interface IETHLockbox is IProxyAdminOwnerBase, ISemver { error ETHLockbox_Unauthorized(); error ETHLockbox_Paused(); + error ETHLockbox_InsufficientBalance(); error ETHLockbox_NoWithdrawalTransactions(); error ETHLockbox_DifferentProxyAdminOwner(); diff --git a/packages/contracts-bedrock/snapshots/abi/ETHLockbox.json b/packages/contracts-bedrock/snapshots/abi/ETHLockbox.json index 8dc5b81723e..4b7bd30e568 100644 --- a/packages/contracts-bedrock/snapshots/abi/ETHLockbox.json +++ b/packages/contracts-bedrock/snapshots/abi/ETHLockbox.json @@ -286,6 +286,11 @@ "name": "ETHLockbox_DifferentProxyAdminOwner", "type": "error" }, + { + "inputs": [], + "name": "ETHLockbox_InsufficientBalance", + "type": "error" + }, { "inputs": [], "name": "ETHLockbox_NoWithdrawalTransactions", diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index f0c3a66ffcf..738b63d4c25 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -4,8 +4,8 @@ "sourceCodeHash": "0xe772f7db8033e4a738850cb28ac4849d3a454c93732135a8a10d4f7cb498088e" }, "src/L1/ETHLockbox.sol": { - "initCodeHash": "0xd28e600379a190ca64a0f602d93a8a2850967a0df9da9d83b45f2501db53f17b", - "sourceCodeHash": "0x1b5229515bd0e4cd67d08185cb4aa88bab219ce8c1fd89575c9f5a50111cf2c1" + "initCodeHash": "0x4ad4f1719b2092cf224d7083c4816c2ab9fa39331eb016557d17aa2ff43ef909", + "sourceCodeHash": "0x45c7ee45398ab4be3a59d93d36b897bbcc1c12adb71c002e17fd6142ce95ef52" }, "src/L1/L1CrossDomainMessenger.sol": { "initCodeHash": "0x03a3c0eb2418aba9f93bb89723ba2ee7cb9e1988ca00f380503c960149c85b7a", @@ -21,7 +21,7 @@ }, "src/L1/OPContractsManager.sol": { "initCodeHash": "0xb68b46bb78e111809dae6e2a0404056691144796c006f988a9f17dfcafdd44e8", - "sourceCodeHash": "0x54667be25944539efec5d2ebedeb888081a224645f8e27ddecc1bc4a51407084" + "sourceCodeHash": "0x7c5ad88de15e2f2910558ccddb58b6a5ba523fb70087c4683e8418034b2dc11d" }, "src/L1/OPContractsManagerInterop.sol": { "initCodeHash": "0x47188fbb81c947426602099104046ce5ccd008292a233585493ce67e905a6082", @@ -33,7 +33,7 @@ }, "src/L1/OptimismPortal2.sol": { "initCodeHash": "0xa66af9c53c197dad8a662a77573e371b55a3c23dac84a8febdd1d07084475b99", - "sourceCodeHash": "0x370650c5bf19add9561b6dc3bfe2c0bee5e5f1a31ab2de17cceccf0a36aabd2c" + "sourceCodeHash": "0xf735d2c64c3675b178fc2ea334aa0e3689622a5e33a5f9aeba6844682b2be05b" }, "src/L1/OptimismPortalInterop.sol": { "initCodeHash": "0xb2b094a8a39f198cd947855dcdd470039059901f53253d867732ca04adb3ba52", diff --git a/packages/contracts-bedrock/src/L1/ETHLockbox.sol b/packages/contracts-bedrock/src/L1/ETHLockbox.sol index 5cd82db2326..e08ac219c8c 100644 --- a/packages/contracts-bedrock/src/L1/ETHLockbox.sol +++ b/packages/contracts-bedrock/src/L1/ETHLockbox.sol @@ -25,6 +25,9 @@ contract ETHLockbox is ProxyAdminOwnerBase, Initializable, ISemver { /// @notice Thrown when the caller is not authorized. error ETHLockbox_Unauthorized(); + /// @notice Thrown when the value to unlock is greater than the balance of the lockbox. + error ETHLockbox_InsufficientBalance(); + /// @notice Thrown when attempting to unlock ETH from the lockbox through a withdrawal transaction. error ETHLockbox_NoWithdrawalTransactions(); @@ -125,6 +128,7 @@ contract ETHLockbox is ProxyAdminOwnerBase, Initializable, ISemver { function unlockETH(uint256 _value) external { if (paused()) revert ETHLockbox_Paused(); if (!authorizedPortals[msg.sender]) revert ETHLockbox_Unauthorized(); + if (_value > address(this).balance) revert ETHLockbox_InsufficientBalance(); /// NOTE: Check l2Sender is not set to avoid this function to be called as a target on a withdrawal transaction if (IOptimismPortal(payable(msg.sender)).l2Sender() != Constants.DEFAULT_L2_SENDER) { revert ETHLockbox_NoWithdrawalTransactions(); @@ -139,17 +143,20 @@ contract ETHLockbox is ProxyAdminOwnerBase, Initializable, ISemver { /// @param _lockbox The address of the ETH lockbox to authorize. function authorizeLockbox(IETHLockbox _lockbox) external { if (msg.sender != proxyAdminOwner()) revert ETHLockbox_Unauthorized(); - if (!_sameproxyAdminOwner(address(_lockbox))) revert ETHLockbox_DifferentProxyAdminOwner(); + if (!_sameProxyAdminOwner(address(_lockbox))) revert ETHLockbox_DifferentProxyAdminOwner(); authorizedLockboxes[address(_lockbox)] = true; emit LockboxAuthorized(address(_lockbox)); } /// @notice Migrates liquidity from the current ETH lockbox to another. + /// @dev Must be called atomically with `OptimismPortal.updateLockbox()` in the same + /// transaction batch, or otherwise the OptimismPortal may not be able to unlock ETH + /// from the ETHLockbox on finalized withdrawals. /// @param _lockbox The address of the ETH lockbox to migrate liquidity to. function migrateLiquidity(IETHLockbox _lockbox) external { if (msg.sender != proxyAdminOwner()) revert ETHLockbox_Unauthorized(); - if (!_sameproxyAdminOwner(address(_lockbox))) revert ETHLockbox_DifferentProxyAdminOwner(); + if (!_sameProxyAdminOwner(address(_lockbox))) revert ETHLockbox_DifferentProxyAdminOwner(); IETHLockbox(_lockbox).receiveLiquidity{ value: address(this).balance }(); emit LiquidityMigrated(address(_lockbox)); @@ -158,7 +165,7 @@ contract ETHLockbox is ProxyAdminOwnerBase, Initializable, ISemver { /// @notice Authorizes a portal to lock and unlock ETH. /// @param _portal The address of the portal to authorize. function _authorizePortal(address _portal) internal { - if (!_sameproxyAdminOwner(_portal)) revert ETHLockbox_DifferentProxyAdminOwner(); + if (!_sameProxyAdminOwner(_portal)) revert ETHLockbox_DifferentProxyAdminOwner(); authorizedPortals[_portal] = true; emit PortalAuthorized(_portal); } diff --git a/packages/contracts-bedrock/src/L1/OPContractsManager.sol b/packages/contracts-bedrock/src/L1/OPContractsManager.sol index 3db21a056d1..75c70ee9733 100644 --- a/packages/contracts-bedrock/src/L1/OPContractsManager.sol +++ b/packages/contracts-bedrock/src/L1/OPContractsManager.sol @@ -619,11 +619,9 @@ contract OPContractsManager is ISemver { ); } - // Deploy the ETHLockbox contract implementation and proxy. - // Initialize the ETHLockbox, and then set the OptimismPortal as an authorized portal. + // Deploy the ETHLockbox proxy. IETHLockbox ethLockbox; { - // Deploy the ETHLockbox contract proxy. ethLockbox = IETHLockbox( deployProxy({ _l2ChainId: l2ChainId, @@ -633,7 +631,7 @@ contract OPContractsManager is ISemver { }) ); - // Initialize the ETHLockbox. + // Initialize the ETHLockbox setting the OptimismPortal as an authorized portal. IOptimismPortal[] memory portals = new IOptimismPortal[](1); portals[0] = IOptimismPortal(payable(opChainAddrs.optimismPortal)); upgradeToAndCall( diff --git a/packages/contracts-bedrock/src/L1/OptimismPortal2.sol b/packages/contracts-bedrock/src/L1/OptimismPortal2.sol index d9398cfb43c..2a083f7787c 100644 --- a/packages/contracts-bedrock/src/L1/OptimismPortal2.sol +++ b/packages/contracts-bedrock/src/L1/OptimismPortal2.sol @@ -354,6 +354,9 @@ contract OptimismPortal2 is Initializable, ResourceMetering, ReinitializableBase } /// @notice Updates the ETHLockbox contract. + /// @dev This function MUST be called atomically with `ETHLockbox.migrateLiquidity()` + /// in the same transaction batch, or otherwise the OptimismPortal may not be able to + /// unlock ETH from the ETHLockbox on finalized withdrawals. /// @param _newLockbox The address of the new ETHLockbox contract. function updateLockbox(IETHLockbox _newLockbox) external { if (msg.sender != proxyAdminOwner()) revert OptimismPortal_Unauthorized(); diff --git a/packages/contracts-bedrock/src/L1/ProxyAdminOwnerBase.sol b/packages/contracts-bedrock/src/L1/ProxyAdminOwnerBase.sol index 16883227e18..9f5861034e5 100644 --- a/packages/contracts-bedrock/src/L1/ProxyAdminOwnerBase.sol +++ b/packages/contracts-bedrock/src/L1/ProxyAdminOwnerBase.sol @@ -20,7 +20,7 @@ abstract contract ProxyAdminOwnerBase { /// @notice Checks if the ProxyAdmin owner of the current contract is the same as the ProxyAdmin owner of the given /// proxy. /// @param _proxy The address of the proxy to check. - function _sameproxyAdminOwner(address _proxy) internal view returns (bool) { + function _sameProxyAdminOwner(address _proxy) internal view returns (bool) { return proxyAdminOwner() == ProxyAdminOwnerBase(_proxy).proxyAdminOwner(); } } diff --git a/packages/contracts-bedrock/test/L1/ETHLockbox.t.sol b/packages/contracts-bedrock/test/L1/ETHLockbox.t.sol index ee95dabea5e..02c8fd2e105 100644 --- a/packages/contracts-bedrock/test/L1/ETHLockbox.t.sol +++ b/packages/contracts-bedrock/test/L1/ETHLockbox.t.sol @@ -205,9 +205,22 @@ contract ETHLockboxTest is CommonTest { ethLockbox.unlockETH(_value); } + /// @notice Tests `unlockETH` reverts when the `_value` input is greater than the balance of the lockbox. + function testFuzz_unlockETH_insufficientBalance_reverts(uint256 _value) public { + _value = bound(_value, address(ethLockbox).balance + 1, type(uint256).max); + + // Expect the revert with `InsufficientBalance` selector + vm.expectRevert(IETHLockbox.ETHLockbox_InsufficientBalance.selector); + + // Call the `unlockETH` function with the portal + vm.prank(address(optimismPortal2)); + ethLockbox.unlockETH(_value); + } + /// @notice Tests `unlockETH` reverts when the portal is not the L2 sender to prevent unlocking ETH from the lockbox /// through a withdrawal transaction. function testFuzz_unlockETH_withdrawalTransaction_reverts(uint256 _value, address _l2Sender) public { + _value = bound(_value, 0, address(ethLockbox).balance); vm.assume(_l2Sender != Constants.DEFAULT_L2_SENDER); // Mock the L2 sender