Solana receiver decode vaa accumulator #1145

swimricky · 2023-11-24T00:11:35Z

Summary

implement postAccumulatorUpdateDataVaa to verify hermes response and then invoke postVaa using CPI
implement postUpdates that uses the postedVaa account and the MerklePriceUpdates and verifies the updates
- this ix doesn't currently save any price info on-chain its just doing verification using the merkle root and price_updates

Notes

in order to fit the data for the postAccumulatorUpdateDataVaa, the client has to take the hermes response, parse it and construct a version of it without the price_updates then serialize and send the bytes as part of the instruction data
the dependencies still need to be cleaned up. There's quite a bit of overlap in functionality from the wormhole repo, the solana_sdk fork, and the wormhole-anchor-sdk.
updated the anchor version to 0.28.0 & solana version to 1.16.20. this was also part of the reason to add in some of the extra dependencies.

To Do(separate PRs):

initialize a config account that will have the configurations for the emitter address, emitter chain and owner of the post vaa account along with an authority that can write to it (aka the remote executor).
update pythnet_sdk to add BorshSerialize/Deserialize for MerklePriceUpdate and Keccak160 so that it can be passed in directly to post_updates
make PR to wormhole-anchor-sdk to include GuardianSet as well as the verify_signatures and post_vaa ix
remove wormhole-solana dependency

vercel · 2023-11-24T00:11:42Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
example-oracle-amm	⬜️ Ignored (Inspect)	Visit Preview		Nov 30, 2023 8:55pm
xc-admin-frontend	⬜️ Ignored (Inspect)	Visit Preview		Nov 30, 2023 8:55pm

jayantk

Thanks. I think there are some issues here though -- see inline comments

target_chains/solana/cli/src/main.rs

target_chains/solana/programs/pyth_solana_receiver/src/error.rs

target_chains/solana/programs/pyth_solana_receiver/src/lib.rs

jayantk · 2023-11-24T14:55:33Z

target_chains/solana/programs/pyth_solana_receiver/src/lib.rs

+    /// * `data` - Bytes of the AccumulatorUpdateData response from hermes with the updates omitted
+    ///           (i.e. the `updates` field is an empty array). The updates are removed so that
+    ///           all the data needed for postVaa can fit in one txn.
+    pub fn post_accumulator_update_vaa(


why do we have this instruction? It's a thin wrapper around posting the VAA, so why not have the client call the post vaa instruction themselves?

I think adding some extra types and structs would help to separate the boundary between the pythnet wireformat and the solana receiver wireformat.

the reason this is a wrapper instead of having the client do postVaa directly was so that we could do on-chain verification of the header in the accumulatorUpdateData using the AccumulatorUpdateData::try_from_slice

the client/cli code is currently doing the exact same thing when it first parses the accumulatorUpdateData bytes before it invokes this ix but i thought it would be good to have this verification on-chain as well

I think the header verification check isn't adding any value here. Someone could circumvent this check by calling post vaa themselves, and then calling the subsequent update method.

All of our security checks need to happen in the post_updates method itself. In this particular case, I don't think the magic number / version checks are security critical so they could potentially be skipped for solana (given that there's a sort of weird flow needed to do the vaa posting).

jayantk · 2023-11-24T14:56:36Z

target_chains/solana/programs/pyth_solana_receiver/src/lib.rs

+}
+
+#[derive(Default, AnchorSerialize, AnchorDeserialize, Clone, PartialEq, Eq)]
+pub struct GuardianSet {


this doesn't seem to be used anywhere in the program code. please delete.

I see from the CLI this seems to duplicate something that already exists in wormhole (?) If so let's just use the wormhole version.

in the cli, i'm currently using a mix of the wormhole repo, the wormhole repo fork with the solana-sdk and the wormhole-anchor-sdk.
the solana-sdk has this but it's using an outdated version of borsh/solana which was why i re-implemented this here for now.

ideally i would like to make a PR to the wormhole-anchor-sdk to add this and some of the other functionality from the sdk-solana repo and remove the solana-sdk fork dependency completely

can you move this struct to the CLI where it's actually used then?

jayantk · 2023-11-24T15:09:52Z

target_chains/solana/programs/pyth_solana_receiver/src/lib.rs

+        let vaa = &ctx.accounts.posted_vaa; // let posted_vaa_data = PostedVaaData::try_deserialize_unchecked(&mut &**vaa.try_borrow_data()?)?;
+        let wh_message = WormholeMessage::try_from_bytes(vaa.payload.as_slice())
+            .map_err(|_| ReceiverError::InvalidWormholeMessage)?;
+        msg!("constructed wh_message {:?}", wh_message);


the checks for the message emitter need to be added here (and removed from the instruction constraints)

can you elaborate on why it needs to be here vs. as an instruction constraint?

i added the message emitter checks here and in the postVaa wrapper. although there's no downside to having it in both, i think it makes more sense being checked in the postAccumulatorUpdateVaa ix than here.

I believe someone can post a vaa directly (by calling PostVAA themselves), then pass that VAA account to this function. If they do that, any security checks in this function are bypassed. (This is also why i think this function is kind of useless)

These checks should not be an instruction constraint because the data source emitter (chain id and address) is configurable via a governance message on all other chains. Using the instruction constraint is much less flexible than using a configuration parameter.

Also you need to check not only that the chain is correct, but also that the emitter address is correct.

.pre-commit-config.yaml

target_chains/solana/programs/pyth_solana_receiver/tests/program_test/mod.rs

jayantk · 2023-11-24T20:34:38Z

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

+            vaa.emitter_chain(),
+            emitter_chain,
+            ReceiverError::InvalidEmitterChain
+        );


