In L1Migrator.sol, the function migrateETH first withdraws eth from the BridgeMinter, and then intends to send all of this eth from L1 to L2. However, the parameters are incorrectly passed to the sendTxToL2 function, so none of this withdrawn eth will actually be transferred to the L2. The function will inadvertently keep all of this eth in the L1Migrator contract, where it will be frozen forever. This represents a large loss to the protocol, and the migration won't properly transfer the eth from L1 to L2, so this is a high severity issue.
Proof of Concept
See the code for migrateETHhere, and see the code for sendTxToL2here.
Now, notice that in migrateETH, the variable amount is the amount of eth that is supposed to be sent to the L2. This variable is passed as the fourth argument to sendTxToL2. However, looking at the implementation of sendTxToL2, the third argument is the amount of eth that is transferred to the L2, whereas the fourth argument is the call value that is associated with the transaction that executes the retryable ticket on the L2. According to the specification here, this means that the inbox will try to get amount eth from the L2 (where it should instead be getting it from the L1), and moreover the inbox isn't even going to send the amount eth to the L2 at all.
Tools Used
Manual inspection.
Recommended Mitigation Steps
After discussion with the sponsor on discord, it was determined that this bug is indeed valid and that the ideal fix would be to change the existing code:
sendTxToL2(
l2MigratorAddr,
address(this), // L2 alias of this contract will receive refunds
msg.value,
amount,
_maxSubmissionCost,
_maxGas,
_gasPriceBid,
""
);
to instead be:
sendTxToL2(
l2MigratorAddr,
address(this), // L2 alias of this contract will receive refunds
msg.value + amount,
amount,
_maxSubmissionCost,
_maxGas,
_gasPriceBid,
""
);
If you inspect the documentation here again closely, you can see that this works, since msg.value + amount eth will be transferred to the L2 first, and then amount of eth will be use as the call value in the retryable ticket. This mitigates the issue.
yondonfu
commented
2 years ago
Handle
harleythedog
Vulnerability details
Impact
In
L1Migrator.sol, the functionmigrateETHfirst withdraws eth from the BridgeMinter, and then intends to send all of this eth from L1 to L2. However, the parameters are incorrectly passed to thesendTxToL2function, so none of this withdrawn eth will actually be transferred to the L2. The function will inadvertently keep all of this eth in theL1Migratorcontract, where it will be frozen forever. This represents a large loss to the protocol, and the migration won't properly transfer the eth from L1 to L2, so this is a high severity issue.Proof of Concept
See the code for
migrateETHhere, and see the code forsendTxToL2here.Now, notice that in
migrateETH, the variableamountis the amount of eth that is supposed to be sent to the L2. This variable is passed as the fourth argument tosendTxToL2. However, looking at the implementation ofsendTxToL2, the third argument is the amount of eth that is transferred to the L2, whereas the fourth argument is the call value that is associated with the transaction that executes the retryable ticket on the L2. According to the specification here, this means that the inbox will try to getamounteth from the L2 (where it should instead be getting it from the L1), and moreover the inbox isn't even going to send theamounteth to the L2 at all.Tools Used
Manual inspection.
Recommended Mitigation Steps
After discussion with the sponsor on discord, it was determined that this bug is indeed valid and that the ideal fix would be to change the existing code:
to instead be:
If you inspect the documentation here again closely, you can see that this works, since
msg.value + amounteth will be transferred to the L2 first, and thenamountof eth will be use as the call value in the retryable ticket. This mitigates the issue.