-
Notifications
You must be signed in to change notification settings - Fork 0
obront - All migrated withdrarwals that require more than 135,175 gas may be bricked #93
Comments
Valid but we believe it to be a medium. There definitely exist edge cases of transactions where this is an issue but the majority of transactions it is not an issue. Based on the following call trace for a finalization of a withdrawal transaction + the address mapping, we believe that this issue is unable to impact transactions transferring ERC20 tokens through the bridge.
|
Would suggest checking for known withdrawals and seeing if this can be a concern (and raising to High in that case) The conditionality leads me to agree with Med |
Sample list of withdrawals Annotated Gas Consumption of integrations I believe there are some cases in which the above txs, which have corresponding events, will require more than 135k gas meaning they are subject to the attack |
I think this is a valid example: relayMessage -> does something -> send a message back Contract: https://etherscan.io/address/0xcEA770441aa5eFCD3f5501b796185Ec3055A76D7/advanced#internaltx |
Escalate for 10 USDC. While the issue is creatively accurate with the specified gas values, it still requires certain conditions to be feasbile (e.g. only withdrarwals require more than 135,175 gas). According to Sherlock's Criteria, it is a valid medium.
Lastly, all the respect to the good efforts put in behind this finding. |
You've created a valid escalation for 10 USDC! To remove the escalation from consideration: Delete your comment. You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final. |
Escalation rejected Lead Judge comment:
This is a valid flaw in the system due to an incorrect assumption. |
This issue's escalations have been rejected! Watsons who escalated this issue will have their escalation amount deducted from their next payout. |
obront
high
All migrated withdrarwals that require more than 135,175 gas may be bricked
Summary
Migrated withdrawals are given an "outer" (Portal) gas limit of
calldata cost + 200,000
, and an "inner" (CrossDomainMessenger) gas limit of0
. The assumption is that the CrossDomainMessenger is replayable, so there is no need to specify a correct gas limit.This is an incorect assumption. For any withdrawals that require more than 135,175 gas, insufficient gas can be sent such that CrossDomainMessenger's external call reverts and the remaining 1/64th of the gas sent is not enough for replayability to be encoded in the Cross Domain Messenger.
However, the remaining 1/64th of gas in the Portal is sufficient to have the transaction finalize, so that the Portal will not process the withdrawal again.
Vulnerability Detail
When old withdrawals are migrated to Bedrock, they are encoded as calls to
L1CrossDomainMessenger.relayMessage()
as follows:As we can see, the
relayMessage()
call uses a gasLimit of zero (see comments above), while the outer gas limit is calculated by theMigrateWithdrawalGasLimit()
function:This calculates the outer gas limit value by adding the calldata cost to 200,000.
Let's move over to the scenario in which these values are used to see why they can cause a problem.
When a transaction is proven, we can call
OptimismPortal.finalizeWithdrawalTransaction()
to execute the transaction. In the case of migrated withdrawals, this executes the following flow:OptimismPortal
calls toL1CrossDomainMessenger
with a gas limit of200,000 + calldata
(200_000 + calldata) * 64/63 * 1/64 > 3174
41,002
gas before making the call, leaving158,998
remaining for the callSafeCall.callWithMinGas()
succeeds, since the inner gas limit is set to 0158,998 * 1/64 = 2,484
for the remaining execution23,823
gas, resulting in an OutOfGas revert135,175
, we will have less than23,823
gas remaining and will revertL1CrossDomainMessenger
occur, and the transaction is not marked infailedMessages
for replayability3174
gas is sufficient to complete the transction on theOptimismPortal
, which setsfinalizedWithdrawals[hash] = true
and locks the withdrawals from ever being made againImpact
Any migrated withdrawal that uses more than
135,175
gas will be bricked if insufficient gas is sent. This could be done by a malicious attacker bricking thousands of pending withdrawals or, more likely, could happen to users who accidentally executed their withdrawal with too little gas and ended up losing it permanently.Code Snippet
https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/op-chain-ops/crossdomain/migrate.go#L55-L97
https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/op-chain-ops/crossdomain/migrate.go#L99-L119
https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L315-L412
https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L291-L383
Tool used
Manual Review
Recommendation
There doesn't seem to be an easy fix for this, except to adjust the migration process so that migrated withdrawals are directly saved as
failedMessages
on theL1CrossDomainMessenger
(and marked asfinalizedWithdrawals
on theOptimismPortal
), rather than needing to be reproven through the normal flow.The text was updated successfully, but these errors were encountered: