Skip to content
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

TRN-382 Prevent EthCallNotarization Resubmission #892

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JCSanPedro
Copy link
Collaborator

@JCSanPedro JCSanPedro commented Oct 9, 2024

This PR is to specifically address the audit issue FRN-37.

The audit ticket describes a vulnerability where EthCallNotarizations can be resubmitted, potentially allowing a single notary to affect the outcome of an eth call notarization. Within the implementation of submit_notarization, the notarization is persisted in storage, but there is additional validation to ensure that a submission has not already been submitted. It has also been noted that for EventNotarizations, there is sufficient validation to prevent resubmission.

To remedy this, we follow the same logic as EventNotarizations does, where within the validate_unsigned implementation, we check the storage to ensure the notarization has not been resubmitted.

NOTE

This PR is effectively non-functional. Notarization submissions do not actually occur in production, as submit_notarization is only triggered in the cases:

  1. Where an event notarization for an eth tx that has been challenged, which will never happen though because we have blocked the challenge functionality. Check out the call filter here, which prevents these calls from executing
  2. For call notarizations which isn't hooked as the eth-state-oracle was not brought along when this repo transitioned from Cennznet -> TRN

Furthermore, the UnsignedValidator was not correctly implemented for the ethy-pallet as it was missing the #[pallet::validate_unsigned] proc macro. This means that unsigned extrinsics being sent to the ethy-pallet were never correctly validated in the first place.

The only value of merging this PR is to:

  1. Say that we addressed the audit problem (ticking boxes essentially)
  2. Ensure that we have addressed this problem once we do eventually reintroduce decentralized notarizatios and/or the eth-state-oracle

A more long term solution would involve refactoring the ethy-pallet in it's entirety (started here).

@JCSanPedro JCSanPedro force-pushed the TRN-382-resubmission-eth-notarization branch from 52574a7 to 7606812 Compare October 9, 2024 21:39
Copy link
Contributor

@JasonTulp JasonTulp left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for going through the weeds to get to this conclusion. Appreciate your time

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