Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(ctb): Account for worst-case CALL gas in SafeCall #5470

Merged
merged 2 commits into from
Apr 24, 2023
Merged

feat(ctb): Account for worst-case CALL gas in SafeCall #5470

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c61224
Account for worst-case cost of `CALL` opcode in `callWithMinGas`
clabby Apr 17, 2023
8796b41
Merge branch 'develop' into clabby/ctb/call-with-min-gas-fix
mergify[bot] Apr 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l1crossdomainmessenger_more.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/l2crossdomainmessenger_more.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/optimismportal.go
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion op-bindings/bindings/optimismportal_more.go
View file
Open in desktop

Large diffs are not rendered by default.

42 changes: 21 additions & 21 deletions packages/contracts-bedrock/.gas-snapshot
View file
Open in desktop
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
View file
Open in desktop
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.
maurelian marked this conversation as resolved.
Show resolved Hide resolved
// 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
View file
Open in desktop
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.
*/
tynes marked this conversation as resolved.
Show resolved Hide resolved
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)))
tynes marked this conversation as resolved.
Show resolved Hide resolved
)
}
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
maurelian marked this conversation as resolved.
Show resolved Hide resolved
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
maurelian marked this conversation as resolved.
Show resolved Hide resolved
_target, // recipient
Expand Down
14 changes: 8 additions & 6 deletions packages/contracts-bedrock/contracts/test/SafeCall.t.sol
View file
Open in desktop
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