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

fix(ctb): Fix finalizeWithdrawalTransaction gas dos #4954

Closed
wants to merge 4 commits into from
Closed

fix(ctb): Fix finalizeWithdrawalTransaction gas dos #4954

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb1151e
Initial changes + sketchy tests
clabby Feb 23, 2023
673b346
Resolve conflicts
clabby Feb 27, 2023
9254946
Update tests
clabby Feb 27, 2023
134816d
Beef up invariant test to fuzz the amount of gas sent to `finalizeWit…
clabby Feb 27, 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/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.

84 changes: 43 additions & 41 deletions packages/contracts-bedrock/.gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,3 @@
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 261340)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 76067)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 348292)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 112945)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 348314)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 112965)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 40409)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 88535)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 75075)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 36386)
GasBenchMark_OptimismPortal:test_proveWithdrawalTransaction_benchmark() (gas: 169229)
Bytes_slice_Test:test_slice_acrossMultipleWords_works() (gas: 9423)
Bytes_slice_Test:test_slice_acrossWords_works() (gas: 1418)
Bytes_slice_Test:test_slice_fromNonZeroIdx_works() (gas: 17154)
Expand All @@ -17,9 +6,6 @@ Bytes_toNibbles_Test:test_toNibbles_expectedResult128Bytes_works() (gas: 129874)
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)
CrossDomainOwnableThroughPortal_Test:test_depositTransaction_crossDomainOwner_succeeds() (gas: 72494)
CrossDomainOwnable_Test:test_onlyOwner_notOwner_reverts() (gas: 10597)
CrossDomainOwnable_Test:test_onlyOwner_succeeds() (gas: 34883)
CrossDomainOwnable2_Test:test_onlyOwner_notMessenger_reverts() (gas: 8416)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner2_reverts() (gas: 62010)
CrossDomainOwnable2_Test:test_onlyOwner_notOwner_reverts() (gas: 16566)
Expand All @@ -36,10 +22,24 @@ CrossDomainOwnable3_Test:test_transferOwnershipNoLocal_succeeds() (gas: 48610)
CrossDomainOwnable3_Test:test_transferOwnership_noLocalZeroAddress_reverts() (gas: 12015)
CrossDomainOwnable3_Test:test_transferOwnership_notOwner_reverts() (gas: 13437)
CrossDomainOwnable3_Test:test_transferOwnership_zeroAddress_reverts() (gas: 12081)
CrossDomainOwnableThroughPortal_Test:test_depositTransaction_crossDomainOwner_succeeds() (gas: 72494)
CrossDomainOwnable_Test:test_onlyOwner_notOwner_reverts() (gas: 10597)
CrossDomainOwnable_Test:test_onlyOwner_succeeds() (gas: 34883)
DeployerWhitelist_Test:test_owner_succeeds() (gas: 7582)
DeployerWhitelist_Test:test_storageSlots_succeeds() (gas: 33395)
FeeVault_Test:test_constructor_succeeds() (gas: 10736)
FeeVault_Test:test_minWithdrawalAmount_succeeds() (gas: 10713)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_0() (gas: 261340)
GasBenchMark_L1CrossDomainMessenger:test_sendMessage_benchmark_1() (gas: 76067)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_0() (gas: 348292)
GasBenchMark_L1StandardBridge_Deposit:test_depositERC20_benchmark_1() (gas: 112945)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_0() (gas: 348314)
GasBenchMark_L1StandardBridge_Deposit:test_depositETH_benchmark_1() (gas: 112965)
GasBenchMark_L1StandardBridge_Finalize:test_finalizeETHWithdrawal_benchmark() (gas: 40409)
GasBenchMark_L2OutputOracle:test_proposeL2Output_benchmark() (gas: 88535)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark() (gas: 75075)
GasBenchMark_OptimismPortal:test_depositTransaction_benchmark_1() (gas: 36386)
GasBenchMark_OptimismPortal:test_proveWithdrawalTransaction_benchmark() (gas: 169229)
GasPriceOracle_Test:test_baseFee_succeeds() (gas: 8325)
GasPriceOracle_Test:test_decimals_succeeds() (gas: 6167)
GasPriceOracle_Test:test_gasPrice_succeeds() (gas: 8294)
Expand All @@ -59,15 +59,15 @@ GovernanceToken_Test:test_mint_fromOwner_succeeds() (gas: 108592)
GovernanceToken_Test:test_transferFrom_succeeds() (gas: 146273)
GovernanceToken_Test:test_transfer_succeeds() (gas: 138108)
Hashing_hashDepositSource_Test:test_hashDepositSource_succeeds() (gas: 633)
L1BlockNumberTest:test_fallback_succeeds() (gas: 18655)
L1BlockNumberTest:test_getL1BlockNumber_succeeds() (gas: 10625)
L1BlockNumberTest:test_receive_succeeds() (gas: 25384)
L1BlockTest:test_basefee_succeeds() (gas: 7554)
L1BlockTest:test_hash_succeeds() (gas: 7576)
L1BlockTest:test_number_succeeds() (gas: 7629)
L1BlockTest:test_sequenceNumber_succeeds() (gas: 7630)
L1BlockTest:test_timestamp_succeeds() (gas: 7640)
L1BlockTest:test_updateValues_succeeds() (gas: 60482)
L1BlockNumberTest:test_fallback_succeeds() (gas: 18655)
L1BlockNumberTest:test_getL1BlockNumber_succeeds() (gas: 10625)
L1BlockNumberTest:test_receive_succeeds() (gas: 25384)
L1CrossDomainMessenger_Test:test_messageVersion_succeeds() (gas: 24715)
L1CrossDomainMessenger_Test:test_relayMessage_legacyOldReplay_reverts() (gas: 49394)
L1CrossDomainMessenger_Test:test_relayMessage_legacyRetryAfterFailureThenSuccess_reverts() (gas: 228293)
Expand Down Expand Up @@ -242,19 +242,19 @@ OptimismMintableERC20_Test:test_mint_notBridge_reverts() (gas: 11121)
OptimismMintableERC20_Test:test_mint_succeeds() (gas: 63566)
OptimismMintableERC20_Test:test_remoteToken_succeeds() (gas: 7689)
OptimismMintableERC20_Test:test_semver_succeeds() (gas: 8812)
OptimismMintableTokenFactory_Test:test_bridge_succeeds() (gas: 7580)
OptimismMintableTokenFactory_Test:test_createStandardL2Token_remoteIsZero_succeeds() (gas: 9390)
OptimismMintableTokenFactory_Test:test_createStandardL2Token_sameTwice_succeeds() (gas: 2523203)
OptimismMintableTokenFactory_Test:test_createStandardL2Token_succeeds() (gas: 1268564)
OptimismMintableERC721Factory_Test:test_constructor_succeeds() (gas: 8285)
OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_succeeds() (gas: 2336687)
OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_zeroRemoteToken_reverts() (gas: 9418)
OptimismMintableERC721_Test:test_burn_notBridge_reverts() (gas: 136966)
OptimismMintableERC721_Test:test_burn_succeeds() (gas: 118832)
OptimismMintableERC721_Test:test_constructor_succeeds() (gas: 28279)
OptimismMintableERC721_Test:test_safeMint_notBridge_reverts() (gas: 11098)
OptimismMintableERC721_Test:test_safeMint_succeeds() (gas: 140524)
OptimismMintableERC721_Test:test_tokenURI_succeeds() (gas: 163442)
OptimismMintableERC721Factory_Test:test_constructor_succeeds() (gas: 8285)
OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_succeeds() (gas: 2336687)
OptimismMintableERC721Factory_Test:test_createOptimismMintableERC721_zeroRemoteToken_reverts() (gas: 9418)
OptimismMintableTokenFactory_Test:test_bridge_succeeds() (gas: 7580)
OptimismMintableTokenFactory_Test:test_createStandardL2Token_remoteIsZero_succeeds() (gas: 9390)
OptimismMintableTokenFactory_Test:test_createStandardL2Token_sameTwice_succeeds() (gas: 2523203)
OptimismMintableTokenFactory_Test:test_createStandardL2Token_succeeds() (gas: 1268564)
OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 11016)
OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15940)
OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_succeeds() (gas: 16056)
Expand All @@ -263,13 +263,13 @@ OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutp
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 203373)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 41798)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 198396)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 199219)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 202497)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 179139)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 237848)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 239372)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 238142)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 239666)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_paused_reverts() (gas: 53621)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 230921)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163739)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_succeeds() (gas: 231215)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 8797746687696163771)
OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 195098)
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 85690)
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidWithdrawalProof_reverts() (gas: 137350)
Expand All @@ -279,6 +279,8 @@ OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayPro
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProveChangedOutputRoot_succeeds() (gas: 279593)
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_replayProve_reverts() (gas: 192570)
OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_succeeds() (gas: 180486)
OptimismPortal_MinGasLimit_Test:test_finalizeWithdrawalTransaction_minGasLimitCheckGap_succeeds() (gas: 1045451943)
OptimismPortal_MinGasLimit_Test:test_finalizeWithdrawalTransaction_minGasLimitEIP150_succeeds() (gas: 1045581787)
OptimismPortal_Test:test_constructor_succeeds() (gas: 20672)
OptimismPortal_Test:test_depositTransaction_contractCreation_reverts() (gas: 14320)
OptimismPortal_Test:test_depositTransaction_createWithZeroValueForContract_succeeds() (gas: 76748)
Expand All @@ -297,17 +299,6 @@ OptimismPortal_Test:test_receive_succeeds() (gas: 127554)
OptimismPortal_Test:test_simple_isOutputFinalized_succeeds() (gas: 24232)
OptimismPortal_Test:test_unpause_onlyGuardian_reverts() (gas: 46151)
OptimismPortal_Test:test_unpause_succeeds() (gas: 31780)
Proxy_Test:test_delegatesToImpl_succeeds() (gas: 45207)
Proxy_Test:test_implementationKey_succeeds() (gas: 20909)
Proxy_Test:test_implementation_isZeroAddress_reverts() (gas: 47626)
Proxy_Test:test_implementation_zeroAddressCaller_succeeds() (gas: 14752)
Proxy_Test:test_ownerKey_succeeds() (gas: 19059)
Proxy_Test:test_ownerProxyCall_notAdmin_succeeds() (gas: 34615)
Proxy_Test:test_proxyCallToImp_notAdmin_succeeds() (gas: 30008)
Proxy_Test:test_upgradeToAndCall_functionDoesNotExist_reverts() (gas: 104565)
Proxy_Test:test_upgradeToAndCall_isPayable_succeeds() (gas: 53742)
Proxy_Test:test_upgradeToAndCall_succeeds() (gas: 125190)
Proxy_Test:test_upgradeTo_clashingFunctionSignatures_succeeds() (gas: 101359)
ProxyAdmin_Test:test_chugsplashChangeProxyAdmin_succeeds() (gas: 35586)
ProxyAdmin_Test:test_chugsplashGetProxyAdmin_succeeds() (gas: 15675)
ProxyAdmin_Test:test_chugsplashGetProxyImplementation_succeeds() (gas: 51084)
Expand All @@ -331,6 +322,17 @@ ProxyAdmin_Test:test_setAddressManager_notOwner_reverts() (gas: 10578)
ProxyAdmin_Test:test_setImplementationName_notOwner_reverts() (gas: 11111)
ProxyAdmin_Test:test_setImplementationName_succeeds() (gas: 38945)
ProxyAdmin_Test:test_setProxyType_notOwner_reverts() (gas: 10814)
Proxy_Test:test_delegatesToImpl_succeeds() (gas: 45207)
Proxy_Test:test_implementationKey_succeeds() (gas: 20909)
Proxy_Test:test_implementation_isZeroAddress_reverts() (gas: 47626)
Proxy_Test:test_implementation_zeroAddressCaller_succeeds() (gas: 14752)
Proxy_Test:test_ownerKey_succeeds() (gas: 19059)
Proxy_Test:test_ownerProxyCall_notAdmin_succeeds() (gas: 34615)
Proxy_Test:test_proxyCallToImp_notAdmin_succeeds() (gas: 30008)
Proxy_Test:test_upgradeToAndCall_functionDoesNotExist_reverts() (gas: 104565)
Proxy_Test:test_upgradeToAndCall_isPayable_succeeds() (gas: 53742)
Proxy_Test:test_upgradeToAndCall_succeeds() (gas: 125190)
Proxy_Test:test_upgradeTo_clashingFunctionSignatures_succeeds() (gas: 101359)
RLPReader_readBytes_Test:test_readBytes_bytestring00_succeeds() (gas: 1878)
RLPReader_readBytes_Test:test_readBytes_bytestring01_succeeds() (gas: 1855)
RLPReader_readBytes_Test:test_readBytes_bytestring7f_succeeds() (gas: 1876)
Expand Down Expand Up @@ -413,4 +415,4 @@ SystemConfig_Setters_TestFail:test_setGasConfig_notOwner_reverts() (gas: 10555)
SystemConfig_Setters_TestFail:test_setGasLimit_notOwner_reverts() (gas: 10658)
SystemConfig_Setters_TestFail:test_setUnsafeBlockSigner_notOwner_reverts() (gas: 10660)
TransferOnionTest:test_constructor_succeeds() (gas: 564855)
TransferOnionTest:test_unwrap_succeeds() (gas: 724958)
TransferOnionTest:test_unwrap_succeeds() (gas: 724958)
61 changes: 44 additions & 17 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 @@ -48,6 +48,12 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
*/
uint256 internal constant FINALIZE_GAS_BUFFER = 20_000;

/**
* @notice The amount of gas consumed between the check for remaining gas and the actual
* `CALL` opcode in the `_safeCall` function.
*/
uint256 internal constant GAS_CHECK_BUFFER = 230;

/**
* @notice Minimum time (in seconds) that must elapse before a withdrawal can be finalized.
*/
Expand Down Expand Up @@ -371,26 +377,11 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
// Mark the withdrawal as finalized so it can't be replayed.
finalizedWithdrawals[withdrawalHash] = true;

// We want to maintain the property that the amount of gas supplied to the call to the
// target contract is at least the gas limit specified by the user. We can do this by
// enforcing that, at this point in time, we still have gaslimit + buffer gas available.
require(
gasleft() >= _tx.gasLimit + FINALIZE_GAS_BUFFER,
"OptimismPortal: insufficient gas to finalize withdrawal"
);

// Set the l2Sender so contracts know who triggered this withdrawal on L2.
l2Sender = _tx.sender;

// Trigger the call to the target contract. We use SafeCall because we don't
// care about the returndata and we don't want target contracts to be able to force this
// call to run out of gas via a returndata bomb.
bool success = SafeCall.call(
_tx.target,
gasleft() - FINALIZE_GAS_BUFFER,
_tx.value,
_tx.data
);
// Make the external call.
bool success = _safeCall(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

// Reset the l2Sender back to the default value.
l2Sender = Constants.DEFAULT_L2_SENDER;
Expand Down Expand Up @@ -482,4 +473,40 @@ contract OptimismPortal is Initializable, ResourceMetering, Semver {
function _isFinalizationPeriodElapsed(uint256 _timestamp) internal view returns (bool) {
return block.timestamp > _timestamp + FINALIZATION_PERIOD_SECONDS;
}

/**
* @notice Performs a safe call to `target` with at *least* the `_minGasLimit` gas supplied.
* Reverts if the callframe does not have enough gas to supply `_minGasLimit` gas
* to the external call.
* @param _target The target to call.
* @param _minGasLimit The *minimum* amount of gas that may be supplied to the call.
* @param _value The amount of ETH to send with the call.
* @param _data The calldata to send with the call.
*/
function _safeCall(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this could probably be shoved into the SafeCall library (maybe SafeCall.callWithMinGasLimit or something and should be very easy to fuzz test. The idea is that the receiving call frame ALWAYS has at least _minGasLimit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could then also use the same logic inside of CrossDomainMessenger.

address _target,
uint256 _minGasLimit,
uint256 _value,
bytes memory _data
) internal returns (bool) {
// We want to maintain the property that the amount of gas supplied to the call to the
// target contract is at least the gas limit specified by the user. We can do this by
// enforcing that, at this point in time, we still have gaslimit + buffer gas available.
require(
gasleft() >= ((_minGasLimit + FINALIZE_GAS_BUFFER) * 64) / 63,
"OptimismPortal: insufficient gas to finalize withdrawal"
);

// Trigger the call to the target contract. We use SafeCall because we don't
// care about the returndata and we don't want target contracts to be able to force this
// call to run out of gas via a returndata bomb.
bool success = SafeCall.call(
_target,
gasleft() - FINALIZE_GAS_BUFFER + GAS_CHECK_BUFFER,
_value,
_data
);

return success;
}
}
Loading