[cantina finding #11] do not revert inside _ccipReceive #173
Conversation
voith
force-pushed
the
add-gaslimit
branch
2 times, most recently
from
97abe70 to
25b480c
Compare
| require(success, MessageCallFailed()); | ||
|
|
||
| emit MessageExecuted(messageId, target, payload); | ||
| (bool success, bytes memory _error) = target.call(payload); |
There was a problem hiding this comment.
@voith this is not "enough" because the _ccipReceive could still revert in the above require statements and those messages can be manually retried.
I guess it won't be a huge issue given that mainnetChainSelector, mainnetSender and target cannot be changed, and they will always revert, but still, they will be "available" to be retried in the future.
There was a problem hiding this comment.
I did think about this. I initially thought that this would be fine but I just read the CCIP docs and they are unclear about the execution of successive messages if the previous one fails.
I think for consistency I'll implement emitting an event and returning early 👍
There was a problem hiding this comment.
@voith I don't think that the queue will be paused given that you are sending messages (from mainnet) with the allowOutOfOrderExecution forced to true.
Would you mind sending the link about the explanation you are referring to so I can double check?
There was a problem hiding this comment.
I was taking a look at FAQ 2: https://docs.chain.link/ccip/concepts/manual-execution#frequently-asked-questions
If a user sends multiple messages and the first message isn't successfully delivered and goes into a manual execution mode, does that mean all subsequent messages from the user will also be stuck?
It depends. If a message goes into manual execution mode due to receiver errors (unhandled exceptions or gas limit issues), subsequent messages don't get automatically blocked, unless they would encounter the same error. However, suppose a message goes into manual execution mode after the Smart Execution time window expires (currently 8 hours). In that case, subsequent messages must wait for the first message to be processed to maintain the default sequence.
Not sure if I'm understanding this correctly.
There was a problem hiding this comment.
Yes, it's not quite clear. I think that it would be better for you to ask directly to Chainlink.
If I have to guess, I think that when we are in the scenario "goes into manual execution mode after the Smart Execution time window expires (currently 8 hours)" it means that the whole Chainlink is in a "special" state, it's not just about your message. That's why I think they are saying that you need to wait for the situation to get resolved and the queue to be de-queued.
But it's not clear, that's why I think that you need to ask to them directly.
There was a problem hiding this comment.
Chainlink stated that messages won't be blocked when allowOutOfOrderExecution = true.
The only require checks in my code are for invalid sender and invalid chain selector. Since messages that fail these checks will always fail, I’m not concerned about retries in these cases. Additionally, with allowOutOfOrderExecution = true, messages won’t be blocked, so I see no need for further changes.
| bytes32 indexed messageId, | ||
| address indexed target, | ||
| bytes payload | ||
| ); | ||
|
|
||
| /// @notice Emitted when a message is successfully executed. |
There was a problem hiding this comment.
@voith the description is wrong. This event is emitted when the execution fails.
| bytes32 indexed messageId, | ||
| address indexed target, | ||
| bytes payload, | ||
| bytes _error |
There was a problem hiding this comment.
@voith I would change the variable name. Usually, you use _ when the variable is private or won't be used. I would call it just error without the _
There was a problem hiding this comment.
error is a keyword, I'll rename it to failure for consistency.
This change is