diff --git a/state-chain/cf-integration-tests/src/solana.rs b/state-chain/cf-integration-tests/src/solana.rs index bec2f7ab2f3..dd099ffeba4 100644 --- a/state-chain/cf-integration-tests/src/solana.rs +++ b/state-chain/cf-integration-tests/src/solana.rs @@ -1,27 +1,35 @@ #![cfg(test)] +use std::marker::PhantomData; + use super::*; use cf_chains::{ address::{AddressConverter, AddressDerivationApi, EncodedAddress}, assets::any::Asset, ccm_checker::CcmValidityError, sol::{ - api::{SolanaEnvironment, SolanaTransactionBuildingError}, + api::{SolanaApi, SolanaEnvironment, SolanaTransactionBuildingError}, sol_tx_core::sol_test_values, + transaction_builder::SolanaTransactionBuilder, SolAddress, SolApiEnvironment, SolCcmAccounts, SolCcmAddress, SolHash, SolPubkey, SolTrackedData, SolanaCrypto, }, CcmChannelMetadata, CcmDepositMetadata, Chain, ChainState, ExecutexSwapAndCallError, - ForeignChainAddress, SetAggKeyWithAggKey, SetAggKeyWithAggKeyError, Solana, SwapOrigin, + ForeignChainAddress, RequiresSignatureRefresh, SetAggKeyWithAggKey, SetAggKeyWithAggKeyError, + Solana, SwapOrigin, TransactionBuilder, }; use cf_primitives::{AccountRole, AuthorityCount, ForeignChain, SwapId}; use cf_test_utilities::{assert_events_match, assert_has_matching_event}; +use cf_utilities::bs58_array; use codec::Encode; use frame_support::traits::{OnFinalize, UnfilteredDispatchable}; use pallet_cf_ingress_egress::DepositWitness; use pallet_cf_validator::RotationPhase; use state_chain_runtime::{ - chainflip::{address_derivation::AddressDerivation, ChainAddressConverter, SolEnvironment}, + chainflip::{ + address_derivation::AddressDerivation, ChainAddressConverter, SolEnvironment, + SolanaTransactionBuilder as RuntimeSolanaTransactionBuilder, + }, Runtime, RuntimeCall, RuntimeEvent, SolanaIngressEgress, SolanaInstance, SolanaThresholdSigner, Swapping, }; @@ -522,3 +530,81 @@ fn failed_ccm_does_not_consume_durable_nonce() { ) }); } + +#[test] +fn solana_resigning() { + use crate::solana::sol_test_values::TEST_DURABLE_NONCE; + + const EPOCH_BLOCKS: u32 = 100; + const MAX_AUTHORITIES: AuthorityCount = 10; + super::genesis::with_test_defaults() + .blocks_per_epoch(EPOCH_BLOCKS) + .max_authorities(MAX_AUTHORITIES) + .with_additional_accounts(&[ + (DORIS, AccountRole::LiquidityProvider, 5 * FLIPPERINOS_PER_FLIP), + (ZION, AccountRole::Broker, 5 * FLIPPERINOS_PER_FLIP), + (ALICE, AccountRole::Broker, 5 * FLIPPERINOS_PER_FLIP), + (BOB, AccountRole::Broker, 5 * FLIPPERINOS_PER_FLIP), + ]) + .build() + .execute_with(|| { + let (mut testnet, _, _) = network::fund_authorities_and_join_auction(MAX_AUTHORITIES); + assert_ok!(RuntimeCall::SolanaVault( + pallet_cf_vaults::Call::::initialize_chain {} + ) + .dispatch_bypass_filter(pallet_cf_governance::RawOrigin::GovernanceApproval.into())); + testnet.move_to_the_next_epoch(); + + setup_sol_environments(); + register_refund_addresses(&DORIS); + setup_pool_and_accounts(vec![Asset::Sol, Asset::SolUsdc], OrderType::LimitOrder); + + const CURRENT_SIGNER: [u8; 32] = [3u8; 32]; + + let mut transaction = SolanaTransactionBuilder::transfer_native( + 10000000, + SolAddress(bs58_array("EwVmJgZwHBzmdVUzdujfbxFdaG25PMzbPLx8F7PvhWgs")), + CURRENT_SIGNER.into(), + (SolAddress(bs58_array("2cNMwUCF51djw2xAiiU54wz1WrU8uG4Q8Kp8nfEuwghw")), TEST_DURABLE_NONCE), + 100, + ).unwrap(); + transaction.signatures = vec![[1u8; 64].into()]; + + let original_account_keys = transaction.message.account_keys.clone(); + + let apicall = SolanaApi { + call_type: cf_chains::sol::api::SolanaTransactionType::Transfer, + transaction, + signer: Some(CURRENT_SIGNER.into()), + _phantom: PhantomData::, + }; + + let modified_call = RuntimeSolanaTransactionBuilder::requires_signature_refresh( + &apicall, + &Default::default(), + Some([5u8; 32].into()), + ); + if let RequiresSignatureRefresh::True(call) = modified_call { + let agg_key = ::current_agg_key().unwrap(); + let transaction = call.clone().unwrap().transaction; + for (modified_key, original_key) in transaction.message.account_keys.iter().zip(original_account_keys.iter()) { + if *original_key != SolPubkey::from(CURRENT_SIGNER) { + assert_eq!(modified_key, original_key); + assert_ne!(*modified_key, SolPubkey::from(agg_key)); + } else { + assert_eq!(*modified_key, SolPubkey::from(agg_key)); + } + } + let serialized_tx = transaction + .clone() + .finalize_and_serialize().unwrap(); + + // Compare against a manually crafted transaction that works with the current test values and + // agg_key. Not comparing the first byte (number of signatures) nor the signature itself + let expected_serialized_tx = hex_literal::hex!("010000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001000306f68d61e8d834034cf583f486f2a08ef53ce4134ed41c4d88f4720c39518745b617eb2b10d3377bda2bc7bea65bec6b8372f4fc3463ec2cd6f9fde4b2c633d192cf1dd130e0341d60a0771ac40ea7900106a423354d2ecd6e609bd5e2ed833dec00000000000000000000000000000000000000000000000000000000000000000306466fe5211732ffecadba72c39be7bc8ce5bbc5f7126b2c439b3a4000000006a7d517192c568ee08a845f73d29788cf035c3145b21ab344d8062ea9400000c27e9074fac5e8d36cf04f94a0606fdd8ddbb420e99a489c7915ce5699e4890004030301050004040000000400090364000000000000000400050284030000030200020c020000008096980000000000").to_vec(); + assert_eq!(&serialized_tx[1+64..], &expected_serialized_tx[1+64..]); + } else { + panic!("RequiresSignatureRefresh is False"); + } + }); +} diff --git a/state-chain/chains/src/lib.rs b/state-chain/chains/src/lib.rs index cd2968588e2..f586f7254a6 100644 --- a/state-chain/chains/src/lib.rs +++ b/state-chain/chains/src/lib.rs @@ -4,6 +4,7 @@ #![feature(split_array)] use core::{fmt::Display, iter::Step}; +use sp_std::marker::PhantomData; use crate::{ benchmarking_value::{BenchmarkValue, BenchmarkValueExtended}, @@ -23,7 +24,8 @@ use frame_support::{ traits::{AtLeast32BitUnsigned, CheckedAdd, CheckedSub}, BoundedVec, DispatchError, }, - Blake2_256, CloneNoBound, DebugNoBound, EqNoBound, Parameter, PartialEqNoBound, StorageHasher, + Blake2_256, CloneNoBound, DebugNoBound, EqNoBound, Never, Parameter, PartialEqNoBound, + StorageHasher, }; use instances::{ChainCryptoInstanceAlias, ChainInstanceAlias}; use scale_info::TypeInfo; @@ -390,7 +392,7 @@ where call: &Call, payload: &<::ChainCrypto as ChainCrypto>::Payload, maybe_current_onchain_key: Option<<::ChainCrypto as ChainCrypto>::AggKey>, - ) -> bool; + ) -> RequiresSignatureRefresh; /// Calculate the Units of gas that is allowed to make this call. fn calculate_gas_limit(_call: &Call) -> Option { @@ -703,7 +705,6 @@ pub struct ChannelRefundParameters { pub refund_address: A, pub min_price: Price, } - impl ChannelRefundParameters { pub fn map_address B>(&self, f: F) -> ChannelRefundParameters { ChannelRefundParameters { @@ -713,3 +714,9 @@ impl ChannelRefundParameters { } } } + +pub enum RequiresSignatureRefresh> { + True(Option), + False, + _Phantom(PhantomData, Never), +} diff --git a/state-chain/chains/src/mocks.rs b/state-chain/chains/src/mocks.rs index d0361b171bd..15d0566a614 100644 --- a/state-chain/chains/src/mocks.rs +++ b/state-chain/chains/src/mocks.rs @@ -428,7 +428,11 @@ impl, Call: ApiCall> _call: &Call, _payload: &<::ChainCrypto as ChainCrypto>::Payload, _maybe_current_on_chain_key: Option<<::ChainCrypto as ChainCrypto>::AggKey>, - ) -> bool { - REQUIRES_REFRESH.with(|is_valid| *is_valid.borrow()) + ) -> RequiresSignatureRefresh { + if REQUIRES_REFRESH.with(|is_valid| *is_valid.borrow()) { + RequiresSignatureRefresh::True(None) + } else { + RequiresSignatureRefresh::False + } } } diff --git a/state-chain/chains/src/sol/api.rs b/state-chain/chains/src/sol/api.rs index 1762a50ea9b..c86d3109504 100644 --- a/state-chain/chains/src/sol/api.rs +++ b/state-chain/chains/src/sol/api.rs @@ -131,9 +131,10 @@ pub enum SolanaTransactionType { pub struct SolanaApi { pub call_type: SolanaTransactionType, pub transaction: SolTransaction, + pub signer: Option, #[doc(hidden)] #[codec(skip)] - _phantom: PhantomData, + pub _phantom: PhantomData, } impl SolanaApi { @@ -158,6 +159,7 @@ impl SolanaApi { Ok(Self { call_type: SolanaTransactionType::BatchFetch, transaction, + signer: None, _phantom: Default::default(), }) } @@ -210,6 +212,7 @@ impl SolanaApi { Self { call_type: SolanaTransactionType::Transfer, transaction, + signer: None, _phantom: Default::default(), }, vec![egress_id], @@ -255,6 +258,7 @@ impl SolanaApi { Ok(Self { call_type: SolanaTransactionType::RotateAggKey, transaction, + signer: None, _phantom: Default::default(), }) } @@ -371,6 +375,7 @@ impl SolanaApi { Ok(Self { call_type: SolanaTransactionType::CcmTransfer, transaction, + signer: None, _phantom: Default::default(), }) } @@ -384,9 +389,10 @@ impl ApiCall for SolanaApi { fn signed( mut self, threshold_signature: &::ThresholdSignature, - _signer: ::AggKey, + signer: ::AggKey, ) -> Self { self.transaction.signatures = vec![*threshold_signature]; + self.signer = Some(signer); self } @@ -403,11 +409,11 @@ impl ApiCall for SolanaApi { } fn refresh_replay_protection(&mut self) { - todo!() + // No replay protection refresh for Solana. } fn signer(&self) -> Option<::AggKey> { - todo!() + self.signer } } @@ -510,6 +516,7 @@ impl SetGovKeyWithAggKey for Solan Ok(Self { call_type: SolanaTransactionType::SetGovKeyWithAggKey, transaction, + signer: None, _phantom: Default::default(), }) } diff --git a/state-chain/pallets/cf-broadcast/src/lib.rs b/state-chain/pallets/cf-broadcast/src/lib.rs index 31dfe6f984b..e9da88579c1 100644 --- a/state-chain/pallets/cf-broadcast/src/lib.rs +++ b/state-chain/pallets/cf-broadcast/src/lib.rs @@ -15,7 +15,7 @@ use cf_primitives::{BroadcastId, ThresholdSignatureRequestId}; use cf_chains::{ address::IntoForeignChainAddress, ApiCall, Chain, ChainCrypto, FeeRefundCalculator, - RetryPolicy, TransactionBuilder, TransactionMetadata as _, + RequiresSignatureRefresh, RetryPolicy, TransactionBuilder, TransactionMetadata as _, }; use cf_traits::{ impl_pallet_safe_mode, offence_reporting::OffenceReporter, BroadcastNomination, Broadcaster, @@ -773,13 +773,18 @@ impl, I: 'static> Pallet { if let Some(broadcast_data) = AwaitingBroadcast::::get(broadcast_id) { // If the broadcast is not pending, we should not retry. - if let Some(api_call) = PendingApiCalls::::get(broadcast_id) { - if T::TransactionBuilder::requires_signature_refresh( - &api_call, - &broadcast_data.threshold_signature_payload, - CurrentOnChainKey::::get(), - ) { + if let Some(mut api_call) = PendingApiCalls::::get(broadcast_id) { + if let RequiresSignatureRefresh::True(maybe_modified_apicall) = + T::TransactionBuilder::requires_signature_refresh( + &api_call, + &broadcast_data.threshold_signature_payload, + CurrentOnChainKey::::get(), + ) { Self::deposit_event(Event::::ThresholdSignatureInvalid { broadcast_id }); + if let Some(modified_apicall) = maybe_modified_apicall { + PendingApiCalls::::insert(broadcast_id, modified_apicall.clone()); + api_call = modified_apicall; + } Self::threshold_sign(api_call, broadcast_id, true); log::info!( "Signature is invalid -> rescheduled threshold signature for broadcast id {}.", diff --git a/state-chain/runtime/src/chainflip.rs b/state-chain/runtime/src/chainflip.rs index 6e4524dd47e..95c559c5e06 100644 --- a/state-chain/runtime/src/chainflip.rs +++ b/state-chain/runtime/src/chainflip.rs @@ -54,8 +54,8 @@ use cf_chains::{ }, AnyChain, ApiCall, Arbitrum, CcmChannelMetadata, CcmDepositMetadata, Chain, ChainCrypto, ChainEnvironment, ChainState, ChannelRefundParameters, DepositChannel, ForeignChain, - ReplayProtectionProvider, SetCommKeyWithAggKey, SetGovKeyWithAggKey, Solana, - TransactionBuilder, + ReplayProtectionProvider, RequiresSignatureRefresh, SetCommKeyWithAggKey, SetGovKeyWithAggKey, + Solana, TransactionBuilder, }; use cf_primitives::{chains::assets, AccountRole, Asset, BasisPoints, Beneficiaries, ChannelId}; use cf_traits::{ @@ -234,8 +234,14 @@ macro_rules! impl_transaction_builder_for_evm_chain { call: &$chain_api<$env>, _payload: &::Payload, maybe_current_on_chain_key: Option<::AggKey> - ) -> bool { - maybe_current_on_chain_key.map_or(false, |current_on_chain_key| call.signer().is_some_and(|signer|current_on_chain_key != signer )) + ) -> RequiresSignatureRefresh> { + maybe_current_on_chain_key.map_or(RequiresSignatureRefresh::False, + |current_on_chain_key| if call.signer().is_some_and(|signer|current_on_chain_key != signer ) { + RequiresSignatureRefresh::True(None) + } else { + RequiresSignatureRefresh::False + } + ) } /// Calculate the gas limit for a this evm chain's call, using the current gas price. @@ -283,10 +289,14 @@ impl TransactionBuilder> for DotTransactio call: &PolkadotApi, payload: &<::ChainCrypto as ChainCrypto>::Payload, _maybe_current_onchain_key: Option<::AggKey>, - ) -> bool { + ) -> RequiresSignatureRefresh> { // Current key and signature are irrelevant. The only thing that can invalidate a polkadot // transaction is if the payload changes due to a runtime version update. - &call.threshold_signature_payload() != payload + if &call.threshold_signature_payload() != payload { + RequiresSignatureRefresh::True(None) + } else { + RequiresSignatureRefresh::False + } } } @@ -308,14 +318,14 @@ impl TransactionBuilder> for BtcTransactionB _call: &BitcoinApi, _payload: &<::ChainCrypto as ChainCrypto>::Payload, _maybe_current_onchain_key: Option<::AggKey>, - ) -> bool { + ) -> RequiresSignatureRefresh> { // The payload for a Bitcoin transaction will never change and so it doesnt need to be // checked here. We also dont need to check for the signature here because even if we are in // the next epoch and the key has changed, the old signature for the btc tx is still valid // since its based on those old input UTXOs. In fact, we never have to resign btc txs and // the btc tx is always valid as long as the input UTXOs are valid. Therefore, we don't have // to check anything here and just rebroadcast. - false + RequiresSignatureRefresh::False } } @@ -341,16 +351,38 @@ impl TransactionBuilder> for SolanaTransaction } fn requires_signature_refresh( - _call: &SolanaApi, + call: &SolanaApi, _payload: &<::ChainCrypto as ChainCrypto>::Payload, - _maybe_current_onchain_key: Option<::AggKey>, - ) -> bool { - // We use Durable Nonce mechanism to avoid the 150 blocks expiry period and so - // transactions won't expire. Then, the only reason to resign would be if the - // payload has been updated or the aggKey has been updated (key rotation). - // The payload won't be refreshed, as explained above, and the the broadcast - // barrier prevents a transaction from requiring to be resigned by a new aggKey. - false + maybe_current_onchain_key: Option<::AggKey>, + ) -> RequiresSignatureRefresh> { + // The only reason to resign would be if the aggKey has been updated on chain (key rotation) + // and this apicall is signed with the old key and is still pending. In this case, we need + // to modify the apicall, by replacing the aggkey with the new key, in the key_accounts in + // the tx's message to create a new valid threshold signing payload. + maybe_current_onchain_key.map_or(RequiresSignatureRefresh::False, |current_on_chain_key| { + match call.signer() { + Some(signer) if signer != current_on_chain_key => { + let mut modified_call = (*call).clone(); + SolanaThresholdSigner::active_epoch_key().map_or( + RequiresSignatureRefresh::False, + |active_epoch_key| { + let current_aggkey = active_epoch_key.key; + for key in modified_call.transaction.message.account_keys.iter_mut() { + if *key == signer.into() { + *key = current_aggkey.into() + } + } + for sig in modified_call.transaction.signatures.iter_mut() { + *sig = Default::default() + } + modified_call.signer = None; + RequiresSignatureRefresh::True(Some(modified_call.clone())) + }, + ) + }, + _ => RequiresSignatureRefresh::False, + } + }) } }