From eb9bce6862836a0b89f24035e0258c73e002e811 Mon Sep 17 00:00:00 2001 From: Luke Parker Date: Sun, 15 Sep 2024 12:48:09 -0400 Subject: [PATCH] Remove OutInstruction's data field It makes sense for networks which support arbitrary data to do as part of their address. This reduces the ability to perform DoSs, achieves better performance, and better uses the type system (as now networks we don't support data on don't have a data field). Updates the Ethereum address definition in serai-client accordingly --- .../ethereum/contracts/contracts/Router.sol | 2 +- processor/primitives/src/payment.rs | 16 +--- processor/scanner/src/scan/mod.rs | 11 --- processor/scanner/src/substrate/mod.rs | 2 +- processor/scheduler/smart-contract/src/lib.rs | 2 +- .../scheduler/utxo/primitives/src/tree.rs | 8 +- processor/scheduler/utxo/standard/src/lib.rs | 4 +- .../utxo/transaction-chaining/src/lib.rs | 4 +- processor/src/tests/signer.rs | 1 - processor/src/tests/wallet.rs | 2 - substrate/client/src/networks/ethereum.rs | 96 +++++++++++++++---- substrate/client/tests/burn.rs | 58 ++++++----- substrate/coins/primitives/src/lib.rs | 4 +- substrate/in-instructions/pallet/src/lib.rs | 6 +- substrate/primitives/src/lib.rs | 50 +--------- tests/coordinator/src/tests/sign.rs | 1 - tests/full-stack/src/tests/mint_and_burn.rs | 2 +- tests/processor/src/tests/send.rs | 2 +- 18 files changed, 121 insertions(+), 150 deletions(-) diff --git a/processor/ethereum/contracts/contracts/Router.sol b/processor/ethereum/contracts/contracts/Router.sol index 65541a10..1d084698 100644 --- a/processor/ethereum/contracts/contracts/Router.sol +++ b/processor/ethereum/contracts/contracts/Router.sol @@ -192,7 +192,7 @@ contract Router { _transferOut(nextAddress, transactions[i].coin, transactions[i].value); // Perform the calls with a set gas budget - (uint24 gas, bytes memory code) = abi.decode(transactions[i].destination, (uint24, bytes)); + (uint32 gas, bytes memory code) = abi.decode(transactions[i].destination, (uint32, bytes)); address(this).call{ gas: gas }(abi.encodeWithSelector(Router.arbitaryCallOut.selector, code)); diff --git a/processor/primitives/src/payment.rs b/processor/primitives/src/payment.rs index 4c1e04f4..59b10f7f 100644 --- a/processor/primitives/src/payment.rs +++ b/processor/primitives/src/payment.rs @@ -3,7 +3,7 @@ use std::io; use scale::{Encode, Decode, IoReader}; use borsh::{BorshSerialize, BorshDeserialize}; -use serai_primitives::{Balance, Data}; +use serai_primitives::Balance; use serai_coins_primitives::OutInstructionWithBalance; use crate::Address; @@ -13,7 +13,6 @@ use crate::Address; pub struct Payment { address: A, balance: Balance, - data: Option>, } impl TryFrom for Payment { @@ -22,15 +21,14 @@ impl TryFrom for Payment { Ok(Payment { address: out_instruction_with_balance.instruction.address.try_into().map_err(|_| ())?, balance: out_instruction_with_balance.balance, - data: out_instruction_with_balance.instruction.data.map(Data::consume), }) } } impl Payment { /// Create a new Payment. - pub fn new(address: A, balance: Balance, data: Option>) -> Self { - Payment { address, balance, data } + pub fn new(address: A, balance: Balance) -> Self { + Payment { address, balance } } /// The address to pay. @@ -41,24 +39,18 @@ impl Payment { pub fn balance(&self) -> Balance { self.balance } - /// The data to associate with this payment. - pub fn data(&self) -> &Option> { - &self.data - } /// Read a Payment. pub fn read(reader: &mut impl io::Read) -> io::Result { let address = A::deserialize_reader(reader)?; let reader = &mut IoReader(reader); let balance = Balance::decode(reader).map_err(io::Error::other)?; - let data = Option::>::decode(reader).map_err(io::Error::other)?; - Ok(Self { address, balance, data }) + Ok(Self { address, balance }) } /// Write the Payment. pub fn write(&self, writer: &mut impl io::Write) -> io::Result<()> { self.address.serialize(writer)?; self.balance.encode_to(writer); - self.data.encode_to(writer); Ok(()) } } diff --git a/processor/scanner/src/scan/mod.rs b/processor/scanner/src/scan/mod.rs index c54dc3e0..b235ff15 100644 --- a/processor/scanner/src/scan/mod.rs +++ b/processor/scanner/src/scan/mod.rs @@ -4,7 +4,6 @@ use std::collections::HashMap; use scale::Decode; use serai_db::{Get, DbTxn, Db}; -use serai_primitives::MAX_DATA_LEN; use serai_in_instructions_primitives::{ Shorthand, RefundableInInstruction, InInstruction, InInstructionWithBalance, }; @@ -56,16 +55,6 @@ fn in_instruction_from_output( let presumed_origin = output.presumed_origin(); let mut data = output.data(); - let max_data_len = usize::try_from(MAX_DATA_LEN).unwrap(); - if data.len() > max_data_len { - log::info!( - "data in output {} exceeded MAX_DATA_LEN ({MAX_DATA_LEN}): {}. skipping", - hex::encode(output.id()), - data.len(), - ); - return (presumed_origin, None); - } - let shorthand = match Shorthand::decode(&mut data) { Ok(shorthand) => shorthand, Err(e) => { diff --git a/processor/scanner/src/substrate/mod.rs b/processor/scanner/src/substrate/mod.rs index a7302e5c..89186c69 100644 --- a/processor/scanner/src/substrate/mod.rs +++ b/processor/scanner/src/substrate/mod.rs @@ -142,7 +142,7 @@ impl ContinuallyRan for SubstrateTask { if let Some(report::ReturnInformation { address, balance }) = return_information { burns.push(OutInstructionWithBalance { - instruction: OutInstruction { address: address.into(), data: None }, + instruction: OutInstruction { address: address.into() }, balance, }); } diff --git a/processor/scheduler/smart-contract/src/lib.rs b/processor/scheduler/smart-contract/src/lib.rs index 7630a026..0c9c690b 100644 --- a/processor/scheduler/smart-contract/src/lib.rs +++ b/processor/scheduler/smart-contract/src/lib.rs @@ -130,7 +130,7 @@ impl> SchedulerTrait for S .returns() .iter() .map(|to_return| { - Payment::new(to_return.address().clone(), to_return.output().balance(), None) + Payment::new(to_return.address().clone(), to_return.output().balance()) }) .collect::>(), ), diff --git a/processor/scheduler/utxo/primitives/src/tree.rs b/processor/scheduler/utxo/primitives/src/tree.rs index b52f3ba3..d5b47309 100644 --- a/processor/scheduler/utxo/primitives/src/tree.rs +++ b/processor/scheduler/utxo/primitives/src/tree.rs @@ -115,11 +115,7 @@ impl TreeTransaction { .filter_map(|(payment, amount)| { amount.map(|amount| { // The existing payment, with the new amount - Payment::new( - payment.address().clone(), - Balance { coin, amount: Amount(amount) }, - payment.data().clone(), - ) + Payment::new(payment.address().clone(), Balance { coin, amount: Amount(amount) }) }) }) .collect() @@ -130,7 +126,7 @@ impl TreeTransaction { .filter_map(|amount| { amount.map(|amount| { // A branch output with the new amount - Payment::new(branch_address.clone(), Balance { coin, amount: Amount(amount) }, None) + Payment::new(branch_address.clone(), Balance { coin, amount: Amount(amount) }) }) }) .collect() diff --git a/processor/scheduler/utxo/standard/src/lib.rs b/processor/scheduler/utxo/standard/src/lib.rs index dc2ccb06..e826c300 100644 --- a/processor/scheduler/utxo/standard/src/lib.rs +++ b/processor/scheduler/utxo/standard/src/lib.rs @@ -489,7 +489,7 @@ impl> SchedulerTrait for Schedul &mut 0, block, vec![forward.clone()], - vec![Payment::new(P::forwarding_address(forward_to_key), forward.balance(), None)], + vec![Payment::new(P::forwarding_address(forward_to_key), forward.balance())], None, ) .await? @@ -501,7 +501,7 @@ impl> SchedulerTrait for Schedul for to_return in update.returns() { let key = to_return.output().key(); let out_instruction = - Payment::new(to_return.address().clone(), to_return.output().balance(), None); + Payment::new(to_return.address().clone(), to_return.output().balance()); let Some(plan) = self .planner .plan_transaction_with_fee_amortization( diff --git a/processor/scheduler/utxo/transaction-chaining/src/lib.rs b/processor/scheduler/utxo/transaction-chaining/src/lib.rs index 93bdf1f3..bb39dcd3 100644 --- a/processor/scheduler/utxo/transaction-chaining/src/lib.rs +++ b/processor/scheduler/utxo/transaction-chaining/src/lib.rs @@ -507,7 +507,7 @@ impl>> Sched &mut 0, block, vec![forward.clone()], - vec![Payment::new(P::forwarding_address(forward_to_key), forward.balance(), None)], + vec![Payment::new(P::forwarding_address(forward_to_key), forward.balance())], None, ) .await? @@ -519,7 +519,7 @@ impl>> Sched for to_return in update.returns() { let key = to_return.output().key(); let out_instruction = - Payment::new(to_return.address().clone(), to_return.output().balance(), None); + Payment::new(to_return.address().clone(), to_return.output().balance()); let Some(plan) = self .planner .plan_transaction_with_fee_amortization( diff --git a/processor/src/tests/signer.rs b/processor/src/tests/signer.rs index 77307ef2..6b445608 100644 --- a/processor/src/tests/signer.rs +++ b/processor/src/tests/signer.rs @@ -184,7 +184,6 @@ pub async fn test_signer( let mut scheduler = N::Scheduler::new::(&mut txn, key, N::NETWORK); let payments = vec![Payment { address: N::external_address(&network, key).await, - data: None, balance: Balance { coin: match N::NETWORK { NetworkId::Serai => panic!("test_signer called with Serai"), diff --git a/processor/src/tests/wallet.rs b/processor/src/tests/wallet.rs index 86a27349..0451f30c 100644 --- a/processor/src/tests/wallet.rs +++ b/processor/src/tests/wallet.rs @@ -88,7 +88,6 @@ pub async fn test_wallet( outputs.clone(), vec![Payment { address: N::external_address(&network, key).await, - data: None, balance: Balance { coin: match N::NETWORK { NetworkId::Serai => panic!("test_wallet called with Serai"), @@ -116,7 +115,6 @@ pub async fn test_wallet( plans[0].payments, vec![Payment { address: N::external_address(&network, key).await, - data: None, balance: Balance { coin: match N::NETWORK { NetworkId::Serai => panic!("test_wallet called with Serai"), diff --git a/substrate/client/src/networks/ethereum.rs b/substrate/client/src/networks/ethereum.rs index 09285169..28ada635 100644 --- a/substrate/client/src/networks/ethereum.rs +++ b/substrate/client/src/networks/ethereum.rs @@ -1,35 +1,93 @@ -use core::{str::FromStr, fmt}; +use core::str::FromStr; +use std::io::Read; use borsh::{BorshSerialize, BorshDeserialize}; -use crate::primitives::ExternalAddress; +use crate::primitives::{MAX_ADDRESS_LEN, ExternalAddress}; -/// A representation of an Ethereum address. -#[derive(Clone, Copy, PartialEq, Eq, Debug, BorshSerialize, BorshDeserialize)] -pub struct Address([u8; 20]); +#[derive(Clone, PartialEq, Eq, Debug, BorshSerialize, BorshDeserialize)] +pub struct ContractDeployment { + /// The gas limit to use for this contract's execution. + /// + /// THis MUST be less than the Serai gas limit. The cost of it will be deducted from the amount + /// transferred. + gas: u32, + /// The initialization code of the contract to deploy. + /// + /// This contract will be deployed (executing the initialization code). No further calls will + /// be made. + code: Vec, +} -impl From<[u8; 20]> for Address { - fn from(address: [u8; 20]) -> Self { - Self(address) +/// A contract to deploy, enabling executing arbitrary code. +impl ContractDeployment { + pub fn new(gas: u32, code: Vec) -> Option { + // The max address length, minus the type byte, minus the size of the gas + const MAX_CODE_LEN: usize = (MAX_ADDRESS_LEN as usize) - (1 + core::mem::size_of::()); + if code.len() > MAX_CODE_LEN { + None?; + } + Some(Self { gas, code }) } } -impl From
for [u8; 20] { - fn from(address: Address) -> Self { - address.0 +/// A representation of an Ethereum address. +#[derive(Clone, PartialEq, Eq, Debug, BorshSerialize, BorshDeserialize)] +pub enum Address { + /// A traditional address. + Address([u8; 20]), + /// A contract to deploy, enabling executing arbitrary code. + Contract(ContractDeployment), +} + +impl From<[u8; 20]> for Address { + fn from(address: [u8; 20]) -> Self { + Address::Address(address) } } impl TryFrom for Address { type Error = (); fn try_from(data: ExternalAddress) -> Result { - Ok(Self(data.as_ref().try_into().map_err(|_| ())?)) + let mut kind = [0xff]; + let mut reader: &[u8] = data.as_ref(); + reader.read_exact(&mut kind).map_err(|_| ())?; + Ok(match kind[0] { + 0 => { + let mut address = [0xff; 20]; + reader.read_exact(&mut address).map_err(|_| ())?; + Address::Address(address) + } + 1 => { + let mut gas = [0xff; 4]; + reader.read_exact(&mut gas).map_err(|_| ())?; + // The code is whatever's left since the ExternalAddress is a delimited container of + // appropriately bounded length + Address::Contract(ContractDeployment { + gas: u32::from_le_bytes(gas), + code: reader.to_vec(), + }) + } + _ => Err(())?, + }) } } impl From
for ExternalAddress { fn from(address: Address) -> ExternalAddress { - // This is 20 bytes which is less than MAX_ADDRESS_LEN - ExternalAddress::new(address.0.to_vec()).unwrap() + let mut res = Vec::with_capacity(1 + 20); + match address { + Address::Address(address) => { + res.push(0); + res.extend(&address); + } + Address::Contract(ContractDeployment { gas, code }) => { + res.push(1); + res.extend(&gas.to_le_bytes()); + res.extend(&code); + } + } + // We only construct addresses whose code is small enough this can safely be constructed + ExternalAddress::new(res).unwrap() } } @@ -40,12 +98,8 @@ impl FromStr for Address { if address.len() != 40 { Err(())? }; - Ok(Self(hex::decode(address.to_lowercase()).map_err(|_| ())?.try_into().unwrap())) - } -} - -impl fmt::Display for Address { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "0x{}", hex::encode(self.0)) + Ok(Address::Address( + hex::decode(address.to_lowercase()).map_err(|_| ())?.try_into().map_err(|_| ())?, + )) } } diff --git a/substrate/client/tests/burn.rs b/substrate/client/tests/burn.rs index a30dabec..b8b849d3 100644 --- a/substrate/client/tests/burn.rs +++ b/substrate/client/tests/burn.rs @@ -12,7 +12,7 @@ use sp_core::Pair; use serai_client::{ primitives::{ - Amount, NetworkId, Coin, Balance, BlockHash, SeraiAddress, Data, ExternalAddress, + Amount, NetworkId, Coin, Balance, BlockHash, SeraiAddress, ExternalAddress, insecure_pair_from_name, }, in_instructions::{ @@ -55,39 +55,35 @@ serai_test!( let block = provide_batch(&serai, batch.clone()).await; let instruction = { - let serai = serai.as_of(block); - let batches = serai.in_instructions().batch_events().await.unwrap(); - assert_eq!( - batches, - vec![InInstructionsEvent::Batch { - network, - id, - block: block_hash, - instructions_hash: Blake2b::::digest(batch.instructions.encode()).into(), - }] - ); + let serai = serai.as_of(block); + let batches = serai.in_instructions().batch_events().await.unwrap(); + assert_eq!( + batches, + vec![InInstructionsEvent::Batch { + network, + id, + block: block_hash, + instructions_hash: Blake2b::::digest(batch.instructions.encode()).into(), + }] + ); - assert_eq!( - serai.coins().mint_events().await.unwrap(), - vec![CoinsEvent::Mint { to: address, balance }] - ); - assert_eq!(serai.coins().coin_supply(coin).await.unwrap(), amount); - assert_eq!(serai.coins().coin_balance(coin, address).await.unwrap(), amount); + assert_eq!( + serai.coins().mint_events().await.unwrap(), + vec![CoinsEvent::Mint { to: address, balance }] + ); + assert_eq!(serai.coins().coin_supply(coin).await.unwrap(), amount); + assert_eq!(serai.coins().coin_balance(coin, address).await.unwrap(), amount); - // Now burn it - let mut rand_bytes = vec![0; 32]; - OsRng.fill_bytes(&mut rand_bytes); - let external_address = ExternalAddress::new(rand_bytes).unwrap(); + // Now burn it + let mut rand_bytes = vec![0; 32]; + OsRng.fill_bytes(&mut rand_bytes); + let external_address = ExternalAddress::new(rand_bytes).unwrap(); - let mut rand_bytes = vec![0; 32]; - OsRng.fill_bytes(&mut rand_bytes); - let data = Data::new(rand_bytes).unwrap(); - - OutInstructionWithBalance { - balance, - instruction: OutInstruction { address: external_address, data: Some(data) }, - } -}; + OutInstructionWithBalance { + balance, + instruction: OutInstruction { address: external_address }, + } + }; let block = publish_tx( &serai, diff --git a/substrate/coins/primitives/src/lib.rs b/substrate/coins/primitives/src/lib.rs index a7b45cf0..53db7382 100644 --- a/substrate/coins/primitives/src/lib.rs +++ b/substrate/coins/primitives/src/lib.rs @@ -13,17 +13,17 @@ use serde::{Serialize, Deserialize}; use scale::{Encode, Decode, MaxEncodedLen}; use scale_info::TypeInfo; -use serai_primitives::{Balance, SeraiAddress, ExternalAddress, Data, system_address}; +use serai_primitives::{Balance, SeraiAddress, ExternalAddress, system_address}; pub const FEE_ACCOUNT: SeraiAddress = system_address(b"Coins-fees"); +// TODO: Replace entirely with just Address #[derive(Clone, PartialEq, Eq, Debug, Encode, Decode, MaxEncodedLen, TypeInfo)] #[cfg_attr(feature = "std", derive(Zeroize))] #[cfg_attr(feature = "borsh", derive(BorshSerialize, BorshDeserialize))] #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub struct OutInstruction { pub address: ExternalAddress, - pub data: Option, } #[derive(Clone, PartialEq, Eq, Debug, Encode, Decode, MaxEncodedLen, TypeInfo)] diff --git a/substrate/in-instructions/pallet/src/lib.rs b/substrate/in-instructions/pallet/src/lib.rs index f90ae412..1cb05c40 100644 --- a/substrate/in-instructions/pallet/src/lib.rs +++ b/substrate/in-instructions/pallet/src/lib.rs @@ -205,11 +205,7 @@ pub mod pallet { let coin_balance = Coins::::balance(IN_INSTRUCTION_EXECUTOR.into(), out_balance.coin); let instruction = OutInstructionWithBalance { - instruction: OutInstruction { - address: out_address.as_external().unwrap(), - // TODO: Properly pass data. Replace address with an OutInstruction entirely? - data: None, - }, + instruction: OutInstruction { address: out_address.as_external().unwrap() }, balance: Balance { coin: out_balance.coin, amount: coin_balance }, }; Coins::::burn_with_instruction(origin.into(), instruction)?; diff --git a/substrate/primitives/src/lib.rs b/substrate/primitives/src/lib.rs index 2cf37e00..b2515a7e 100644 --- a/substrate/primitives/src/lib.rs +++ b/substrate/primitives/src/lib.rs @@ -59,10 +59,7 @@ pub fn borsh_deserialize_bounded_vec for ExternalAddress { } } -// Should be enough for a Uniswap v3 call -pub const MAX_DATA_LEN: u32 = 512; -#[derive(Clone, PartialEq, Eq, Debug, Encode, Decode, MaxEncodedLen, TypeInfo)] -#[cfg_attr(feature = "borsh", derive(BorshSerialize, BorshDeserialize))] -#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] -pub struct Data( - #[cfg_attr( - feature = "borsh", - borsh( - serialize_with = "borsh_serialize_bounded_vec", - deserialize_with = "borsh_deserialize_bounded_vec" - ) - )] - BoundedVec>, -); - -#[cfg(feature = "std")] -impl Zeroize for Data { - fn zeroize(&mut self) { - self.0.as_mut().zeroize() - } -} - -impl Data { - #[cfg(feature = "std")] - pub fn new(data: Vec) -> Result { - Ok(Data(data.try_into().map_err(|_| "data length exceeds {MAX_DATA_LEN}")?)) - } - - pub fn data(&self) -> &[u8] { - self.0.as_ref() - } - - #[cfg(feature = "std")] - pub fn consume(self) -> Vec { - self.0.into_inner() - } -} - -impl AsRef<[u8]> for Data { - fn as_ref(&self) -> &[u8] { - self.0.as_ref() - } -} - /// Lexicographically reverses a given byte array. pub fn reverse_lexicographic_order(bytes: [u8; N]) -> [u8; N] { let mut res = [0u8; N]; diff --git a/tests/coordinator/src/tests/sign.rs b/tests/coordinator/src/tests/sign.rs index db8a7203..6e9142fe 100644 --- a/tests/coordinator/src/tests/sign.rs +++ b/tests/coordinator/src/tests/sign.rs @@ -247,7 +247,6 @@ async fn sign_test() { balance, instruction: OutInstruction { address: ExternalAddress::new(b"external".to_vec()).unwrap(), - data: None, }, }; serai diff --git a/tests/full-stack/src/tests/mint_and_burn.rs b/tests/full-stack/src/tests/mint_and_burn.rs index ce19808f..8987facc 100644 --- a/tests/full-stack/src/tests/mint_and_burn.rs +++ b/tests/full-stack/src/tests/mint_and_burn.rs @@ -493,7 +493,7 @@ async fn mint_and_burn_test() { move |nonce, coin, amount, address| async move { let out_instruction = OutInstructionWithBalance { balance: Balance { coin, amount: Amount(amount) }, - instruction: OutInstruction { address, data: None }, + instruction: OutInstruction { address }, }; serai diff --git a/tests/processor/src/tests/send.rs b/tests/processor/src/tests/send.rs index 8dfb5353..4c811e2b 100644 --- a/tests/processor/src/tests/send.rs +++ b/tests/processor/src/tests/send.rs @@ -246,7 +246,7 @@ fn send_test() { }, block: substrate_block_num, burns: vec![OutInstructionWithBalance { - instruction: OutInstruction { address: wallet.address(), data: None }, + instruction: OutInstruction { address: wallet.address() }, balance: Balance { coin: balance_sent.coin, amount: amount_minted }, }], batches: vec![batch.batch.id],