Merge pull request #5529 from ethereum-optimism/fix/min-gaslimit-deposit

contracts-bedrock: update portal min gas limit

packages/contracts-bedrock/contracts/L1/OptimismPortal.sol

@@ -140,7 +140,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
     }
 
     /**
-     * @custom:semver 1.4.0
+     * @custom:semver 1.5.0
      *
      * @param _l2Oracle                  Address of the L2OutputOracle contract.
      * @param _guardian                  Address that can pause deposits and withdrawals.
@@ -152,7 +152,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         address _guardian,
         bool _paused,
         SystemConfig _config
-    ) Semver(1, 4, 0) {
+    ) Semver(1, 5, 0) {
         L2_ORACLE = _l2Oracle;
         GUARDIAN = _guardian;
         SYSTEM_CONFIG = _config;
@@ -186,6 +186,17 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
         emit Unpaused(msg.sender);
     }
 
+    /**
+     * @notice Computes the minimum gas limit for a deposit. The minimum gas limit
+     *         linearly increases based on the size of the calldata. This is to prevent
+     *         users from creating L2 resource usage without paying for it. This function
+     *         can be used when interacting with the portal to ensure forwards compatibility.
+     *
+     */
+    function minimumGasLimit(uint64 _byteCount) public pure returns (uint64) {
+        return _byteCount * 16 + 21000;
+    }
+
     /**
      * @notice Accepts value so that users can send ETH directly to this contract and have the
      *         funds be deposited to their address on L2. This is intended as a convenience
@@ -436,8 +447,18 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
             );
         }
 
-        // Prevent depositing transactions that have too small of a gas limit.
-        require(_gasLimit >= 21_000, "OptimismPortal: gas limit must cover instrinsic gas cost");
+        // Prevent depositing transactions that have too small of a gas limit. Users should pay
+        // more for more resource usage.
+        require(
+            _gasLimit >= minimumGasLimit(uint64(_data.length)),
+            "OptimismPortal: gas limit too small"
+        );
+
+        // Prevent the creation of deposit transactions that have too much calldata. This gives an
+        // upper limit on the size of unsafe blocks over the p2p network. 120kb is chosen to ensure
+        // that the transaction can fit into the p2p network policy of 128kb even though deposit
+        // transactions are not gossipped over the p2p network.
+        require(_data.length <= 120_000, "OptimismPortal: data too large");
 
         // Transform the from-address to its alias if the caller is a contract.
         address from = msg.sender;

packages/contracts-bedrock/contracts/test/CrossDomainMessenger.t.sol

@@ -22,6 +22,21 @@ contract CrossDomainMessenger_BaseGas_Test is Messenger_Initializer {
     function testFuzz_baseGas_succeeds(uint32 _minGasLimit) external view {
         L1Messenger.baseGas(hex"ff", _minGasLimit);
     }
+
+    /**
+     * @notice The baseGas function should always return a value greater than
+     *         or equal to the minimum gas limit value on the OptimismPortal.
+     *         This guarantees that the messengers will always pass sufficient
+     *         gas to the OptimismPortal.
+     */
+    function testFuzz_baseGas_portalMinGasLimit_succeeds(bytes memory _data, uint32 _minGasLimit)
+        external
+    {
+        vm.assume(_data.length <= type(uint64).max);
+        uint64 baseGas = L1Messenger.baseGas(_data, _minGasLimit);
+        uint64 minGasLimit = op.minimumGasLimit(uint64(_data.length));
+        assertTrue(baseGas >= minGasLimit);
+    }
 }
 
 /**

packages/contracts-bedrock/contracts/test/CrossDomainOwnable.t.sol

@@ -56,7 +56,7 @@ contract CrossDomainOwnableThroughPortal_Test is Portal_Initializer {
         op.depositTransaction({
             _to: address(setter),
             _value: 0,
-            _gasLimit: 21_000,
+            _gasLimit: 30_000,
             _isCreation: false,
             _data: abi.encodeWithSelector(XDomainSetter.set.selector, 1)
         });

packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol

@@ -110,12 +110,29 @@ contract OptimismPortal_Test is Portal_Initializer {
         op.depositTransaction(address(1), 1, 0, true, hex"");
     }
 
+    /**
+     * @notice Prevent deposits from being too large to have a sane upper bound
+     *         on unsafe blocks sent over the p2p network.
+     */
+    function test_depositTransaction_largeData_reverts() external {
+        uint256 size = 120_001;
+        uint64 gasLimit = op.minimumGasLimit(uint64(size));
+        vm.expectRevert("OptimismPortal: data too large");
+        op.depositTransaction({
+            _to: address(0),
+            _value: 0,
+            _gasLimit: gasLimit,
+            _isCreation: false,
+            _data: new bytes(size)
+        });
+    }
+
     /**
      * @notice Prevent gasless deposits from being force processed in L2 by
      *         ensuring that they have a large enough gas limit set.
      */
     function test_depositTransaction_smallGasLimit_reverts() external {
-        vm.expectRevert("OptimismPortal: gas limit must cover instrinsic gas cost");
+        vm.expectRevert("OptimismPortal: gas limit too small");
         op.depositTransaction({
             _to: address(1),
             _value: 0,
@@ -125,6 +142,40 @@ contract OptimismPortal_Test is Portal_Initializer {
         });
     }
 
+    /**
+     * @notice Fuzz for too small of gas limits
+     */
+    function testFuzz_depositTransaction_smallGasLimit_succeeds(
+        bytes memory _data,
+        bool _shouldFail
+    ) external {
+        vm.assume(_data.length <= type(uint64).max);
+
+        uint64 gasLimit = op.minimumGasLimit(uint64(_data.length));
+        if (_shouldFail) {
+            gasLimit = uint64(bound(gasLimit, 0, gasLimit - 1));
+            vm.expectRevert("OptimismPortal: gas limit too small");
+        }
+
+        op.depositTransaction({
+            _to: address(0x40),
+            _value: 0,
+            _gasLimit: gasLimit,
+            _isCreation: false,
+            _data: _data
+        });
+    }
+
+    /**
+     * @notice Ensure that the 0 calldata case is covered and there is a linearly
+     *         increasing gas limit for larger calldata sizes.
+     */
+    function test_minimumGasLimit_succeeds() external {
+        assertEq(op.minimumGasLimit(0), 21_000);
+        assertTrue(op.minimumGasLimit(2) > op.minimumGasLimit(1));
+        assertTrue(op.minimumGasLimit(3) > op.minimumGasLimit(2));
+    }
+
     // Test: depositTransaction should emit the correct log when an EOA deposits a tx with 0 value
     function test_depositTransaction_noValueEOA_succeeds() external {
         // EOA emulation