diff --git a/snapshots/abi/FeeDisburser.json b/snapshots/abi/FeeDisburser.json index 4dfa787a5..1ec04656e 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,44 @@ "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 +217,61 @@ "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" + }, + { + "inputs": [], + "name": "ArrayLengthMismatch", + "type": "error" + }, { "inputs": [], "name": "FeeVaultMustWithdrawToFeeDisburser", @@ -174,9 +292,34 @@ "name": "IntervalTooLow", "type": "error" }, + { + "inputs": [], + "name": "InvalidInitialization", + "type": "error" + }, + { + "inputs": [], + "name": "NotInitializing", + "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..b2b30d9e6 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": "0xad537c451b201a39abd40d8cb3a297e88eb492e43f720886b3e11c6be5f9dffe", + "sourceCodeHash": "0x58f5e74899c9e5b954dd29f2a44c5a6c374787df3806c5a4720298aea76f0e45" }, "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 aaa8b914d..facc5f3d4 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 { Initializable } from "src/vendor/Initializable.sol"; /// @custom:proxied true /// @title FeeDisburser /// @notice Withdraws funds from system FeeVault contracts and bridges to L1. -contract FeeDisburser is ISemver { +contract FeeDisburser is Initializable, 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,17 @@ 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 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 + ); + //////////////////////////////////////////////////////////////// /// Errors //////////////////////////////////////////////////////////////// @@ -78,6 +103,32 @@ 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 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. + /// 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 //////////////////////////////////////////////////////////////// @@ -99,7 +150,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 +168,57 @@ contract FeeDisburser is ISemver { return; } - lastDisbursementTime = block.timestamp; + uint256 disbursementTime = block.timestamp; + lastDisbursementTime = disbursementTime; + + 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) { + 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. + /// Called via upgradeAndCall when upgrading to this version. + /// + /// @param systemAddresses_ The system addresses being funded. + /// @param targetBalances_ The target balances for system addresses. + function initialize( + address payable[] memory systemAddresses_, + uint256[] memory targetBalances_ + ) + external + reinitializer(2) + { + uint256 systemAddressesLength = systemAddresses_.length; + 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_; } /// @notice Receives ETH fees withdrawn from L2 FeeVaults. @@ -132,9 +226,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 { + 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 + }); } //////////////////////////////////////////////////////////////// diff --git a/test/L2/FeeDisburser.t.sol b/test/L2/FeeDisburser.t.sol index 75287a33d..2a4928ffc 100644 --- a/test/L2/FeeDisburser.t.sol +++ b/test/L2/FeeDisburser.t.sol @@ -8,6 +8,12 @@ import { IFeeVault, Types } from "interfaces/L2/IFeeVault.sol"; import { IStandardBridge } from "interfaces/universal/IStandardBridge.sol"; import { Predeploys } from "src/libraries/Predeploys.sol"; +contract ReentrantReceiver { + receive() external payable { + FeeDisburser(payable(msg.sender)).disburseFees(); + } +} + /// @title FeeDisburserTest /// @notice Comprehensive unit and fuzz tests for the FeeDisburser contract contract FeeDisburserTest is Test { @@ -15,6 +21,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 + ); // Constants uint32 constant WITHDRAWAL_MIN_GAS = 35_000; @@ -26,6 +35,7 @@ contract FeeDisburserTest is Test { address payable constant L1_WALLET = payable(address(0x1001)); address constant ALICE = address(0xA11CE); address constant BOB = address(0xB0B); + address payable constant SYSTEM_ADDR = payable(address(0x5001)); // Contract instances FeeDisburser feeDisburser; @@ -97,6 +107,20 @@ contract FeeDisburserTest is Test { emit NoFeesCollected(); } + 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 // ============================================================ @@ -768,4 +792,220 @@ contract FeeDisburserTest is Test { assertTrue(gasUsed < 200_000, "Gas usage too high for all-vault case"); } + + // ============================================================ + // initialize Tests + // ============================================================ + + function test_initialize_success() public { + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); + + feeDisburser.initialize(addrs, balances); + + assertEq(feeDisburser.systemAddresses(0), SYSTEM_ADDR); + assertEq(feeDisburser.targetBalances(0), 1 ether); + } + + function test_initialize_revert_alreadyInitialized() public { + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 1 ether); + feeDisburser.initialize(addrs, balances); + + vm.expectRevert(); + feeDisburser.initialize(addrs, balances); + } + + 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); + for (uint256 i; i < n; i++) { + addrs[i] = payable(address(uint160(0x6000 + i))); + balances[i] = 1 ether; + } + + vm.expectRevert(FeeDisburser.TooManySystemAddresses.selector); + feeDisburser.initialize(addrs, balances); + } + + 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; + + vm.expectRevert(FeeDisburser.ArrayLengthMismatch.selector); + feeDisburser.initialize(addrs, balances); + } + + function test_initialize_revert_zeroAddress() public { + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(payable(address(0)), 1 ether); + + vm.expectRevert(FeeDisburser.ZeroAddress.selector); + feeDisburser.initialize(addrs, balances); + } + + function test_initialize_revert_zeroTargetBalance() public { + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, 0); + + vm.expectRevert(FeeDisburser.ZeroTargetBalance.selector); + feeDisburser.initialize(addrs, balances); + } + + // ============================================================ + // disburseFees with System Address Refunds + // ============================================================ + + function test_disburseFees_success_systemAddressRefunded() public { + uint256 feeAmount = 10 ether; + uint256 targetBal = 2 ether; + + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); + feeDisburser.initialize(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + + 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, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); + feeDisburser.initialize(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + + _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_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, uint256[] memory balances) = _makeSingleConfig(SYSTEM_ADDR, targetBal); + feeDisburser.initialize(addrs, balances); + + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + + 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); + } + + 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; + feeDisburser.initialize(addrs, bals); + + uint256 feeAmount = 10 ether; + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + + 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); + // expectCall verifies the bridged amount; mocked payable balance semantics vary by Foundry version. + } + + function test_disburseFees_success_revertingRecipient() public { + address payable bad = payable(address(0x9999)); + + (address payable[] memory addrs, uint256[] memory balances) = _makeSingleConfig(bad, 1 ether); + feeDisburser.initialize(addrs, balances); + + uint256 feeAmount = 2 ether; + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + + // 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(bad, false, 1 ether, 1 ether); + + _expectFeesDisbursed(feeAmount); + + feeDisburser.disburseFees(); + + assertEq(bad.balance, 0); + } + + // ============================================================ + // Reentrancy Test + // ============================================================ + + function test_disburseFees_reentrancy_innerCallBlocked() public { + ReentrantReceiver attacker = new ReentrantReceiver(); + + (address payable[] memory addrs, uint256[] memory balances) = + _makeSingleConfig(payable(address(attacker)), 1 ether); + feeDisburser.initialize(addrs, balances); + + uint256 feeAmount = 2 ether; + _mockVaultWithdrawal(Predeploys.SEQUENCER_FEE_WALLET, feeAmount); + + // 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); + } }