From f45b9c99c1085afcde0e44e53ba07e6dc0d0f0be Mon Sep 17 00:00:00 2001 From: Pi Lanningham Date: Sun, 11 Feb 2024 15:31:52 -0500 Subject: [PATCH 1/6] Resolves SSW-001 Resolves the critical finding that we don't check for the pool output address! This likely crept in because we were focused on checking the staking credential, but this would have been a fairly critical bug! --- validators/pool.ak | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/validators/pool.ak b/validators/pool.ak index 0a1eaff..06c8466 100644 --- a/validators/pool.ak +++ b/validators/pool.ak @@ -86,6 +86,13 @@ validator(settings_policy_id: PolicyId) { expect InlineDatum(output_datum) = pool_output.datum expect output_datum: PoolDatum = output_datum + // Ensure that the pool output is to the same payment credential; This is critical, because it ensures that the pool NFT + // or liquidity aren't paid to some other script in control of an attacker. + // Note that we check the stake credential is correctly updated (or not) in the various redeemer cases below. + // We also check that the pool output has the correct output, which ensures it contains the pool NFT, + // meaning this can't just be a "token output" with the correct payment credential, but everything paid elsewhere. + expect pool_output.address.payment_credential == ScriptCredential(pool_script_hash) + // Similarly, destructure the pool datum we found on the output, to access the fields we need to process the scoop let PoolDatum { identifier: actual_identifier, From ac095631d37a06ea5fcde397870a31a7ed01ac88 Mon Sep 17 00:00:00 2001 From: Franco Luque Date: Tue, 27 Feb 2024 20:51:04 -0300 Subject: [PATCH 2/6] remove redundant check check for payment part of output address is already done --- validators/pool.ak | 1 - 1 file changed, 1 deletion(-) diff --git a/validators/pool.ak b/validators/pool.ak index 06c8466..9783146 100644 --- a/validators/pool.ak +++ b/validators/pool.ak @@ -296,7 +296,6 @@ validator(settings_policy_id: PolicyId) { // that worked with the void datum, and paid to the real treasury with the correct datum. // TODO: should we just let the treasury admin specify the datum on the redeemer? Or include it in the settings? expect treasury_output.datum == InlineDatum(Void) - expect pool_output.address.payment_credential == pool_input.address.payment_credential expect list.any(settings_datum.authorized_staking_keys, fn(a) { pool_output.address.stake_credential == Some(Inline(a)) }) From 00b0ab66894667a9e688b81b232569fb9f98f675 Mon Sep 17 00:00:00 2001 From: card Date: Thu, 29 Feb 2024 02:16:17 -0500 Subject: [PATCH 3/6] Test that we cannot send pool output to non-pool address --- validators/tests/pool.ak | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/validators/tests/pool.ak b/validators/tests/pool.ak index 59705af..e599be6 100644 --- a/validators/tests/pool.ak +++ b/validators/tests/pool.ak @@ -9,7 +9,7 @@ use aiken/transaction.{ ScriptContext, Spend, Transaction, TransactionId, } use aiken/transaction/credential.{ - Address, VerificationKeyCredential, + Address, VerificationKeyCredential, from_verification_key, } use aiken/transaction/value.{Value} use shared.{ @@ -635,3 +635,14 @@ test mint_test() { mint_test_modify(identity, identity, identity, identity) } +// make sure pool_output.address is checked to be the pool address +test mint_test_wrong_address () { + let minted = mint_test_modify( + // change pool nft output address to destination that shouldn't be possible + fn (output) { Output{..output, address: from_verification_key(#"6af53ff4f054348ad825c692dd9db8f1760a8e0eacf9af9f99306513") }}, + identity, + identity, + identity + ) + minted +} \ No newline at end of file From 14f4930c22c5bad91e0f55ded46f3c7e0a6f2ad5 Mon Sep 17 00:00:00 2001 From: Pi Lanningham Date: Thu, 29 Feb 2024 03:30:58 -0500 Subject: [PATCH 4/6] Make wrong address test a negative test We want to make sure that these transactions *fail* if we spend to the wrong address on the output --- validators/tests/pool.ak | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/validators/tests/pool.ak b/validators/tests/pool.ak index e599be6..8e88f25 100644 --- a/validators/tests/pool.ak +++ b/validators/tests/pool.ak @@ -1,4 +1,5 @@ use aiken/builtin +use aiken/cbor use aiken/bytearray use aiken/dict use aiken/hash @@ -636,7 +637,7 @@ test mint_test() { } // make sure pool_output.address is checked to be the pool address -test mint_test_wrong_address () { +!test mint_test_wrong_address () { let minted = mint_test_modify( // change pool nft output address to destination that shouldn't be possible fn (output) { Output{..output, address: from_verification_key(#"6af53ff4f054348ad825c692dd9db8f1760a8e0eacf9af9f99306513") }}, From 055b77f04d8d9e1727b9f83643046fcf4d9039d0 Mon Sep 17 00:00:00 2001 From: Pi Lanningham Date: Thu, 29 Feb 2024 03:49:11 -0500 Subject: [PATCH 5/6] Cleanup tests Add tests for the main issue that was fixed, and some other things related to tweaking the pool address --- validators/tests/pool.ak | 89 +++++++++++++++++++++++++--------------- 1 file changed, 57 insertions(+), 32 deletions(-) diff --git a/validators/tests/pool.ak b/validators/tests/pool.ak index 8e88f25..149b487 100644 --- a/validators/tests/pool.ak +++ b/validators/tests/pool.ak @@ -1,5 +1,4 @@ use aiken/builtin -use aiken/cbor use aiken/bytearray use aiken/dict use aiken/hash @@ -10,7 +9,7 @@ use aiken/transaction.{ ScriptContext, Spend, Transaction, TransactionId, } use aiken/transaction/credential.{ - Address, VerificationKeyCredential, from_verification_key, + Address, VerificationKeyCredential, from_verification_key, from_script, with_delegation_key } use aiken/transaction/value.{Value} use shared.{ @@ -36,6 +35,8 @@ type ScoopTestOptions { edit_escrow_destination: Option
, edit_fee: Option, edit_swap_fees: Option<(Int,Int)>, + edit_pool_input_address: Option
, + edit_pool_output_address: Option
, edit_pool_output_value: Option, edit_settings_datum: Option, } @@ -47,6 +48,8 @@ fn default_scoop_test_options() -> ScoopTestOptions { edit_escrow_destination: None, edit_fee: None, edit_swap_fees: None, + edit_pool_input_address: None, + edit_pool_output_address: None, edit_pool_output_value: None, edit_settings_datum: None, } @@ -102,8 +105,6 @@ test pool_validator_ignores_fee() { } test scoop_high_swap_fees() { - let hash_of_pool_script = - #"00000000000000000000000000000000000000000000000000000000" let pool_id = #"00000000000000000000000000000000000000000000000000000000" let pool_nft_name = shared.pool_nft_name(pool_id) let dummy_policy_id = @@ -129,7 +130,7 @@ test scoop_high_swap_fees() { dummy_asset_name, 1_000_000_000 - ( 9_802_950 + 9_611_678 ), ) - |> value.add(hash_of_pool_script, pool_nft_name, 1), + |> value.add(pool_script_hash, pool_nft_name, 1), ), } scoop(options) @@ -154,6 +155,35 @@ test output_missing_nft() fail { scoop(options) } +const pool_script_hash = #"00000000000000000000000000000000000000000000000000000000" +const random_hash = #"6af53ff4f054348ad825c692dd9db8f1760a8e0eacf9af9f99306513" +const other_hash = #"01010101010101010101010101010101010101010101010101010101" +test scoop_pool_output_wallet_address() fail { + let options = + ScoopTestOptions { + ..default_scoop_test_options(), + edit_pool_output_address: Some(from_verification_key(random_hash)), + } + scoop(options) +} +test scoop_pool_output_wrong_script() fail { + let options = + ScoopTestOptions { + ..default_scoop_test_options(), + edit_pool_output_address: Some(from_script(random_hash)), + } + scoop(options) +} +test scoop_pool_output_change_staking_credential() fail { + let options = + ScoopTestOptions { + ..default_scoop_test_options(), + edit_pool_input_address: Some(from_script(pool_script_hash) |> with_delegation_key(random_hash)), + edit_pool_output_address: Some(from_script(pool_script_hash) |> with_delegation_key(other_hash)), + } + scoop(options) +} + test scooper_not_in_settings() fail { let somebody = #"11111111111111111111111111111111111111111111111111111111" let options = @@ -198,8 +228,6 @@ fn scoop(options: ScoopTestOptions) { #"9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77" let dummy_asset_name = #"53554e444145" let scooper = #"00000000000000000000000000000000000000000000000000000000" - let hash_of_pool_script = - #"00000000000000000000000000000000000000000000000000000000" let hash_of_escrow_script = #"00000000000000000000000000000000000000000000000000000000" let user_addr = @@ -230,15 +258,15 @@ fn scoop(options: ScoopTestOptions) { protocol_fees: 7_000_000, } let pool_nft_name = shared.pool_nft_name(pool_id) - let pool_address = script_address(hash_of_pool_script) + let pool_address = script_address(pool_script_hash) let pool_input = Input { output_reference: mk_output_reference(0), output: Output { - address: pool_address, + address: option.or_else(options.edit_pool_input_address, pool_address), value: value.from_lovelace(1_000_000_000 + 2_000_000) |> value.add(dummy_policy_id, dummy_asset_name, 1_000_000_000) - |> value.add(hash_of_pool_script, pool_nft_name, 1), + |> value.add(pool_script_hash, pool_nft_name, 1), datum: InlineDatum(pool_datum), reference_script: None, }, @@ -310,7 +338,7 @@ fn scoop(options: ScoopTestOptions) { } let pool_output = Output { - address: pool_address, + address: option.or_else(options.edit_pool_output_address, pool_address), value: option.or_else( options.edit_pool_output_value, value.from_lovelace(1_000_000_000 + 20_000_000 + 5_000_000 + 2_000_000) @@ -319,7 +347,7 @@ fn scoop(options: ScoopTestOptions) { dummy_asset_name, 1_000_000_000 - ( 9_896_088 + 9_702_095 ), ) - |> value.add(hash_of_pool_script, pool_nft_name, 1), + |> value.add(pool_script_hash, pool_nft_name, 1), ), datum: InlineDatum(pool_out_datum), reference_script: None, @@ -359,7 +387,6 @@ fn scoop_swap_deposit(options: ScoopTestOptions) { let dummy_policy_id = #"9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77" let dummy_asset_name = #"53554e444145" let scooper = #"00000000000000000000000000000000000000000000000000000000" - let hash_of_pool_script = #"00000000000000000000000000000000000000000000000000000000" let hash_of_escrow_script = #"00000000000000000000000000000000000000000000000000000000" let user_addr = wallet_address( #"6af53ff4f054348ad825c692dd9db8f1760a8e0eacf9af9f99306513" @@ -393,14 +420,14 @@ fn scoop_swap_deposit(options: ScoopTestOptions) { protocol_fees: 7_000_000, } let pool_nft_name = shared.pool_nft_name(pool_id) - let pool_address = script_address(hash_of_pool_script) + let pool_address = script_address(pool_script_hash) let pool_input = Input { output_reference: mk_output_reference(0), output: Output { address: pool_address, value: value.from_lovelace(1_000_000_000 + 2_000_000) |> value.add(dummy_policy_id, dummy_asset_name, 1_000_000_000) - |> value.add(hash_of_pool_script, pool_nft_name, 1), + |> value.add(pool_script_hash, pool_nft_name, 1), datum: InlineDatum(pool_datum), reference_script: None, }, @@ -474,7 +501,7 @@ fn scoop_swap_deposit(options: ScoopTestOptions) { let escrow2_out = Output { address: option.or_else(options.edit_escrow_destination, user_addr), value: value.from_lovelace(2_000_000) - |> value.add(hash_of_pool_script, pool_lp_name(pool_id), 9_900_990) + |> value.add(pool_script_hash, pool_lp_name(pool_id), 9_900_990) |> value.add(dummy_policy_id, dummy_asset_name, 196_990), datum: NoDatum, reference_script: None, @@ -484,7 +511,7 @@ fn scoop_swap_deposit(options: ScoopTestOptions) { value: option.or_else(options.edit_pool_output_value, value.from_lovelace(1_000_000_000 + 20_000_000 + 5_000_000 + 2_000_000) |> value.add(dummy_policy_id, dummy_asset_name, 1_000_000_000 - 9_896_088 + 10_000_000 - 196_990) - |> value.add(hash_of_pool_script, pool_nft_name, 1)), + |> value.add(pool_script_hash, pool_nft_name, 1)), datum: InlineDatum(pool_out_datum), reference_script: None, } @@ -497,7 +524,7 @@ fn scoop_swap_deposit(options: ScoopTestOptions) { fee: option.or_else(options.edit_fee, value.from_lovelace(1_000_000)), mint: value.to_minted_value( value.from_lovelace(0) - |> value.add(hash_of_pool_script, pool_lp_name(pool_id), 9_900_990) + |> value.add(pool_script_hash, pool_lp_name(pool_id), 9_900_990) ), certificates: [], withdrawals: dict.new(), @@ -521,7 +548,6 @@ fn scoop_swap_deposit(options: ScoopTestOptions) { result } -const hash_of_pool_script = #"00000000000000000000000000000000000000000000000000000000" fn pool_test_tx_input() -> Input { let funds_input = new_tx_input( @@ -533,12 +559,12 @@ fn pool_test_tx_input() -> Input { funds_input } -test mint_test_two_nfts() { +test mint_test_two_nfts() fail { let pool_id = pool_ident_from_input(pool_test_tx_input()) let (_, new_pool_nft_token, _) = shared.pool_token_names(pool_id) // if we add on another pool NFT token to the pool output, it should fail - !mint_test_modify( - fn(output) { Output { ..output, value: value.add(output.value, hash_of_pool_script, new_pool_nft_token, 1) } }, + mint_test_modify( + fn(output) { Output { ..output, value: value.add(output.value, pool_script_hash, new_pool_nft_token, 1) } }, identity, identity, identity @@ -560,8 +586,7 @@ fn mint_test_modify( modify_ref_output: fn(Output) -> Output, modify_datum: fn(Datum) -> Datum) -> Bool { let settings_policy_id = #"00000000000000000000000000000000000000000000000000000000" - let hash_of_pool_script = #"00000000000000000000000000000000000000000000000000000000" - let pool_address = script_address(hash_of_pool_script) + let pool_address = script_address(pool_script_hash) let rberry_policy_id = #"9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77" let rberry_token_name = #"524245525259" let user_address = @@ -584,14 +609,14 @@ fn mint_test_modify( )) let pool_output_val = value.from_asset(rberry_policy_id, rberry_token_name, 1_000_000_000) - |> value.add(hash_of_pool_script, new_pool_nft_token, 1) + |> value.add(pool_script_hash, new_pool_nft_token, 1) |> value.merge(value.from_lovelace(1_002_000_000)) let pool_output = new_tx_output(pool_address, 0, inline_pool_datum) // 1_002_000_000 = 1_000_000_000 ADA for pool + 2_000_000 ADA for protocol_fees |> add_asset_to_tx_output(pool_output_val) |> modify_pool_output let lp_output_val = - value.from_asset(hash_of_pool_script, pool_lp_name(pool_id), 1_000_000_000) + value.from_asset(pool_script_hash, pool_lp_name(pool_id), 1_000_000_000) |> value.merge(value.from_lovelace(2_000_000)) let lp_output = new_tx_output(user_address, 0, NoDatum) // we can probably get rid of the rider, it gets auto added @@ -599,7 +624,7 @@ fn mint_test_modify( |> modify_lp_output let ref_output_val = - value.from_asset(hash_of_pool_script, new_pool_ref_token, 1) + value.from_asset(pool_script_hash, new_pool_ref_token, 1) |> value.merge(value.from_lovelace(2_000_000)) let ref_output = new_tx_output(user_address, 0, NoDatum) // we can probably get rid of the rider, it gets auto added @@ -614,11 +639,11 @@ fn mint_test_modify( let ctx = interval.between(1,2) |> build_txn_context() - |> mint_assets(hash_of_pool_script, value.to_minted_value( + |> mint_assets(pool_script_hash, value.to_minted_value( value.from_lovelace(0) - |> value.add(hash_of_pool_script, new_pool_lp_token, 1_000_000_000) - |> value.add(hash_of_pool_script, new_pool_nft_token, 1) - |> value.add(hash_of_pool_script, new_pool_ref_token, 1) + |> value.add(pool_script_hash, new_pool_lp_token, 1_000_000_000) + |> value.add(pool_script_hash, new_pool_nft_token, 1) + |> value.add(pool_script_hash, new_pool_ref_token, 1) )) |> add_tx_input(funds_input) |> add_tx_ref_input(settings_input) @@ -637,7 +662,7 @@ test mint_test() { } // make sure pool_output.address is checked to be the pool address -!test mint_test_wrong_address () { +test mint_test_wrong_address () fail { let minted = mint_test_modify( // change pool nft output address to destination that shouldn't be possible fn (output) { Output{..output, address: from_verification_key(#"6af53ff4f054348ad825c692dd9db8f1760a8e0eacf9af9f99306513") }}, From 8c4b421d2e2e3888bfc6f01babf804753f7e34b7 Mon Sep 17 00:00:00 2001 From: Pi Lanningham Date: Thu, 29 Feb 2024 03:58:28 -0500 Subject: [PATCH 6/6] Resolve SSW-001 *Actually* resolve SSW-001; turns out I had misread the finding and stumbled on another finding where we weren't checking the output address on the scoop path. Woops! This shoudl fix SSW-001, which was related to pool minting, and fixes some tests related to it --- validators/pool.ak | 7 +++++++ validators/tests/pool.ak | 14 ++++++++++---- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/validators/pool.ak b/validators/pool.ak index dd4a760..66b6028 100644 --- a/validators/pool.ak +++ b/validators/pool.ak @@ -443,6 +443,13 @@ validator(settings_policy_id: PolicyId) { pool_output_datum.market_open <= pool_output_datum.fee_finalized, } + // Make sure that the pool output is paid into own_policy_id (the pool script, remember this is a multivalidator) + // and that one of the valid staking addresses is attached + expect pool_output.address.payment_credential == ScriptCredential(own_policy_id) + expect list.any(settings_datum.authorized_staking_keys, fn(a) { + pool_output.address.stake_credential == Some(Inline(a)) + }) + // And then check each of the conditions above as the condition for minting and { coin_pair_ordering_is_canonical, diff --git a/validators/tests/pool.ak b/validators/tests/pool.ak index 149b487..c79b00c 100644 --- a/validators/tests/pool.ak +++ b/validators/tests/pool.ak @@ -586,7 +586,7 @@ fn mint_test_modify( modify_ref_output: fn(Output) -> Output, modify_datum: fn(Datum) -> Datum) -> Bool { let settings_policy_id = #"00000000000000000000000000000000000000000000000000000000" - let pool_address = script_address(pool_script_hash) + let pool_address = script_address(pool_script_hash) |> with_delegation_key(#"725011d2c296eb3341e159b6c5c6991de11e81062b95108c9aa024ad") let rberry_policy_id = #"9a9693a9a37912a5097918f97918d15240c92ab729a0b7c4aa144d77" let rberry_token_name = #"524245525259" let user_address = @@ -631,7 +631,7 @@ fn mint_test_modify( |> add_asset_to_tx_output(ref_output_val) |> modify_ref_output - let poolMintRedeemer = CreatePool { + let pool_mint_redeemer = CreatePool { assets: ((#"", #""), (rberry_policy_id, rberry_token_name)), pool_output: 0, metadata_output: 2, @@ -653,7 +653,7 @@ fn mint_test_modify( |> add_tx_output(lp_output) |> add_tx_output(pool_output) - let result = pool_validator.mint(settings_policy_id, poolMintRedeemer, ctx) + let result = pool_validator.mint(settings_policy_id, pool_mint_redeemer, ctx) result } @@ -665,7 +665,13 @@ test mint_test() { test mint_test_wrong_address () fail { let minted = mint_test_modify( // change pool nft output address to destination that shouldn't be possible - fn (output) { Output{..output, address: from_verification_key(#"6af53ff4f054348ad825c692dd9db8f1760a8e0eacf9af9f99306513") }}, + fn (output) { + Output{ + ..output, + // TODO: move some of this stuff to constants? + address: from_verification_key(random_hash) |> with_delegation_key(#"725011d2c296eb3341e159b6c5c6991de11e81062b95108c9aa024ad") + } + }, identity, identity, identity