Combined contract by javerett · Pull Request #2 · nonergodic/core-bridge

javerett · 2025-02-14T15:33:19Z

No description provided.

nonergodic · 2025-02-24T19:00:50Z

+	function pullGuardianSets() public {
+		// For each guardian set after the current one
+		uint32 coreGuardianSetIndex = _coreV1.getCurrentGuardianSetIndex();
+		for (uint32 i = uint32(_guardianSetExpirationTime.length); i <= coreGuardianSetIndex; ++i) {


Suggested change

for (uint32 i = uint32(_guardianSetExpirationTime.length); i <= coreGuardianSetIndex; ++i) {

for (uint i = _guardianSetExpirationTime.length; i <= coreGuardianSetIndex; ++i) {

Loop counters should always be uint(256). Using a different datatype only increases gas cost and you'll never overflow the boundary anyway.

nonergodic · 2025-02-27T10:17:13Z

+	function eagerOr(bool lhs, bool rhs) private pure returns (bool ret) {
+		assembly ("memory-safe") {
+			ret := or(lhs, rhs)
+		}
+	}


This is available in the utils of the Solidity SDK and should be taken from there.

nonergodic · 2025-02-27T10:21:52Z

+			}
+
+			// Write the guardian set to the ExtStore and verify the index
+			uint32 extIndex = uint32(_extWrite(data));


No need for casts once the loop counter is simply a uint.

nonergodic · 2025-02-27T11:36:34Z

+				result_entry = abi.encodePacked(expirationTime, guardianSetAddrs);
+			}
+
+			result = abi.encodePacked(result, result_entry);


Instead of doing it only once here, this should be copy pasted to each individual branch and combined with the abi.encodePacked of that branch. solc is sadly not smart enough to do this sort of thing so this will incur an additional, unnecessary, looping memcopy (looping because we likely can't rely on he MEMCOPY opcode to exist everywhere we deploy this contract).

nonergodic · 2025-02-27T11:36:56Z

+		uint8 version;
+		(version, offset) = data.asUint8CdUnchecked(offset);
+		if (version != RAW_DISPATCH_PROTOCOL_VERSION) revert InvalidDispatchVersion(version);


likewise no need for a version.

nonergodic · 2025-02-27T11:37:09Z

+		if (version != RAW_DISPATCH_PROTOCOL_VERSION) revert InvalidDispatchVersion(version);
+
+		bytes memory result;
+		uint length = data.length;


nonergodic · 2025-02-27T11:37:53Z

+				uint32 dataLength;
+				(dataLength, offset) = data.asUint32CdUnchecked(offset);
+
+				bytes calldata encodedVaa = data[offset:offset + dataLength];
+				VaaBody memory vaaBody = decodeAndVerifyVaa(encodedVaa);


BytesParsing.sliceUint16PrefixedCdUnchecked

nonergodic · 2025-03-18T06:33:32Z

+	using VaaLib for bytes;
+	using UncheckedIndexing for address[];
+
+	ICoreBridge private _coreV1;


Suggested change

ICoreBridge private _coreV1;

ICoreBridge private immutable _coreBridge;

V1 suggests that there is a v2 when really there is only a v2 for verification.

nonergodic · 2025-03-18T06:36:26Z

+	uint32[] private _guardianSetExpirationTime;
+
+	// Get the guardian addresses for a given guardian set index using the ExtStore
+	function getGuardianSetInfo(uint32 index) public view returns (uint32 expirationTime, address[] memory guardianAddrs) {


Properly functioning code should never create a Panic, not even on invalid external input.
from the Solidity documentation

nonergodic · 2025-03-18T06:44:44Z

File should use spaces, not tabs (and my preference is 2 space indentation, but whatever) and a max line length of 100 (except for urls that can't be line-broken)

nonergodic · 2025-03-18T06:47:15Z

+			uint upper = (limit == 0 || currentGuardianSetLength - oldGuardianSetLength < limit) ? currentGuardianSetLength : oldGuardianSetLength + limit;
+
+			// Pull and append the guardian sets
+			for (uint i = oldGuardianSetLength; i < upper; i++) {


Suggested change

for (uint i = oldGuardianSetLength; i < upper; i++) {

for (uint i = oldGuardianSetLength; i < upper; ++i) {

nit for consistency

nonergodic · 2025-03-18T06:54:51Z

+	//   address (160 bits)
+	uint256[] private _pastThresholdInfo;
+
+	bytes32[][] private _shards;


This connection is currently communicated

Oh... this should have said "is currently NOT communicated" ...

nonergodic · 2025-03-18T07:16:08Z

+				(
+					,
+					,
+					,
+					,
+					,
+					,
+					bytes calldata payload
+				) = _decodeAndVerifyVaa(encodedVaa);


It could go on one line if you like? I found this a bit easier to read, but it's still not pretty haha

nonergodic · 2025-03-18T07:21:12Z

+				bytes calldata encodedVaa;
+				(encodedVaa, offset) = data.sliceUint16PrefixedCdUnchecked(offset);
+
+				// Decode the VAA


Suggested change

// Decode the VAA

// Decode and verify the VAA

and arguably the comments aren't really adding anything in this function

nonergodic · 2025-03-18T07:22:45Z

+		uint8 shardsLength;
+		(shardsLength, offset) = payload.asUint8MemUnchecked(offset);
+		shards = new bytes32[](shardsLength);
+		for (uint i = 0; i < shardsLength; i++) {


Suggested change

for (uint i = 0; i < shardsLength; i++) {

for (uint i = 0; i < shardsLength; ++i) {

nonergodic · 2025-03-18T07:26:15Z

+				if (module != MODULE_VERIFICATION_V2) revert InvalidModule(module);
+				if (action != ACTION_APPEND_THRESHOLD_KEY) revert InvalidAction(action);


These 2 checks should be done in the decode function imho. exec should only be doing parsing and dispatching work, imho.

nonergodic · 2025-03-18T07:27:46Z

errors can be standalone (don't have to be declared inside a contract) and I'm not sure if you can't just declare them twice (presumably you moved it here because you can't?)

Yep, it complains about redefinition

… combined-contract

nonergodic · 2025-04-16T21:26:24Z

+pub struct VAA {
+  // Header
+  pub version: u8,
+  pub guardian_set_index: u32,


should be tss_index or something similar

nonergodic · 2025-04-16T21:31:42Z

+	pub fn get_seed_bytes(encoded_vaa: &[u8]) -> &[u8] {
+		// Extract the index from the encoded VAA


Suggested change

pub fn get_seed_bytes(encoded_vaa: &[u8]) -> &[u8] {

// Extract the index from the encoded VAA

pub fn get_tss_index(encoded_vaa: &[u8]) -> &[u8] {

nonergodic · 2025-04-16T21:33:34Z

+#[derive(InitSpace)]
+pub struct ThresholdKey {
+  pub bump: u8,
+  pub index: u32, // TODO: Is this needed?


Yes, keep this and drop the bump. One can then verify that the account is right by just checking that the owner is the oracle, that the discriminator matches (both done automatically by anchor) and the tss_index can be used to verify that it's the right address too.

nonergodic · 2025-04-16T21:35:12Z

+    Ok(())
+  }
+
+  pub fn verify_vaa(ctx: Context<VerifyVaa>, raw_vaa: Vec<u8>) -> Result<VAA> {


You can have this as an instruction, but the functionality itself should come from a library. There's no reason to force users to do a precious CPI for something that they can just as easily just verify themselves.

nonergodic · 2025-04-16T21:59:17Z

+		let mut payload = Vec::new();
+		cursor.read_to_end(&mut payload)?;
+
+		let double_hash = hash(&hash(&data[body_start..]).to_bytes()).to_bytes();


I'm not convinced you should be returning any hash here, and if you do, it should definitely be hashed only once. Double hashing was a mistake in the old EVM core bridge, but everywhere else, the hash of a VAA is always considered the single hash (e.g. Solana core bridge impl).

This is also documented in the TS-SDK, the Solidity SDK, and the docs.

nonergodic · 2025-04-16T22:01:32Z

+    if vaa.version != 2 {
+      return Err(VAAError::InvalidVersion.into());
+    }
+
+    // Check if the threshold key index matches the VAA index
+    let threshold_key = &mut ctx.accounts.threshold_key;
+    if threshold_key.index != vaa.guardian_set_index {
+      return Err(VAAError::InvalidIndex.into());
+    }
+
+    // Check if the threshold key has expired
+    if threshold_key.expiration != 0 && threshold_key.expiration < Clock::get().unwrap().unix_timestamp as u64 {
+      return Err(VAAError::GuardianSetExpired.into());
+    }


Move checks to accounts struct if possible.

nonergodic · 2025-04-16T22:02:13Z

+    if vaa.meta.emitter_chain != CHAIN_ID_SOLANA {
+      return Err(AppendThresholdKeyError::InvalidGovernanceChainId.into());
+    }
+
+    if vaa.meta.emitter_address != GOVERNANCE_ADDRESS {
+      return Err(AppendThresholdKeyError::InvalidGovernanceAddress.into());
+    }


Move checks to accounts struct.

nonergodic · 2025-04-16T22:04:57Z

+const GOVERNANCE_ADDRESS: [u8; 32] = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4];
+
+#[derive(Accounts)]
+#[instruction(new_index: u32, vaa_hash: [u8; 32])]


what's the purpose of the vaa_hash value here? The derivation doesn't guarantee anything, does it? Seems superfluous.

Yes, again, you should be able to take inspiration from TBRv3. It defines a type PostedRelayerMessage which it uses like so.

So you in turn can just use the analogous type from the core bridge for your governance message.

nonergodic · 2025-04-16T22:06:21Z

+const GOVERNANCE_ADDRESS: [u8; 32] = [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4];
+
+#[derive(Accounts)]
+#[instruction(new_index: u32, vaa_hash: [u8; 32])]


Likewise, there should be no need to pass in the new_index either. If the optional account does not exist, the index must be 0, otherwise you can take it from the account and add +1.

nonergodic · 2025-04-16T22:12:40Z

+    if new_index == 0 {
+      if ctx.accounts.old_threshold_key.is_some() {
+        return Err(AppendThresholdKeyError::InvalidOldThresholdKey.into());
+      }
+    } else {
+      if let Some(old_threshold_key) = &ctx.accounts.old_threshold_key {
+        if old_threshold_key.index != new_index - 1 {
+          return Err(AppendThresholdKeyError::InvalidOldThresholdKey.into());
+        }
+      } else {
+        return Err(AppendThresholdKeyError::InvalidOldThresholdKey.into());
+      }
+    }


Eliminate as mentioned above.

… combined-contract

These set a bogus discriminator for `PostedVaaData`. The discriminator is only used to emit the IDL so it's safe in terms of contract code. The IDL should be fixed afterwards probably.

… combined-contract

…ught.

This makes the semantics of the append threshold key VAA identical to the Solana implementation.

… combined-contract

JoshuaAverett added 3 commits January 31, 2025 10:27

Combined contracts, updated governance model

e5d080e

Updates for the new spec

003c13e

Remove old tests and add common interface for verification errors

01a12c1

nonergodic requested changes Feb 27, 2025

View reviewed changes

JoshuaAverett added 2 commits March 7, 2025 21:56

First round of updates

32d8de3

Add encode/decode functions

ae0b6bc

nonergodic requested changes Mar 18, 2025

View reviewed changes

JoshuaAverett and others added 5 commits March 20, 2025 12:18

Another round of corrections.

8102478

Make contract compile using the latest wormhole solidity sdk

a51c8f8

Move EVM code to its own folder and add checks for governance emitter

5efbb40

First pass of the solana code

ddeb014

Merge branch 'combined-contract' of github.com:XLabs/core-bridge into…

348fa72

… combined-contract

nonergodic requested changes Apr 16, 2025

View reviewed changes

JoshuaAverett and others added 17 commits April 17, 2025 12:53

Clean up account verification and a few other updates

a5fcd5f

Add Guardian TLS Registration to the contract

35e9c34

Minor changes and cargo update. Currently solana does not build.

187054f

Merge branch 'combined-contract' of github.com:XLabs/core-bridge into…

60ee633

… combined-contract

solana: derives serialization traits for VAA

530cbeb

solana: points at modified wormhole anchor bindings

81c9409

These set a bogus discriminator for `PostedVaaData`. The discriminator is only used to emit the IDL so it's safe in terms of contract code. The IDL should be fixed afterwards probably.

basic TS setup

e6f5f85

solana: fix import in test

44ca5a6

solana: sets up mocha

52088f9

Updates to solidity based on code review

35826a5

Fix for missing pullGuardianSets

2f2b4c3

Second round of code review

a9d5a94

Update Wormhole Solidity SDK dependency

680e9e0

Initial tests for solana

7495c9b

Merge branch 'combined-contract' of github.com:XLabs/core-bridge into…

eeeb593

… combined-contract

Tests run again!

3ed4a2d

Move TLS data to shards and add EIP712 encoding support

466e11c

JoshuaAverett and others added 29 commits May 12, 2025 15:27

Use Schnorr signatures

bb49a58

Updated to use Chainlink signature scheme

7f79de3

Big refactor and switch to schnorr

aa11772

Handle initialization a bit more carefully. Still needs some more tho…

2619a38

…ught.

Bug fix and minor formatting updates

d3ffd46

Check to ensure we don't have TSS sets not matching guardian sets

bc38300

Fix typo

3a170a8

evm: fixes some typos

90dc2a6

evm: decouples logic from data structure implementation

9e49bd7

solana: reworks tests to use the latest wormhole sdk

6cb90de

evm: updates makefile and foundry config

082a748

evm: fixes syntax error

d87f8b8

evm: some minor improvements and removing obsolete comments

e867c2f

evm: pulls only guardian sets starting from a specific index

e8ed134

evm: cleans up imports

bed8014

evm: renames a few classes and functions

c6c794a

evm: comments on register guardian verification

4b518fa

evm: makes the expiration time set only for past guardians

5f3ab80

This makes the semantics of the append threshold key VAA identical to the Solana implementation.

evm: removes redundant code

56db3eb

Refactor and cleanup

240bfda

Merge branch 'combined-contract' of github.com:XLabs/core-bridge into…

5984ff1

… combined-contract

evm: removes unused code

fb64344

misc: removes trailing whitespace

5fa5594

misc: adds solana recipes to makefile

b787770

Minor improvement in V2 verification path

88c7ac0

Merge branch 'combined-contract' of github.com:XLabs/core-bridge into…

d9db1c4

… combined-contract

Domain info and a bit more cleaning

a0ddf13

Add tests for everything besides V2 signatures

c855cd1

Added V2 tests

670ed8a

scnale deleted the combined-contract branch June 5, 2025 20:05

	for (uint32 i = uint32(_guardianSetExpirationTime.length); i <= coreGuardianSetIndex; ++i) {
	for (uint i = _guardianSetExpirationTime.length; i <= coreGuardianSetIndex; ++i) {

	ICoreBridge private _coreV1;
	ICoreBridge private immutable _coreBridge;

	for (uint i = oldGuardianSetLength; i < upper; i++) {
	for (uint i = oldGuardianSetLength; i < upper; ++i) {

	for (uint i = 0; i < shardsLength; i++) {
	for (uint i = 0; i < shardsLength; ++i) {

		if (module != MODULE_VERIFICATION_V2) revert InvalidModule(module);
		if (action != ACTION_APPEND_THRESHOLD_KEY) revert InvalidAction(action);

		pub fn get_seed_bytes(encoded_vaa: &[u8]) -> &[u8] {
		// Extract the index from the encoded VAA

	pub fn get_seed_bytes(encoded_vaa: &[u8]) -> &[u8] {
	// Extract the index from the encoded VAA
	pub fn get_tss_index(encoded_vaa: &[u8]) -> &[u8] {

Conversation

javerett commented Feb 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants