From 66783e5ef18633eb2030b0942cdcda40b83f5e25 Mon Sep 17 00:00:00 2001 From: Jayant Krishnamurthy Date: Wed, 1 Mar 2023 08:29:12 -0800 Subject: [PATCH] add docs for audit (#645) Co-authored-by: Jayant Krishnamurthy --- target_chains/cosmwasm/contracts/README.md | 5 ++ .../cosmwasm/contracts/pyth/src/contract.rs | 53 +++++++++++++++---- .../cosmwasm/contracts/pyth/src/governance.rs | 31 +++++++---- .../cosmwasm/contracts/pyth/src/state.rs | 6 ++- 4 files changed, 75 insertions(+), 20 deletions(-) diff --git a/target_chains/cosmwasm/contracts/README.md b/target_chains/cosmwasm/contracts/README.md index 86cf02cd..9b45316b 100644 --- a/target_chains/cosmwasm/contracts/README.md +++ b/target_chains/cosmwasm/contracts/README.md @@ -37,3 +37,8 @@ Available price feeds on these networks can be find below: | Network | Available Price Feeds | | ----------------- | ------------------------------------------------------------------------------------------------------------------------------------ | | Injective Testnet | [https://pyth.network/developers/price-feed-ids#injective-testnet](https://pyth.network/developers/price-feed-ids#injective-testnet) | + +## Developing + +The cosmwasm contract lives in the `pyth` subdirectory. +From that directory, you can build the contract with `cargo build` and run unit tests with `cargo test`. diff --git a/target_chains/cosmwasm/contracts/pyth/src/contract.rs b/target_chains/cosmwasm/contracts/pyth/src/contract.rs index 61e120d8..a75c07c6 100644 --- a/target_chains/cosmwasm/contracts/pyth/src/contract.rs +++ b/target_chains/cosmwasm/contracts/pyth/src/contract.rs @@ -10,6 +10,7 @@ use { UpgradeContract, }, GovernanceInstruction, + GovernanceModule, }, msg::{ InstantiateMsg, @@ -112,7 +113,12 @@ pub fn instantiate( Ok(Response::default()) } -pub fn parse_vaa(deps: DepsMut, block_time: u64, data: &Binary) -> StdResult { +/// Verify that `data` represents an authentic Wormhole VAA. +/// +/// *Warning* this function does not verify the emitter of the wormhole message; it only checks +/// that the wormhole signatures are valid. The caller is responsible for checking that the message +/// originates from the expected emitter. +pub fn parse_and_verify_vaa(deps: DepsMut, block_time: u64, data: &Binary) -> StdResult { let cfg = config_read(deps.storage).load()?; let vaa: ParsedVAA = deps.querier.query(&QueryRequest::Wasm(WasmQuery::Smart { contract_addr: cfg.wormhole_contract.to_string(), @@ -134,6 +140,11 @@ pub fn execute(deps: DepsMut, env: Env, info: MessageInfo, msg: ExecuteMsg) -> S } } +/// Update the on-chain price feeds given the array of price update VAAs `data`. +/// Each price update VAA must be a valid Wormhole message and sent from an authorized emitter. +/// +/// This method additionally requires the caller to pay a fee to the contract; the +/// magnitude of the fee depends on both the data and the current contract configuration. fn update_price_feeds( mut deps: DepsMut, env: Env, @@ -142,6 +153,7 @@ fn update_price_feeds( ) -> StdResult { let state = config_read(deps.storage).load()?; + // Check that a sufficient fee was sent with the message if state.fee.amount.u128() > 0 && !has_coins(info.funds.as_ref(), &get_update_fee(&deps.as_ref(), data)?) { @@ -151,7 +163,7 @@ fn update_price_feeds( let mut total_attestations: usize = 0; let mut new_attestations: usize = 0; for datum in data { - let vaa = parse_vaa(deps.branch(), env.block.time.seconds(), datum)?; + let vaa = parse_and_verify_vaa(deps.branch(), env.block.time.seconds(), datum)?; verify_vaa_from_data_source(&state, &vaa)?; let data = &vaa.payload; @@ -170,19 +182,24 @@ fn update_price_feeds( .add_attribute("num_updated", format!("{new_attestations}"))) } +/// Execute a governance instruction provided as the VAA `data`. +/// The VAA must come from an authorized governance emitter. +/// See [GovernanceInstruction] for descriptions of the supported operations. fn execute_governance_instruction( mut deps: DepsMut, env: Env, _info: MessageInfo, data: &Binary, ) -> StdResult { - let vaa = parse_vaa(deps.branch(), env.block.time.seconds(), data)?; + let vaa = parse_and_verify_vaa(deps.branch(), env.block.time.seconds(), data)?; let state = config_read(deps.storage).load()?; + verify_vaa_from_governance_source(&state, &vaa)?; // store updates to the config as a result of this action in here. let mut updated_config: ConfigInfo = state.clone(); - verify_vaa_from_governance_source(&state, &vaa)?; + // Governance messages must be applied in order. This check prevents replay attacks where + // previous messages are re-applied. if vaa.sequence <= state.governance_sequence_number { return Err(PythContractError::OldGovernanceMessage)?; } else { @@ -193,10 +210,18 @@ fn execute_governance_instruction( let instruction = GovernanceInstruction::deserialize(&data[..]) .map_err(|_| PythContractError::InvalidGovernancePayload)?; + // Check that the instruction is intended for this chain. + // chain_id = 0 means the instruction applies to all chains if instruction.target_chain_id != state.chain_id && instruction.target_chain_id != 0 { return Err(PythContractError::InvalidGovernancePayload)?; } + // Check that the instruction is intended for this target chain contract (as opposed to + // other Pyth contracts that may live on the same chain). + if instruction.module != GovernanceModule::Target { + return Err(PythContractError::InvalidGovernancePayload)?; + } + let response = match instruction.action { UpgradeContract { code_id } => { if instruction.target_chain_id == 0 { @@ -205,7 +230,8 @@ fn execute_governance_instruction( upgrade_contract(&env.contract.address, code_id)? } AuthorizeGovernanceDataSourceTransfer { claim_vaa } => { - let parsed_claim_vaa = parse_vaa(deps.branch(), env.block.time.seconds(), &claim_vaa)?; + let parsed_claim_vaa = + parse_and_verify_vaa(deps.branch(), env.block.time.seconds(), &claim_vaa)?; transfer_governance(&mut updated_config, &state, &parsed_claim_vaa)? } SetDataSources { data_sources } => { @@ -252,8 +278,9 @@ fn execute_governance_instruction( Ok(response) } -/// Transfers governance to the data source provided in `parsed_claim_vaa`. Stores the new -/// governance parameters in `next_config`. +/// Transfers governance to the data source provided in `parsed_claim_vaa`. +/// This function updates the contract config in `next_config`; it is the caller's responsibility +/// to save this configuration in the on-chain storage. fn transfer_governance( next_config: &mut ConfigInfo, current_config: &ConfigInfo, @@ -263,6 +290,10 @@ fn transfer_governance( GovernanceInstruction::deserialize(parsed_claim_vaa.payload.as_slice()) .map_err(|_| PythContractError::InvalidGovernancePayload)?; + // Check that the requester is asking to govern this target chain contract. + // chain_id == 0 means they're asking for governance of all target chain contracts. + // (this check doesn't matter for security because we have already checked the information + // in the authorization message.) if claim_vaa_instruction.target_chain_id != current_config.chain_id && claim_vaa_instruction.target_chain_id != 0 { @@ -342,6 +373,7 @@ fn verify_vaa_from_governance_source(state: &ConfigInfo, vaa: &ParsedVAA) -> Std Ok(()) } +/// Update the on-chain storage for any new price updates provided in `batch_attestation`. fn process_batch_attestation( deps: &mut DepsMut, env: &Env, @@ -454,8 +486,9 @@ pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> StdResult { } } -pub fn query_price_feed(deps: &Deps, address: &[u8]) -> StdResult { - match price_info_read(deps.storage).load(address) { +/// Get the most recent value of the price feed indicated by `feed_id`. +pub fn query_price_feed(deps: &Deps, feed_id: &[u8]) -> StdResult { + match price_info_read(deps.storage).load(feed_id) { Ok(price_info) => Ok(PriceFeedResponse { price_feed: price_info.price_feed, }), @@ -463,6 +496,8 @@ pub fn query_price_feed(deps: &Deps, address: &[u8]) -> StdResult StdResult { let config = config_read(deps.storage).load()?; diff --git a/target_chains/cosmwasm/contracts/pyth/src/governance.rs b/target_chains/cosmwasm/contracts/pyth/src/governance.rs index 0edfe20e..5f4382ba 100644 --- a/target_chains/cosmwasm/contracts/pyth/src/governance.rs +++ b/target_chains/cosmwasm/contracts/pyth/src/governance.rs @@ -24,7 +24,7 @@ const PYTH_GOVERNANCE_MAGIC: &[u8] = b"PTGM"; #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] #[repr(u8)] pub enum GovernanceModule { - /// The PythNet executor contract + /// The PythNet executor contract. Messages sent to the Executor = 0, /// A target chain contract (like this one!) Target = 1, @@ -48,16 +48,33 @@ impl GovernanceModule { } /// The action to perform to change the state of the target chain contract. +/// +/// Note that the order of the enum cannot be changed, as the integer representation of +/// each field must be preserved for backward compatibility. #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, JsonSchema)] #[repr(u8)] pub enum GovernanceAction { - UpgradeContract { code_id: u64 }, // 0 + /// Upgrade the code for the contract to the code uploaded at code_id + UpgradeContract { code_id: u64 }, // 0 + /// This action is the second step of a governance handoff process. + /// The handoff is as follows: + /// 1. The new governance emitter creates a VAA containing a RequestGovernanceDataSourceTransfer action + /// 2. The existing governance emitter creates a AuthorizeGovernanceDataSourceTransfer message where + /// claim_vaa is the VAA from step 1. + /// 3. The VAA from step 2 is submitted to the contract. + /// + /// This 2-step process ensures that the new emitter is able to send VAAs before the transfer + /// is completed. AuthorizeGovernanceDataSourceTransfer { claim_vaa: Binary }, // 1 - SetDataSources { data_sources: Vec }, // 2 - // Set the fee to val * (10 ** expo) + /// Set the set of authorized emitters for price update messages. + SetDataSources { data_sources: Vec }, // 2 + /// Set the fee to val * (10 ** expo) SetFee { val: u64, expo: u64 }, // 3 - // Set the default valid period to the provided number of seconds + /// Set the default valid period to the provided number of seconds SetValidPeriod { valid_seconds: u64 }, // 4 + /// The first step of the governance handoff process (see documentation + /// on AuthorizeGovernanceDataSourceTransfer). `governance_data_source_index` is an incrementing + /// sequence number that ensures old transfer messages cannot be replayed. RequestGovernanceDataSourceTransfer { governance_data_source_index: u32 }, // 5 } @@ -83,10 +100,6 @@ impl GovernanceInstruction { let module_num = bytes.read_u8()?; let module = GovernanceModule::from_u8(module_num)?; - if module != GovernanceModule::Target { - return Err(format!("Invalid governance module {module_num}",).into()); - } - let action_type: u8 = bytes.read_u8()?; let target_chain_id: u16 = bytes.read_u16::()?; diff --git a/target_chains/cosmwasm/contracts/pyth/src/state.rs b/target_chains/cosmwasm/contracts/pyth/src/state.rs index 89215db9..2a58a338 100644 --- a/target_chains/cosmwasm/contracts/pyth/src/state.rs +++ b/target_chains/cosmwasm/contracts/pyth/src/state.rs @@ -31,6 +31,8 @@ use { pub static CONFIG_KEY: &[u8] = b"config"; pub static PRICE_INFO_KEY: &[u8] = b"price_info_v4"; +/// A `PythDataSource` identifies a specific contract (given by its Wormhole `emitter`) on +/// a specific blockchain (given by `chain_id`). #[derive(Serialize, Deserialize, Clone, Debug, PartialEq, Eq, Hash, JsonSchema)] pub struct PythDataSource { pub emitter: Binary, @@ -54,8 +56,8 @@ pub struct ConfigInfo { // governance messages, whereas the one above is generated by Pyth and only applicable to governance // source transfers. pub governance_sequence_number: u64, - // FIXME: This id needs to agree with the wormhole chain id. - // We should read this directly from wormhole. + // Warning: This id needs to agree with the wormhole chain id. + // We should read this directly from wormhole, but their contract doesn't expose it. pub chain_id: u16, pub valid_time_period: Duration,