Skip to content

feat: add resend msg feature changes on fmas #12

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Apr 10, 2025

Conversation

0xDiscotech
Copy link

No description provided.

@0xDiscotech 0xDiscotech requested a review from agusduha April 9, 2025 23:30
@0xDiscotech 0xDiscotech self-assigned this Apr 9, 2025
Comment on lines 69 to 70
- **Risk Assessment:** High.
- Potential Impact: Critical. Several scenarios could lead to severe consequences, including:
- Inability to `relayETH` in `SuperchainWETH`
- Inability to `relayERC20` in `SuperchainTokenBridge` to mint `SuperchainERC20`
- Any other contract awaiting a time-sensitive `relayMessage`.
If the message is not included within the expiration window, both the message and the underlying action could be considered lost.
- **Risk Assessment:** Low.
- Potential Impact: Low. The message will need to be re-sent, using the `resendMessage` feature.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest not deleting the scenario where for some reason you couldn't relay the msg. It is ok to remove the expired logs one but the FM still there in case something goes wrong while relaying

Copy link
Author

@0xDiscotech 0xDiscotech Apr 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm ok, I wanted to do that, but I removed these items:

including:
    - Inability to `relayETH` in `SuperchainWETH`
    - Inability to `relayERC20` in `SuperchainTokenBridge` to mint `SuperchainERC20`
    - Any other contract awaiting a time-sensitive `relayMessage`.

I will add them back, but using medium as risk since they can be re-sent, if that's ok for you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only problem is that the risk is not low if funds are stuck for another reason

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pushed here 4ea3073

Comment on lines 78 to 79
- **Recovery Path(s):** Depends on sequencer/chain governor policy operations.
- **Recovery Path(s):**
- If not expired, re-relaying the message on destination.
- If expired, calling `resendMessage` on origin chain to make it available again to be relayed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't feel that this change is worth it. The resendMessage is more like a mitigation than a recovery path. The recovery path is for cases where the resend is not helping


Some consequences of this issue are:

- Inability to `relayETH` in `SuperchainWETH`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: SuperchainETHBridge now

Comment on lines 76 to 78
- **Risk Assessment:** Medium.
- **Potential Impact:** High. If the message failure is not related to a time-sensitive operation but is due to the message being incorrect, it could be considered lost.
- **Likelihood:** Low. Block builders/sequencers are generally controlled by a single entity per chain. Additionally, if the relay failed on destination due to an incorrect state and after some time it is safe to be relayed again, you can do it even if it expired by calling `resendMessage` on origin chain.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Impact and Likelihood should be indented below risk

@0xDiscotech 0xDiscotech changed the base branch from main to sc-feat/resend-msg-fma April 10, 2025 20:11
@0xDiscotech 0xDiscotech marked this pull request as ready for review April 10, 2025 20:11
@0xDiscotech 0xDiscotech merged commit 4f84195 into sc-feat/resend-msg-fma Apr 10, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants