Skip to content

Commit

Permalink
Account for worst-case cost of CALL opcode in callWithMinGas
Browse files Browse the repository at this point in the history
  • Loading branch information
clabby committed Apr 24, 2023
1 parent ed42bc5 commit 1c61224
Show file tree
Hide file tree
Showing 12 changed files with 121 additions and 74 deletions.
2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger_more.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger_more.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/optimismportal.go

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/optimismportal_more.go

Large diffs are not rendered by default.

42 changes: 21 additions & 21 deletions packages/contracts-bedrock/.gas-snapshot
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@ Bytes_toNibbles_Test:test_toNibbles_expectedResult5Bytes_works() (gas: 6132)
Bytes_toNibbles_Test:test_toNibbles_zeroLengthInput_works() (gas: 944)
CrossDomainMessenger_BaseGas_Test:test_baseGas_succeeds() (gas: 20097)
CrossDomainOwnable2_Test:test_onlyOwner_notMessenger_reverts() (gas: 8416)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 57254)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 57323)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16566)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 73282)
CrossDomainOwnable2_Test:test_onlyOwner_succeeds() (gas: 73351)
CrossDomainOwnable3_Test:test_constructor_succeeds() (gas: 10554)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notMessenger_reverts() (gas: 28334)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 73730)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner2_reverts() (gas: 73799)
CrossDomainOwnable3_Test:test_crossDomainOnlyOwner_notOwner_reverts() (gas: 31978)
CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 91287)
CrossDomainOwnable3_Test:test_crossDomainTransferOwnership_succeeds() (gas: 91356)
CrossDomainOwnable3_Test:test_localOnlyOwner_notOwner_reverts() (gas: 13193)
CrossDomainOwnable3_Test:test_localOnlyOwner_succeeds() (gas: 35220)
CrossDomainOwnable3_Test:test_localTransferOwnership_succeeds() (gas: 52128)
Expand Down Expand Up @@ -70,18 +70,18 @@ L1BlockTest:test_timestamp_succeeds() (gas: 7640)
L1BlockTest:test_updateValues_succeeds() (gas: 60482)
L1CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 24715)
L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 49394)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 209286)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203184)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 123784)
L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77098)
L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197091)
L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 74034)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 209424)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailure_succeeds() (gas: 203322)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterSuccess_reverts() (gas: 123853)
L1CrossDomainMessenger_Test:test_relayMessage_legacy_succeeds() (gas: 77167)
L1CrossDomainMessenger_Test:test_relayMessage_retryAfterFailure_succeeds() (gas: 197229)
L1CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 74103)
L1CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 56540)
L1CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 12365)
L1CrossDomainMessenger_Test:test_replayMessage_withValue_reverts() (gas: 31063)
L1CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 304740)
L1CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 1496124)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84563)
L1CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 84632)
L1CrossDomainMessenger_Test:test_xDomainSender_notSet_reverts() (gas: 24296)
L1ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 52707)
L1ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 27310)
Expand Down Expand Up @@ -118,13 +118,13 @@ L1StandardBridge_Getter_Test:test_getters_succeeds() (gas: 32173)
L1StandardBridge_Initialize_Test:test_initialize_succeeds() (gas: 22050)
L1StandardBridge_Receive_Test:test_receive_succeeds() (gas: 525438)
L2CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 8411)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 163159)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 48640)
L2CrossDomainMessenger_Test:test_relayMessage_retry_succeeds() (gas: 163297)
L2CrossDomainMessenger_Test:test_relayMessage_succeeds() (gas: 48709)
L2CrossDomainMessenger_Test:test_relayMessage_toSystemContract_reverts() (gas: 29021)
L2CrossDomainMessenger_Test:test_relayMessage_v2_reverts() (gas: 11711)
L2CrossDomainMessenger_Test:test_sendMessage_succeeds() (gas: 122508)
L2CrossDomainMessenger_Test:test_sendMessage_twice_succeeds() (gas: 134826)
L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 48139)
L2CrossDomainMessenger_Test:test_xDomainMessageSender_reset_succeeds() (gas: 48208)
L2CrossDomainMessenger_Test:test_xDomainSender_senderNotSet_reverts() (gas: 10590)
L2ERC721Bridge_Test:test_bridgeERC721To_localTokenZeroAddress_reverts() (gas: 26431)
L2ERC721Bridge_Test:test_bridgeERC721To_remoteTokenZeroAddress_reverts() (gas: 21814)
Expand Down Expand Up @@ -264,13 +264,13 @@ OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutp
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 207497)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 41753)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 199441)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 205795)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 205862)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 180206)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 243812)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245528)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 243881)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 245597)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_paused_reverts() (gas: 53576)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 234941)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163864)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 235010)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163867)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 197019)
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 85690)
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidWithdrawalProof_reverts() (gas: 137350)
Expand Down Expand Up @@ -404,8 +404,8 @@ ResourceMetering_Test:test_meter_updateTenEmptyBlocks_succeeds() (gas: 23747)
ResourceMetering_Test:test_meter_updateTwoEmptyBlocks_succeeds() (gas: 23703)
ResourceMetering_Test:test_meter_useMax_succeeds() (gas: 20020816)
ResourceMetering_Test:test_meter_useMoreThanMax_reverts() (gas: 19549)
SafeCall_call_Test:test_callWithMinGas_noLeakageHigh_succeeds() (gas: 2075873614)
SafeCall_call_Test:test_callWithMinGas_noLeakageLow_succeeds() (gas: 753665282)
SafeCall_Test:test_callWithMinGas_noLeakageHigh_succeeds() (gas: 1020974534)
SafeCall_Test:test_callWithMinGas_noLeakageLow_succeeds() (gas: 1094812728)
Semver_Test:test_behindProxy_succeeds() (gas: 506748)
Semver_Test:test_version_succeeds() (gas: 9418)
SequencerFeeVault_Test:test_constructor_succeeds() (gas: 5526)
Expand Down
12 changes: 5 additions & 7 deletions packages/contracts-bedrock/contracts/L1/OptimismPortal.sol
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
}

