node: Add Transfer Verifier mechanism #4169

johnsaigle · 2024-11-22T15:06:11Z

Transfer Verifier

This PR is a joint effort between myself, @pleasew8t, and @mdulin2.

Overview

This PR introduces the Transfer Verifier, a mechanism to improve Token
Bridge security by validating transaction receipts that correspond to Publish
Message events emitted from the Core Bridge.

When the core bridge emits a Publish Message event, the Transfer Verifier does
the following:

Fetches the transaction artifacts corresponding to the Publish Message event
(e.g. an Ethereum receipt transaction)
Checks whether the publish message corresponds to a token transfer and is
emitted by the Token Bridge contract
Cross-references the data in the publish message against actual deposits or
transfers into the token bridge
If there is publish message with no valid corresponding deposit or transfers
into the Token Bridge, the program emits an error to signal to the node that
there is a security incident.

This provides protection in the event that an attacker devises a way to emit
arbitrary publish message events from the core contract or else somehow tricks
the Token Bridge into making token transfer calls without any backing tokens.
While existing Integrity Checks (Governor and Accountant) would help mitigate
this attack, Transfer Verifier acts as another layer of protection.

What this PR contains

This PR contains two initial implementations for Ethereum and Sui.

Both implementations are bundled into a Transfer Verifier package. This package
is used by a new transfer-verifier binary that exists as a subcommand within
guardiand. This binary can be configured to monitor the mainnet token bridge and core
bridge contracts for this chain.

Testing

Both implementations contain a large number of unit tests. They can be invoked
as a normal part of testing the node via make test.

The Ethereum implementation has integration tests built into the Tilt set-up:

After the eth-devnet container is set-up, the transfer-verifier binary is
invoked to monitor the Ethereum devnet
A shell script invokes a malicious call to the core contract via cast (part
of the Foundry toolset)
The transfer-verifier binary logs an error when detecting the malicious call
A separate monitoring container looks for the presence of this error log and
returns a success when it appears

In order to impersonate a call from the Token Bridge to the Core Bridge, the
--auto-impersonate flag was added to the anvil invocation in eth-devnet.

Future Work

The Transfer Verifier package is used here as a standalone binary. Its
fundamental output are logs that can be monitored and manually acted upon.

The next iteration of this project will be to integrate the package directly in
the Ethereum and Sui watchers

Integration into the Ethereum watcher

Ethereum has an advantage over the Sui implementation due to the robust logging
and tooling present in the EVM and its ecosystem. We can validate the relevant
information for Transfer Verification in a highly robust way and detect
violations of its invariants with certainty.

As a result, we can use the logic in the Transfer Verifier to block
transactions at the level of the Ethereum watcher so that malicious message
publications will not be processed by the network.

Integration into the Sui watcher

The nature of Sui makes it relatively difficult to provide guarantees about the
state of the core bridge relative to the token bridge. The Sui implementation
cannot provide the same guarantees as the Ethereum implementation due to limits
inherent in the chain.

It is still possible to integrate this protection into the Sui watcher but it
would be better to delay messages for e.g. 24h in this case rather than reject
them outright.

Additional implementations for other chains

This is an abstract algorithm that can in theory be applied to any chain supported by the Token Bridge.
Designing a Solana implementation of this script would be a fruitful choice
given that it is one of the chains that makes the most use of the Token Bridge.

bruce-riley

Not done reviewing yet, but I wanted to post the comments I have so far.

node/cmd/transfer-verifier/transfer-verifier-evm.go

bruce-riley · 2024-12-03T14:43:04Z

node/cmd/transfer-verifier/transfer-verifier-evm.go

