Platform: Code4rena
Start Date: 01/12/2022
Pot Size: $26,900 USDC
Total HM: 3
Participants: 19
Period: 4 days
Judge: GalloDaSballo
Id: 188
League: ETH
Rank: 4/19
Findings: 2
Award: $1,755.92
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: AkshaySrivastav
Also found by: csanuragjain, hihen, ladboy233
1702.4987 USDC - $1,702.50
https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-optimism/EthereumToOptimismExecutor.sol#L45-L59 https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol#L31-L45
The CrossChainExecutorArbitrum and CrossChainExecutorOptimism contracts both use CallLib library to invoke Calls on external contract. As per the CallLib library implementation, any failing Call results in the entire transaction getting reverted.
The CrossChainExecutor contracts does not store whether the calls in CallLib.Call[] were already attempted which failed.
This creates several issues for CrossChainExecutor contracts.
Offchain components can be tricked to submit failing Call[]s again and again. This can be used to drain the offchain component of gas.
Once a failing Call[] was invoked (which failed) and if again the same Call[] is invoked, the transaction should revert with CallsAlreadyExecuted error but it reverts with CallFailure error.
It is difficult to determine whether a to-be executed Call[] is pending or the invocation was already tried but failed.
PoCs for the above issues are listed below.
contract Foo { function bar() public { for(uint256 i; ; i++) {} } }
Foo.bar() call in the CrossChainRelayer contract with maxGasLimit as the _gasLimit parameter.Foo.bar() call by calling the CrossChainExecutor.executeCalls(). This transaction reverts costing the transport layer client maxGasLimit gas.CrossChainExecutor, the transport layer still assumes the relayed call as pending which needs to be executed. The transport layer client again tries to execute the pending relayed call which reverts again.contract Foo { function bar() public { revert(); } }
Foo.bar() call in the CrossChainRelayer contract.Foo.bar() call by calling the CrossChainExecutor.executeCalls(). This transaction gets reverted.Foo.bar() call again. This call should get reverted with CallsAlreadyExecuted error but it gets reverted with CallFailure error.Manual review
The CrossChainExecutor contract should store whether a relayed call was attempted to be executed to make sure the execution cannot be tried again.
The CallLib library can be changed to not completely revert the transaction when any individual Call gets failed.
#0 - GalloDaSballo
2022-12-06T19:07:24Z
Not convinced by High Severity but the fact that you cannot determine whether calls were already attempted seems valid
#1 - c4-judge
2022-12-11T20:30:01Z
GalloDaSballo marked the issue as primary issue
#2 - c4-sponsor
2022-12-15T17:49:09Z
PierrickGT marked the issue as sponsor confirmed
#3 - c4-sponsor
2022-12-15T17:51:15Z
PierrickGT marked the issue as sponsor disputed
#4 - c4-sponsor
2022-12-15T18:00:34Z
PierrickGT marked the issue as sponsor confirmed
#5 - c4-sponsor
2022-12-15T18:00:39Z
PierrickGT marked the issue as disagree with severity
#6 - PierrickGT
2022-12-15T18:02:53Z
Indeed, in the current implementation, it's pretty difficult to know which calls succeeded and which calls failed.
So we've added two events:
event CallSuccess(uint256 callIndex, bytes successData);event CallFailure(uint256 callIndex, bytes errorData);When a Call fails, we emit the CallFailure event and exit early the loop going through the batch calls.
CallLib.executeCalls will return false and then the transaction will revert with the custom error ExecuteCallsFailed.
If all calls have executed successfully, CallLib.executeCalls will return true and then the ExecutedCalls event will be emitted
This way, it's possible to know which calls succeeded and which didn't. If one Call fails, the entire transaction must revert cause the user may have intended to execute all the calls in one transaction and maybe some calls depends on others to succeed.
I think this issue should be labeled as 2 (Med Risk) since it would indeed have been difficult for the transport layer client to figure out why the transaction failed and if it was worth replaying in the future.
#7 - GalloDaSballo
2022-12-24T00:23:34Z
I think the finding was well thought out and can tell it helped shaped the protocol.
I believe Medium severity could be reasonably marked, however I think Low Severity to be the most appropriate one.
Specifically:
For those reasons I believe QA Low (Notable finding for Relayer / Service Operators) to be the most appropriate.
I will flag this during triage to get more opinions
#8 - c4-judge
2022-12-24T00:23:49Z
GalloDaSballo changed the severity to 2 (Med Risk)
#9 - GalloDaSballo
2022-12-24T00:24:02Z
Am downgrading to Med with the goal of downgrading to Low per the above, but will do so after Triage
#10 - GalloDaSballo
2022-12-26T21:01:10Z
Some additional thinking I'm having is that a failed tx could remain un-broadcasted for an indefinite amount of time, and this could create issues for the receiving contract if / when the contract is made to not revert.
#11 - GalloDaSballo
2022-12-26T23:25:50Z
Specifically the fact that a failed tx can be relayed in the future (no expiration) seems to create a risk that can cause loss of value, which leads to me believe there is a valid argument for Medium Severity
#12 - GalloDaSballo
2022-12-26T23:35:00Z
After further thinking, I believe the most appropriate severity is Medium.
The Warden has shown how the code allows the execution of old failed txs, while that is fine, I believe the lack of expiry can create situations in which a old message could be broadcasted and the broadcasting of it could cause a non idempotent behavior.
The simplest example I can think of would be an unpause tx, that fails up until a set of contracts are paused, which would put a paused system (probably because of an exploit or the need for immediate stop) back into the unpaused state.
While the externalities are multiple, I believe because:
That Medium Severity is the most appropriate.
Personally I would recommend considering a way to make calls expire after some time to avoid potential gotchas (or integrators may want to verify that via a nonce system or similar)
#13 - c4-judge
2022-12-26T23:44:27Z
GalloDaSballo marked the issue as selected for report
53.4153 USDC - $53.42
nonce increment statements can be modified to save gas.
The statements in relayCalls functions of CrossChainRelayerOptimism, CrossChainRelayerArbitrum and CrossChainRelayerPolygon contracts:
nonce++; uint256 _nonce = nonce;
can be replaced with
uint _nonce = ++nonce;
executor variable should be made immutable.
The executor variables in CrossChainRelayerOptimism and CrossChainRelayerArbitrum contracts can be made immutable. The address of the executor contract can be calculated deterministically before deployment.
relayer variable should be made immutable.
The relayer variables in CrossChainExecutorOptimism and CrossChainExecutorArbitrum contracts can be made immutable. The address of the relayer contract can be calculated deterministically before deployment or it may be already be known before CrossChainExecutor deployments.
#0 - GalloDaSballo
2022-12-14T23:11:16Z
5
Immutables -> TODO
#1 - PierrickGT
2022-12-15T00:20:18Z
Not really clear how much gas we would save by using ++nonce instead of nonce++.
The immutable suggestion is interesting but it would make the deployment more complex and it would leave room for errors if the address is not properly computed before deployment.
So for these reasons, I have acknowledged the issue but we won't implement these changes.
#2 - c4-sponsor
2022-12-15T00:20:22Z
PierrickGT marked the issue as sponsor acknowledged
#3 - GalloDaSballo
2022-12-26T21:20:49Z
1050 (immutables)
1055
#4 - c4-judge
2022-12-26T23:13:37Z
GalloDaSballo marked the issue as grade-b