/**
* @custom:semver 1.3.1
* @custom:semver 1.4.0
*
* @param _l2Oracle Address of the L2OutputOracle contract.
* @param _guardian Address that can pause deposits and withdrawals.
Expand All @@ -152,7 +152,7 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
address _guardian,
bool _paused,
SystemConfig _config
) Semver(1, 3, 1) {
) Semver(1, 4, 0) {
L2_ORACLE = _l2Oracle;
GUARDIAN = _guardian;
SYSTEM_CONFIG = _config;
Expand Down Expand Up @@ -388,11 +388,9 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
// SafeCall.callWithMinGas to ensure two key properties
// 1. Target contracts cannot force this call to run out of gas by returning a very large
// amount of data (and this is OK because we don't care about the returndata here).
// 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, if there is not enough gas remaining to complete the execution after the
// call returns, this function will revert.
// 2. The amount of gas provided to the execution context of the target is at least the
// gas limit specified by the user. If there is not enough gas in the current context
// to accomplish this, `callWithMinGas` will revert.
bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

// Reset the l2Sender back to the default value.
Expand Down
59 changes: 43 additions & 16 deletions packages/contracts-bedrock/contracts/libraries/SafeCall.sol
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,41 @@ library SafeCall {
return _success;
}

/**
* @notice Helper function to determine if there is sufficient gas remaining within the context
* to guarantee that the minimum gas requirement for a call will be met as well as
* optionally reserving a specified amount of gas for after the call has concluded.
* @param _minGas The minimum amount of gas that may be passed to the target context.
* @param _reservedGas Optional amount of gas to reserve for the caller after the execution
* of the target context.
* @return `true` if there is enough gas remaining to safely supply `_minGas` to the target
* context as well as reserve `_reservedGas` for the caller after the execution of
* the target context.
* @dev !!!!! FOOTGUN ALERT !!!!!
* 1.) The 40_000 base buffer is to account for the worst case of the dynamic cost of the
* `CALL` opcode's `address_access_cost`, `positive_value_cost`, and
* `value_to_empty_account_cost` factors with an added buffer of 5,700 gas. It is
* still possible to self-rekt by initiating a withdrawal with 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.
* 2.) This function should *directly* precede the external call if possible. There is an
* added buffer to account for gas consumed between this check and the call, but it
* is only 5,700 gas.
* 3.) 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
* truncated.
* 4.) Use wisely. This function is not a silver bullet.
*/
function hasMinGas(uint256 _minGas, uint256 _reservedGas) internal view returns (bool) {
bool _hasMinGas;
assembly {
_hasMinGas := iszero(
lt(gas(), add(div(mul(_minGas, 64), 63), add(40000, _reservedGas)))
)
}
return _hasMinGas;
}