+		&txverifier.TVAddresses{
+			CoreBridgeAddr:  common.HexToAddress(*evmCoreContract),
+			TokenBridgeAddr: common.HexToAddress(*evmTokenBridgeContract),
+			// TODO: should be a CLI parameter so that we could support other EVM chains


Do you want to address this TODO now? Seems like would be easy to add.

The code should hopefully be generic enough to work with other EVM chains but the only testing I've done is with Ethereum. For that reason my preference would be to merge this as effectively Ethereum-only for now, with the idea that transfer verification could be extended to other chains later if we need it.

Well, this means this will only work on ETH mainnet. Users might have the impression that they can set the RPC and the core and token bridge address for either Sepolia or any other EVM, but things will not work correctly because the WETH address would be wrong, right?

node/cmd/transfer-verifier/transfer-verifier-evm.go

bruce-riley · 2024-12-03T14:55:48Z

node/cmd/transfer-verifier/transfer-verifier-sui.go

+	suiRPC                *string
+	suiCoreContract       *string
+	suiTokenBridgeEmitter *string
+	// TODO: rename to package ID


Should you do this before people start using the command so they don't have to update later?

node/cmd/transfer-verifier/transfer-verifier-sui.go

node/cmd/transfer-verifier/transfer-verifier-evm.go

node/cmd/transfer-verifier/transfer-verifier-sui.go

panoel · 2024-12-03T21:05:57Z

node/pkg/transfer-verifier/transfer-verifier-sui.go

+// }
+
+// Filter to be used for querying events
+// The `MoveEventType` filter doesn't seem to be available in the documentation. However, there is an example


Are you saying the way the watcher does this is undocumented? That seems...odd..

Yeah the docs don't offer this event filter: https://docs.sui.io/guides/developer/sui-101/using-events

panoel · 2024-12-03T22:23:10Z

node/pkg/transfer-verifier/transfer-verifier-sui-structs.go

+//   - ensure that the coin types match
+func (o ObjectChange) ValidateTypeInformation(expectedPackageId string) (success bool) {
+	// AI generated regex
+	re := regexp.MustCompile(`^0x2::dynamic_field::Field<([^:]+)::token_registry::Key<([^>]+)>, ([^:]+)::([^<]+)<([^>]+)>>$`)


I think this panics on error. That seems a bit extreme? Should we consider something less extreme?

I was thinking about this, and the thing is that the actual regex remains static. Is there a reason it would compile during tests, but not during a live production run?

node/pkg/transfer-verifier/transfer-verifier-sui-structs.go

node/pkg/transfer-verifier/transfer-verifier-evm-structs.go

bruce-riley · 2024-12-04T17:37:17Z

node/pkg/transfer-verifier/transfer-verifier-evm-structs.go

+func (s *Subscription) Subscribe(ctx context.Context) {
+	go func() {
+		for {
+			select {


Do you need to handle ctx.Done()?

bruce-riley · 2024-12-11T20:57:45Z

node/cmd/transfer-verifier/transfer-verifier-evm.go

+		&txverifier.TVAddresses{
+			CoreBridgeAddr:  common.HexToAddress(*evmCoreContract),
+			TokenBridgeAddr: common.HexToAddress(*evmTokenBridgeContract),
+			// TODO: should be a CLI parameter so that we could support other EVM chains


Well, this means this will only work on ETH mainnet. Users might have the impression that they can set the RPC and the core and token bridge address for either Sepolia or any other EVM, but things will not work correctly because the WETH address would be wrong, right?

bruce-riley · 2024-12-12T19:12:36Z

node/pkg/transfer-verifier/transfer-verifier-evm.go

+	}
+
+	// Get the full transaction receipt for this log.
+	receipt, txReceiptErr := tv.evmConnector.TransactionReceipt(ctx, vLog.Raw.TxHash)


I know it's not applicable in this standalone app, but it will matter when this is bolted into the watcher. The watcher will have already called TransactionReceipt, so you will want to restructure this code so that can be passed in.

johnsaigle added enhancement New feature or request node evm sui labels Nov 22, 2024

johnsaigle force-pushed the transfer-verifier-rebased branch from 0f118a9 to c711b03 Compare November 26, 2024 20:47

johnsaigle marked this pull request as ready for review November 26, 2024 21:22

johnsaigle requested review from evan-gray, bruce-riley, panoel, SEJeff, kcsongor, a5-pickle and gator-boi as code owners November 26, 2024 21:22

bruce-riley reviewed Dec 3, 2024

View reviewed changes

panoel reviewed Dec 3, 2024

View reviewed changes

node/cmd/transfer-verifier/transfer-verifier-sui.go Show resolved Hide resolved

panoel reviewed Dec 3, 2024

View reviewed changes

node/pkg/transfer-verifier/transfer-verifier-sui-structs.go Outdated Show resolved Hide resolved

bruce-riley reviewed Dec 4, 2024

View reviewed changes

node/pkg/transfer-verifier/transfer-verifier-evm-structs.go Outdated Show resolved Hide resolved

johnsaigle and others added 5 commits December 9, 2024 15:22

node: Add Transfer Verifier mechanism

03c29e1

address pr comments

1acd68a

rename CLI arguments

0e9656d

rename eth to evm where appropriate

b6be4ff

add parameters for loki logging

6ae4219

johnsaigle force-pushed the transfer-verifier-rebased branch from 8661aea to 6ae4219 Compare December 9, 2024 20:22

resolve errcheck problems

3df7b65

bruce-riley reviewed Dec 11, 2024

View reviewed changes

bruce-riley reviewed Dec 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

node: Add Transfer Verifier mechanism #4169

node: Add Transfer Verifier mechanism #4169

johnsaigle commented Nov 22, 2024 •

edited

Loading

bruce-riley left a comment

bruce-riley Dec 3, 2024

johnsaigle Dec 9, 2024 •

edited

Loading

bruce-riley Dec 11, 2024 •

edited

Loading

bruce-riley Dec 3, 2024

panoel Dec 3, 2024

pleasew8t Dec 9, 2024

panoel Dec 3, 2024

pleasew8t Dec 9, 2024

bruce-riley Dec 4, 2024

bruce-riley Dec 11, 2024 •

edited

Loading

bruce-riley Dec 12, 2024

node: Add Transfer Verifier mechanism #4169

Are you sure you want to change the base?

node: Add Transfer Verifier mechanism #4169

Conversation

johnsaigle commented Nov 22, 2024 • edited Loading

Transfer Verifier

Overview

What this PR contains

Testing

Future Work

Integration into the Ethereum watcher

Integration into the Sui watcher

Additional implementations for other chains

bruce-riley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

bruce-riley Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bruce-riley Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnsaigle commented Nov 22, 2024 •

edited

Loading

johnsaigle Dec 9, 2024 •

edited

Loading

bruce-riley Dec 11, 2024 •

edited

Loading

bruce-riley Dec 11, 2024 •

edited

Loading