I think you also need to check that the emitter address is the pyth program on the provided chain.

I suggest referencing the EVM implementation and making sure that every security check there is included here.

(this comment is tied to the theme above -- every PR should leave the code in a state that is OK from a security perspective)

jayantk · 2023-11-24T20:37:29Z

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

+    pub fn post_updates(
+        ctx: Context<PostUpdates>,
+        vaa_hash: [u8; 32], // used for pda seeds
+        emitter_chain: u16,


don't put a security critical parameter as an argument. this is just asking for us to forget about this TODO and leave a security vuln in the codebase. I would prefer you hardcoding a constant with the TODO above than this.

In general, I think we should strive to ensure that every PR leaves the code in a state that is OK from a security perspective. It may not be as configurable or flexible as we would like, or the functionality may not be there, but at least it's secure. That approach ensures that our own human fallibility doesn't cause security holes in our code.

jayantk · 2023-11-24T20:40:59Z

target_chains/solana/programs/pyth_solana_receiver/src/lib.rs

+        let vaa = &ctx.accounts.posted_vaa; // let posted_vaa_data = PostedVaaData::try_deserialize_unchecked(&mut &**vaa.try_borrow_data()?)?;
+        let wh_message = WormholeMessage::try_from_bytes(vaa.payload.as_slice())
+            .map_err(|_| ReceiverError::InvalidWormholeMessage)?;
+        msg!("constructed wh_message {:?}", wh_message);


I believe someone can post a vaa directly (by calling PostVAA themselves), then pass that VAA account to this function. If they do that, any security checks in this function are bypassed. (This is also why i think this function is kind of useless)

These checks should not be an instruction constraint because the data source emitter (chain id and address) is configurable via a governance message on all other chains. Using the instruction constraint is much less flexible than using a configuration parameter.

Also you need to check not only that the chain is correct, but also that the emitter address is correct.

guibescos · 2023-11-24T22:49:33Z

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

+    #[allow(unused_variables)]
+    pub fn post_updates(
+        ctx: Context<PostUpdates>,
+        vaa_hash: [u8; 32], // used for pda seeds


I don't think you need vaa_hash as a parameters here. We are trusting the posted VAA based on it being owned by wormhole + the discriminator

i don't mind removing this but technically any vaa that has been posted by the wormhole program would be owned by wormhole and have the right discriminator. The merkle root prove would fail however in that situation if the wrong vaa was used

i removed the vaa_hash from the ix parameters but added the signature_set account into the PostUpdates context since it also contains the hash & we can verify that it's the correct signature_set since the vaa has a field for the signature set pubkey.

guibescos · 2023-11-24T22:50:44Z

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

+    )]
+    pub posted_vaa:       Box<Account<'info, AnchorVaa>>,
+    /// CHECK: program that called post_vaa
+    pub post_vaa_program: AccountInfo<'info>,


I think you don't need this account

i removed this and changed the key derivation to be in the ix instead. right now im hard-coding that the owner of the posted_vaa program is the wormhole program but based off this feedback from @jayantk #1145 (comment)
i think we want it to be configurable so i will include it in the PR that adds a config account that only the authority can write to

guibescos · 2023-11-24T22:52:39Z

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

+                return err!(ReceiverError::InvalidPriceUpdate);
+            }
+            let msg = from_slice::<byteorder::BE, Message>(&message_vec)
+                .map_err(|_| ReceiverError::InvalidAccumulatorMessage)?;


shouldn't this be a DeserializedMessageFailed?

updated this thanks

guibescos

For governance I recommend you just create a config account that has the config (emitter_address, emitter_chain) and an authority that can update this fields.
We can use the remote executor as the authority later.

swimricky · 2023-11-27T18:52:58Z

For governance I recommend you just create a config account that has the config (emitter_address, emitter_chain) and an authority that can update this fields. We can use the remote executor as the authority later.

