Ecosystem/serai
Ecosystem/serai
1
0
Fork 0
mirror of https://github.com/serai-dex/serai.git synced 2025-01-22 02:34:55 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Add OutputType::Forwarded to ensure a user's transfer in isn't misclassified

Browse source 
If a user transferred in without an InInstruction, and the amount exactly
matched a forwarded output, the user's output would fulfill the
forwarding. Then the forwarded output would come along, have no InInstruction,
and be refunded (to the prior multisig) when the user should've been refunded.

Adding this new address type resolves such concerns.
This commit is contained in:
Luke Parker 2023-11-09 14:24:13 -05:00
parent b51204a4eb
commit 42e8f2c8d8
No known key found for this signature in database
5 changed files with 83 additions and 71 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

101
processor/src/multisigs/mod.rs
View file

@ -381,19 +381,15 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
costs of embedding arbitrary data.
Since we can't rely on the Eventuality system to detect if it's a forwarded transaction,
due to the asynchonicity of the Eventuality system, we instead interpret an External
output with no InInstruction, which has an amount associated with an InInstruction
being forwarded, as having been forwarded. This does create a specific edge case where
a user who doesn't include an InInstruction may not be refunded however, if they share
an exact amount with an expected-to-be-forwarded transaction. This is deemed acceptable.
TODO: Add a fourth address, forwarded_address, to prevent this.
due to the asynchonicity of the Eventuality system, we instead interpret an Forwarded
output which has an amount associated with an InInstruction which was forwarded as having
been forwarded.
*/
Plan {
key: self.existing.as_ref().unwrap().key,
payments: vec![Payment {
address: N::address(self.new.as_ref().unwrap().key),
address: N::forward_address(self.new.as_ref().unwrap().key),
data: None,
balance: output.balance(),
}],
@ -582,6 +578,7 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
}
false
}
OutputType::Forwarded => false,
}
});
plans
@ -728,18 +725,15 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
running_operating_costs,
);
// This will false positive on a transaction being refunded which meets these conditions
// That doesn't impact any of the following code at this time yet shows consideration
// is needed here in the future
let to_be_forwarded = {
let output = &plan.inputs[0];
(step == RotationStep::ForwardFromExisting) &&
(output.kind() == OutputType::External) &&
(output.key() == self.existing.as_ref().unwrap().key)
};
if to_be_forwarded {
assert_eq!(plan.inputs.len(), 1);
}
// TODO: Don't do an error-prone post-detection for if this is being forwarded
// Be told this is being forwarded
let to_be_forwarded = (step == RotationStep::ForwardFromExisting) &&
plan
.payments
.first()
.map(|payment| payment.address == N::forward_address(self.new.as_ref().unwrap().key))
.unwrap_or(false) &&
plan.change.is_none();
// If we're forwarding this output, don't take the opportunity to amortze operating costs
// The scanner handler below, in order to properly save forwarded outputs' instructions,
@ -823,26 +817,36 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
.expect("didn't have the block number for a block we just scanned");
let step = self.current_rotation_step(block_number);
// If these aren't externally received funds, don't handle it as an instruction
// Instructions created from this block
let mut instructions = vec![];
// If any of these outputs were forwarded, create their instruction now
for output in &outputs {
if output.kind() != OutputType::Forwarded {
continue;
}
if let Some(instruction) =
MultisigsDb::<N, D>::take_forwarded_output(txn, output.balance())
{
instructions.push(instruction);
}
}
// If the remaining outputs aren't externally received funds, don't handle them as
// instructions
outputs.retain(|output| output.kind() == OutputType::External);
// These plans are of limited context. They're only allowed the outputs newly received
// within this block and are intended to handle forwarding transactions/refunds
// Those all end up as plans with a single input, leading to a later check:
/*
if to_be_forwarded {
assert_eq!(plan.inputs.len(), 1);
}
Hence the name, which places that assumed precondition into verbiage here.
*/
let mut single_input_plans = vec![];
let mut plans = vec![];
// If the old multisig is explicitly only supposed to forward, create all such plans now
if step == RotationStep::ForwardFromExisting {
let mut i = 0;
while i < outputs.len() {
let output = &outputs[i];
let single_input_plans = &mut single_input_plans;
let plans = &mut plans;
let txn = &mut *txn;
#[allow(clippy::redundant_closure_call)]
@ -852,12 +856,12 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
return true;
}
let plans_at_start = single_input_plans.len();
let plans_at_start = plans.len();
let (refund_to, instruction) = instruction_from_output::<N>(output);
if let Some(mut instruction) = instruction {
// Build a dedicated Plan forwarding this
let forward_plan = self.forward_plan(output.clone());
single_input_plans.push(forward_plan.clone());
plans.push(forward_plan.clone());
// Set the instruction for this output to be returned
// We need to set it under the amount it's forwarded with, so prepare its forwarding
@ -879,12 +883,12 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
} else if let Some(refund_to) = refund_to {
if let Ok(refund_to) = refund_to.consume().try_into() {
// Build a dedicated Plan refunding this
single_input_plans.push(Self::refund_plan(output.clone(), refund_to));
plans.push(Self::refund_plan(output.clone(), refund_to));
}
}
// Only keep if we didn't make a Plan consuming it
plans_at_start == single_input_plans.len()
plans_at_start == plans.len()
})()
.await;
if should_retain {
@ -895,7 +899,6 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
}
}
let mut instructions = vec![];
for output in outputs {
// If this is an External transaction to the existing multisig, and we're either solely
// forwarding or closing the existing multisig, drop it
@ -909,33 +912,15 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
}
let (refund_to, instruction) = instruction_from_output::<N>(&output);
let refund = || {
if let Some(refund_to) = refund_to {
if let Ok(refund_to) = refund_to.consume().try_into() {
single_input_plans.push(Self::refund_plan(output.clone(), refund_to))
}
}
};
let instruction = if let Some(instruction) = instruction {
instruction
} else {
// If the output data is empty, this may be a forwarded transaction from a prior
// multisig
// If it's not empty, it's corrupt in some way and should be refunded
if !output.data().is_empty() {
refund();
continue;
if let Some(refund_to) = refund_to {
if let Ok(refund_to) = refund_to.consume().try_into() {
plans.push(Self::refund_plan(output.clone(), refund_to));
}
if let Some(instruction) =
MultisigsDb::<N, D>::take_forwarded_output(txn, output.balance())
{
instruction
} else {
// If it's not a forwarded output, refund
refund();
continue;
}
continue;
};
// Delay External outputs received to new multisig earlier than expected
@ -956,7 +941,7 @@ impl<D: Db, N: Network> MultisigManager<D, N> {
// Save the plans created while scanning
// TODO: Should we combine all of these plans?
MultisigsDb::<N, D>::set_plans_from_scanning(txn, block_number, single_input_plans);
MultisigsDb::<N, D>::set_plans_from_scanning(txn, block_number, plans);
// If any outputs were delayed, append them into this block
match step {

7
processor/src/networks/bitcoin.rs
View file

@ -302,6 +302,7 @@ impl BlockTrait<Bitcoin> for Block {
const KEY_DST: &[u8] = b"Serai Bitcoin Output Offset";
static BRANCH_OFFSET: Lazy<Scalar> = Lazy::new(|| Secp256k1::hash_to_F(KEY_DST, b"branch"));
static CHANGE_OFFSET: Lazy<Scalar> = Lazy::new(|| Secp256k1::hash_to_F(KEY_DST, b"change"));
static FORWARD_OFFSET: Lazy<Scalar> = Lazy::new(|| Secp256k1::hash_to_F(KEY_DST, b"forward"));
// Always construct the full scanner in order to ensure there's no collisions
fn scanner(
@ -325,6 +326,7 @@ fn scanner(
register(OutputType::Branch, *BRANCH_OFFSET);
register(OutputType::Change, *CHANGE_OFFSET);
register(OutputType::Forwarded, *FORWARD_OFFSET);
(scanner, offsets, kinds)
}
@ -550,6 +552,11 @@ impl Network for Bitcoin {
Self::address(key + (ProjectivePoint::GENERATOR * offsets[&OutputType::Change]))
}
fn forward_address(key: ProjectivePoint) -> Address {
let (_, offsets, _) = scanner(key);
Self::address(key + (ProjectivePoint::GENERATOR * offsets[&OutputType::Forwarded]))
}
async fn get_latest_block_number(&self) -> Result<usize, NetworkError> {
self.rpc.get_latest_block_number().await.map_err(|_| NetworkError::ConnectionError)
}

7
processor/src/networks/mod.rs
View file

@ -71,6 +71,9 @@ pub enum OutputType {
// Should be added to the available UTXO pool with no further action
Change,
// Forwarded output from the prior multisig
Forwarded,
}
impl OutputType {
@ -79,6 +82,7 @@ impl OutputType {
OutputType::External => 0,
OutputType::Branch => 1,
OutputType::Change => 2,
OutputType::Forwarded => 3,
}])
}
@ -89,6 +93,7 @@ impl OutputType {
0 => OutputType::External,
1 => OutputType::Branch,
2 => OutputType::Change,
3 => OutputType::Forwarded,
_ => Err(io::Error::new(io::ErrorKind::Other, "invalid OutputType"))?,
})
}
@ -293,6 +298,8 @@ pub trait Network: 'static + Send + Sync + Clone + PartialEq + Eq + Debug {
fn branch_address(key: <Self::Curve as Ciphersuite>::G) -> Self::Address;
/// Address for the given group key to use for change.
fn change_address(key: <Self::Curve as Ciphersuite>::G) -> Self::Address;
/// Address for forwarded outputs from prior multisigs.
fn forward_address(key: <Self::Curve as Ciphersuite>::G) -> Self::Address;
/// Get the latest block's number.
async fn get_latest_block_number(&self) -> Result<usize, NetworkError>;

9
processor/src/networks/monero.rs
View file

@ -49,6 +49,7 @@ pub struct Output(SpendableOutput, Vec<u8>);
const EXTERNAL_SUBADDRESS: Option<SubaddressIndex> = SubaddressIndex::new(0, 0);
const BRANCH_SUBADDRESS: Option<SubaddressIndex> = SubaddressIndex::new(1, 0);
const CHANGE_SUBADDRESS: Option<SubaddressIndex> = SubaddressIndex::new(2, 0);
const FORWARD_SUBADDRESS: Option<SubaddressIndex> = SubaddressIndex::new(3, 0);
impl OutputTrait<Monero> for Output {
// While we could use (tx, o), using the key ensures we won't be susceptible to the burning bug.
@ -61,6 +62,7 @@ impl OutputTrait<Monero> for Output {
EXTERNAL_SUBADDRESS => OutputType::External,
BRANCH_SUBADDRESS => OutputType::Branch,
CHANGE_SUBADDRESS => OutputType::Change,
FORWARD_SUBADDRESS => OutputType::Forwarded,
_ => panic!("unrecognized address was scanned for"),
}
}
@ -262,6 +264,7 @@ impl Monero {
debug_assert!(EXTERNAL_SUBADDRESS.is_none());
scanner.register_subaddress(BRANCH_SUBADDRESS.unwrap());
scanner.register_subaddress(CHANGE_SUBADDRESS.unwrap());
scanner.register_subaddress(FORWARD_SUBADDRESS.unwrap());
scanner
}
@ -484,6 +487,10 @@ impl Network for Monero {
Self::address_internal(key, CHANGE_SUBADDRESS)
}
fn forward_address(key: EdwardsPoint) -> Address {
Self::address_internal(key, FORWARD_SUBADDRESS)
}
async fn get_latest_block_number(&self) -> Result<usize, NetworkError> {
// Monero defines height as chain length, so subtract 1 for block number
Ok(self.rpc.get_height().await.map_err(map_rpc_err)? - 1)
@ -520,7 +527,7 @@ impl Network for Monero {
// This just ensures nothing invalid makes it through
for tx_outputs in &txs {
for output in tx_outputs {
assert!([EXTERNAL_SUBADDRESS, BRANCH_SUBADDRESS, CHANGE_SUBADDRESS]
assert!([EXTERNAL_SUBADDRESS, BRANCH_SUBADDRESS, CHANGE_SUBADDRESS, FORWARD_SUBADDRESS]
.contains(&output.output.metadata.subaddress));
}
}

26
processor/src/tests/addresses.rs
View file

@ -22,7 +22,7 @@ async fn spend<N: Network, D: Db>(
keys: &HashMap<Participant, ThresholdKeys<N::Curve>>,
scanner: &mut ScannerHandle<N, D>,
outputs: Vec<N::Output>,
) -> Vec<N::Output> {
) {
let key = keys[&Participant::new(1).unwrap()].group_key();
let mut keys_txs = HashMap::new();
@ -62,11 +62,11 @@ async fn spend<N: Network, D: Db>(
assert_eq!(outputs.len(), 1);
// Make sure this is actually a change output
assert_eq!(outputs[0].kind(), OutputType::Change);
assert_eq!(outputs[0].key(), key);
let mut txn = db.txn();
assert_eq!(scanner.ack_block(&mut txn, block).await.1, outputs);
scanner.release_lock().await;
txn.commit();
outputs
}
ScannerEvent::Completed(_, _, _, _) => {
panic!("unexpectedly got eventuality completion");
@ -96,31 +96,37 @@ pub async fn test_addresses<N: Network>(network: N) {
network.mine_block().await;
}
// Receive funds to the branch address and make sure it's properly identified
let block_id = network.test_send(N::branch_address(key)).await.id();
// Receive funds to the various addresses and make sure they're properly identified
let mut received_outputs = vec![];
for (kind, address) in [
(OutputType::External, N::address(key)),
(OutputType::Branch, N::branch_address(key)),
(OutputType::Change, N::change_address(key)),
(OutputType::Forwarded, N::forward_address(key)),
] {
let block_id = network.test_send(address).await.id();
// Verify the Scanner picked them up
let outputs =
match timeout(Duration::from_secs(30), scanner.events.recv()).await.unwrap().unwrap() {
ScannerEvent::Block { is_retirement_block, block, outputs } => {
scanner.multisig_completed.send(false).unwrap();
assert!(!is_retirement_block);
assert_eq!(block, block_id);
assert_eq!(outputs.len(), 1);
assert_eq!(outputs[0].kind(), OutputType::Branch);
assert_eq!(outputs[0].kind(), kind);
assert_eq!(outputs[0].key(), key);
let mut txn = db.txn();
assert_eq!(scanner.ack_block(&mut txn, block).await.1, outputs);
scanner.release_lock().await;
txn.commit();
outputs
received_outputs.extend(outputs);
}
ScannerEvent::Completed(_, _, _, _) => {
panic!("unexpectedly got eventuality completion");
}
};
}
// Spend the branch output, creating a change output and ensuring we actually get change
let outputs = spend(&mut db, &network, &keys, &mut scanner, outputs).await;
// Also test spending the change output
spend(&mut db, &network, &keys, &mut scanner, outputs).await;
spend(&mut db, &network, &keys, &mut scanner, received_outputs).await;
}