diff --git a/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol b/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol index 8a0ec09d7aeb8..99c9b0ca3aba0 100644 --- a/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol +++ b/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol @@ -391,10 +391,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver { // 2. The amount of gas provided to the call to the target contract is at least the gas // limit specified by the user. If there is not enough gas in the callframe to // accomplish this, `callWithMinGas` will revert. - // Additionally, we use 0 `_reservedGas` here because it does not matter if this context - // reverts with "Out of Gas." This is because the user will be able to re-attempt - // finalization of the withdrawal with a higher gas limit. - bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, 0, _tx.data); + bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data); // Reset the l2Sender back to the default value. l2Sender = Constants.DEFAULT_L2_SENDER; diff --git a/packages/contracts-bedrock/contracts/libraries/SafeCall.sol b/packages/contracts-bedrock/contracts/libraries/SafeCall.sol index 76534ab27b395..ea4cc021d473b 100644 --- a/packages/contracts-bedrock/contracts/libraries/SafeCall.sol +++ b/packages/contracts-bedrock/contracts/libraries/SafeCall.sol @@ -43,19 +43,17 @@ library SafeCall { * @param _target Address to call * @param _minGas The minimum amount of gas that may be passed to the call * @param _value Amount of value to pass to the call - * @param _reservedGas The amount of gas to reserve on top of the 40_000 gas base buffer * @param _calldata Calldata to pass to the call */ function callWithMinGas( address _target, uint256 _minGas, uint256 _value, - uint256 _reservedGas, bytes memory _calldata ) internal returns (bool) { bool _success; assembly { - // Assertion: gasleft() >= (_minGas * 64) / 63 + (40_000 + _reservedGas) + // Assertion: gasleft() >= (_minGas * 64) / 63 + 40_000 // // Because EIP-150 ensures that a maximum of 63/64ths of the remaining gas in the call // frame may be passed to a subcontext, we need to ensure that the gas will not be @@ -69,7 +67,7 @@ library SafeCall { // still possible to self-rekt by passing a minimum gas limit that does not account // for the `memory_expansion_cost + code_execution_cost` factors of the dynamic // cost of the `CALL` opcode. - if lt(gas(), add(div(mul(_minGas, 64), 63), add(40000, _reservedGas))) { + if lt(gas(), add(div(mul(_minGas, 64), 63), 40000)) { // Store the "Error(string)" selector in scratch space. mstore(0, 0x08c379a0) // Store the pointer to the string length in scratch space. @@ -96,7 +94,7 @@ library SafeCall { // factors of the dynamic cost of the `CALL` opcode), the call will receive at least // the minimum amount of gas specified. _success := call( - sub(gas(), _reservedGas), // gas + gas(), // gas _target, // recipient _value, // ether value add(_calldata, 32), // inloc diff --git a/packages/contracts-bedrock/contracts/test/SafeCall.t.sol b/packages/contracts-bedrock/contracts/test/SafeCall.t.sol index 816c18121b2ab..e4631fdfdfd7f 100644 --- a/packages/contracts-bedrock/contracts/test/SafeCall.t.sol +++ b/packages/contracts-bedrock/contracts/test/SafeCall.t.sol @@ -73,7 +73,7 @@ contract SafeCall_Test is CommonTest { vm.expectCallMinGas(to, value, minGas, data); vm.prank(from); - bool success = SafeCall.callWithMinGas(to, minGas, value, 0, data); + bool success = SafeCall.callWithMinGas(to, minGas, value, data); assertTrue(success, "call not successful"); if (from == to) { @@ -90,9 +90,9 @@ contract SafeCall_Test is CommonTest { for (uint64 i = 40_000; i < 100_000; i++) { uint256 snapshot = vm.snapshot(); - // 65_879 is the exact amount of gas required to make the safe call + // 65_867 is the exact amount of gas required to make the safe call // successfully. - if (i < 65_879) { + if (i < 65_867) { assertFalse(caller.makeSafeCall(i, 25_000)); } else { vm.expectCallMinGas( @@ -114,9 +114,9 @@ contract SafeCall_Test is CommonTest { for (uint64 i = 15_200_000; i < 15_300_000; i++) { uint256 snapshot = vm.snapshot(); - // 15,278_578 is the exact amount of gas required to make the safe call + // 15_278_566 is the exact amount of gas required to make the safe call // successfully. - if (i < 15_278_578) { + if (i < 15_278_566) { assertFalse(caller.makeSafeCall(i, 15_000_000)); } else { vm.expectCallMinGas( @@ -152,7 +152,6 @@ contract SimpleSafeCaller { address(this), minGas, 0, - 0, abi.encodeWithSelector(this.setA.selector, 1) ); } diff --git a/packages/contracts-bedrock/contracts/test/invariants/SafeCall.t.sol b/packages/contracts-bedrock/contracts/test/invariants/SafeCall.t.sol index 4a15e7945cfac..9715f7b6d8d4e 100644 --- a/packages/contracts-bedrock/contracts/test/invariants/SafeCall.t.sol +++ b/packages/contracts-bedrock/contracts/test/invariants/SafeCall.t.sol @@ -36,10 +36,9 @@ contract SafeCall_Succeeds_Invariants is Test { function performSafeCallMinGas( address to, - uint64 minGas, - uint16 reservedGas + uint64 minGas ) external payable { - SafeCall.callWithMinGas(to, minGas, msg.value, reservedGas, hex""); + SafeCall.callWithMinGas(to, minGas, msg.value, hex""); } } @@ -74,10 +73,9 @@ contract SafeCall_Fails_Invariants is Test { function performSafeCallMinGas( address to, - uint64 minGas, - uint16 reservedGas + uint64 minGas ) external payable { - SafeCall.callWithMinGas(to, minGas, msg.value, reservedGas, hex""); + SafeCall.callWithMinGas(to, minGas, msg.value, hex""); } } @@ -96,8 +94,7 @@ contract SafeCaller_Actor is StdUtils { uint64 gas, uint64 minGas, address to, - uint8 value, - uint16 reservedGas + uint8 value ) external { // Only send to EOAs - we exclude the console as it has no code but reverts when called // with a selector that doesn't exist due to the foundry hook. @@ -110,11 +107,11 @@ contract SafeCaller_Actor is StdUtils { gas = uint64(bound(gas, minGas, (minGas * 64) / 63)); } else { // Bound the gas passed to - // [((minGas * 64) / 63) + 40_000 + reservedGas + 1000, type(uint64).max] + // [((minGas * 64) / 63) + 40_000 + 1000, type(uint64).max] // The extra 1000 gas is to account for the gas used by the `SafeCall.call` call // itself. gas = uint64( - bound(gas, ((minGas * 64) / 63) + 40_000 + reservedGas + 1000, type(uint64).max) + bound(gas, ((minGas * 64) / 63) + 40_000 + 1000, type(uint64).max) ); } @@ -126,8 +123,7 @@ contract SafeCaller_Actor is StdUtils { abi.encodeWithSelector( SafeCall_Succeeds_Invariants.performSafeCallMinGas.selector, to, - minGas, - reservedGas + minGas ) ); diff --git a/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol b/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol index 683f2926cc4c7..71fd685ba62af 100644 --- a/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol +++ b/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol @@ -143,6 +143,18 @@ abstract contract CrossDomainMessenger is */ uint64 public constant MIN_GAS_CALLDATA_OVERHEAD = 16; + /** + * @notice Gas reserved for finalizing the execution of `relayMessage` after the safe call + * as well as paying for the call itself. + * @dev The worst-case cost of the CALL opcode is + * `34_300 + memory_expansion_cost + code_execution_cost` gas, and the code following + * the `SafeCall.call` call in `relayMessage` costs around 20_000 gas. This buffer is + * conservative to combat minimum gas limit griefing attacks. If there is not enough + * gas remaining to cover the reserved gas, the message will be able to be replayed + * with a higher gas limit. + */ + uint64 public constant RELAY_RESERVED_GAS = 80_000; + /** * @notice Address of the paired CrossDomainMessenger contract on the other chain. */ @@ -347,19 +359,19 @@ abstract contract CrossDomainMessenger is "CrossDomainMessenger: message has already been relayed" ); + // If there is not enough gas left to perform the external call and finish the execution, + // silently fail and assign the message to the failedMessages mapping. // If `xDomainMsgSender` is not the default L2 sender, this function // is being re-entered. This marks the message as failed to allow it // to be replayed. - if (xDomainMsgSender != Constants.DEFAULT_L2_SENDER) { + if (gasleft() < _minGasLimit + RELAY_RESERVED_GAS || xDomainMsgSender != Constants.DEFAULT_L2_SENDER) { failedMessages[versionedHash] = true; emit FailedRelayedMessage(versionedHash); return; } xDomainMsgSender = _sender; - // Add an extra 30_000 reserved gas to retain after the safe call to ensure that there - // is enough gas to finish the execution within this context afterwards. - bool success = SafeCall.callWithMinGas(_target, _minGasLimit, _value, 30_000, _message); + bool success = SafeCall.call(_target, gasleft(), _value, _message); xDomainMsgSender = Constants.DEFAULT_L2_SENDER; if (success) {