thanks for this, that was the eventual plan but i'm going to add the config/governance in a separate PR since this one already got a little bit out of hand size wise

…ddr for now

…gnatureset for postupdates

jayantk

This is much improved from the last round. I have some minor comments, but also one blocking comment, which is:

I am confused about the VAA verification part of this code. What is the right way to check that we are getting a valid (meaning signature verified) VAA account? We shouldn't be inventing something here -- we should be using whatever the prescribed approach from wormhole is.

(Also, meta-comment: the code should be documented enough to explain what wormhole guarantees and why this approach is secure. I shouldn't be guessing as to the security here.)

target_chains/solana/cli/src/main.rs

target_chains/solana/programs/pyth-solana-receiver/src/error.rs

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

jayantk · 2023-11-28T18:30:12Z

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

+            vaa.key(),
+            expected_vaa_pubkey,
+            ReceiverError::InvalidVaaAccountKey
+        );


What are the security requirements for verifying that the posted VAA is correctly signed? I don't understand why we need the signature_set here. How do other programs do this?

this wormhole example seems like it's using the vaa_hash https://github.com/wormhole-foundation/wormhole-scaffolding/blob/main/solana/programs/01_hello_world/src/context.rs#L243

not sure if that's the current best practice or not. hard to tell from their docs.

Hey a VAA can only be posted after checking the signatures.
I think the requirements are checking the program owner and the discriminator.

checking the vaa_pubkey is an extra layer of security. we have 1 layer from the merkle root in the vaa.payload and proving against it, then this vaa_pubkey check is an additional layer as well.

the signature_set is passed in so we can check 2 things:

the pubkey of the signature_set matches the posted_vaa's signature_set pubkey

the signature_set contains the vaa_hash which we use to derive the expected vaa_pubkey.

i was originally passing in the vaa_hash like the wormhole example you posted but switched to this implementation because of this discussion #1145 (comment)

wormhole checks the vaa a a few times

verify_signatures when it initializes the signature_set account using the secp256k1 ix data - https://github.com/wormhole-foundation/wormhole/blob/main/solana/bridge/program/src/api/verify_signature.rs#L164-L169

post_vaa
a. checking the key of the vaa account - https://github.com/wormhole-foundation/wormhole/blob/main/solana/bridge/program/src/api/post_vaa.rs#L107-L108C13
b. hashing the relevant fields and checking against the signature_set.hash - https://github.com/wormhole-foundation/wormhole/blob/main/solana/bridge/program/src/api/post_vaa.rs#L214

i took a look at the wormhole-token-bridge program completeTransfer ix since it logically follows a similar flow (verifySignatures -> postVaa -> completeTransfer). it doesn't seem to have any checks related the actual hash/key of the posted vaa, just some of the metadata and fields related to the actual token transfer
https://github.com/wormhole-foundation/wormhole/blob/main/solana/modules/token_bridge/program/src/api/complete_transfer.rs#L103-L118

I still don't get passing the signature set signature set.
A posted VAA is a VAA whose signatures have already been checked.
People should be able to pass whatever posted VAA to this program, if the proof doesn't match the VAA it will just fail.

the signature set is just a different way to pass the vaa_hash since it also contains the hash of the data. this is just an extra check b/c like you said, if the proof doesn't match the vaa then it'll fail.

I don't understand why you need this extra check, let's just keep it simple

ok ill remove this account then and update the comments that we're assuming wormhole has done the necessary security checks when the verify_signatures & post_vaa ixs were called and the merkle root from the posted_vaa.payload + proving will fail if the vaa is wrong anyways

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

target_chains/solana/programs/pyth-solana-receiver/src/state.rs

jayantk · 2023-11-28T18:40:57Z

target_chains/solana/programs/pyth-solana-receiver/tests/program_test/mod.rs

-        // https://docs.rs/solana-program/1.6.4/solana_program/bpf_loader_upgradeable/index.html
-        let (programdata_key, _) =
-            Pubkey::find_program_address(&[&program_key.to_bytes()], &bpf_loader_upgradeable::id());
+        let mut program_test = ProgramTest::new("pyth_solana_receiver", ID, None);


i really think you shouldn't change this deployment code. We're doing it this way for a reason (to test upgrades) and you're just going to have to rewrite it again.

ok will look into reverting this to original state

i reset it back to the original state but just a note that i tried adding back the original update ix and using that test case and it failed with a strange InvalidAccountData. I tried debugging the source of this but wasn't able to figure it out. i'll have to look into it later. The test runs fine when i use ProgramTest::new("pyth_solana_receiver", ID, None);

