diff --git a/packages/contracts-bedrock/interfaces/L1/IFeesDepositor.sol b/packages/contracts-bedrock/interfaces/L1/IFeesDepositor.sol index 3e943124b4d19..30c76e962e0ec 100644 --- a/packages/contracts-bedrock/interfaces/L1/IFeesDepositor.sol +++ b/packages/contracts-bedrock/interfaces/L1/IFeesDepositor.sol @@ -10,29 +10,25 @@ interface IFeesDepositor is ISemver, IProxyAdminOwnedBase, IReinitializableBase event Initialized(uint8 version); event FundsReceived(address indexed sender, uint256 amount, uint256 newBalance); event FeesDeposited(address indexed l2Recipient, uint256 amount); - event MinDepositAmountUpdated(uint96 oldminDepositAmount, uint96 newminDepositAmount); + event MinDepositAmountUpdated(uint96 oldMinDepositAmount, uint96 newMinDepositAmount); event L2RecipientUpdated(address oldL2Recipient, address newL2Recipient); event GasLimitUpdated(uint64 oldGasLimit, uint64 newGasLimit); - event DepositDataUpdated(bytes oldDepositData, bytes newDepositData); function minDepositAmount() external view returns (uint96); function portal() external view returns (IOptimismPortal); function l2Recipient() external view returns (address); function gasLimit() external view returns (uint64); - function depositData() external view returns (bytes memory); function initialize( uint96 _minDepositAmount, address _l2Recipient, IOptimismPortal _portal, - uint64 _gasLimit, - bytes memory _depositData + uint64 _gasLimit ) external; - function setMinDepositAmount(uint96 _minDepositAmount) external; - function setL2Recipient(address _l2Recipient) external; - function setGasLimit(uint64 _gasLimit) external; - function setDepositData(bytes memory _depositData) external; + function setMinDepositAmount(uint96 _newMinDepositAmount) external; + function setL2Recipient(address _newL2Recipient) external; + function setGasLimit(uint64 _newGasLimit) external; receive() external payable; diff --git a/packages/contracts-bedrock/interfaces/L2/IL1Withdrawer.sol b/packages/contracts-bedrock/interfaces/L2/IL1Withdrawer.sol index c5c3eb3ebb6d6..59bae92afe1b9 100644 --- a/packages/contracts-bedrock/interfaces/L2/IL1Withdrawer.sol +++ b/packages/contracts-bedrock/interfaces/L2/IL1Withdrawer.sol @@ -5,6 +5,7 @@ import { ISemver } from "interfaces/universal/ISemver.sol"; interface IL1Withdrawer is ISemver { error L1Withdrawer_OnlyProxyAdminOwner(); + error L1Withdrawer_WithdrawalGasLimitTooLow(); event WithdrawalInitiated(address indexed recipient, uint256 amount); event FundsReceived(address indexed sender, uint256 amount, uint256 newBalance); diff --git a/packages/contracts-bedrock/interfaces/L2/ISharesCalculator.sol b/packages/contracts-bedrock/interfaces/L2/ISharesCalculator.sol index 895d1a95c15ba..b47ce282d4a51 100644 --- a/packages/contracts-bedrock/interfaces/L2/ISharesCalculator.sol +++ b/packages/contracts-bedrock/interfaces/L2/ISharesCalculator.sol @@ -1,12 +1,26 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; +/// @title ISharesCalculator +/// @notice Interface for a contract that calculates the recipients and amounts for fee distribution. +/// @dev Meant to be called by the FeeSplitter contract. interface ISharesCalculator { + /// @notice Struct to hold the recipient and amount for each fee share. + /// @param recipient The address that will receive the fee share + /// @param amount The amount of ETH to be sent to the recipient struct ShareInfo { address payable recipient; uint256 amount; } + /// @notice Returns the recipients and amounts for fee distribution. + /// @dev Any implementation MUST return ShareInfo where the sum of all amounts equals + /// the total revenue (sum of all vault balances) as it will revert otherwise + /// @param _sequencerFeeVaultBalance Balance of the sequencer fee vault. + /// @param _baseFeeVaultBalance Balance of the base fee vault. + /// @param _operatorFeeVaultBalance Balance of the operator fee vault. + /// @param _l1FeeVaultBalance Balance of the L1 fee vault. + /// @return shareInfo Array of ShareInfo structs containing recipients and amounts. function getRecipientsAndAmounts( uint256 _sequencerFeeVaultBalance, uint256 _baseFeeVaultBalance, diff --git a/packages/contracts-bedrock/snapshots/abi/FeesDepositor.json b/packages/contracts-bedrock/snapshots/abi/FeesDepositor.json index 2ef464d822a28..9f255b0dc37b5 100644 --- a/packages/contracts-bedrock/snapshots/abi/FeesDepositor.json +++ b/packages/contracts-bedrock/snapshots/abi/FeesDepositor.json @@ -8,19 +8,6 @@ "stateMutability": "payable", "type": "receive" }, - { - "inputs": [], - "name": "depositData", - "outputs": [ - { - "internalType": "bytes", - "name": "", - "type": "bytes" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [], "name": "gasLimit", @@ -68,11 +55,6 @@ "internalType": "uint64", "name": "_gasLimit", "type": "uint64" - }, - { - "internalType": "bytes", - "name": "_depositData", - "type": "bytes" } ], "name": "initialize", @@ -145,24 +127,11 @@ "stateMutability": "view", "type": "function" }, - { - "inputs": [ - { - "internalType": "bytes", - "name": "_depositData", - "type": "bytes" - } - ], - "name": "setDepositData", - "outputs": [], - "stateMutability": "nonpayable", - "type": "function" - }, { "inputs": [ { "internalType": "uint64", - "name": "_gasLimit", + "name": "_newGasLimit", "type": "uint64" } ], @@ -175,7 +144,7 @@ "inputs": [ { "internalType": "address", - "name": "_l2Recipient", + "name": "_newL2Recipient", "type": "address" } ], @@ -188,7 +157,7 @@ "inputs": [ { "internalType": "uint96", - "name": "_minDepositAmount", + "name": "_newMinDepositAmount", "type": "uint96" } ], @@ -210,25 +179,6 @@ "stateMutability": "view", "type": "function" }, - { - "anonymous": false, - "inputs": [ - { - "indexed": false, - "internalType": "bytes", - "name": "oldDepositData", - "type": "bytes" - }, - { - "indexed": false, - "internalType": "bytes", - "name": "newDepositData", - "type": "bytes" - } - ], - "name": "DepositDataUpdated", - "type": "event" - }, { "anonymous": false, "inputs": [ @@ -330,13 +280,13 @@ { "indexed": false, "internalType": "uint96", - "name": "oldminDepositAmount", + "name": "oldMinDepositAmount", "type": "uint96" }, { "indexed": false, "internalType": "uint96", - "name": "newminDepositAmount", + "name": "newMinDepositAmount", "type": "uint96" } ], diff --git a/packages/contracts-bedrock/snapshots/abi/L1Withdrawer.json b/packages/contracts-bedrock/snapshots/abi/L1Withdrawer.json index e3d3292be3c9c..933b67ef8a5b6 100644 --- a/packages/contracts-bedrock/snapshots/abi/L1Withdrawer.json +++ b/packages/contracts-bedrock/snapshots/abi/L1Withdrawer.json @@ -220,5 +220,10 @@ "inputs": [], "name": "L1Withdrawer_OnlyProxyAdminOwner", "type": "error" + }, + { + "inputs": [], + "name": "L1Withdrawer_WithdrawalGasLimitTooLow", + "type": "error" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/snapshots/semver-lock.json b/packages/contracts-bedrock/snapshots/semver-lock.json index d7de489ad0dbc..19420c7e88e9a 100644 --- a/packages/contracts-bedrock/snapshots/semver-lock.json +++ b/packages/contracts-bedrock/snapshots/semver-lock.json @@ -8,8 +8,8 @@ "sourceCodeHash": "0x6c9d3e2dee44c234d59ab93b6564536dfd807f1c4a02a82d5393bc53cb15b8b7" }, "src/L1/FeesDepositor.sol:FeesDepositor": { - "initCodeHash": "0x3db0c0c8c85294f014e5aea2f12a24565184836fa92f615df84b93e07026cb1f", - "sourceCodeHash": "0xe6734ce2591585aafe0cdf3955533140dcf83c3af8bad922ffd937df5630739b" + "initCodeHash": "0x3b1acae6460e04be9a855a24de87524f19827239da08f05934c3dd6183a8b5c5", + "sourceCodeHash": "0x613dc957e3bb409d1c7fb767971308b9cb1ed3c953407c8c641a6ec2bbf42fe8" }, "src/L1/L1CrossDomainMessenger.sol:L1CrossDomainMessenger": { "initCodeHash": "0x3dc659aafb03bd357f92abfc6794af89ee0ddd5212364551637422bf8d0b00f9", @@ -84,8 +84,8 @@ "sourceCodeHash": "0x2e85139090b9072e368a5ff36e5fcf2a90b219c6024e06a7998904cd75412395" }, "src/L2/L1Withdrawer.sol:L1Withdrawer": { - "initCodeHash": "0x491befab56c270410b21dc33eb20d03502916e01cbda1a88371619953c6ebd10", - "sourceCodeHash": "0xbd1728f6e8e6a1201a7b88f59d0439a00d53be2a1610a504f909ad82308d6218" + "initCodeHash": "0xb25010651e421d159204b9c9d3ae3d3569e57de82eabcff6a1d2560a21b88a82", + "sourceCodeHash": "0xde60d5d9f89f6c1195b11e21b2c3287afaa756a749c052530b76f26b4cdf6c0a" }, "src/L2/L2CrossDomainMessenger.sol:L2CrossDomainMessenger": { "initCodeHash": "0xe160be403df12709c371c33195d1b9c3b5e9499e902e86bdabc8eed749c3fd61", @@ -149,7 +149,7 @@ }, "src/L2/SuperchainRevSharesCalculator.sol:SuperchainRevSharesCalculator": { "initCodeHash": "0xdfff95660d2d470e198054bb1717a30a45a806d2eaa3720fb43acaa3356c9a3e", - "sourceCodeHash": "0xc3f488d08c9d2c1a2f2a0cde93c1274df58c87aebcf4e384e475a9d9deb1ea51" + "sourceCodeHash": "0xabf78321b187c9d36dcf8799c079f89f0f2edb1e0ea588c27b1bad07615b9193" }, "src/L2/SuperchainTokenBridge.sol:SuperchainTokenBridge": { "initCodeHash": "0xb0d25dc03b9c84b07b263921c2b717e6caad3f4297fa939207e35978d7d25abe", diff --git a/packages/contracts-bedrock/snapshots/storageLayout/FeesDepositor.json b/packages/contracts-bedrock/snapshots/storageLayout/FeesDepositor.json index 774d7320ee73d..3dd22ca68e144 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/FeesDepositor.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/FeesDepositor.json @@ -40,12 +40,5 @@ "offset": 0, "slot": "2", "type": "uint64" - }, - { - "bytes": "32", - "label": "depositData", - "offset": 0, - "slot": "3", - "type": "bytes" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/src/L1/FeesDepositor.sol b/packages/contracts-bedrock/src/L1/FeesDepositor.sol index 4a546e3f3e100..8a7480858792f 100644 --- a/packages/contracts-bedrock/src/L1/FeesDepositor.sol +++ b/packages/contracts-bedrock/src/L1/FeesDepositor.sol @@ -23,9 +23,6 @@ contract FeesDepositor is ProxyAdminOwnedBase, Initializable, ReinitializableBas /// @notice The gas limit for the deposit transaction. uint64 public gasLimit; - /// @notice The data for the deposit transaction. - bytes public depositData; - /// @notice Emitted when fees are received. /// @param sender The sender of the fees. /// @param amount The amount of fees received. @@ -38,9 +35,9 @@ contract FeesDepositor is ProxyAdminOwnedBase, Initializable, ReinitializableBas event FeesDeposited(address indexed l2Recipient, uint256 amount); /// @notice Emitted when the deposit threshold is updated. - /// @param oldminDepositAmount The old deposit threshold. - /// @param newminDepositAmount The new deposit threshold. - event MinDepositAmountUpdated(uint96 oldminDepositAmount, uint96 newminDepositAmount); + /// @param oldMinDepositAmount The old deposit threshold. + /// @param newMinDepositAmount The new deposit threshold. + event MinDepositAmountUpdated(uint96 oldMinDepositAmount, uint96 newMinDepositAmount); /// @notice Emitted when the L2 recipient is updated. /// @param oldL2Recipient The old L2 recipient. @@ -52,11 +49,6 @@ contract FeesDepositor is ProxyAdminOwnedBase, Initializable, ReinitializableBas /// @param newGasLimit The new gas limit. event GasLimitUpdated(uint64 oldGasLimit, uint64 newGasLimit); - /// @notice Emitted when the deposit data is updated. - /// @param oldDepositData The old deposit data. - /// @param newDepositData The new deposit data. - event DepositDataUpdated(bytes oldDepositData, bytes newDepositData); - /// @notice Semantic version. /// @custom:semver 1.0.0 string public constant version = "1.0.0"; @@ -71,13 +63,11 @@ contract FeesDepositor is ProxyAdminOwnedBase, Initializable, ReinitializableBas /// @param _l2Recipient The L2 recipient of the fees. /// @param _portal The portal contract. /// @param _gasLimit The gas limit for the deposit transaction. - /// @param _depositData The deposit data for the deposit transaction. function initialize( uint96 _minDepositAmount, address _l2Recipient, IOptimismPortal _portal, - uint64 _gasLimit, - bytes memory _depositData + uint64 _gasLimit ) external reinitializer(initVersion()) @@ -89,54 +79,47 @@ contract FeesDepositor is ProxyAdminOwnedBase, Initializable, ReinitializableBas minDepositAmount = _minDepositAmount; l2Recipient = _l2Recipient; gasLimit = _gasLimit; - depositData = _depositData; } /// @notice Receives ETH and deposits it to the L2 recipient through the portal when the threshold is reached. + /// @dev Be aware that when the DepositTransaction is sent, the `from` address will be the alias of this contract + /// address. receive() external payable { uint256 balance = address(this).balance; emit FundsReceived(msg.sender, msg.value, balance); if (balance >= minDepositAmount) { address recipient = l2Recipient; - portal.depositTransaction{ value: balance }(recipient, balance, gasLimit, false, depositData); + portal.depositTransaction{ value: balance }(recipient, balance, gasLimit, false, ""); emit FeesDeposited(recipient, balance); } } /// @notice Updates the deposit threshold. - /// @param _minDepositAmount The new deposit threshold. - function setMinDepositAmount(uint96 _minDepositAmount) external { + /// @param _newMinDepositAmount The new deposit threshold. + function setMinDepositAmount(uint96 _newMinDepositAmount) external { _assertOnlyProxyAdminOwner(); - uint96 oldminDepositAmount = minDepositAmount; - minDepositAmount = _minDepositAmount; - emit MinDepositAmountUpdated(oldminDepositAmount, _minDepositAmount); + uint96 oldMinDepositAmount = minDepositAmount; + minDepositAmount = _newMinDepositAmount; + emit MinDepositAmountUpdated(oldMinDepositAmount, _newMinDepositAmount); } /// @notice Updates the L2 recipient for the deposit transaction. - /// @param _l2Recipient The new L2 recipient. - function setL2Recipient(address _l2Recipient) external { + /// @dev The L2 recipient MUST be able to receive ether or the deposit on L2 will fail. + /// @param _newL2Recipient The new L2 recipient. + function setL2Recipient(address _newL2Recipient) external { _assertOnlyProxyAdminOwner(); address oldL2Recipient = l2Recipient; - l2Recipient = _l2Recipient; - emit L2RecipientUpdated(oldL2Recipient, _l2Recipient); + l2Recipient = _newL2Recipient; + emit L2RecipientUpdated(oldL2Recipient, _newL2Recipient); } /// @notice Updates the gas limit for the deposit transaction. - /// @param _gasLimit The new gas limit. - function setGasLimit(uint64 _gasLimit) external { + /// @param _newGasLimit The new gas limit. + function setGasLimit(uint64 _newGasLimit) external { _assertOnlyProxyAdminOwner(); uint64 oldGasLimit = gasLimit; - gasLimit = _gasLimit; - emit GasLimitUpdated(oldGasLimit, _gasLimit); - } - - /// @notice Updates the deposit data. - /// @param _depositData The new deposit data. - function setDepositData(bytes memory _depositData) external { - _assertOnlyProxyAdminOwner(); - bytes memory oldDepositData = depositData; - depositData = _depositData; - emit DepositDataUpdated(oldDepositData, _depositData); + gasLimit = _newGasLimit; + emit GasLimitUpdated(oldGasLimit, _newGasLimit); } } diff --git a/packages/contracts-bedrock/src/L2/L1Withdrawer.sol b/packages/contracts-bedrock/src/L2/L1Withdrawer.sol index 5747901d5cddb..69b2b19ce097c 100644 --- a/packages/contracts-bedrock/src/L2/L1Withdrawer.sol +++ b/packages/contracts-bedrock/src/L2/L1Withdrawer.sol @@ -14,6 +14,11 @@ contract L1Withdrawer is ISemver { /// @notice Thrown when the caller is not the ProxyAdmin owner. error L1Withdrawer_OnlyProxyAdminOwner(); + /// @notice Thrown when the withdrawal gas limit is too low. + error L1Withdrawer_WithdrawalGasLimitTooLow(); + + uint256 internal constant MIN_WITHDRAWAL_GAS = 250_000; + /// @notice The minimum amount of ETH that must be accumulated before a withdrawal is initiated. uint256 public minWithdrawalAmount; @@ -21,6 +26,7 @@ contract L1Withdrawer is ISemver { address public recipient; /// @notice The L1 gas limit set when initiating withdrawals. + /// @dev withdrawalGasLimit should be overestimated to account for expensive receive() uint96 public withdrawalGasLimit; /// @notice Emitted when a withdrawal to L1 is initiated. @@ -58,6 +64,9 @@ contract L1Withdrawer is ISemver { /// @param _recipient The L1 address that will receive withdrawals. /// @param _withdrawalGasLimit The gas limit for the L1 withdrawal transaction. constructor(uint256 _minWithdrawalAmount, address _recipient, uint96 _withdrawalGasLimit) { + if (_withdrawalGasLimit < MIN_WITHDRAWAL_GAS) { + revert L1Withdrawer_WithdrawalGasLimitTooLow(); + } minWithdrawalAmount = _minWithdrawalAmount; recipient = _recipient; withdrawalGasLimit = _withdrawalGasLimit; @@ -89,6 +98,8 @@ contract L1Withdrawer is ISemver { } /// @notice Updates the recipient address. Only callable by the ProxyAdmin owner. + /// @dev The recipient MUST be able to receive ether or L1Withdrawer#receive will fail + /// when the withdrawal is finalized. /// @param _newRecipient The new recipient address. function setRecipient(address _newRecipient) external { if (msg.sender != IProxyAdmin(Predeploys.PROXY_ADMIN).owner()) { @@ -105,6 +116,9 @@ contract L1Withdrawer is ISemver { if (msg.sender != IProxyAdmin(Predeploys.PROXY_ADMIN).owner()) { revert L1Withdrawer_OnlyProxyAdminOwner(); } + if (_newWithdrawalGasLimit < MIN_WITHDRAWAL_GAS) { + revert L1Withdrawer_WithdrawalGasLimitTooLow(); + } uint96 oldWithdrawalGasLimit = withdrawalGasLimit; withdrawalGasLimit = _newWithdrawalGasLimit; emit WithdrawalGasLimitUpdated(oldWithdrawalGasLimit, _newWithdrawalGasLimit); diff --git a/packages/contracts-bedrock/src/L2/SuperchainRevSharesCalculator.sol b/packages/contracts-bedrock/src/L2/SuperchainRevSharesCalculator.sol index acfc95c43d167..ff3ec4f33f015 100644 --- a/packages/contracts-bedrock/src/L2/SuperchainRevSharesCalculator.sol +++ b/packages/contracts-bedrock/src/L2/SuperchainRevSharesCalculator.sol @@ -57,6 +57,7 @@ contract SuperchainRevSharesCalculator is ISemver, ISharesCalculator { } /// @notice Returns the recipients and amounts for fee distribution. + /// @dev The recipients returned MUST be able to receive ether or FeeSplitter#disburseFees will fail. /// @param _sequencerFeeRevenue Revenue from sequencer fees. /// @param _baseFeeRevenue Revenue from base fees. /// @param _operatorFeeRevenue Revenue from operator fees. diff --git a/packages/contracts-bedrock/test/L1/FeesDepositor.t.sol b/packages/contracts-bedrock/test/L1/FeesDepositor.t.sol index 3fc8ee35c5917..6322c9f94189a 100644 --- a/packages/contracts-bedrock/test/L1/FeesDepositor.t.sol +++ b/packages/contracts-bedrock/test/L1/FeesDepositor.t.sol @@ -16,17 +16,15 @@ contract FeesDepositor_TestInit is CommonTest { // Events event FeesDeposited(address indexed l2Recipient, uint256 amount); event FundsReceived(address indexed sender, uint256 amount, uint256 newBalance); - event MinDepositAmountUpdated(uint96 oldminDepositAmount, uint96 newminDepositAmount); + event MinDepositAmountUpdated(uint96 oldMinDepositAmount, uint96 newMinDepositAmount); event L2RecipientUpdated(address oldL2Recipient, address newL2Recipient); event GasLimitUpdated(uint64 oldGasLimit, uint64 newGasLimit); - event DepositDataUpdated(bytes oldDepositData, bytes newDepositData); // Test state FeesDepositor feesDepositor; address l2Recipient = makeAddr("l2Recipient"); uint96 minDepositAmount = 1 ether; uint64 gasLimit = 150_000; - bytes depositData = hex"1234"; address depositFeesRecipient; /// @notice Test setup. @@ -51,7 +49,7 @@ contract FeesDepositor_TestInit is CommonTest { // Initialize through proxy vm.prank(proxyAdminOwner); - feesDepositor.initialize(minDepositAmount, l2Recipient, optimismPortal2, gasLimit, depositData); + feesDepositor.initialize(minDepositAmount, l2Recipient, optimismPortal2, gasLimit); // Set depositFeesRecipient depositFeesRecipient = @@ -66,7 +64,7 @@ contract FeesDepositor_Initialize_Test is FeesDepositor_TestInit { /// standard deployment script and instead is deployed manually, that's why we have this test. function test_cannotReinitialize_succeeds() public { vm.expectRevert("Initializable: contract is already initialized"); - feesDepositor.initialize(minDepositAmount, l2Recipient, optimismPortal2, gasLimit, depositData); + feesDepositor.initialize(minDepositAmount, l2Recipient, optimismPortal2, gasLimit); } } @@ -87,7 +85,7 @@ contract FeesDepositor_Receive_Test is FeesDepositor_TestInit { vm.expectCall( address(optimismPortal2), _amount, - abi.encodeCall(IOptimismPortal.depositTransaction, (l2Recipient, _amount, gasLimit, false, depositData)), + abi.encodeCall(IOptimismPortal.depositTransaction, (l2Recipient, _amount, gasLimit, false, "")), 0 ); @@ -114,7 +112,7 @@ contract FeesDepositor_Receive_Test is FeesDepositor_TestInit { vm.expectCall( address(optimismPortal2), _sendAmount, - abi.encodeCall(IOptimismPortal.depositTransaction, (l2Recipient, _sendAmount, gasLimit, false, depositData)) + abi.encodeCall(IOptimismPortal.depositTransaction, (l2Recipient, _sendAmount, gasLimit, false, "")) ); (bool success,) = address(feesDepositor).call{ value: _sendAmount }(""); @@ -164,7 +162,7 @@ contract FeesDepositor_Receive_Test is FeesDepositor_TestInit { vm.expectCall( address(optimismPortal2), totalAmount, - abi.encodeCall(IOptimismPortal.depositTransaction, (l2Recipient, totalAmount, gasLimit, false, depositData)) + abi.encodeCall(IOptimismPortal.depositTransaction, (l2Recipient, totalAmount, gasLimit, false, "")) ); (bool success2,) = address(feesDepositor).call{ value: _secondAmount }(""); @@ -178,6 +176,23 @@ contract FeesDepositor_Receive_Test is FeesDepositor_TestInit { "depositFeesRecipient balance 2" ); } + + /// @notice Fuzz test to ensure receive function gas usage never exceeds 200,000 gas. + /// @dev This test verifies the security requirement that receive() doesn't consume excessive gas, + /// preventing potential issues with withdrawal gas limits. The limit includes buffer for + /// measurement overhead and future contract changes. + function testFuzz_receive_gasUsageWithinLimit_succeeds(uint256 _amount) external { + _amount = bound(_amount, 0, type(uint256).max - depositFeesRecipient.balance); + + vm.deal(address(this), _amount); + + uint256 gasBefore = gasleft(); + (bool success,) = address(feesDepositor).call{ value: _amount }(""); + uint256 gasUsed = gasBefore - gasleft(); + + assertTrue(success, "Receive call should succeed"); + assertLe(gasUsed, 200_000, "Receive function gas usage should not exceed 200,000 gas"); + } } /// @title FeesDepositor_SetMinDepositAmount_Test @@ -266,32 +281,3 @@ contract FeesDepositor_SetGasLimit_Test is FeesDepositor_TestInit { assertEq(feesDepositor.gasLimit(), gasLimit); } } - -/// @title FeesDepositor_SetDepositData_Test -/// @notice Tests the setDepositData function of the `FeesDepositor` contract. -contract FeesDepositor_SetDepositData_Test is FeesDepositor_TestInit { - function testFuzz_setDepositData_asOwner_succeeds(bytes memory _newDepositData) external { - address owner = proxyAdmin.owner(); - - vm.expectEmit(address(feesDepositor)); - emit DepositDataUpdated(depositData, _newDepositData); - - vm.prank(owner); - feesDepositor.setDepositData(_newDepositData); - - assertEq(feesDepositor.depositData(), _newDepositData); - } - - function testFuzz_setDepositData_asNonOwner_reverts(address _caller) external { - address owner = proxyAdmin.owner(); - vm.assume(_caller != owner); - - bytes memory newDepositData = hex"5678"; - - vm.expectRevert(IProxyAdminOwnedBase.ProxyAdminOwnedBase_NotProxyAdminOwner.selector); - vm.prank(_caller); - feesDepositor.setDepositData(newDepositData); - - assertEq(feesDepositor.depositData(), depositData); - } -} diff --git a/packages/contracts-bedrock/test/L2/L1Withdrawer.t.sol b/packages/contracts-bedrock/test/L2/L1Withdrawer.t.sol index 8beaa94ef5c01..4239e35621eff 100644 --- a/packages/contracts-bedrock/test/L2/L1Withdrawer.t.sol +++ b/packages/contracts-bedrock/test/L2/L1Withdrawer.t.sol @@ -5,6 +5,7 @@ import { CommonTest } from "test/setup/CommonTest.sol"; import { IL2ToL1MessagePasser } from "interfaces/L2/IL2ToL1MessagePasser.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; import { IL1Withdrawer } from "interfaces/L2/IL1Withdrawer.sol"; +import { DeployUtils } from "scripts/libraries/DeployUtils.sol"; /// @title L1Withdrawer_TestInit /// @notice Base test contract with initialization for `L1Withdrawer` tests. @@ -20,6 +21,8 @@ contract L1Withdrawer_TestInit is CommonTest { uint256 minWithdrawalAmount = 10 ether; uint96 withdrawalGasLimit = 300_000; + uint256 internal constant MIN_WITHDRAWAL_GAS_LIMIT = 250_000; + /// @notice Test setup. function setUp() public virtual override { // Enable revenue sharing before calling parent setUp @@ -28,6 +31,51 @@ contract L1Withdrawer_TestInit is CommonTest { } } +contract L1Withdrawer_Constructor_Test is L1Withdrawer_TestInit { + function testFuzz_constructor_succeeds( + uint256 _minWithdrawalAmount, + address _recipient, + uint96 _withdrawalGasLimit + ) + external + { + _withdrawalGasLimit = uint96(bound(uint256(_withdrawalGasLimit), MIN_WITHDRAWAL_GAS_LIMIT, type(uint96).max)); + + IL1Withdrawer withdrawer = IL1Withdrawer( + DeployUtils.create1({ + _name: "L1Withdrawer", + _args: DeployUtils.encodeConstructor( + abi.encodeCall(IL1Withdrawer.__constructor__, (_minWithdrawalAmount, _recipient, _withdrawalGasLimit)) + ) + }) + ); + + assertEq(withdrawer.minWithdrawalAmount(), _minWithdrawalAmount); + assertEq(withdrawer.recipient(), _recipient); + assertEq(withdrawer.withdrawalGasLimit(), _withdrawalGasLimit); + } + + function testFuzz_constructor_lowGasLimit_reverts( + uint256 _minWithdrawalAmount, + address _recipient, + uint96 _withdrawalGasLimit + ) + external + { + _withdrawalGasLimit = uint96(bound(uint256(_withdrawalGasLimit), 0, MIN_WITHDRAWAL_GAS_LIMIT - 1)); + + vm.expectRevert(IL1Withdrawer.L1Withdrawer_WithdrawalGasLimitTooLow.selector); + IL1Withdrawer( + DeployUtils.create1({ + _name: "L1Withdrawer", + _args: DeployUtils.encodeConstructor( + abi.encodeCall(IL1Withdrawer.__constructor__, (_minWithdrawalAmount, _recipient, _withdrawalGasLimit)) + ) + }) + ); + } +} + /// @title L1Withdrawer_Receive_Test /// @notice Tests the receive function of the `L1Withdrawer` contract. contract L1Withdrawer_Receive_Test is L1Withdrawer_TestInit { @@ -178,6 +226,9 @@ contract L1Withdrawer_SetWithdrawalGasLimit_Test is L1Withdrawer_TestInit { function testFuzz_setWithdrawalGasLimit_asOwner_succeeds(uint96 _newWithdrawalGasLimit) external { address owner = proxyAdmin.owner(); + _newWithdrawalGasLimit = + uint96(bound(uint256(_newWithdrawalGasLimit), MIN_WITHDRAWAL_GAS_LIMIT, type(uint96).max)); + vm.expectEmit(address(l1Withdrawer)); emit WithdrawalGasLimitUpdated(withdrawalGasLimit, _newWithdrawalGasLimit); @@ -191,7 +242,7 @@ contract L1Withdrawer_SetWithdrawalGasLimit_Test is L1Withdrawer_TestInit { address owner = proxyAdmin.owner(); vm.assume(_caller != owner); - uint96 newWithdrawalGasLimit = 200_000; + uint96 newWithdrawalGasLimit = 250_000; vm.expectRevert(IL1Withdrawer.L1Withdrawer_OnlyProxyAdminOwner.selector); vm.prank(_caller); @@ -199,4 +250,12 @@ contract L1Withdrawer_SetWithdrawalGasLimit_Test is L1Withdrawer_TestInit { assertEq(l1Withdrawer.withdrawalGasLimit(), withdrawalGasLimit); } + + function testFuzz_setWithdrawalGasLimit_lowGasLimit_reverts(uint96 _newWithdrawalGasLimit) external { + _newWithdrawalGasLimit = uint96(bound(uint256(_newWithdrawalGasLimit), 0, MIN_WITHDRAWAL_GAS_LIMIT - 1)); + + vm.prank(proxyAdmin.owner()); + vm.expectRevert(IL1Withdrawer.L1Withdrawer_WithdrawalGasLimitTooLow.selector); + l1Withdrawer.setWithdrawalGasLimit(_newWithdrawalGasLimit); + } } diff --git a/packages/contracts-bedrock/test/L2/RevenueSharingIntegration.t.sol b/packages/contracts-bedrock/test/L2/RevenueSharingIntegration.t.sol index 70cbe2049fb0e..be1db50ab4d2a 100644 --- a/packages/contracts-bedrock/test/L2/RevenueSharingIntegration.t.sol +++ b/packages/contracts-bedrock/test/L2/RevenueSharingIntegration.t.sol @@ -265,8 +265,9 @@ contract RevenueSharingIntegration_Test is CommonTest { _operatorFees2 = bound(_operatorFees2, 0, 100 ether); _l1Fees2 = bound(_l1Fees2, 0, 100 ether); - // Handle case where first disbursement has all zero fees - if (_l1Fees == 0 && _sequencerFees == 0 && _baseFees == 0 && _operatorFees == 0) { + // Handle case where grossShare == 0 + // It is 0 when it sums up to less than 40 because of the GROSS_SHARE_BPS = 250 and BASIS_POINT_SCALE = 10_000 + if (_l1Fees + _sequencerFees + _baseFees + _operatorFees < 40) { vm.expectRevert(ISuperchainRevSharesCalculator.SharesCalculator_ZeroGrossShare.selector); superchainRevSharesCalculator.getRecipientsAndAmounts(_sequencerFees, _baseFees, _operatorFees, _l1Fees); return; @@ -351,8 +352,9 @@ contract RevenueSharingIntegration_Test is CommonTest { // ========== SECOND DISBURSEMENT ========== - // Handle case where second disbursement has all zero fees - if (_l1Fees2 == 0 && _sequencerFees2 == 0 && _baseFees2 == 0 && _operatorFees2 == 0) { + // Handle case where grossShare == 0 + // It is 0 when it sums up to less than 40 because of the GROSS_SHARE_BPS = 250 and BASIS_POINT_SCALE = 10_000 + if (_l1Fees2 + _sequencerFees2 + _baseFees2 + _operatorFees2 < 40) { vm.expectRevert(ISuperchainRevSharesCalculator.SharesCalculator_ZeroGrossShare.selector); superchainRevSharesCalculator.getRecipientsAndAmounts(_sequencerFees2, _baseFees2, _operatorFees2, _l1Fees2); return;