Make MultiCall a parameter to CrossChainReceiverFactory
#456
Conversation
…771-forwarding `MultiCall`) is implemented correctly
…trieve an alternative `MultiCall` address if it fails
🛡️ Immunefi PR ReviewsWe noticed that your project isn't set up for automatic code reviews. If you'd like this PR reviewed by the Immunefi team, you can request it manually using the link below: Once submitted, we'll take care of assigning a reviewer and follow up here. |
duncancmt
force-pushed
the
dcmt/crosschainreceiver3
branch
from
552d74d to
57dd4bc
Compare
duncancmt
force-pushed
the
dcmt/crosschainreceiver3
branch
from
89cf73f to
f277832
Compare
src/CrossChainReceiverFactory.sol
Outdated
| calls[0].revertPolicy = IMultiCall.RevertPolicy.CONTINUE; | ||
|
|
||
| bytes memory data = abi.encodeCall(IMultiCall.multicall, (calls, 0)); | ||
| (isWeird,) = EIP150_MULTICALL_ADDRESS.call{gas: 100_000}(data); |
There was a problem hiding this comment.
This function is very nice and the check works for MegaEth but what if EIP150Ratio increases? Then the remaining gas is supposed to be lower and I think this will revert. If you don't think that is possible scenario for a future chain then this is good.
There was a problem hiding this comment.
If the denominator for the EIP150 ratio increases (in other words, the amount of retained gas in each context decreases), then this check won't fire and we'll use the "normal" ERC2771-forwarding MultiCall.
I think this is fine for two reasons:
- that's very unlikely to happen, for precisely the reason that MegaETH decreased the ratio
- ERC2771-forwarding MultiCall still works fine in that scenario. It just requires that you slightly overprovision the gas that you send relative to what is actually required on the chain.
Another criticism that you could levy here is that "what if the ratio decreases, but not very much?" i.e. "what if the retained gas was 1/63 instead of 1/64?". In that case, because the difference between ~1570 gas is and ~1545 gas is less than the 100 gas overhead (minimum) charged by CALL, MultiCall would revert and we would not detect that the chain has weird gas rules. To be honest, I'm not really sure what to do about that. I could measure the exact gas overhead of CALL (plus putting its arguments on the stack) and try to do a separate experiment that exactly accounts for that discrepancy to detect and subtle oddities with a chain's EIP-150 implementation. I'm not sure that's such a good idea though. What happens when the cost of (e.g.) PUSH changes or the gas overhead of CALL is again tweaked in some subtle fashion.
Do you have any good ideas?
There was a problem hiding this comment.
Not clear what you mean by "it won't fire". Multicall has this line
if iszero(lt(remainingGas, afterGas)) { invalid() }so if the actual afterGas is lower, then it will make the entire multicall call revert, which was the case I was trying to point out. My understanding is, in one side (e.g MegaEth one) the revert is not marked as OOG, so false is returned as success, and, on the other side, it is going to be marked as OOG and the revert will be bubbled making this code revert as well.
Other than that, I agree with everything else in your reply.
About ideas, will think about it, but, is it too crazy to do a similar approach to the one used in _WNATIVE? 🤔
…cting `CrossChainReceiverFactory`. Namely, it is a mandatory "argument"
No description provided.