also is there a repo that tests the program upgrade flow? https://github.com/pyth-network/pyth-client/blob/main/program/rust/src/tests/pyth_simulator.rs also has this setup but it doesn't look like the actual program upgrade is tested there, the upgrade authority is checked for the upd_permissions test

target_chains/solana/cli/src/main.rs

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs

add documentation and todos, revert programsimulator to use upgradable bpf loader

…eadme

…nvalidaccountdata issue

target_chains/solana/cli/src/main.rs

guibescos · 2023-11-30T17:40:24Z

target_chains/solana/programs/pyth-solana-receiver/src/error.rs

+    #[msg("Received an invalid signature set")]
+    InvalidSignatureSet,
+    #[msg("The pubkey of the the posted vaa account is invalid")]
+    InvalidVaaAccountKey,


Many errors here are no longer used

guibescos

It's looking good, I'll let Jayant take one last look

jayantk · 2023-11-30T20:38:12Z

target_chains/solana/programs/pyth-solana-receiver/tests/program_test/mod.rs

+
+    PathBuf::from(target_dir).join("deploy/pyth_solana_receiver.so")
+    };
+}


this is not the right way to do this. you should use a build.rs script to ensure the bpf gets built when you run the normal build. pyth-client has a good example

i guess this can wait for a later pr though. this pr is pretty big already

i pulled this from the pyth-client - https://github.com/pyth-network/pyth-client/blob/main/program/rust/src/tests/pyth_simulator.rs#L73-L108

…amically against config

swimricky added 4 commits November 23, 2023 13:40

feat: impl postAccumulatorUpdateVaa and postUpdates and CLI test changes

05724a7

feat: clean up

f4f678f

refactor: reorganize and fix tests

8bd6989

refactor: clean up

ed9464d

swimricky requested review from jayantk, ali-behjati, Reisen and guibescos and removed request for jayantk November 24, 2023 00:19

chore: add rust-toolchain for solana target contract dir

1cfa133

jayantk requested changes Nov 24, 2023

View reviewed changes

swimricky added 2 commits November 24, 2023 10:34

refactor: clean up and address feedback

561c5e2

refactor: make emitter chain & post vaa program configurable

3478494

jayantk reviewed Nov 24, 2023

View reviewed changes

guibescos reviewed Nov 24, 2023

View reviewed changes

feat: remove postAccUpdateVaa and use wh post_vaa in cli

06b9a19

swimricky added 5 commits November 27, 2023 11:21

fix: add check for emitter address, hardcode expected emitter chain/a…

410bd66

…ddr for now

chore: cleanup

a18d93a

chore: clean up

7400e7f

feat: address feedback, move checks from context into account, use si…

8a61a9c

…gnatureset for postupdates

feat: use pubkey and require_keys_eq! for emitter addr check

c259dd3

swimricky requested review from guibescos and jayantk November 28, 2023 15:56

jayantk reviewed Nov 28, 2023

View reviewed changes

swimricky commented Nov 29, 2023

View reviewed changes

target_chains/solana/programs/pyth-solana-receiver/src/lib.rs Outdated Show resolved Hide resolved

refactor: address feedback

49eb7d6

add documentation and todos, revert programsimulator to use upgradable bpf loader

swimricky requested a review from jayantk November 29, 2023 19:44

swimricky added 2 commits November 30, 2023 09:17

feat: remove signature_set account from post_updates, update docs & r…

b7b1368

…eadme

test: add lazy static for setting up pyth binary but still has same i…

2ff9f46

…nvalidaccountdata issue

guibescos reviewed Nov 30, 2023

View reviewed changes

target_chains/solana/cli/src/main.rs Outdated Show resolved Hide resolved

chore: clean up

6f69c09

guibescos reviewed Nov 30, 2023

View reviewed changes

refactor: remove unused errors

7596bd6

jayantk approved these changes Nov 30, 2023

View reviewed changes

feat: use UncheckedAccount for posted_vaa so owner can be checked dyn…

83ec60a

…amically against config

swimricky merged commit 8202181 into main Nov 30, 2023

swimricky deleted the solana-receiver-decodeVaa-accumulator branch November 30, 2023 21:04

Solana receiver decode vaa accumulator #1145

Solana receiver decode vaa accumulator #1145

Uh oh!

Conversation

swimricky commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

To Do(separate PRs):

Uh oh!

vercel bot commented Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

jayantk Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

guibescos left a comment

Choose a reason for hiding this comment

Uh oh!

swimricky commented Nov 27, 2023

Uh oh!

jayantk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

swimricky commented Nov 24, 2023 •

edited

Loading

vercel bot commented Nov 24, 2023 •

edited

Loading

jayantk Nov 24, 2023 •

edited

Loading

guibescos Nov 29, 2023 •

edited

Loading

guibescos Nov 29, 2023 •

edited

Loading