From 27494575824d62cac2e3461c4ebcfd54200558e3 Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Wed, 8 Jul 2026 00:27:48 -0400 Subject: [PATCH 01/10] feat(L2): add system address refunding to FeeDisburser Before bridging collected fees to L1, FeeDisburser now tops up configured L2 system addresses to their target balances, mirroring the BalanceTracker pattern on L1. Remaining balance after refunds is bridged to L1_WALLET as before. Generated with Claude Code Co-Authored-By: Claude --- src/L2/FeeDisburser.sol | 162 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 155 insertions(+), 7 deletions(-) diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index aaa8b914d..248a0ebcc 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -5,11 +5,13 @@ import { IL2StandardBridge } from "interfaces/L2/IL2StandardBridge.sol"; import { IFeeVault, Types } from "interfaces/L2/IFeeVault.sol"; import { ISemver } from "interfaces/universal/ISemver.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; +import { SafeCall } from "src/libraries/SafeCall.sol"; +import { ProxyAdminOwnedBase } from "src/universal/ProxyAdminOwnedBase.sol"; /// @custom:proxied true /// @title FeeDisburser /// @notice Withdraws funds from system FeeVault contracts and bridges to L1. -contract FeeDisburser is ISemver { +contract FeeDisburser is ProxyAdminOwnedBase, ISemver { //////////////////////////////////////////////////////////////// /// Constants //////////////////////////////////////////////////////////////// @@ -17,6 +19,9 @@ contract FeeDisburser is ISemver { /// @notice The minimum gas limit for the FeeDisburser withdrawal transaction to L1. uint32 public constant WITHDRAWAL_MIN_GAS = 35_000; + /// @notice The maximum number of system addresses that can be funded. + uint256 public constant MAX_SYSTEM_ADDRESS_COUNT = 20; + //////////////////////////////////////////////////////////////// /// Immutables //////////////////////////////////////////////////////////////// @@ -39,6 +44,15 @@ contract FeeDisburser is ISemver { /// This variable is deprecated and its value should not be relied upon. uint256 public netFeeRevenue; + /// @notice Reentrancy guard status for disburseFees. + uint256 private _disburseFeesEntered; + + /// @notice The L2 system addresses being funded. + address payable[] public systemAddresses; + + /// @notice The target balances for L2 system addresses. + uint256[] public targetBalances; + //////////////////////////////////////////////////////////////// /// Events //////////////////////////////////////////////////////////////// @@ -59,6 +73,21 @@ contract FeeDisburser is ISemver { /// @notice Emitted when no fees are collected from FeeVaults at time of disbursement. event NoFeesCollected(); + /// @notice Emitted when the FeeDisburser sends funds to a system address. + /// + /// @param systemAddress The system address being funded. + /// @param success A boolean denoting whether a fund send occurred and its success or failure. + /// @param balanceNeeded The amount of funds the given system address needs to reach its target balance. + /// @param balanceSent The amount of funds sent to the system address. + event ProcessedFunds( + address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent + ); + + /// @notice Emitted when the L2 system address refund configuration is updated. + /// + /// @param systemAddressCount The number of configured system addresses. + event SystemAddressesUpdated(uint256 systemAddressCount); + //////////////////////////////////////////////////////////////// /// Errors //////////////////////////////////////////////////////////////// @@ -78,6 +107,33 @@ contract FeeDisburser is ISemver { /// @notice Thrown when a FeeVault's recipient is not set to the FeeDisburser contract. error FeeVaultMustWithdrawToFeeDisburser(); + /// @notice Thrown when system address and target balance array lengths do not match. + error ArrayLengthMismatch(); + + /// @notice Thrown when system address configuration is empty. + error EmptySystemAddresses(); + + /// @notice Thrown when system address configuration exceeds the maximum length. + error TooManySystemAddresses(); + + /// @notice Thrown when a system address target balance is zero. + error ZeroTargetBalance(); + + /// @notice Thrown when disburseFees is reentered. + error ReentrantCall(); + + //////////////////////////////////////////////////////////////// + /// Modifiers + //////////////////////////////////////////////////////////////// + + /// @notice Prevents reentrancy into disburseFees while preserving the existing storage layout. + modifier nonReentrantDisbursement() { + if (_disburseFeesEntered != 0) revert ReentrantCall(); + _disburseFeesEntered = 1; + _; + _disburseFeesEntered = 0; + } + //////////////////////////////////////////////////////////////// /// Constructor //////////////////////////////////////////////////////////////// @@ -99,7 +155,7 @@ contract FeeDisburser is ISemver { //////////////////////////////////////////////////////////////// /// @notice Withdraws funds from FeeVaults and bridges to L1. - function disburseFees() external virtual { + function disburseFees() external virtual nonReentrantDisbursement { if (block.timestamp < lastDisbursementTime + FEE_DISBURSEMENT_INTERVAL) revert IntervalNotReached(); // Sequencer, base, and L1 FeeVaults will withdraw fees to the FeeDisburser contract. @@ -117,14 +173,45 @@ contract FeeDisburser is ISemver { return; } - lastDisbursementTime = block.timestamp; + uint256 disbursementTime = block.timestamp; + lastDisbursementTime = disbursementTime; + + _processSystemAddressRefunds(); + + uint256 bridgeBalance = address(this).balance; + if (bridgeBalance == 0) { + emit FeesDisbursed(disbursementTime, 0, 0); + return; + } // Send remaining funds to L1 wallet on L1 - IL2StandardBridge(payable(Predeploys.L2_STANDARD_BRIDGE)).bridgeETHTo{ value: address(this).balance }( + IL2StandardBridge(payable(Predeploys.L2_STANDARD_BRIDGE)).bridgeETHTo{ value: bridgeBalance }( L1_WALLET, WITHDRAWAL_MIN_GAS, bytes("") ); - emit FeesDisbursed(lastDisbursementTime, 0, feeBalance); + emit FeesDisbursed(disbursementTime, 0, bridgeBalance); + } + + /// @notice Configures the L2 system addresses to refund and their target balances. + /// + /// @dev Callable only by the ProxyAdmin owner. + /// + /// @param systemAddresses_ The system addresses being funded. + /// @param targetBalances_ The target balances for system addresses. + function setSystemAddresses( + address payable[] memory systemAddresses_, + uint256[] memory targetBalances_ + ) + external + virtual + { + _assertOnlyProxyAdminOwner(); + _validateSystemAddressConfig(systemAddresses_, targetBalances_); + + systemAddresses = systemAddresses_; + targetBalances = targetBalances_; + + emit SystemAddressesUpdated(systemAddresses_.length); } /// @notice Receives ETH fees withdrawn from L2 FeeVaults. @@ -132,9 +219,34 @@ contract FeeDisburser is ISemver { emit FeesReceived(msg.sender, msg.value); } - /// @custom:semver 1.0.0 + /// @custom:semver 1.1.0 function version() external pure virtual returns (string memory) { - return "1.0.0"; + return "1.1.0"; + } + + //////////////////////////////////////////////////////////////// + /// Internal Functions + //////////////////////////////////////////////////////////////// + + /// @notice Checks the balance of the target address and refills it back up to the target balance if needed. + /// + /// @param systemAddress The system address being funded. + /// @param targetBalance The target balance for the system address being funded. + function _refillBalanceIfNeeded(address systemAddress, uint256 targetBalance) internal virtual { + uint256 systemAddressBalance = systemAddress.balance; + if (systemAddressBalance >= targetBalance) { + emit ProcessedFunds({ systemAddress: systemAddress, success: false, balanceNeeded: 0, balanceSent: 0 }); + return; + } + + uint256 valueNeeded = targetBalance - systemAddressBalance; + uint256 feeDisburserBalance = address(this).balance; + uint256 valueToSend = valueNeeded > feeDisburserBalance ? feeDisburserBalance : valueNeeded; + + bool success = SafeCall.send({ _target: systemAddress, _gas: gasleft(), _value: valueToSend }); + emit ProcessedFunds({ + systemAddress: systemAddress, success: success, balanceNeeded: valueNeeded, balanceSent: valueToSend + }); } //////////////////////////////////////////////////////////////// @@ -155,4 +267,40 @@ contract FeeDisburser is ISemver { IFeeVault(feeVault).withdraw(); } } + + /// @notice Refills configured system addresses up to their target balances. + function _processSystemAddressRefunds() private { + uint256 systemAddressesLength = systemAddresses.length; + for (uint256 i; i < systemAddressesLength;) { + _refillBalanceIfNeeded({ systemAddress: systemAddresses[i], targetBalance: targetBalances[i] }); + unchecked { + i++; + } + } + } + + /// @notice Validates system address refund configuration. + /// + /// @param systemAddresses_ The system addresses being funded. + /// @param targetBalances_ The target balances for system addresses. + function _validateSystemAddressConfig( + address payable[] memory systemAddresses_, + uint256[] memory targetBalances_ + ) + private + pure + { + uint256 systemAddressesLength = systemAddresses_.length; + if (systemAddressesLength == 0) revert EmptySystemAddresses(); + if (systemAddressesLength > MAX_SYSTEM_ADDRESS_COUNT) revert TooManySystemAddresses(); + if (systemAddressesLength != targetBalances_.length) revert ArrayLengthMismatch(); + + for (uint256 i; i < systemAddressesLength;) { + if (systemAddresses_[i] == address(0)) revert ZeroAddress(); + if (targetBalances_[i] == 0) revert ZeroTargetBalance(); + unchecked { + i++; + } + } + } } From 9f84e3fa848dd2123f1fb5c8fc6728f06bda74a6 Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Thu, 9 Jul 2026 15:35:04 -0400 Subject: [PATCH 02/10] test(L2): add tests for FeeDisburser system address refunding Adds tests covering the new setSystemAddresses function, disburseFees with system address refunds, and reentrancy guard. Also fixes setSystemAddresses access control to use _assertOnlyProxyAdmin() instead of _assertOnlyProxyAdminOwner() so the L2 alias (raw EIP-1967 admin slot) can call it directly via deposit tx without requiring a ProxyAdmin contract. Generated with Claude Code Co-Authored-By: Claude --- src/L2/FeeDisburser.sol | 2 +- test/L2/FeeDisburser.t.sol | 298 +++++++++++++++++++++++++++++++++++++ 2 files changed, 299 insertions(+), 1 deletion(-) diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index 248a0ebcc..f48a57ba0 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -205,7 +205,7 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { external virtual { - _assertOnlyProxyAdminOwner(); + _assertOnlyProxyAdmin(); _validateSystemAddressConfig(systemAddresses_, targetBalances_); systemAddresses = systemAddresses_; diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 75287a33d..1f10cd661 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -6,7 +6,21 @@ import { Test } from "lib/forge-std/src/Test.sol"; import { FeeDisburser } from "src/L2/FeeDisburser.sol"; import { IFeeVault, Types } from "interfaces/L2/IFeeVault.sol"; import { IStandardBridge } from "interfaces/universal/IStandardBridge.sol"; +import { ProxyAdminOwnedBase } from "src/universal/ProxyAdminOwnedBase.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; +import { Constants } from "src/libraries/Constants.sol"; + +contract ReentrantReceiver { + FeeDisburser public target; + + constructor(FeeDisburser _target) { + target = _target; + } + + receive() external payable { + target.disburseFees(); + } +} /// @title FeeDisburserTest /// @notice Comprehensive unit and fuzz tests for the FeeDisburser contract @@ -15,6 +29,8 @@ contract FeeDisburserTest is Test { event FeesDisbursed(uint256 disbursementTime, uint256 deprecated, uint256 totalFeesDisbursed); event FeesReceived(address indexed sender, uint256 amount); event NoFeesCollected(); + event ProcessedFunds(address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent); + event SystemAddressesUpdated(uint256 systemAddressCount); // Constants uint32 constant WITHDRAWAL_MIN_GAS = 35_000; @@ -26,6 +42,8 @@ contract FeeDisburserTest is Test { address payable constant L1_WALLET = payable(address(0x1001)); address constant ALICE = address(0xA11CE); address constant BOB = address(0xB0B); + address constant PROXY_ADMIN = address(0xAD1); + address payable constant SYSTEM_ADDR = payable(address(0x5001)); // Contract instances FeeDisburser feeDisburser; @@ -97,6 +115,16 @@ contract FeeDisburserTest is Test { emit NoFeesCollected(); } + function _setProxyAdmin() internal { + vm.store(address(feeDisburser), Constants.PROXY_OWNER_ADDRESS, bytes32(uint256(uint160(PROXY_ADMIN)))); + } + + function _setSystemAddresses(address payable[] memory addrs, uint256[] memory balances) internal { + _setProxyAdmin(); + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs, balances); + } + // ============================================================ // Constructor Tests // ============================================================ @@ -768,4 +796,274 @@ contract FeeDisburserTest is Test { assertTrue(gasUsed < 200_000, "Gas usage too high for all-vault case"); } + + // ============================================================ + // setSystemAddresses Tests + // ============================================================ + + function test_setSystemAddresses_success() public { + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = 1 ether; + + _setProxyAdmin(); + + vm.expectEmit(address(feeDisburser)); + emit SystemAddressesUpdated(1); + + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs, balances); + + assertEq(feeDisburser.systemAddresses(0), SYSTEM_ADDR); + assertEq(feeDisburser.targetBalances(0), 1 ether); + } + + function test_setSystemAddresses_success_overwrites() public { + address payable[] memory addrs1 = new address payable[](1); + addrs1[0] = SYSTEM_ADDR; + uint256[] memory balances1 = new uint256[](1); + balances1[0] = 1 ether; + _setSystemAddresses(addrs1, balances1); + + address payable[] memory addrs2 = new address payable[](1); + addrs2[0] = payable(ALICE); + uint256[] memory balances2 = new uint256[](1); + balances2[0] = 2 ether; + + vm.expectEmit(address(feeDisburser)); + emit SystemAddressesUpdated(1); + + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs2, balances2); + + assertEq(feeDisburser.systemAddresses(0), ALICE); + assertEq(feeDisburser.targetBalances(0), 2 ether); + } + + function test_setSystemAddresses_revert_notProxyAdmin() public { + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = 1 ether; + + _setProxyAdmin(); + + vm.expectRevert(ProxyAdminOwnedBase.ProxyAdminOwnedBase_NotProxyAdmin.selector); + vm.prank(ALICE); + feeDisburser.setSystemAddresses(addrs, balances); + } + + function test_setSystemAddresses_revert_emptySystemAddresses() public { + _setProxyAdmin(); + + vm.expectRevert(FeeDisburser.EmptySystemAddresses.selector); + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(new address payable[](0), new uint256[](0)); + } + + function test_setSystemAddresses_revert_tooManySystemAddresses() public { + uint256 n = feeDisburser.MAX_SYSTEM_ADDRESS_COUNT() + 1; + address payable[] memory addrs = new address payable[](n); + uint256[] memory balances = new uint256[](n); + for (uint256 i; i < n; i++) { + addrs[i] = payable(address(uint160(0x6000 + i))); + balances[i] = 1 ether; + } + + _setProxyAdmin(); + + vm.expectRevert(FeeDisburser.TooManySystemAddresses.selector); + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs, balances); + } + + function test_setSystemAddresses_revert_arrayLengthMismatch() public { + address payable[] memory addrs = new address payable[](2); + addrs[0] = SYSTEM_ADDR; + addrs[1] = payable(ALICE); + uint256[] memory balances = new uint256[](1); + balances[0] = 1 ether; + + _setProxyAdmin(); + + vm.expectRevert(FeeDisburser.ArrayLengthMismatch.selector); + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs, balances); + } + + function test_setSystemAddresses_revert_zeroAddress() public { + address payable[] memory addrs = new address payable[](1); + addrs[0] = payable(address(0)); + uint256[] memory balances = new uint256[](1); + balances[0] = 1 ether; + + _setProxyAdmin(); + + vm.expectRevert(FeeDisburser.ZeroAddress.selector); + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs, balances); + } + + function test_setSystemAddresses_revert_zeroTargetBalance() public { + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = 0; + + _setProxyAdmin(); + + vm.expectRevert(FeeDisburser.ZeroTargetBalance.selector); + vm.prank(PROXY_ADMIN); + feeDisburser.setSystemAddresses(addrs, balances); + } + + // ============================================================ + // disburseFees with System Address Refunds + // ============================================================ + + function test_disburseFees_success_systemAddressRefunded() public { + uint256 feeAmount = 10 ether; + uint256 targetBal = 2 ether; + + vm.deal(SYSTEM_ADDR, 0); + + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = targetBal; + _setSystemAddresses(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + uint256 bridgeAmount = feeAmount - targetBal; + _expectBridgeETH(bridgeAmount); + + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(SYSTEM_ADDR, true, targetBal, targetBal); + + _expectFeesDisbursed(bridgeAmount); + + feeDisburser.disburseFees(); + + assertEq(SYSTEM_ADDR.balance, targetBal); + } + + function test_disburseFees_success_systemAddressAlreadyFunded() public { + uint256 feeAmount = 10 ether; + uint256 targetBal = 2 ether; + + vm.deal(SYSTEM_ADDR, targetBal); + + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = targetBal; + _setSystemAddresses(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + _expectBridgeETH(feeAmount); + + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(SYSTEM_ADDR, false, 0, 0); + + _expectFeesDisbursed(feeAmount); + + feeDisburser.disburseFees(); + + assertEq(SYSTEM_ADDR.balance, targetBal); + } + + function test_disburseFees_success_allFundsConsumedByRefund() public { + uint256 feeAmount = 5 ether; + uint256 targetBal = 10 ether; + + vm.deal(SYSTEM_ADDR, 0); + + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = targetBal; + _setSystemAddresses(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + // No bridge call — balanceSent capped at available feeAmount + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(SYSTEM_ADDR, true, targetBal, feeAmount); + + _expectFeesDisbursed(0); + + feeDisburser.disburseFees(); + + assertEq(SYSTEM_ADDR.balance, feeAmount); + assertEq(address(feeDisburser).balance, 0); + } + + function test_disburseFees_success_partialRefundDueToLowBalance() public { + uint256 targetBal = 5 ether; + uint256 systemAddrStartBal = 1 ether; + uint256 feeAmount = 2 ether; // not enough to fully top up (needs 4 ether, has 2) + + vm.deal(SYSTEM_ADDR, systemAddrStartBal); + + address payable[] memory addrs = new address payable[](1); + addrs[0] = SYSTEM_ADDR; + uint256[] memory balances = new uint256[](1); + balances[0] = targetBal; + _setSystemAddresses(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + uint256 valueNeeded = targetBal - systemAddrStartBal; // 4 ether + uint256 valueSent = feeAmount; // only 2 ether available + + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(SYSTEM_ADDR, true, valueNeeded, valueSent); + + _expectFeesDisbursed(0); + + feeDisburser.disburseFees(); + + assertEq(SYSTEM_ADDR.balance, systemAddrStartBal + valueSent); + assertEq(address(feeDisburser).balance, 0); + } + + // ============================================================ + // Reentrancy Test + // ============================================================ + + function test_disburseFees_reentrancy_innerCallBlocked() public { + ReentrantReceiver attacker = new ReentrantReceiver(feeDisburser); + + address payable[] memory addrs = new address payable[](1); + addrs[0] = payable(address(attacker)); + uint256[] memory balances = new uint256[](1); + balances[0] = 1 ether; + _setSystemAddresses(addrs, balances); + + uint256 feeAmount = 2 ether; + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + // Inner disburseFees reverts (reentrancy), so SafeCall returns success=false and ETH is not sent + _expectBridgeETH(feeAmount); + + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(address(attacker), false, 1 ether, 1 ether); + + feeDisburser.disburseFees(); + + assertEq(address(attacker).balance, 0); + } } From 23831072971b3fabd62e9996049dd43972ffbc51 Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Thu, 9 Jul 2026 15:39:05 -0400 Subject: [PATCH 03/10] refactor(L2): simplify FeeDisburser by inlining private helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inline _processSystemAddressRefunds into disburseFees and _validateSystemAddressConfig into setSystemAddresses — both were private, single-caller wrappers. Add _makeSingleConfig test helper to collapse repeated 4-line array setup. Drop no-op vm.deal(addr, 0) calls. FeeDisburser now matches BalanceTracker's pattern on L1. Generated with Claude Code Co-Authored-By: Claude --- src/L2/FeeDisburser.sol | 59 +++++++++++------------------- test/L2/FeeDisburser.t.sol | 74 ++++++++++++++------------------------ 2 files changed, 47 insertions(+), 86 deletions(-) diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index f48a57ba0..f0518fd16 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -176,7 +176,13 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { uint256 disbursementTime = block.timestamp; lastDisbursementTime = disbursementTime; - _processSystemAddressRefunds(); + uint256 systemAddressesLength = systemAddresses.length; + for (uint256 i; i < systemAddressesLength;) { + _refillBalanceIfNeeded({ systemAddress: systemAddresses[i], targetBalance: targetBalances[i] }); + unchecked { + i++; + } + } uint256 bridgeBalance = address(this).balance; if (bridgeBalance == 0) { @@ -206,12 +212,24 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { virtual { _assertOnlyProxyAdmin(); - _validateSystemAddressConfig(systemAddresses_, targetBalances_); + + uint256 systemAddressesLength = systemAddresses_.length; + if (systemAddressesLength == 0) revert EmptySystemAddresses(); + if (systemAddressesLength > MAX_SYSTEM_ADDRESS_COUNT) revert TooManySystemAddresses(); + if (systemAddressesLength != targetBalances_.length) revert ArrayLengthMismatch(); + + for (uint256 i; i < systemAddressesLength;) { + if (systemAddresses_[i] == address(0)) revert ZeroAddress(); + if (targetBalances_[i] == 0) revert ZeroTargetBalance(); + unchecked { + i++; + } + } systemAddresses = systemAddresses_; targetBalances = targetBalances_; - emit SystemAddressesUpdated(systemAddresses_.length); + emit SystemAddressesUpdated(systemAddressesLength); } /// @notice Receives ETH fees withdrawn from L2 FeeVaults. @@ -268,39 +286,4 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { } } - /// @notice Refills configured system addresses up to their target balances. - function _processSystemAddressRefunds() private { - uint256 systemAddressesLength = systemAddresses.length; - for (uint256 i; i < systemAddressesLength;) { - _refillBalanceIfNeeded({ systemAddress: systemAddresses[i], targetBalance: targetBalances[i] }); - unchecked { - i++; - } - } - } - - /// @notice Validates system address refund configuration. - /// - /// @param systemAddresses_ The system addresses being funded. - /// @param targetBalances_ The target balances for system addresses. - function _validateSystemAddressConfig( - address payable[] memory systemAddresses_, - uint256[] memory targetBalances_ - ) - private - pure - { - uint256 systemAddressesLength = systemAddresses_.length; - if (systemAddressesLength == 0) revert EmptySystemAddresses(); - if (systemAddressesLength > MAX_SYSTEM_ADDRESS_COUNT) revert TooManySystemAddresses(); - if (systemAddressesLength != targetBalances_.length) revert ArrayLengthMismatch(); - - for (uint256 i; i < systemAddressesLength;) { - if (systemAddresses_[i] == address(0)) revert ZeroAddress(); - if (targetBalances_[i] == 0) revert ZeroTargetBalance(); - unchecked { - i++; - } - } - } } diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 1f10cd661..5a8fac827 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -125,6 +125,20 @@ contract FeeDisburserTest is Test { feeDisburser.setSystemAddresses(addrs, balances); } + function _makeSingleConfig( + address payable addr, + uint256 balance + ) + internal + pure + returns (address payable[] memory addrs, uint256[] memory balances) + { + addrs = new address payable[](1); + addrs[0] = addr; + balances = new uint256[](1); + balances[0] = balance; + } + // ============================================================ // Constructor Tests // ============================================================ @@ -802,10 +816,7 @@ contract FeeDisburserTest is Test { // ============================================================ function test_setSystemAddresses_success() public { - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = 1 ether; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); _setProxyAdmin(); @@ -820,16 +831,10 @@ contract FeeDisburserTest is Test { } function test_setSystemAddresses_success_overwrites() public { - address payable[] memory addrs1 = new address payable[](1); - addrs1[0] = SYSTEM_ADDR; - uint256[] memory balances1 = new uint256[](1); - balances1[0] = 1 ether; + (address payable[] memory addrs1, uint256[] memory balances1) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); _setSystemAddresses(addrs1, balances1); - address payable[] memory addrs2 = new address payable[](1); - addrs2[0] = payable(ALICE); - uint256[] memory balances2 = new uint256[](1); - balances2[0] = 2 ether; + (address payable[] memory addrs2, uint256[] memory balances2) = _makeSingleConfig(payable(ALICE), 2 ether); vm.expectEmit(address(feeDisburser)); emit SystemAddressesUpdated(1); @@ -842,10 +847,7 @@ contract FeeDisburserTest is Test { } function test_setSystemAddresses_revert_notProxyAdmin() public { - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = 1 ether; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); _setProxyAdmin(); @@ -893,10 +895,7 @@ contract FeeDisburserTest is Test { } function test_setSystemAddresses_revert_zeroAddress() public { - address payable[] memory addrs = new address payable[](1); - addrs[0] = payable(address(0)); - uint256[] memory balances = new uint256[](1); - balances[0] = 1 ether; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(0)), 1 ether); _setProxyAdmin(); @@ -906,10 +905,7 @@ contract FeeDisburserTest is Test { } function test_setSystemAddresses_revert_zeroTargetBalance() public { - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = 0; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 0); _setProxyAdmin(); @@ -926,12 +922,7 @@ contract FeeDisburserTest is Test { uint256 feeAmount = 10 ether; uint256 targetBal = 2 ether; - vm.deal(SYSTEM_ADDR, 0); - - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = targetBal; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); _setSystemAddresses(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -957,10 +948,7 @@ contract FeeDisburserTest is Test { vm.deal(SYSTEM_ADDR, targetBal); - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = targetBal; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); _setSystemAddresses(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -983,12 +971,7 @@ contract FeeDisburserTest is Test { uint256 feeAmount = 5 ether; uint256 targetBal = 10 ether; - vm.deal(SYSTEM_ADDR, 0); - - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = targetBal; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); _setSystemAddresses(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -1014,10 +997,7 @@ contract FeeDisburserTest is Test { vm.deal(SYSTEM_ADDR, systemAddrStartBal); - address payable[] memory addrs = new address payable[](1); - addrs[0] = SYSTEM_ADDR; - uint256[] memory balances = new uint256[](1); - balances[0] = targetBal; + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); _setSystemAddresses(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -1045,10 +1025,8 @@ contract FeeDisburserTest is Test { function test_disburseFees_reentrancy_innerCallBlocked() public { ReentrantReceiver attacker = new ReentrantReceiver(feeDisburser); - address payable[] memory addrs = new address payable[](1); - addrs[0] = payable(address(attacker)); - uint256[] memory balances = new uint256[](1); - balances[0] = 1 ether; + (address payable[] memory addrs, uint256[] memory balances) = + _makeSingleConfig(payable(address(attacker)), 1 ether); _setSystemAddresses(addrs, balances); uint256 feeAmount = 2 ether; From 9589b6f486ed669f73f8cb6a63c21a3f90c4872e Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Thu, 9 Jul 2026 15:49:35 -0400 Subject: [PATCH 04/10] fix(FeeDisburser): address code review findings from PR #358 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix ProcessedFunds NatSpec: clarify balanceSent is amount attempted, not received - Remove EmptySystemAddresses error; allow empty arrays to clear config - Change reentrancy guard from 0/1/0 to 1/2/1 to keep slot warm after first call - Fix setSystemAddresses NatSpec: "ProxyAdmin owner" → "ProxyAdmin" - Remove virtual from _refillBalanceIfNeeded (matches BalanceTracker pattern) - Add RevertingReceiver helper and tests for clearing config, multiple system addresses ordering, and reverting recipients Generated with Claude Code Co-Authored-By: Claude --- src/L2/FeeDisburser.sol | 20 +++++----- test/L2/FeeDisburser.t.sol | 82 ++++++++++++++++++++++++++++++++++++-- 2 files changed, 89 insertions(+), 13 deletions(-) diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index f0518fd16..68525e263 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -78,7 +78,8 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { /// @param systemAddress The system address being funded. /// @param success A boolean denoting whether a fund send occurred and its success or failure. /// @param balanceNeeded The amount of funds the given system address needs to reach its target balance. - /// @param balanceSent The amount of funds sent to the system address. + /// @param balanceSent The amount of funds attempted to be sent. When success is false, the + /// recipient rejected the transfer and no funds were actually received. event ProcessedFunds( address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent ); @@ -110,9 +111,6 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { /// @notice Thrown when system address and target balance array lengths do not match. error ArrayLengthMismatch(); - /// @notice Thrown when system address configuration is empty. - error EmptySystemAddresses(); - /// @notice Thrown when system address configuration exceeds the maximum length. error TooManySystemAddresses(); @@ -127,11 +125,13 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { //////////////////////////////////////////////////////////////// /// @notice Prevents reentrancy into disburseFees while preserving the existing storage layout. + /// Uses 1/2 sentinel values so the slot stays nonzero after first use, keeping + /// subsequent SSTOREs warm. Uninitialized (0) is treated as not-entered. modifier nonReentrantDisbursement() { - if (_disburseFeesEntered != 0) revert ReentrantCall(); - _disburseFeesEntered = 1; + if (_disburseFeesEntered == 2) revert ReentrantCall(); + _disburseFeesEntered = 2; _; - _disburseFeesEntered = 0; + _disburseFeesEntered = 1; } //////////////////////////////////////////////////////////////// @@ -199,8 +199,9 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { } /// @notice Configures the L2 system addresses to refund and their target balances. + /// Passing empty arrays clears the configuration and disables refunding. /// - /// @dev Callable only by the ProxyAdmin owner. + /// @dev Callable only by the ProxyAdmin. /// /// @param systemAddresses_ The system addresses being funded. /// @param targetBalances_ The target balances for system addresses. @@ -214,7 +215,6 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { _assertOnlyProxyAdmin(); uint256 systemAddressesLength = systemAddresses_.length; - if (systemAddressesLength == 0) revert EmptySystemAddresses(); if (systemAddressesLength > MAX_SYSTEM_ADDRESS_COUNT) revert TooManySystemAddresses(); if (systemAddressesLength != targetBalances_.length) revert ArrayLengthMismatch(); @@ -250,7 +250,7 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { /// /// @param systemAddress The system address being funded. /// @param targetBalance The target balance for the system address being funded. - function _refillBalanceIfNeeded(address systemAddress, uint256 targetBalance) internal virtual { + function _refillBalanceIfNeeded(address systemAddress, uint256 targetBalance) internal { uint256 systemAddressBalance = systemAddress.balance; if (systemAddressBalance >= targetBalance) { emit ProcessedFunds({ systemAddress: systemAddress, success: false, balanceNeeded: 0, balanceSent: 0 }); diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 5a8fac827..5adff6afb 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -22,6 +22,12 @@ contract ReentrantReceiver { } } +contract RevertingReceiver { + receive() external payable { + revert(); + } +} + /// @title FeeDisburserTest /// @notice Comprehensive unit and fuzz tests for the FeeDisburser contract contract FeeDisburserTest is Test { @@ -856,12 +862,21 @@ contract FeeDisburserTest is Test { feeDisburser.setSystemAddresses(addrs, balances); } - function test_setSystemAddresses_revert_emptySystemAddresses() public { - _setProxyAdmin(); + function test_setSystemAddresses_success_clearConfig() public { + // First set some addresses + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); + _setSystemAddresses(addrs, balances); + assertEq(feeDisburser.systemAddresses(0), SYSTEM_ADDR); + + // Clear config with empty arrays + vm.expectEmit(address(feeDisburser)); + emit SystemAddressesUpdated(0); - vm.expectRevert(FeeDisburser.EmptySystemAddresses.selector); vm.prank(PROXY_ADMIN); feeDisburser.setSystemAddresses(new address payable[](0), new uint256[](0)); + + vm.expectRevert(); + feeDisburser.systemAddresses(0); } function test_setSystemAddresses_revert_tooManySystemAddresses() public { @@ -1018,6 +1033,67 @@ contract FeeDisburserTest is Test { assertEq(address(feeDisburser).balance, 0); } + function test_disburseFees_success_multipleSystemAddresses() public { + address payable addr1 = payable(address(0x6001)); + address payable addr2 = payable(address(0x6002)); + uint256 target1 = 3 ether; + uint256 target2 = 2 ether; + + address payable[] memory addrs = new address payable[](2); + addrs[0] = addr1; + addrs[1] = addr2; + uint256[] memory bals = new uint256[](2); + bals[0] = target1; + bals[1] = target2; + _setSystemAddresses(addrs, bals); + + uint256 feeAmount = 10 ether; + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + uint256 bridgeAmount = feeAmount - target1 - target2; + _expectBridgeETH(bridgeAmount); + + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(addr1, true, target1, target1); + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(addr2, true, target2, target2); + + _expectFeesDisbursed(bridgeAmount); + + feeDisburser.disburseFees(); + + assertEq(addr1.balance, target1); + assertEq(addr2.balance, target2); + assertEq(address(feeDisburser).balance, 0); + } + + function test_disburseFees_success_revertingRecipient() public { + RevertingReceiver bad = new RevertingReceiver(); + + (address payable[] memory addrs, uint256[] memory balances) = + _makeSingleConfig(payable(address(bad)), 1 ether); + _setSystemAddresses(addrs, balances); + + uint256 feeAmount = 2 ether; + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + vm.deal(Predeploys.BASE_FEE_VAULT, 0); + vm.deal(Predeploys.L1_FEE_VAULT, 0); + + // SafeCall.send catches the revert; full fee amount still bridges + _expectBridgeETH(feeAmount); + + vm.expectEmit(address(feeDisburser)); + emit ProcessedFunds(address(bad), false, 1 ether, 1 ether); + + _expectFeesDisbursed(feeAmount); + + feeDisburser.disburseFees(); + + assertEq(address(bad).balance, 0); + } + // ============================================================ // Reentrancy Test // ============================================================ From 76a12848c20ae44c50e2ca8fbd705e118f1bcca1 Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Thu, 9 Jul 2026 16:10:25 -0400 Subject: [PATCH 05/10] chore: forge fmt Generated with Claude Code Co-Authored-By: Claude --- scripts/L2Genesis.s.sol | 6 ++-- scripts/universal/MultisigScript.sol | 54 ++++++++++++++-------------- src/L2/FeeDisburser.sol | 1 - test/L2/FeeDisburser.t.sol | 7 ++-- 4 files changed, 33 insertions(+), 35 deletions(-) diff --git a/scripts/L2Genesis.s.sol b/scripts/L2Genesis.s.sol index bf1412b4e..681697d85 100644 --- a/scripts/L2Genesis.s.sol +++ b/scripts/L2Genesis.s.sol @@ -393,10 +393,8 @@ contract L2Genesis is Script { // Initialize the predeploy IFeeVault(payable(_vaultAddr)) .initialize({ - _recipient: _recipient, - _minWithdrawalAmount: _minWithdrawalAmount, - _withdrawalNetwork: _withdrawalNetwork - }); + _recipient: _recipient, _minWithdrawalAmount: _minWithdrawalAmount, _withdrawalNetwork: _withdrawalNetwork + }); vm.stopPrank(); } diff --git a/scripts/universal/MultisigScript.sol b/scripts/universal/MultisigScript.sol index 6556cef84..28ad4d4d0 100644 --- a/scripts/universal/MultisigScript.sol +++ b/scripts/universal/MultisigScript.sol @@ -785,17 +785,17 @@ abstract contract MultisigScript is Script { function _encodeTransactionData(address safe, Call memory call) internal view returns (bytes memory) { return IGnosisSafe(safe) .encodeTransactionData({ - to: call.target, - value: call.value, - data: call.data, - operation: call.operation, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: address(0), - _nonce: _getNonce(safe) - }); + to: call.target, + value: call.value, + data: call.data, + operation: call.operation, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: address(0), + _nonce: _getNonce(safe) + }); } /// @notice Encodes the transaction as EIP-712 structured JSON for hardware wallet signing. @@ -850,11 +850,11 @@ abstract contract MultisigScript is Script { IGnosisSafe(safe) .checkSignatures({ - dataHash: hash, - data: _encodeTransactionData({ safe: safe, call: call }), // NOTE: This field is the data preimage but - // not strictly required as `checkSignatures` ignores it. - signatures: signatures - }); + dataHash: hash, + data: _encodeTransactionData({ safe: safe, call: call }), // NOTE: This field is the data preimage but + // not strictly required as `checkSignatures` ignores it. + signatures: signatures + }); } /// @notice Gets the transaction hash for the given safe and call. @@ -928,17 +928,17 @@ abstract contract MultisigScript is Script { return IGnosisSafe(safe) .execTransaction({ - to: call.target, - value: call.value, - data: call.data, - operation: call.operation, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: signatures - }); + to: call.target, + value: call.value, + data: call.data, + operation: call.operation, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: signatures + }); } /// @notice Gets the type for the given call. diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index 68525e263..79307d123 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -285,5 +285,4 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { IFeeVault(feeVault).withdraw(); } } - } diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 5adff6afb..5dcfe3911 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -35,7 +35,9 @@ contract FeeDisburserTest is Test { event FeesDisbursed(uint256 disbursementTime, uint256 deprecated, uint256 totalFeesDisbursed); event FeesReceived(address indexed sender, uint256 amount); event NoFeesCollected(); - event ProcessedFunds(address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent); + event ProcessedFunds( + address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent + ); event SystemAddressesUpdated(uint256 systemAddressCount); // Constants @@ -1072,8 +1074,7 @@ contract FeeDisburserTest is Test { function test_disburseFees_success_revertingRecipient() public { RevertingReceiver bad = new RevertingReceiver(); - (address payable[] memory addrs, uint256[] memory balances) = - _makeSingleConfig(payable(address(bad)), 1 ether); + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(bad)), 1 ether); _setSystemAddresses(addrs, balances); uint256 feeAmount = 2 ether; From bd5133a7fbac7b26c154eedebec9e9ae5d6d0167 Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Thu, 9 Jul 2026 16:15:54 -0400 Subject: [PATCH 06/10] revert: restore L2Genesis and MultisigScript to pre-fmt state These files are not part of this PR; the fmt changes are noise. Generated with Claude Code Co-Authored-By: Claude --- scripts/L2Genesis.s.sol | 6 ++-- scripts/universal/MultisigScript.sol | 54 ++++++++++++++-------------- 2 files changed, 31 insertions(+), 29 deletions(-) diff --git a/scripts/L2Genesis.s.sol b/scripts/L2Genesis.s.sol index 681697d85..bf1412b4e 100644 --- a/scripts/L2Genesis.s.sol +++ b/scripts/L2Genesis.s.sol @@ -393,8 +393,10 @@ contract L2Genesis is Script { // Initialize the predeploy IFeeVault(payable(_vaultAddr)) .initialize({ - _recipient: _recipient, _minWithdrawalAmount: _minWithdrawalAmount, _withdrawalNetwork: _withdrawalNetwork - }); + _recipient: _recipient, + _minWithdrawalAmount: _minWithdrawalAmount, + _withdrawalNetwork: _withdrawalNetwork + }); vm.stopPrank(); } diff --git a/scripts/universal/MultisigScript.sol b/scripts/universal/MultisigScript.sol index 28ad4d4d0..6556cef84 100644 --- a/scripts/universal/MultisigScript.sol +++ b/scripts/universal/MultisigScript.sol @@ -785,17 +785,17 @@ abstract contract MultisigScript is Script { function _encodeTransactionData(address safe, Call memory call) internal view returns (bytes memory) { return IGnosisSafe(safe) .encodeTransactionData({ - to: call.target, - value: call.value, - data: call.data, - operation: call.operation, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: address(0), - _nonce: _getNonce(safe) - }); + to: call.target, + value: call.value, + data: call.data, + operation: call.operation, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: address(0), + _nonce: _getNonce(safe) + }); } /// @notice Encodes the transaction as EIP-712 structured JSON for hardware wallet signing. @@ -850,11 +850,11 @@ abstract contract MultisigScript is Script { IGnosisSafe(safe) .checkSignatures({ - dataHash: hash, - data: _encodeTransactionData({ safe: safe, call: call }), // NOTE: This field is the data preimage but - // not strictly required as `checkSignatures` ignores it. - signatures: signatures - }); + dataHash: hash, + data: _encodeTransactionData({ safe: safe, call: call }), // NOTE: This field is the data preimage but + // not strictly required as `checkSignatures` ignores it. + signatures: signatures + }); } /// @notice Gets the transaction hash for the given safe and call. @@ -928,17 +928,17 @@ abstract contract MultisigScript is Script { return IGnosisSafe(safe) .execTransaction({ - to: call.target, - value: call.value, - data: call.data, - operation: call.operation, - safeTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: address(0), - refundReceiver: payable(address(0)), - signatures: signatures - }); + to: call.target, + value: call.value, + data: call.data, + operation: call.operation, + safeTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: address(0), + refundReceiver: payable(address(0)), + signatures: signatures + }); } /// @notice Gets the type for the given call. From 7d73adfd002031f72e688c8a874a75cd691bf95c Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Thu, 9 Jul 2026 23:44:59 -0400 Subject: [PATCH 07/10] fix(FeeDisburser): address Jack's review comments - Replace setSystemAddresses with initialize(reinitializer(2)) so config is set atomically via upgradeAndCall, resolving P2 access control issue - Regenerate ABI, storage layout, and semver-lock snapshots (P1) - Simplify ReentrantReceiver to be stateless (use msg.sender in receive) - Replace RevertingReceiver helper with vm.mockCallRevert - Remove redundant vm.deal(vault, 0) lines in refund tests (fresh state is 0) - Delete test_disburseFees_success_allFundsConsumedByRefund (covered by the more general partial refund test) Generated with Claude Code Co-Authored-By: Claude --- snapshots/abi/FeeDisburser.json | 212 ++++++++++++++++++++++ snapshots/semver-lock.json | 4 +- snapshots/storageLayout/FeeDisburser.json | 21 +++ src/L2/FeeDisburser.sol | 13 +- test/L2/FeeDisburser.t.sol | 163 +++-------------- 5 files changed, 269 insertions(+), 144 deletions(-) diff --git a/snapshots/abi/FeeDisburser.json b/snapshots/abi/FeeDisburser.json index 4dfa787a5..c8dfbfed2 100644 --- a/snapshots/abi/FeeDisburser.json +++ b/snapshots/abi/FeeDisburser.json @@ -45,6 +45,19 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [], + "name": "MAX_SYSTEM_ADDRESS_COUNT", + "outputs": [ + { + "internalType": "uint256", + "name": "", + "type": "uint256" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [], "name": "WITHDRAWAL_MIN_GAS", @@ -65,6 +78,24 @@ "stateMutability": "nonpayable", "type": "function" }, + { + "inputs": [ + { + "internalType": "address payable[]", + "name": "systemAddresses_", + "type": "address[]" + }, + { + "internalType": "uint256[]", + "name": "targetBalances_", + "type": "uint256[]" + } + ], + "name": "initialize", + "outputs": [], + "stateMutability": "nonpayable", + "type": "function" + }, { "inputs": [], "name": "lastDisbursementTime", @@ -91,6 +122,70 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [], + "name": "proxyAdmin", + "outputs": [ + { + "internalType": "contract IProxyAdmin", + "name": "", + "type": "address" + } + ], + "stateMutability": "view", + "type": "function" + }, + { + "inputs": [], + "name": "proxyAdminOwner", + "outputs": [ + { + "internalType": "address", + "name": "", + "type": "address" + } + ], + "stateMutability": "view", + "type": "function" + }, + { + "inputs": [ + { + "internalType": "uint256", + "name": "", + "type": "uint256" + } + ], + "name": "systemAddresses", + "outputs": [ + { + "internalType": "address payable", + "name": "", + "type": "address" + } + ], + "stateMutability": "view", + "type": "function" + }, + { + "inputs": [ + { + "internalType": "uint256", + "name": "", + "type": "uint256" + } + ], + "name": "targetBalances", + "outputs": [ + { + "internalType": "uint256", + "name": "", + "type": "uint256" + } + ], + "stateMutability": "view", + "type": "function" + }, { "inputs": [], "name": "version", @@ -148,12 +243,74 @@ "name": "FeesReceived", "type": "event" }, + { + "anonymous": false, + "inputs": [ + { + "indexed": false, + "internalType": "uint64", + "name": "version", + "type": "uint64" + } + ], + "name": "Initialized", + "type": "event" + }, { "anonymous": false, "inputs": [], "name": "NoFeesCollected", "type": "event" }, + { + "anonymous": false, + "inputs": [ + { + "indexed": true, + "internalType": "address", + "name": "systemAddress", + "type": "address" + }, + { + "indexed": true, + "internalType": "bool", + "name": "success", + "type": "bool" + }, + { + "indexed": false, + "internalType": "uint256", + "name": "balanceNeeded", + "type": "uint256" + }, + { + "indexed": false, + "internalType": "uint256", + "name": "balanceSent", + "type": "uint256" + } + ], + "name": "ProcessedFunds", + "type": "event" + }, + { + "anonymous": false, + "inputs": [ + { + "indexed": false, + "internalType": "uint256", + "name": "systemAddressCount", + "type": "uint256" + } + ], + "name": "SystemAddressesUpdated", + "type": "event" + }, + { + "inputs": [], + "name": "ArrayLengthMismatch", + "type": "error" + }, { "inputs": [], "name": "FeeVaultMustWithdrawToFeeDisburser", @@ -174,9 +331,64 @@ "name": "IntervalTooLow", "type": "error" }, + { + "inputs": [], + "name": "InvalidInitialization", + "type": "error" + }, + { + "inputs": [], + "name": "NotInitializing", + "type": "error" + }, + { + "inputs": [], + "name": "ProxyAdminOwnedBase_NotProxyAdmin", + "type": "error" + }, + { + "inputs": [], + "name": "ProxyAdminOwnedBase_NotProxyAdminOrProxyAdminOwner", + "type": "error" + }, + { + "inputs": [], + "name": "ProxyAdminOwnedBase_NotProxyAdminOwner", + "type": "error" + }, + { + "inputs": [], + "name": "ProxyAdminOwnedBase_NotResolvedDelegateProxy", + "type": "error" + }, + { + "inputs": [], + "name": "ProxyAdminOwnedBase_NotSharedProxyAdminOwner", + "type": "error" + }, + { + "inputs": [], + "name": "ProxyAdminOwnedBase_ProxyAdminNotFound", + "type": "error" + }, + { + "inputs": [], + "name": "ReentrantCall", + "type": "error" + }, + { + "inputs": [], + "name": "TooManySystemAddresses", + "type": "error" + }, { "inputs": [], "name": "ZeroAddress", "type": "error" + }, + { + "inputs": [], + "name": "ZeroTargetBalance", + "type": "error" } ] \ No newline at end of file diff --git a/snapshots/semver-lock.json b/snapshots/semver-lock.json index a0b3f4d4e..79d1b6549 100644 --- a/snapshots/semver-lock.json +++ b/snapshots/semver-lock.json @@ -64,8 +64,8 @@ "sourceCodeHash": "0xcb329746df0baddd3dc03c6c88da5d6bdc0f0a96d30e6dc78d0891bb1e935032" }, "src/L2/FeeDisburser.sol:FeeDisburser": { - "initCodeHash": "0x1278027e3756e2989e80c0a7b513e221a5fe0d3dbd9ded108375a29b2c1f3d57", - "sourceCodeHash": "0xac49a0ecf22b8a7bb3ebef830a2d27b19050f9b08941186e8563d5113cf0ce9c" + "initCodeHash": "0xf62b41a2d35184a77e9993b953959ff9297d31114ee7f16b773001ecb94a9f69", + "sourceCodeHash": "0x538d5f4be6edc07bdcd8af451590381dc69afceacd7e7e0c21d44137b8a2f5e8" }, "src/L2/GasPriceOracle.sol:GasPriceOracle": { "initCodeHash": "0xf72c23d9c3775afd7b645fde429d09800622d329116feb5ff9829634655123ca", diff --git a/snapshots/storageLayout/FeeDisburser.json b/snapshots/storageLayout/FeeDisburser.json index 53ffb02a4..093900bf6 100644 --- a/snapshots/storageLayout/FeeDisburser.json +++ b/snapshots/storageLayout/FeeDisburser.json @@ -12,5 +12,26 @@ "offset": 0, "slot": "1", "type": "uint256" + }, + { + "bytes": "32", + "label": "_disburseFeesEntered", + "offset": 0, + "slot": "2", + "type": "uint256" + }, + { + "bytes": "32", + "label": "systemAddresses", + "offset": 0, + "slot": "3", + "type": "address payable[]" + }, + { + "bytes": "32", + "label": "targetBalances", + "offset": 0, + "slot": "4", + "type": "uint256[]" } ] \ No newline at end of file diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index 79307d123..2d17bd02d 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -6,12 +6,13 @@ import { IFeeVault, Types } from "interfaces/L2/IFeeVault.sol"; import { ISemver } from "interfaces/universal/ISemver.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; import { SafeCall } from "src/libraries/SafeCall.sol"; +import { Initializable } from "src/vendor/Initializable.sol"; import { ProxyAdminOwnedBase } from "src/universal/ProxyAdminOwnedBase.sol"; /// @custom:proxied true /// @title FeeDisburser /// @notice Withdraws funds from system FeeVault contracts and bridges to L1. -contract FeeDisburser is ProxyAdminOwnedBase, ISemver { +contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { //////////////////////////////////////////////////////////////// /// Constants //////////////////////////////////////////////////////////////// @@ -199,21 +200,17 @@ contract FeeDisburser is ProxyAdminOwnedBase, ISemver { } /// @notice Configures the L2 system addresses to refund and their target balances. - /// Passing empty arrays clears the configuration and disables refunding. - /// - /// @dev Callable only by the ProxyAdmin. + /// Called via upgradeAndCall when upgrading to this version. /// /// @param systemAddresses_ The system addresses being funded. /// @param targetBalances_ The target balances for system addresses. - function setSystemAddresses( + function initialize( address payable[] memory systemAddresses_, uint256[] memory targetBalances_ ) external - virtual + reinitializer(2) { - _assertOnlyProxyAdmin(); - uint256 systemAddressesLength = systemAddresses_.length; if (systemAddressesLength > MAX_SYSTEM_ADDRESS_COUNT) revert TooManySystemAddresses(); if (systemAddressesLength != targetBalances_.length) revert ArrayLengthMismatch(); diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 5dcfe3911..f2b8e06b6 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -6,25 +6,11 @@ import { Test } from "lib/forge-std/src/Test.sol"; import { FeeDisburser } from "src/L2/FeeDisburser.sol"; import { IFeeVault, Types } from "interfaces/L2/IFeeVault.sol"; import { IStandardBridge } from "interfaces/universal/IStandardBridge.sol"; -import { ProxyAdminOwnedBase } from "src/universal/ProxyAdminOwnedBase.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; -import { Constants } from "src/libraries/Constants.sol"; contract ReentrantReceiver { - FeeDisburser public target; - - constructor(FeeDisburser _target) { - target = _target; - } - - receive() external payable { - target.disburseFees(); - } -} - -contract RevertingReceiver { receive() external payable { - revert(); + FeeDisburser(payable(msg.sender)).disburseFees(); } } @@ -50,7 +36,6 @@ contract FeeDisburserTest is Test { address payable constant L1_WALLET = payable(address(0x1001)); address constant ALICE = address(0xA11CE); address constant BOB = address(0xB0B); - address constant PROXY_ADMIN = address(0xAD1); address payable constant SYSTEM_ADDR = payable(address(0x5001)); // Contract instances @@ -123,14 +108,8 @@ contract FeeDisburserTest is Test { emit NoFeesCollected(); } - function _setProxyAdmin() internal { - vm.store(address(feeDisburser), Constants.PROXY_OWNER_ADDRESS, bytes32(uint256(uint160(PROXY_ADMIN)))); - } - - function _setSystemAddresses(address payable[] memory addrs, uint256[] memory balances) internal { - _setProxyAdmin(); - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs, balances); + function _initialize(address payable[] memory addrs, uint256[] memory balances) internal { + feeDisburser.initialize(addrs, balances); } function _makeSingleConfig( @@ -820,68 +799,30 @@ contract FeeDisburserTest is Test { } // ============================================================ - // setSystemAddresses Tests + // initialize Tests // ============================================================ - function test_setSystemAddresses_success() public { + function test_initialize_success() public { (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); - _setProxyAdmin(); - vm.expectEmit(address(feeDisburser)); emit SystemAddressesUpdated(1); - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs, balances); + feeDisburser.initialize(addrs, balances); assertEq(feeDisburser.systemAddresses(0), SYSTEM_ADDR); assertEq(feeDisburser.targetBalances(0), 1 ether); } - function test_setSystemAddresses_success_overwrites() public { - (address payable[] memory addrs1, uint256[] memory balances1) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); - _setSystemAddresses(addrs1, balances1); - - (address payable[] memory addrs2, uint256[] memory balances2) = _makeSingleConfig(payable(ALICE), 2 ether); - - vm.expectEmit(address(feeDisburser)); - emit SystemAddressesUpdated(1); - - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs2, balances2); - - assertEq(feeDisburser.systemAddresses(0), ALICE); - assertEq(feeDisburser.targetBalances(0), 2 ether); - } - - function test_setSystemAddresses_revert_notProxyAdmin() public { + function test_initialize_revert_alreadyInitialized() public { (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); - - _setProxyAdmin(); - - vm.expectRevert(ProxyAdminOwnedBase.ProxyAdminOwnedBase_NotProxyAdmin.selector); - vm.prank(ALICE); - feeDisburser.setSystemAddresses(addrs, balances); - } - - function test_setSystemAddresses_success_clearConfig() public { - // First set some addresses - (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); - _setSystemAddresses(addrs, balances); - assertEq(feeDisburser.systemAddresses(0), SYSTEM_ADDR); - - // Clear config with empty arrays - vm.expectEmit(address(feeDisburser)); - emit SystemAddressesUpdated(0); - - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(new address payable[](0), new uint256[](0)); + feeDisburser.initialize(addrs, balances); vm.expectRevert(); - feeDisburser.systemAddresses(0); + feeDisburser.initialize(addrs, balances); } - function test_setSystemAddresses_revert_tooManySystemAddresses() public { + function test_initialize_revert_tooManySystemAddresses() public { uint256 n = feeDisburser.MAX_SYSTEM_ADDRESS_COUNT() + 1; address payable[] memory addrs = new address payable[](n); uint256[] memory balances = new uint256[](n); @@ -890,45 +831,33 @@ contract FeeDisburserTest is Test { balances[i] = 1 ether; } - _setProxyAdmin(); - vm.expectRevert(FeeDisburser.TooManySystemAddresses.selector); - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs, balances); + feeDisburser.initialize(addrs, balances); } - function test_setSystemAddresses_revert_arrayLengthMismatch() public { + function test_initialize_revert_arrayLengthMismatch() public { address payable[] memory addrs = new address payable[](2); addrs[0] = SYSTEM_ADDR; addrs[1] = payable(ALICE); uint256[] memory balances = new uint256[](1); balances[0] = 1 ether; - _setProxyAdmin(); - vm.expectRevert(FeeDisburser.ArrayLengthMismatch.selector); - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs, balances); + feeDisburser.initialize(addrs, balances); } - function test_setSystemAddresses_revert_zeroAddress() public { + function test_initialize_revert_zeroAddress() public { (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(0)), 1 ether); - _setProxyAdmin(); - vm.expectRevert(FeeDisburser.ZeroAddress.selector); - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs, balances); + feeDisburser.initialize(addrs, balances); } - function test_setSystemAddresses_revert_zeroTargetBalance() public { + function test_initialize_revert_zeroTargetBalance() public { (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 0); - _setProxyAdmin(); - vm.expectRevert(FeeDisburser.ZeroTargetBalance.selector); - vm.prank(PROXY_ADMIN); - feeDisburser.setSystemAddresses(addrs, balances); + feeDisburser.initialize(addrs, balances); } // ============================================================ @@ -940,11 +869,9 @@ contract FeeDisburserTest is Test { uint256 targetBal = 2 ether; (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _setSystemAddresses(addrs, balances); + _initialize(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); uint256 bridgeAmount = feeAmount - targetBal; _expectBridgeETH(bridgeAmount); @@ -966,11 +893,9 @@ contract FeeDisburserTest is Test { vm.deal(SYSTEM_ADDR, targetBal); (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _setSystemAddresses(addrs, balances); + _initialize(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); _expectBridgeETH(feeAmount); @@ -984,29 +909,6 @@ contract FeeDisburserTest is Test { assertEq(SYSTEM_ADDR.balance, targetBal); } - function test_disburseFees_success_allFundsConsumedByRefund() public { - uint256 feeAmount = 5 ether; - uint256 targetBal = 10 ether; - - (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _setSystemAddresses(addrs, balances); - - _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); - - // No bridge call — balanceSent capped at available feeAmount - vm.expectEmit(address(feeDisburser)); - emit ProcessedFunds(SYSTEM_ADDR, true, targetBal, feeAmount); - - _expectFeesDisbursed(0); - - feeDisburser.disburseFees(); - - assertEq(SYSTEM_ADDR.balance, feeAmount); - assertEq(address(feeDisburser).balance, 0); - } - function test_disburseFees_success_partialRefundDueToLowBalance() public { uint256 targetBal = 5 ether; uint256 systemAddrStartBal = 1 ether; @@ -1015,11 +917,9 @@ contract FeeDisburserTest is Test { vm.deal(SYSTEM_ADDR, systemAddrStartBal); (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _setSystemAddresses(addrs, balances); + _initialize(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); uint256 valueNeeded = targetBal - systemAddrStartBal; // 4 ether uint256 valueSent = feeAmount; // only 2 ether available @@ -1047,12 +947,10 @@ contract FeeDisburserTest is Test { uint256[] memory bals = new uint256[](2); bals[0] = target1; bals[1] = target2; - _setSystemAddresses(addrs, bals); + _initialize(addrs, bals); uint256 feeAmount = 10 ether; _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); uint256 bridgeAmount = feeAmount - target1 - target2; _expectBridgeETH(bridgeAmount); @@ -1072,27 +970,26 @@ contract FeeDisburserTest is Test { } function test_disburseFees_success_revertingRecipient() public { - RevertingReceiver bad = new RevertingReceiver(); + address payable bad = payable(address(0x9999)); - (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(bad)), 1 ether); - _setSystemAddresses(addrs, balances); + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(bad, 1 ether); + _initialize(addrs, balances); uint256 feeAmount = 2 ether; _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); // SafeCall.send catches the revert; full fee amount still bridges + vm.mockCallRevert(bad, 1 ether, bytes(""), bytes("")); _expectBridgeETH(feeAmount); vm.expectEmit(address(feeDisburser)); - emit ProcessedFunds(address(bad), false, 1 ether, 1 ether); + emit ProcessedFunds(bad, false, 1 ether, 1 ether); _expectFeesDisbursed(feeAmount); feeDisburser.disburseFees(); - assertEq(address(bad).balance, 0); + assertEq(bad.balance, 0); } // ============================================================ @@ -1100,16 +997,14 @@ contract FeeDisburserTest is Test { // ============================================================ function test_disburseFees_reentrancy_innerCallBlocked() public { - ReentrantReceiver attacker = new ReentrantReceiver(feeDisburser); + ReentrantReceiver attacker = new ReentrantReceiver(); (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(attacker)), 1 ether); - _setSystemAddresses(addrs, balances); + _initialize(addrs, balances); uint256 feeAmount = 2 ether; _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); - vm.deal(Predeploys.BASE_FEE_VAULT, 0); - vm.deal(Predeploys.L1_FEE_VAULT, 0); // Inner disburseFees reverts (reentrancy), so SafeCall returns success=false and ETH is not sent _expectBridgeETH(feeAmount); From f2f8e56feb1bacf0f890792731d9a032901b1afa Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Fri, 10 Jul 2026 14:54:45 -0400 Subject: [PATCH 08/10] fix FeeDisburser mocked bridge test Co-authored-by: Codex --- test/L2/FeeDisburser.t.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index f2b8e06b6..4adc507ac 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -966,7 +966,7 @@ contract FeeDisburserTest is Test { assertEq(addr1.balance, target1); assertEq(addr2.balance, target2); - assertEq(address(feeDisburser).balance, 0); + // expectCall verifies the bridged amount; mocked payable balance semantics vary by Foundry version. } function test_disburseFees_success_revertingRecipient() public { From 566067f613409f9e0c0598503bd17e11657c65be Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Fri, 10 Jul 2026 15:40:11 -0400 Subject: [PATCH 09/10] refactor FeeDisburser refund simplifications Co-authored-by: Codex --- snapshots/abi/FeeDisburser.json | 71 +---------------------- snapshots/semver-lock.json | 4 +- snapshots/storageLayout/FeeDisburser.json | 11 +--- src/L2/FeeDisburser.sol | 33 +---------- test/L2/FeeDisburser.t.sol | 20 ++----- 5 files changed, 14 insertions(+), 125 deletions(-) diff --git a/snapshots/abi/FeeDisburser.json b/snapshots/abi/FeeDisburser.json index c8dfbfed2..1a9726e29 100644 --- a/snapshots/abi/FeeDisburser.json +++ b/snapshots/abi/FeeDisburser.json @@ -122,32 +122,6 @@ "stateMutability": "view", "type": "function" }, - { - "inputs": [], - "name": "proxyAdmin", - "outputs": [ - { - "internalType": "contract IProxyAdmin", - "name": "", - "type": "address" - } - ], - "stateMutability": "view", - "type": "function" - }, - { - "inputs": [], - "name": "proxyAdminOwner", - "outputs": [ - { - "internalType": "address", - "name": "", - "type": "address" - } - ], - "stateMutability": "view", - "type": "function" - }, { "inputs": [ { @@ -293,19 +267,6 @@ "name": "ProcessedFunds", "type": "event" }, - { - "anonymous": false, - "inputs": [ - { - "indexed": false, - "internalType": "uint256", - "name": "systemAddressCount", - "type": "uint256" - } - ], - "name": "SystemAddressesUpdated", - "type": "event" - }, { "inputs": [], "name": "ArrayLengthMismatch", @@ -343,37 +304,7 @@ }, { "inputs": [], - "name": "ProxyAdminOwnedBase_NotProxyAdmin", - "type": "error" - }, - { - "inputs": [], - "name": "ProxyAdminOwnedBase_NotProxyAdminOrProxyAdminOwner", - "type": "error" - }, - { - "inputs": [], - "name": "ProxyAdminOwnedBase_NotProxyAdminOwner", - "type": "error" - }, - { - "inputs": [], - "name": "ProxyAdminOwnedBase_NotResolvedDelegateProxy", - "type": "error" - }, - { - "inputs": [], - "name": "ProxyAdminOwnedBase_NotSharedProxyAdminOwner", - "type": "error" - }, - { - "inputs": [], - "name": "ProxyAdminOwnedBase_ProxyAdminNotFound", - "type": "error" - }, - { - "inputs": [], - "name": "ReentrantCall", + "name": "Reentrancy", "type": "error" }, { diff --git a/snapshots/semver-lock.json b/snapshots/semver-lock.json index 79d1b6549..26f72f823 100644 --- a/snapshots/semver-lock.json +++ b/snapshots/semver-lock.json @@ -64,8 +64,8 @@ "sourceCodeHash": "0xcb329746df0baddd3dc03c6c88da5d6bdc0f0a96d30e6dc78d0891bb1e935032" }, "src/L2/FeeDisburser.sol:FeeDisburser": { - "initCodeHash": "0xf62b41a2d35184a77e9993b953959ff9297d31114ee7f16b773001ecb94a9f69", - "sourceCodeHash": "0x538d5f4be6edc07bdcd8af451590381dc69afceacd7e7e0c21d44137b8a2f5e8" + "initCodeHash": "0x46c25925d76d6a6e617f29a4c1d3b1d5239353cd43b27b11a291a5c20b6817bc", + "sourceCodeHash": "0x94f559aa6bd8d4c49329b948b17a418dd72abdd6c3cfdb48657664b0146f7968" }, "src/L2/GasPriceOracle.sol:GasPriceOracle": { "initCodeHash": "0xf72c23d9c3775afd7b645fde429d09800622d329116feb5ff9829634655123ca", diff --git a/snapshots/storageLayout/FeeDisburser.json b/snapshots/storageLayout/FeeDisburser.json index 093900bf6..cc620f81d 100644 --- a/snapshots/storageLayout/FeeDisburser.json +++ b/snapshots/storageLayout/FeeDisburser.json @@ -13,25 +13,18 @@ "slot": "1", "type": "uint256" }, - { - "bytes": "32", - "label": "_disburseFeesEntered", - "offset": 0, - "slot": "2", - "type": "uint256" - }, { "bytes": "32", "label": "systemAddresses", "offset": 0, - "slot": "3", + "slot": "2", "type": "address payable[]" }, { "bytes": "32", "label": "targetBalances", "offset": 0, - "slot": "4", + "slot": "3", "type": "uint256[]" } ] \ No newline at end of file diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index 2d17bd02d..0a90deaac 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -7,12 +7,12 @@ import { ISemver } from "interfaces/universal/ISemver.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; import { SafeCall } from "src/libraries/SafeCall.sol"; import { Initializable } from "src/vendor/Initializable.sol"; -import { ProxyAdminOwnedBase } from "src/universal/ProxyAdminOwnedBase.sol"; +import { ReentrancyGuard } from "lib/solady/src/utils/ReentrancyGuard.sol"; /// @custom:proxied true /// @title FeeDisburser /// @notice Withdraws funds from system FeeVault contracts and bridges to L1. -contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { +contract FeeDisburser is Initializable, ReentrancyGuard, ISemver { //////////////////////////////////////////////////////////////// /// Constants //////////////////////////////////////////////////////////////// @@ -45,9 +45,6 @@ contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { /// This variable is deprecated and its value should not be relied upon. uint256 public netFeeRevenue; - /// @notice Reentrancy guard status for disburseFees. - uint256 private _disburseFeesEntered; - /// @notice The L2 system addresses being funded. address payable[] public systemAddresses; @@ -85,11 +82,6 @@ contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent ); - /// @notice Emitted when the L2 system address refund configuration is updated. - /// - /// @param systemAddressCount The number of configured system addresses. - event SystemAddressesUpdated(uint256 systemAddressCount); - //////////////////////////////////////////////////////////////// /// Errors //////////////////////////////////////////////////////////////// @@ -118,23 +110,6 @@ contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { /// @notice Thrown when a system address target balance is zero. error ZeroTargetBalance(); - /// @notice Thrown when disburseFees is reentered. - error ReentrantCall(); - - //////////////////////////////////////////////////////////////// - /// Modifiers - //////////////////////////////////////////////////////////////// - - /// @notice Prevents reentrancy into disburseFees while preserving the existing storage layout. - /// Uses 1/2 sentinel values so the slot stays nonzero after first use, keeping - /// subsequent SSTOREs warm. Uninitialized (0) is treated as not-entered. - modifier nonReentrantDisbursement() { - if (_disburseFeesEntered == 2) revert ReentrantCall(); - _disburseFeesEntered = 2; - _; - _disburseFeesEntered = 1; - } - //////////////////////////////////////////////////////////////// /// Constructor //////////////////////////////////////////////////////////////// @@ -156,7 +131,7 @@ contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { //////////////////////////////////////////////////////////////// /// @notice Withdraws funds from FeeVaults and bridges to L1. - function disburseFees() external virtual nonReentrantDisbursement { + function disburseFees() external virtual nonReentrant { if (block.timestamp < lastDisbursementTime + FEE_DISBURSEMENT_INTERVAL) revert IntervalNotReached(); // Sequencer, base, and L1 FeeVaults will withdraw fees to the FeeDisburser contract. @@ -225,8 +200,6 @@ contract FeeDisburser is Initializable, ProxyAdminOwnedBase, ISemver { systemAddresses = systemAddresses_; targetBalances = targetBalances_; - - emit SystemAddressesUpdated(systemAddressesLength); } /// @notice Receives ETH fees withdrawn from L2 FeeVaults. diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 4adc507ac..2a4928ffc 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -24,7 +24,6 @@ contract FeeDisburserTest is Test { event ProcessedFunds( address indexed systemAddress, bool indexed success, uint256 balanceNeeded, uint256 balanceSent ); - event SystemAddressesUpdated(uint256 systemAddressCount); // Constants uint32 constant WITHDRAWAL_MIN_GAS = 35_000; @@ -108,10 +107,6 @@ contract FeeDisburserTest is Test { emit NoFeesCollected(); } - function _initialize(address payable[] memory addrs, uint256[] memory balances) internal { - feeDisburser.initialize(addrs, balances); - } - function _makeSingleConfig( address payable addr, uint256 balance @@ -805,9 +800,6 @@ contract FeeDisburserTest is Test { function test_initialize_success() public { (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); - vm.expectEmit(address(feeDisburser)); - emit SystemAddressesUpdated(1); - feeDisburser.initialize(addrs, balances); assertEq(feeDisburser.systemAddresses(0), SYSTEM_ADDR); @@ -869,7 +861,7 @@ contract FeeDisburserTest is Test { uint256 targetBal = 2 ether; (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _initialize(addrs, balances); + feeDisburser.initialize(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -893,7 +885,7 @@ contract FeeDisburserTest is Test { vm.deal(SYSTEM_ADDR, targetBal); (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _initialize(addrs, balances); + feeDisburser.initialize(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -917,7 +909,7 @@ contract FeeDisburserTest is Test { vm.deal(SYSTEM_ADDR, systemAddrStartBal); (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); - _initialize(addrs, balances); + feeDisburser.initialize(addrs, balances); _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -947,7 +939,7 @@ contract FeeDisburserTest is Test { uint256[] memory bals = new uint256[](2); bals[0] = target1; bals[1] = target2; - _initialize(addrs, bals); + feeDisburser.initialize(addrs, bals); uint256 feeAmount = 10 ether; _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -973,7 +965,7 @@ contract FeeDisburserTest is Test { address payable bad = payable(address(0x9999)); (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(bad, 1 ether); - _initialize(addrs, balances); + feeDisburser.initialize(addrs, balances); uint256 feeAmount = 2 ether; _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); @@ -1001,7 +993,7 @@ contract FeeDisburserTest is Test { (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(attacker)), 1 ether); - _initialize(addrs, balances); + feeDisburser.initialize(addrs, balances); uint256 feeAmount = 2 ether; _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); From 4dca0990cf6b7cd47c78289cf1e05e93d6f79113 Mon Sep 17 00:00:00 2001 From: Thanh Trinh Date: Fri, 10 Jul 2026 16:01:39 -0400 Subject: [PATCH 10/10] restore FeeDisburser storage reentrancy guard Co-authored-by: Codex --- snapshots/abi/FeeDisburser.json | 2 +- snapshots/semver-lock.json | 4 ++-- snapshots/storageLayout/FeeDisburser.json | 11 ++++++++-- src/L2/FeeDisburser.sol | 25 ++++++++++++++++++++--- 4 files changed, 34 insertions(+), 8 deletions(-) diff --git a/snapshots/abi/FeeDisburser.json b/snapshots/abi/FeeDisburser.json index 1a9726e29..1ec04656e 100644 --- a/snapshots/abi/FeeDisburser.json +++ b/snapshots/abi/FeeDisburser.json @@ -304,7 +304,7 @@ }, { "inputs": [], - "name": "Reentrancy", + "name": "ReentrantCall", "type": "error" }, { diff --git a/snapshots/semver-lock.json b/snapshots/semver-lock.json index 26f72f823..b2b30d9e6 100644 --- a/snapshots/semver-lock.json +++ b/snapshots/semver-lock.json @@ -64,8 +64,8 @@ "sourceCodeHash": "0xcb329746df0baddd3dc03c6c88da5d6bdc0f0a96d30e6dc78d0891bb1e935032" }, "src/L2/FeeDisburser.sol:FeeDisburser": { - "initCodeHash": "0x46c25925d76d6a6e617f29a4c1d3b1d5239353cd43b27b11a291a5c20b6817bc", - "sourceCodeHash": "0x94f559aa6bd8d4c49329b948b17a418dd72abdd6c3cfdb48657664b0146f7968" + "initCodeHash": "0xad537c451b201a39abd40d8cb3a297e88eb492e43f720886b3e11c6be5f9dffe", + "sourceCodeHash": "0x58f5e74899c9e5b954dd29f2a44c5a6c374787df3806c5a4720298aea76f0e45" }, "src/L2/GasPriceOracle.sol:GasPriceOracle": { "initCodeHash": "0xf72c23d9c3775afd7b645fde429d09800622d329116feb5ff9829634655123ca", diff --git a/snapshots/storageLayout/FeeDisburser.json b/snapshots/storageLayout/FeeDisburser.json index cc620f81d..093900bf6 100644 --- a/snapshots/storageLayout/FeeDisburser.json +++ b/snapshots/storageLayout/FeeDisburser.json @@ -15,16 +15,23 @@ }, { "bytes": "32", - "label": "systemAddresses", + "label": "_disburseFeesEntered", "offset": 0, "slot": "2", + "type": "uint256" + }, + { + "bytes": "32", + "label": "systemAddresses", + "offset": 0, + "slot": "3", "type": "address payable[]" }, { "bytes": "32", "label": "targetBalances", "offset": 0, - "slot": "3", + "slot": "4", "type": "uint256[]" } ] \ No newline at end of file diff --git a/src/L2/FeeDisburser.sol b/src/L2/FeeDisburser.sol index 0a90deaac..facc5f3d4 100644 --- a/src/L2/FeeDisburser.sol +++ b/src/L2/FeeDisburser.sol @@ -7,12 +7,11 @@ import { ISemver } from "interfaces/universal/ISemver.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; import { SafeCall } from "src/libraries/SafeCall.sol"; import { Initializable } from "src/vendor/Initializable.sol"; -import { ReentrancyGuard } from "lib/solady/src/utils/ReentrancyGuard.sol"; /// @custom:proxied true /// @title FeeDisburser /// @notice Withdraws funds from system FeeVault contracts and bridges to L1. -contract FeeDisburser is Initializable, ReentrancyGuard, ISemver { +contract FeeDisburser is Initializable, ISemver { //////////////////////////////////////////////////////////////// /// Constants //////////////////////////////////////////////////////////////// @@ -45,6 +44,9 @@ contract FeeDisburser is Initializable, ReentrancyGuard, ISemver { /// This variable is deprecated and its value should not be relied upon. uint256 public netFeeRevenue; + /// @notice Reentrancy guard status for disburseFees. + uint256 private _disburseFeesEntered; + /// @notice The L2 system addresses being funded. address payable[] public systemAddresses; @@ -110,6 +112,23 @@ contract FeeDisburser is Initializable, ReentrancyGuard, ISemver { /// @notice Thrown when a system address target balance is zero. error ZeroTargetBalance(); + /// @notice Thrown when disburseFees is reentered. + error ReentrantCall(); + + //////////////////////////////////////////////////////////////// + /// Modifiers + //////////////////////////////////////////////////////////////// + + /// @notice Prevents reentrancy into disburseFees while preserving the existing storage layout. + /// Uses 1/2 sentinel values so the slot stays nonzero after first use, keeping + /// subsequent SSTOREs warm. Uninitialized (0) is treated as not-entered. + modifier nonReentrantDisbursement() { + if (_disburseFeesEntered == 2) revert ReentrantCall(); + _disburseFeesEntered = 2; + _; + _disburseFeesEntered = 1; + } + //////////////////////////////////////////////////////////////// /// Constructor //////////////////////////////////////////////////////////////// @@ -131,7 +150,7 @@ contract FeeDisburser is Initializable, ReentrancyGuard, ISemver { //////////////////////////////////////////////////////////////// /// @notice Withdraws funds from FeeVaults and bridges to L1. - function disburseFees() external virtual nonReentrant { + function disburseFees() external virtual nonReentrantDisbursement { if (block.timestamp < lastDisbursementTime + FEE_DISBURSEMENT_INTERVAL) revert IntervalNotReached(); // Sequencer, base, and L1 FeeVaults will withdraw fees to the FeeDisburser contract.