refactor: prevent ETH from being sent to the contract directly

Author: passandscore

contracts/contracts/core/GasTankDepositor.sol

@@ -10,10 +10,8 @@ contract GasTankDepositor {
     address public immutable RPC_SERVICE;
     uint256 public immutable MAXIMUM_DEPOSIT;
 
-    event FundsRecovered(address indexed owner, uint256 indexed amount);
     event GasTankFunded(address indexed smartAccount, address indexed caller, uint256 indexed amount);
 
-    error FailedToRecoverFunds(address owner, uint256 amount);
     error FailedToFundGasTank(address rpcProvider, uint256 transferAmount);
     error RPCServiceNotSet(address provider);
     error NotRPCService(address caller);
@@ -40,22 +38,14 @@ contract GasTankDepositor {
         MAXIMUM_DEPOSIT = _maxDeposit;
     }
 
-    receive() external payable { /* ETH transfers allowed. */ }
+    receive() external payable { 
+        revert Errors.InvalidReceive();
+    }
 
     fallback() external payable {
         revert Errors.InvalidFallback();
     }
 
-    /// @notice Recovers funds inadvertently sent to this contract directly.
-    function recoverFunds() external onlyRPCService {
-        uint256 balance = address(this).balance;
-
-        (bool success,) = RPC_SERVICE.call{value: balance}("");
-        require(success, FailedToRecoverFunds(RPC_SERVICE, balance));
-
-        emit FundsRecovered(RPC_SERVICE, balance);
-    }
-
     /// @notice Transfers ETH from the EOA's balance to the Gas RPC Service.
     /// @param _amount The amount to fund the gas tank with.
     /// @dev Only the EOA can call this function.

contracts/test/core/GasTankDepositorTest.sol

@@ -3,6 +3,7 @@ pragma solidity 0.8.26;
 
 import "forge-std/Test.sol";
 import {GasTankDepositor} from "../../contracts/core/GasTankDepositor.sol";
+import {Errors} from "../../contracts/utils/Errors.sol";
 
 contract GasTankDepositorTest is Test {
     uint256 public constant ALICE_PK = uint256(0xA11CE);
@@ -27,6 +28,21 @@ contract GasTankDepositorTest is Test {
 
     //=======================TESTS=======================
 
+    function testConstructorSetsValues() public view {
+        assertEq(_gasTankDepositorImpl.RPC_SERVICE(), rpcService);
+        assertEq(_gasTankDepositorImpl.MAXIMUM_DEPOSIT(), MAXIMUM_DEPOSIT);
+    }
+
+    function testConstructorRevertsWhenRpcServiceIsZero() public {
+        vm.expectRevert(abi.encodeWithSelector(GasTankDepositor.RPCServiceNotSet.selector, ZERO_ADDRESS));
+        new GasTankDepositor(ZERO_ADDRESS, MAXIMUM_DEPOSIT);
+    }
+
+    function testConstructorRevertsWhenMaxDepositIsZero() public {
+        vm.expectRevert(abi.encodeWithSelector(GasTankDepositor.MaximumDepositNotMet.selector, 0, 0));
+        new GasTankDepositor(rpcService, 0);
+    }
+
     function testSetsDelegationCodeAtAddress() public {
         // Initial code is empty
         assertEq(alice.code.length, 0);
@@ -50,62 +66,35 @@ contract GasTankDepositorTest is Test {
         assertEq(alice.code.length, 0);
     }
 
-    //=======================TESTS FOR IMPROPER CALLS TO THE GAS TANK MANAGER=======================
+    //=======================TESTS FOR IMPROPER CALLS TO THE GAS TANK DEPOSITOR=======================
 
     function testFallbackRevert() public {
-        bytes memory badData =
-            abi.encodeWithSelector(GasTankDepositor.recoverFunds.selector, address(0x55555), 1 ether, 1 ether, 1 ether);
+        bytes memory badData = abi.encodeWithSelector(bytes4(0x12345678));
         vm.prank(alice);
-        (bool success,) = address(_gasTankDepositorImpl).call{value: 1 ether}(badData);
+        (bool success, bytes memory returnData) = address(_gasTankDepositorImpl).call{value: 1 ether}(badData);
         assertFalse(success);
+        bytes4 errorSelector = bytes4(returnData);
+        assertEq(errorSelector, Errors.InvalidFallback.selector);
     }
 
-    function testReceiveNoRevert() public {
-        uint256 beforeBalance = alice.balance;
+    function testReceiveRevert() public {
         vm.prank(bob);
-        (bool success,) = address(alice).call{value: 1 ether}("");
-        assertTrue(success);
-        uint256 afterBalance = alice.balance;
-        assertEq(afterBalance, beforeBalance + 1 ether, "balance not increased");
+        (bool success, bytes memory returnData) = address(_gasTankDepositorImpl).call{value: 1 ether}("");
+        assertFalse(success);
+        bytes4 errorSelector = bytes4(returnData);
+        assertEq(errorSelector, Errors.InvalidReceive.selector);
     }
 
-    function testFundsSentDirectlyToDelegateAddress() public {
-        uint256 beforeBalance = address(_gasTankDepositorImpl).balance;
 
+    function testEOAReceiveNoRevert() public {
+        uint256 beforeBalance = alice.balance;
         vm.prank(bob);
-        (bool success,) = address(_gasTankDepositorImpl).call{value: 1 ether}("");
+        (bool success,) = address(alice).call{value: 1 ether}("");
         assertTrue(success);
-
-        uint256 afterBalance = address(_gasTankDepositorImpl).balance;
+        uint256 afterBalance = alice.balance;
         assertEq(afterBalance, beforeBalance + 1 ether, "balance not increased");
     }
 
-    function testWithdrawsFundsDirectlyFromDelegateAddress() public {
-        uint256 gasTankBeforeBalance = address(_gasTankDepositorImpl).balance;
-        uint256 depositAmount = 1 ether;
-
-        vm.prank(bob);
-        (bool success,) = address(_gasTankDepositorImpl).call{value: depositAmount}("");
-        assertTrue(success);
-
-        uint256 gasTankAfterBalance = address(_gasTankDepositorImpl).balance;
-        assertEq(gasTankAfterBalance, gasTankBeforeBalance + depositAmount, "balance not increased");
-
-        vm.prank(rpcService);
-        uint256 rpcServiceBeforeBalance = rpcService.balance;
-        _gasTankDepositorImpl.recoverFunds();
-        uint256 rpcServiceAfterBalance = rpcService.balance;
-
-        assertEq(address(_gasTankDepositorImpl).balance, 0, "funds not drained");
-        assertEq(rpcServiceAfterBalance, rpcServiceBeforeBalance + depositAmount, "balance not recovered");
-    }
-
-    function testRevertsWhenRecoverFundsIsCalledByUnknownCaller() public {
-        vm.prank(bob);
-        vm.expectRevert(abi.encodeWithSelector(GasTankDepositor.NotRPCService.selector, bob));
-        _gasTankDepositorImpl.recoverFunds();
-    }
-
     //=======================TESTS FOR FUNDING THE GAS TANK=======================
 
     function testRpcServiceFundsMaximumDeposit() public {
@@ -171,6 +160,38 @@ contract GasTankDepositorTest is Test {
         GasTankDepositor(payable(alice)).fundGasTank(MAXIMUM_DEPOSIT);
     }
 
+    function testEOAFundsWithZeroAmount() public {
+        _delegate();
+
+        uint256 rpcBalanceBefore = rpcService.balance;
+        _expectGasTankFunded(alice, 0);
+
+        vm.prank(alice);
+        GasTankDepositor(payable(alice)).fundGasTank(0);
+
+        assertEq(rpcService.balance, rpcBalanceBefore);
+    }
+
+    function testMultipleEOAFundings() public {
+        _delegate();
+
+        uint256 amount1 = 0.5 ether;
+        uint256 amount2 = 0.3 ether;
+        uint256 rpcBalanceBefore = rpcService.balance;
+
+        // First funding
+        _expectGasTankFunded(alice, amount1);
+        vm.prank(alice);
+        GasTankDepositor(payable(alice)).fundGasTank(amount1);
+
+        // Second funding
+        _expectGasTankFunded(alice, amount2);
+        vm.prank(alice);
+        GasTankDepositor(payable(alice)).fundGasTank(amount2);
+
+        assertEq(rpcService.balance, rpcBalanceBefore + amount1 + amount2);
+    }
+
     //=======================HELPERS=======================
 
     function _delegate() internal {