build(contracts): enforce compiler warnings and fix test return value checks (#18264)

* build(contracts): enforce compiler warnings as errors

- Add deny_warnings to foundry.toml default profile
- Add set -e to pre-pr command in justfile

* test(contracts-bedrock): fix unchecked low-level call return values in FeeSplitter tests

- add proper return value checks for low-level .call() operations
- fix Warning 9302 in FeeSplitter.t.sol (3 occurrences)
- fix Warning 9302 in FeeSplitterVaults.t.sol (1 occurrence)

* build(contracts): ignore too-many-warnings error in foundry config

* fix(test): fix assertion logic in FeeSplitter expectRevert tests

- remove assert(!success) after vm.expectRevert() calls
- add success variable reference to suppress unused variable warning

Author: aliersh

packages/contracts-bedrock/foundry.toml

@@ -68,7 +68,8 @@ fs_permissions = [
 ]
 
 # 5159 error code is selfdestruct error code
-ignored_error_codes = ["transient-storage", "code-size", "init-code-size", 5159]
+ignored_error_codes = ["transient-storage", "code-size", "init-code-size", "too-many-warnings", 5159]
+deny_warnings = true
 ffi = true
 
 # We set the gas limit to max int64 to avoid running out of gas during testing, since the default

packages/contracts-bedrock/justfile

@@ -352,6 +352,7 @@ pre-commit *ARGS:
 # Cleans, builds, lints, and runs all checks.
 pre-pr *ARGS:
   #!/bin/bash
+  set -e
   # Optionally clean the previous build.
   # --clean is typically not needed but can force a clean build if you suspect cache issues.
   if [[ "{{ARGS}}" == *"--clean"* ]]; then

packages/contracts-bedrock/test/L2/FeeSplitter.t.sol

@@ -153,6 +153,7 @@ contract FeeSplitter_Initialize_Test is FeeSplitter_TestInit {
 contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
     /// @notice Test that receive function reverts when sender is not an approved vault
     function testFuzz_feeSplitterReceive_whenNotApprovedVault_reverts(address _caller, uint256 _amount) public {
+        vm.assume(_caller != address(0));
         vm.assume(_caller != Predeploys.SEQUENCER_FEE_WALLET);
         vm.assume(_caller != Predeploys.BASE_FEE_VAULT);
         vm.assume(_caller != Predeploys.OPERATOR_FEE_VAULT);
@@ -161,11 +162,13 @@ contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
 
         vm.prank(_caller);
         vm.expectRevert(IFeeSplitter.FeeSplitter_SenderNotCurrentVault.selector);
-        payable(address(feeSplitter)).call{ value: _amount }("");
+        (bool revertsAsExpected,) = payable(address(feeSplitter)).call{ value: _amount }("");
+        assertTrue(revertsAsExpected, "FeeSplitter_Test: call did not revert");
     }
 
     /// @notice Test receive function from non-approved vault reverts even during disbursement
     function testFuzz_feeSplitterReceive_whenNonFeeVault_reverts(address _caller, uint256 _amount) public {
+        vm.assume(_caller != address(0));
         vm.assume(_caller != Predeploys.SEQUENCER_FEE_WALLET);
         vm.assume(_caller != Predeploys.BASE_FEE_VAULT);
         vm.assume(_caller != Predeploys.OPERATOR_FEE_VAULT);
@@ -177,7 +180,8 @@ contract FeeSplitter_Receive_Test is FeeSplitter_TestInit {
 
         // Now we test the actual sender validation
         vm.expectRevert(IFeeSplitter.FeeSplitter_SenderNotCurrentVault.selector);
-        payable(address(feeSplitter)).call{ value: _amount }("");
+        (bool revertsAsExpected,) = payable(address(feeSplitter)).call{ value: _amount }("");
+        assertTrue(revertsAsExpected, "FeeSplitter_Test: call did not revert");
     }
 
     /// @notice Test receive function works during disbursement from SequencerFeeVault
@@ -659,7 +663,8 @@ contract FeeSplitter_DisburseFees_Test is FeeSplitter_TestInit {
         // Attempt to send ETH from the vault - should revert because transient storage was cleared
         vm.prank(_vault);
         vm.expectRevert(IFeeSplitter.FeeSplitter_SenderNotCurrentVault.selector);
-        payable(address(feeSplitter)).call{ value: _attemptAmount }("");
+        (bool revertsAsExpected,) = payable(address(feeSplitter)).call{ value: _attemptAmount }("");
+        assertTrue(revertsAsExpected, "FeeSplitter_Test: call did not revert");
     }
 }
 

packages/contracts-bedrock/test/L2/FeeSplitterVaults.t.sol

@@ -52,7 +52,8 @@ contract FeeSplitterVaults_Receive_Test is Test {
                 }
 
                 vm.prank(_selectedVault);
-                payable(address(feeSplitter)).call{ value: _amount }("");
+                (bool success,) = payable(address(feeSplitter)).call{ value: _amount }("");
+                require(success, "FeeSplitterVaults_Test: call failed");
             }
         }
     }