Skip to content

Signalling #89

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

Open
wants to merge 90 commits into
base: main
Choose a base branch
from
Open

Signalling #89

wants to merge 90 commits into from

Conversation

LeoPatOZ
Copy link
Collaborator

@LeoPatOZ LeoPatOZ commented Mar 27, 2025

This PR aims at adding additional signalling functionality.

  1. Adding a canonical ETH bridge
  2. Simplifying the signal service
  3. Added historical commitment tracking

Copy link
Collaborator

@ggonzalez94 ggonzalez94 left a comment

Choose a reason for hiding this comment

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

Just did a first pass on the signaling mechanism, will continue with the CheckpointSyncer and briding. The most important feedback so far is that we are missing some parameters when verifying the signals

/// @dev Signal a `value` at a namespaced slot. See `deriveSlot`.
function signal(uint64 chainId, address account, bytes32 value) internal returns (bytes32) {
bytes32 slot = deriveSlot(chainId, account, value);
slot.getBytes32Slot().value = value;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we already have the storage slot, is there any gas savings from doing this directly with assembly? I'm not familiar with the implementation of getBytes32Slot, I'll check it out - this is just not to miss the idea

assembly {
      sstore(slot, value)
  }

Copy link
Member

Choose a reason for hiding this comment

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

Hey, the implementation is:

    function getBytes32Slot(bytes32 slot) internal pure returns (Bytes32Slot storage r) {
        assembly ("memory-safe") {
            r.slot := slot
        }
    }

LeoPatOZ and others added 3 commits March 28, 2025 19:54
Co-authored-by: Gustavo Gonzalez <gustavo.gonzalez@openzeppelin.com>
Co-authored-by: Gustavo Gonzalez <gustavo.gonzalez@openzeppelin.com>
simplify

comment

updated event

explain root
re-do order

comment
@nikeshnazareth
Copy link
Collaborator

nikeshnazareth commented Apr 11, 2025

  • firstly, the signal-service.md documentation is no longer accurate

Yes I was planning on updating this once the PR was merged as I just wanted to update it once 😄


  • we are expecting to deploy a global (singleton) SignalService contract on both L1 and L2

Yes


  • it contains two distinct storage patterns (I'm ignoring _claimed because that's obvious):
    • the _commitments mapping is a privileged place that's used to keep validated state roots of the other chain (ignoring the state root / block hash distinction for now). So on L1 it will have L2 checkpoints and on L2 it will have L1 anchor blocks.
    • signals can be posted by anyone to a pseudorandom location based on the sender/value
  • when sending a signal on the source chain, the value is saved in the SignalService contract on the source chain

Yes


  • when verifying a signal on the destination chain, the value is checked against the commitment saved in the destination chain. This means we're proving the signal existed in the state root on the other chain. However, the relevant account is address(this) (ie. the address of the SignalService on the destination chain). For this to work, we assume that the signal service is deployed at the same address on both chains. Is that right? How do we intend to make that happen? Are we hardcoding this value into the rollups?

Yes it should be deployed to the same address just to make verifying easier. I believe by deploying it with the same parameters it should deploy to the same address naturally (at least this happens during testing) - do you see an issue with this?


  • the height of a signal doesn't matter. We use it to identify which state root to check, but any state root that has the signal is fine.
  • An ETH deposit is achieved by sending ETH to the SignalService, where it is locked, and a corresponding signal is saved. On the other chain, the signal is verified, and the funds are sent out from that contract.

yes correct


  • As I understand it, this implies we've preloaded the SignalService on the L2 with a bunch of ETH so that it already has funds to facilitate the claim on L2.

Yes i believe Daniel / someone said that the SignalService / Bridge on the L2 holds unlimited ETH


  • An ETH withdrawal can go through the same logic. It's just a deposit on L2 that is claimed on L1. This works because the only funds (outside the SignalService) on L2 should have come from a previous bridge transfer, so all the L2 funds are escrowed in the L1 SignalService

That's my understanding yes


At a high level this makes sense to me, but there are some things that seem weird:

  • When the CheckpointTracker saves a proven checkpoint, and then immediately stores it in the SignalService, this seems entirely redundant. We should save it in just one place.

The reason we do this is store store 'historical' checkpoints. I think we discussed that if checkpoints are updated rapidly and storage / account proofs generated based only of the latest state root, they might become invalid by the time you submit them to the SignalService. With he commitment store you can specify the height you're proof corresponds to. (I agree this might be an unnecessary overhead if checkpoints only update every hour or so). Or are you referring to something else ?


  • My understanding of why we're syncing the checkpoints to a separate SignalService is because we have multiple trackers (at least one per rollup) and we want a central place where everything is saved. But in that case:
    • we should use role-based access control so multiple trackers can save the checkpoint
    • we need some way of deduping between the different checkpoints for different rollups
    • we need to make sure deposits are only redeemable on one chain
    • we probably want to include the source chain inside the signal
  • Obviously this system is not designed to handle multiple L2s, but I'm not sure why.

For now we have decided that the SS will only allow communication between two chains L1 <-> L2. We we have removed the notion of dest chain and source chain #99. I think we might re-introduce that in the future. Although if the current SS is deployed on multiple L2s at the same address, then i agree that

  • we need to make sure deposits are only redeemable on one chain

becomes an issue as now you're deposit it technically valid about all SS across all different rollups. Perhaps this is a good argument to re-introduce chain ids now.


  • As far as I can tell, the deposit id is signalled like any other signal. This means that I can just call sendSignal with a valid deposit ID (without sending any ETH), and then redeem it on the other chain

Hmm yes actually this does seem like a huge issue... Im confused as i believe the current taiko implementation follows the same logic. Here https://github.com/taikoxyz/taiko-mono/blob/main/packages/protocol/contracts/shared/bridge/Bridge.sol#L173-L176 and here https://github.com/taikoxyz/taiko-mono/blob/main/packages/protocol/contracts/shared/bridge/Bridge.sol#L256-L262

  • claimDeposit also calls the generic _verifySignal function, which creates the option of keeping or skipping the accountProof. In the case there is no account proof, we are not enforcing that we're only looking for signals in the SignalService. That seems risky to me. I'm don't think it's directly exploitable because in the case we have a full state root, I doubt you can make a valid proof (or partial proof) with an account proof, but I think that depends on the details of the internals of the LibTrie structure. I think we should insist that claim deposits always have an account proof (and always check against the signal service address).

Maybe i dont understand the exploit, as the storage proof will only be valid against the trusted storageRoot of the signal service? You can make a valid storage proof for another account but it wont match the trusted root the signal service gets correct?

@nikeshnazareth
Copy link
Collaborator

I have some other suggestions for things to improve. I'm happy to make the changes but I thought I'd mention it here so the rationale is clear.

  • In LibSignal, we could reorder the functions so the value is the first parameter and we can treat everything as operations on the value. At the moment we use the syntax value.signal() but not value.signaled(sender) or value.verifySignal(...). I think if we do the first then we should do the others too.
  • isSignalStored has a comment that it returns false if the signal itself is zero. It also seems a bit weird that we save value as the storage value, but just check that it's non-zero when seeing if the signal exists. Should we check that it actually has the expected value for robustness? If so, we could save the hash of the value so that 0 is still a valid signal, or we could just save a specific non-zero constant for all signals.
  • The SignalService._verifySignal function calls one of two different versions of LibSignal.verifySignal, depending on whether there is an account proof. I think it would be clearer to move that branch into the LibSignal.verifySignal function itself. I think that also fixes some naming issues (where root is described as a state root but it's a storage root, and there is a stateProof instead of a storageProof)
  • Is the parameter gas: gasleft() redundant (in _sendETH)?

@nikeshnazareth
Copy link
Collaborator

The reason we do this is store store 'historical' checkpoints...

I agree with the rationale for storing historical checkpoints - I'm objecting to duplicating it. I'll create a draft for moving the tracker storage into the commitment store. I think this would also naturally let rollups overwrite obsolete checkpoints.

For now we have decided that the SS will only allow communication between two chains L1 <-> L2.

This doesn't really make sense to me because we're treating it as a singleton and hardcoding the address. So this decision implicitly means that there can only ever be one rollup using this stack. I can understand making a simple version with core features and slowly expanding that over time (which we've already done several times in this repo) but I don't think that applies in this case because:

  • we'd have to modify the interface and the processing logic once we introduce new chains.
  • this version includes new functionality (eg. an authorizer address) that probably will be removed when we generalise.
    I think generalising it from the start will be simpler, and will also help clarify the design / assumptions. I'll create a draft for your review.

Maybe i dont understand the exploit, as the storage proof will only be valid against the trusted storageRoot of the signal service? You can make a valid storage proof for another account but it wont match the trusted root the signal service gets correct?

I think I'm saying something closer to this:

  • imagine we have a checkpoint that includes the full state root
  • someone tries to verify a proof that has an empty account proof
  • the code will then incorrectly interpret the state root as a storage root (the root of a simple Merkle tree I think) and then see if the signal can be interpreted as a valid leaf (or intermediate node) in that tree
  • whether it is possible for that proof to succeed depends on deeply understanding the Merkle-Patricia trie structure and under what conditions we can make it partially behave like a storage root. Since they are both variations of a merkle tree, it's not obvious to me that this is impossible (although I guess it is). Note that at this point we are not using the signal service account, so we can potentially focus on a more controllable part of the tree.
  • but either way, the high level point is just that it makes me nervous that there is an independent path you can use to attempt an invalid proof. Can you remind me why were are supporting both cases?

@LeoPatOZ
Copy link
Collaborator Author

LeoPatOZ commented Apr 16, 2025

  • In LibSignal, we could reorder the functions so the value is the first parameter and we can treat everything as operations on the value. At the moment we use the syntax value.signal() but not value.signaled(sender) or value.verifySignal(...). I think if we do the first then we should do the others too.

This is now fixed

  • isSignalStored has a comment that it returns false if the signal itself is zero. It also seems a bit weird that we save value as the storage value, but just check that it's non-zero when seeing if the signal exists. Should we check that it actually has the expected value for robustness? If so, we could save the hash of the value so that 0 is still a valid signal, or we could just save a specific non-zero constant for all signals.

Okay i went with the "hash the underlying value" approach i think its better this way - less edge cases

  • The SignalService._verifySignal function calls one of two different versions of LibSignal.verifySignal, depending on whether there is an account proof. I think it would be clearer to move that branch into the LibSignal.verifySignal function itself. I think that also fixes some naming issues (where root is described as a state root but it's a storage root, and there is a stateProof instead of a storageProof)

Okay they are now merged

@LeoPatOZ
Copy link
Collaborator Author

  • As far as I can tell, the deposit id is signalled like any other signal. This means that I can just call sendSignal with a valid deposit ID (without sending any ETH), and then redeem it on the other chain

Okay i changed this now to have a separate namespace param - one for eth deposits and one for generic signals. (Im not convinced on all the namings so open to suggestions) but at least this isnt an issue

remove log
@nikeshnazareth
Copy link
Collaborator

I just realised I updated LibTrieProof but it has Taiko as a security contact. Are we treating it like an external library that needs to remain identical? If not, should we push the whole "if there is an account proof get the storage root first" logic to the LibTrieProof to avoid duplicating all the parameters in LibSignal?

@nikeshnazareth nikeshnazareth mentioned this pull request Apr 21, 2025
4 tasks
Copy link

Changes to gas cost

Generated at commit: daa3158fed75aa90a8020d15947caf7ac9cc9fcc, compared to commit: 6d08743d2312340033442e3745582597c7041ff9

🧾 Summary (10% most significant diffs)

Contract Method Avg (+/-) %
CheckpointTracker proveTransition +29,710 ❌ +40.33%
ProverManager prove +26,872 ❌ +26.77%

Full diff report 👇
Contract Deployment Cost (+/-) Method Min (+/-) % Avg (+/-) % Median (+/-) % Max (+/-) % # Calls (+/-)
CheckpointTracker 979,437 (+69,605) getProvenCheckpoint
proveTransition
4,941 (+22)
103,384 (+29,710)
+0.45%
+40.33%
4,941 (+22)
103,384 (+29,710)
+0.45%
+40.33%
4,941 (+22)
103,384 (+29,710)
+0.45%
+40.33%
4,941 (+22)
103,384 (+29,710)
+0.45%
+40.33%
2 (0)
1 (0)
ProverManager 2,854,874 (+444) evictProver
finalizePastPeriod
payPublicationFee
prove
123,262 (+22)
56,111 (+22)
33,563 (+216)
105,186 (+18,477)
+0.02%
+0.04%
+0.65%
+21.31%
123,262 (+22)
56,342 (+22)
51,981 (+180)
127,244 (+26,872)
+0.02%
+0.04%
+0.35%
+26.77%
123,262 (+22)
56,342 (+22)
59,728 (+172)
129,121 (+26,799)
+0.02%
+0.04%
+0.29%
+26.19%
123,262 (+22)
56,574 (+22)
59,728 (+172)
141,802 (+29,722)
+0.02%
+0.04%
+0.29%
+26.52%
1 (0)
2 (0)
37 (0)
6 (0)

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.

4 participants