/**
* @notice Perform a low level call without copying any returndata. This function
* will revert if the call cannot be performed with the specified minimum
Expand All @@ -52,16 +87,10 @@ library SafeCall {
bytes memory _calldata
) internal returns (bool) {
bool _success;
bool _hasMinGas = hasMinGas(_minGas, 0);
assembly {
// Assertion: gasleft() >= ((_minGas + 200) * 64) / 63
//
// 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
// truncated to hold this function's invariant: "If a call is performed by
// `callWithMinGas`, it must receive at least the specified minimum gas limit." In
// addition, exactly 51 gas is consumed between the below `GAS` opcode and the `CALL`
// opcode, so it is factored in with some extra room for error.
if lt(gas(), div(mul(64, add(_minGas, 200)), 63)) {
// Assertion: gasleft() >= (_minGas * 64) / 63 + 40_000
if iszero(_hasMinGas) {
// Store the "Error(string)" selector in scratch space.
mstore(0, 0x08c379a0)
// Store the pointer to the string length in scratch space.
Expand All @@ -82,13 +111,11 @@ library SafeCall {
revert(28, 100)
}

// The call will be supplied at least (((_minGas + 200) * 64) / 63) - 49 gas due to the
// above assertion. This ensures that, in all circumstances, the call will
// receive at least the minimum amount of gas specified.
// We can prove this property by solving the inequalities:
// ((((_minGas + 200) * 64) / 63) - 49) >= _minGas
// ((((_minGas + 200) * 64) / 63) - 51) * (63 / 64) >= _minGas
// Both inequalities hold true for all possible values of `_minGas`.
// The call will be supplied at least ((_minGas * 64) / 63) gas due to the
// above assertion. This ensures that, in all circumstances (except for when the
// `_minGas` does not account for the `memory_expansion_cost` and `code_execution_cost`
// factors of the dynamic cost of the `CALL` opcode), the call will receive at least
// the minimum amount of gas specified.
_success := call(
gas(), // gas
_target, // recipient
Expand Down
14 changes: 8 additions & 6 deletions packages/contracts-bedrock/contracts/test/SafeCall.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ pragma solidity 0.8.15;
import { CommonTest } from "./CommonTest.t.sol";
import { SafeCall } from "../libraries/SafeCall.sol";

contract SafeCall_call_Test is CommonTest {
contract SafeCall_Test is CommonTest {
function testFuzz_call_succeeds(
address from,
address to,
Expand Down Expand Up @@ -63,6 +63,8 @@ contract SafeCall_call_Test is CommonTest {
vm.assume(to != address(0x000000000000000000636F6e736F6c652e6c6f67));
// don't call the create2 deployer
vm.assume(to != address(0x4e59b44847b379578588920cA78FbF26c0B4956C));
// don't call the FFIInterface
vm.assume(to != address(0x5615dEB798BB3E4dFa0139dFa1b3D433Cc23b72f));

assertEq(from.balance, 0, "from balance is 0");
vm.deal(from, value);
Expand All @@ -89,12 +91,12 @@ contract SafeCall_call_Test is CommonTest {
function test_callWithMinGas_noLeakageLow_succeeds() external {
SimpleSafeCaller caller = new SimpleSafeCaller();

for (uint64 i = 5000; i < 50_000; i++) {
for (uint64 i = 40_000; i < 100_000; i++) {
uint256 snapshot = vm.snapshot();

// 26,071 is the exact amount of gas required to make the safe call
// 65_903 is the exact amount of gas required to make the safe call
// successfully.
if (i < 26_071) {
if (i < 65_903) {
assertFalse(caller.makeSafeCall(i, 25_000));
} else {
vm.expectCallMinGas(
Expand All @@ -116,9 +118,9 @@ contract SafeCall_call_Test is CommonTest {
for (uint64 i = 15_200_000; i < 15_300_000; i++) {
uint256 snapshot = vm.snapshot();

// 15,238,769 is the exact amount of gas required to make the safe call
// 15_278_602 is the exact amount of gas required to make the safe call
// successfully.
if (i < 15_238_769) {
if (i < 15_278_602) {
assertFalse(caller.makeSafeCall(i, 15_000_000));
} else {
vm.expectCallMinGas(
Expand Down
Loading

0 comments on commit 1c61224

Please sign in to comment.