-
Notifications
You must be signed in to change notification settings - Fork 63
fix(svm): M-01 Deposit Tokens Transferred from Depositor Token Account Instead of Signer #958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
fix(svm): M-01 Deposit Tokens Transferred from Depositor Token Account Instead of Signer #958
Conversation
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
…rred-from-depositor-token-account
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
let state_seed_bytes = state.seed.to_le_bytes(); | ||
let signer_seeds: &[&[u8]] = &[b"delegate", &state_seed_bytes, &delegate_hash, &[bump]]; | ||
|
||
// Relayer must have delegated output_amount to the delegate PDA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Relayer must have delegated output_amount to the delegate PDA | |
// Depositor must have delegated input_amount to the delegate PDA |
let bump = ctx.bumps.delegate; | ||
let delegate_hash = | ||
derive_delegate_seed_hash(input_token, output_token, input_amount, output_amount, destination_chain_id); | ||
let state_seed_bytes = state.seed.to_le_bytes(); | ||
let signer_seeds: &[&[u8]] = &[b"delegate", &state_seed_bytes, &delegate_hash, &[bump]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be cleaner to move seed derivation back in the transfer_from
helper, just pass additional delegate hash and bump to it
#[account( | ||
seeds = [ | ||
b"delegate", | ||
state.seed.to_le_bytes().as_ref(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do tests depend on this? if not, we better drop the state seed here as its 0 on public networks
seeds = [ | ||
b"delegate", | ||
state.seed.to_le_bytes().as_ref(), | ||
&derive_delegate_seed_hash(input_token, output_token, input_amount, output_amount, destination_chain_id), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still would allow someone to steal delegation from separate tx by replacing recipient and message. I think it would be safer and more consistent if we hashed all deposit parameters
@@ -32,6 +32,10 @@ pub struct FillRelay<'info> { | |||
)] | |||
pub state: Account<'info, State>, | |||
|
|||
#[account(seeds = [b"delegate", state.seed.to_le_bytes().as_ref(), relay_hash.as_ref()], bump)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this still would allow someone to steal delegation from separate tx by replacing repayment address and chain. I think it would be safer and more consistent if we hashed all fill_relay parameters. maybe except for relay_data as that is already represented by relay_hash
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do tests depend on state.seed
? if not, we better drop the state seed here as its 0 on public networks
let bump = ctx.bumps.delegate; | ||
let state_seed_bytes = state.seed.to_le_bytes(); | ||
let signer_seeds: &[&[u8]] = &[b"delegate", &state_seed_bytes, &relay_hash, &[bump]]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider moving seed derivation in transfer_from
helper
let mut data = Vec::with_capacity(32 + 32 + 8 + 8 + 8); | ||
data.extend_from_slice(input_token.as_ref()); | ||
data.extend_from_slice(output_token.as_ref()); | ||
data.extend_from_slice(&input_amount.to_le_bytes()); | ||
data.extend_from_slice(&output_amount.to_le_bytes()); | ||
data.extend_from_slice(&destination_chain_id.to_le_bytes()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use AnchorSerialize::serialize
or try_to_vec
, so that we don't need to be explicit on individual field encoding
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
Signed-off-by: Pablo Maldonado <[email protected]>
OZ identified the followin issue:
We replaced the one “state” PDA with two distinct PDAs—one for deposit and one for fill_relay. Now users must explicitly delegate to the correct PDA before anyone can pull their tokens, restoring safe third‑party deposits and eliminating the risk of a single authority being misused to steal funds.