diff --git a/Cargo.lock b/Cargo.lock index aafc72c6..3c1c8185 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2458,6 +2458,7 @@ dependencies = [ name = "test-faucet" version = "0.1.0" dependencies = [ + "gmx-keys", "soroban-sdk", "test-token", ] @@ -2466,6 +2467,7 @@ dependencies = [ name = "test-token" version = "0.1.0" dependencies = [ + "gmx-keys", "soroban-sdk", ] diff --git a/contracts/adl_handler/src/lib.rs b/contracts/adl_handler/src/lib.rs index cdb6c731..d1189499 100644 --- a/contracts/adl_handler/src/lib.rs +++ b/contracts/adl_handler/src/lib.rs @@ -48,9 +48,6 @@ pub enum Error { NotProfitable = 6, MarketPaused = 9, PositionNotFound = 7, - /// Max PnL factor for ADL is not configured (0) for the requested market/side. - /// Callers must set a non-zero value via DataStore before ADL can be evaluated. - MissingMaxPnlConfig = 8, /// Issue #643: update_oracle called with the current oracle address /// (no-op) or with this contract's own address / another of its stored /// instance addresses (order_handler, data_store, role_store, admin) — diff --git a/contracts/oracle/src/lib.rs b/contracts/oracle/src/lib.rs index 0c786575..1c2ce677 100644 --- a/contracts/oracle/src/lib.rs +++ b/contracts/oracle/src/lib.rs @@ -44,7 +44,6 @@ pub enum Error { InvalidPrice = 4, // min > max or zero StalePrice = 5, // timestamp too old PriceNotFound = 6, - InvalidSignature = 7, /// clear_prices called with more than MAX_CLEAR_PRICES_BATCH_SIZE tokens (issue #619). BatchSizeLimitExceeded = 9, } @@ -273,6 +272,10 @@ impl Oracle { ); let pubkey = get_keeper_pubkey(&env, &data_store, sp.keeper_index); // ed25519_verify takes (&BytesN<32> pubkey, &Bytes message, &BytesN<64> sig) + // and panics with a host-level crypto error on bad signature. + // There is no SDK try_ helper (soroban-sdk 25) to convert this into a + // contract Error::InvalidSignature, so the dead variant was removed + // (#576); callers see the host error instead of a typed contract error. env.crypto().ed25519_verify(&pubkey, &msg, &sp.signature); // Check circuit breaker before overwriting price diff --git a/contracts/role_store/src/lib.rs b/contracts/role_store/src/lib.rs index eece0b49..26d60350 100644 --- a/contracts/role_store/src/lib.rs +++ b/contracts/role_store/src/lib.rs @@ -575,4 +575,143 @@ mod tests { )) ); } + + // ── Issue #564: revoke-side RoleMemberCount/RoleMembers coverage ───────── + + /// Grant a role to two accounts, revoke one, and assert the count + /// decremented by exactly one and the remaining member list is correct. + #[test] + fn test_revoke_decrements_count_and_removes_member() { + let (env, admin, contract_id) = setup(); + let client = RoleStoreClient::new(&env, &contract_id); + let ctrl = roles::controller(&env); + let k1 = Address::generate(&env); + let k2 = Address::generate(&env); + + client.grant_role(&admin, &k1, &ctrl); + client.grant_role(&admin, &k2, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 2); + let members = client.get_role_members(&ctrl, &0, &10); + assert_eq!(members.len(), 2); + + client.revoke_role(&admin, &k1, &ctrl); + + assert_eq!( + client.get_role_member_count(&ctrl), + 1, + "count must decrement by exactly one after revoke" + ); + assert!(!client.has_role(&k1, &ctrl)); + assert!(client.has_role(&k2, &ctrl)); + + let members = client.get_role_members(&ctrl, &0, &10); + assert_eq!(members.len(), 1); + assert_eq!(members.get_unchecked(0), k2); + // revoked account must no longer appear + assert!(!vec_contains_b32(&client.get_roles(&k1), &ctrl)); + } + + /// Revoke one member out of three and assert the other two remain in order + /// and vec_remove_addr removed only the target (middle of the list). + #[test] + fn test_revoke_one_of_many_leaves_others() { + let (env, admin, contract_id) = setup(); + let client = RoleStoreClient::new(&env, &contract_id); + let ctrl = roles::controller(&env); + let k1 = Address::generate(&env); + let k2 = Address::generate(&env); + let k3 = Address::generate(&env); + + client.grant_role(&admin, &k1, &ctrl); + client.grant_role(&admin, &k2, &ctrl); + client.grant_role(&admin, &k3, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 3); + + // revoke the middle member (k2) — exercises vec_remove_addr not just popping tail + client.revoke_role(&admin, &k2, &ctrl); + + assert_eq!(client.get_role_member_count(&ctrl), 2); + assert!(!client.has_role(&k2, &ctrl)); + assert!(client.has_role(&k1, &ctrl)); + assert!(client.has_role(&k3, &ctrl)); + + let members = client.get_role_members(&ctrl, &0, &10); + assert_eq!(members.len(), 2); + // remaining members must be k1 and k3 (k2 removed from middle) + assert_eq!(members.get_unchecked(0), k1); + assert_eq!(members.get_unchecked(1), k3); + + // revoke the first member (k1) — exercises remove from start of list + client.revoke_role(&admin, &k1, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 1); + let members = client.get_role_members(&ctrl, &0, &10); + assert_eq!(members.len(), 1); + assert_eq!(members.get_unchecked(0), k3); + } + + /// Second revoke of the same account/role must be a harmless no-op + /// (internal_revoke_role is documented as idempotent). Count must not + /// go negative or decrement twice. + #[test] + fn test_idempotent_revoke() { + let (env, admin, contract_id) = setup(); + let client = RoleStoreClient::new(&env, &contract_id); + let ctrl = roles::controller(&env); + let keeper = Address::generate(&env); + + client.grant_role(&admin, &keeper, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 1); + + client.revoke_role(&admin, &keeper, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 0); + assert!(!client.has_role(&keeper, &ctrl)); + + // second revoke — must not panic, must not decrement below zero + client.revoke_role(&admin, &keeper, &ctrl); + assert_eq!( + client.get_role_member_count(&ctrl), + 0, + "second revoke must be a no-op and not go negative" + ); + assert!(!client.has_role(&keeper, &ctrl)); + let members = client.get_role_members(&ctrl, &0, &10); + assert_eq!(members.len(), 0); + + // revoking an account that never held the role at all must also be a no-op + let never_holder = Address::generate(&env); + client.revoke_role(&admin, &never_holder, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 0); + } + + /// Revoke-side bookkeeping for get_roles: after revoke, the account's + /// role list must no longer contain the revoked role while other roles + /// remain — mirroring get_roles_reflects_grants_and_revokes. + #[test] + fn test_revoke_updates_account_roles_and_member_list_together() { + let (env, admin, contract_id) = setup(); + let client = RoleStoreClient::new(&env, &contract_id); + let ctrl = roles::controller(&env); + let order_keeper = roles::order_keeper(&env); + let user = Address::generate(&env); + let other = Address::generate(&env); + + // give user two roles and other one role + client.grant_role(&admin, &user, &ctrl); + client.grant_role(&admin, &user, &order_keeper); + client.grant_role(&admin, &other, &ctrl); + assert_eq!(client.get_role_member_count(&ctrl), 2); + + // revoke ctrl from user — user should keep order_keeper, other keeps ctrl + client.revoke_role(&admin, &user, &ctrl); + + assert_eq!(client.get_role_member_count(&ctrl), 1); + let members = client.get_role_members(&ctrl, &0, &10); + assert_eq!(members.len(), 1); + assert_eq!(members.get_unchecked(0), other); + + let user_roles = client.get_roles(&user); + assert_eq!(user_roles.len(), 1); + assert_eq!(user_roles.get_unchecked(0), order_keeper); + assert!(!vec_contains_b32(&user_roles, &ctrl)); + } } diff --git a/contracts/test_faucet/Cargo.toml b/contracts/test_faucet/Cargo.toml index 074e4350..96bc3c14 100644 --- a/contracts/test_faucet/Cargo.toml +++ b/contracts/test_faucet/Cargo.toml @@ -10,6 +10,7 @@ crate-type = ["cdylib", "lib"] [dependencies] soroban-sdk = { workspace = true } +gmx-keys = { path = "../../libs/keys" } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } diff --git a/contracts/test_faucet/src/lib.rs b/contracts/test_faucet/src/lib.rs index 7e5e3c5f..23ed235c 100644 --- a/contracts/test_faucet/src/lib.rs +++ b/contracts/test_faucet/src/lib.rs @@ -7,16 +7,9 @@ use soroban_sdk::{ contract, contractclient, contracterror, contractimpl, contracttype, panic_with_error, - symbol_short, Address, BytesN, Env, Vec, + symbol_short, Address, Env, Vec, }; -/// `network_id` (SHA-256 of the network passphrase) for the Stellar public -/// network. Test faucets must never be initialized here (issue #400). -const MAINNET_NETWORK_ID: [u8; 32] = [ - 0x7a, 0xc3, 0x39, 0x97, 0x54, 0x4e, 0x31, 0x75, 0xd2, 0x66, 0xbd, 0x02, 0x24, 0x39, 0xb2, 0x2c, - 0xdb, 0x16, 0x50, 0x8c, 0x01, 0x16, 0x3f, 0x26, 0xe5, 0xcb, 0x2a, 0x3e, 0x10, 0x45, 0xa9, 0x79, -]; - #[allow(dead_code)] #[contractclient(name = "TestTokenClient")] trait ITestToken { @@ -60,7 +53,7 @@ pub struct TestFaucet; #[contractimpl] impl TestFaucet { pub fn initialize(env: Env, admin: Address, cooldown_ledgers: u32) { - require_not_mainnet(&env); + gmx_keys::require_not_mainnet(&env, Error::MainnetNotAllowed as u32); // Issue #612: require the admin's own auth so a front-runner who // observes the deploy transaction (or predicts the deterministic // contract address) can't call initialize first with themselves as @@ -179,12 +172,6 @@ fn do_claim(env: &Env, account: &Address, token: Address) -> i128 { amount } -fn require_not_mainnet(env: &Env) { - if env.ledger().network_id() == BytesN::from_array(env, &MAINNET_NETWORK_ID) { - panic_with_error!(env, Error::MainnetNotAllowed); - } -} - fn get_admin(env: &Env) -> Address { env.storage() .instance() @@ -355,7 +342,7 @@ mod tests { fn initialize_rejects_mainnet_network_id() { let env = Env::default(); env.mock_all_auths(); - env.ledger().set_network_id(MAINNET_NETWORK_ID); + env.ledger().set_network_id(gmx_keys::MAINNET_NETWORK_ID); let admin = Address::generate(&env); let faucet_id = env.register(TestFaucet, ()); TestFaucetClient::new(&env, &faucet_id).initialize(&admin, &10); diff --git a/contracts/test_token/Cargo.toml b/contracts/test_token/Cargo.toml index 46e240f9..85384b61 100644 --- a/contracts/test_token/Cargo.toml +++ b/contracts/test_token/Cargo.toml @@ -10,6 +10,7 @@ crate-type = ["cdylib", "lib"] [dependencies] soroban-sdk = { workspace = true } +gmx-keys = { path = "../../libs/keys" } [dev-dependencies] soroban-sdk = { workspace = true, features = ["testutils"] } diff --git a/contracts/test_token/src/lib.rs b/contracts/test_token/src/lib.rs index 8c2c5b0f..83cfb391 100644 --- a/contracts/test_token/src/lib.rs +++ b/contracts/test_token/src/lib.rs @@ -8,16 +8,9 @@ use soroban_sdk::{ contract, contracterror, contractimpl, contracttype, panic_with_error, symbol_short, Address, - BytesN, Env, String, + Env, String, }; -/// `network_id` (SHA-256 of the network passphrase) for the Stellar public -/// network. Test tokens must never be initialized here (issue #400). -const MAINNET_NETWORK_ID: [u8; 32] = [ - 0x7a, 0xc3, 0x39, 0x97, 0x54, 0x4e, 0x31, 0x75, 0xd2, 0x66, 0xbd, 0x02, 0x24, 0x39, 0xb2, 0x2c, - 0xdb, 0x16, 0x50, 0x8c, 0x01, 0x16, 0x3f, 0x26, 0xe5, 0xcb, 0x2a, 0x3e, 0x10, 0x45, 0xa9, 0x79, -]; - #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] @@ -65,7 +58,7 @@ pub struct TestToken; #[contractimpl] impl TestToken { pub fn initialize(env: Env, owner: Address, decimal: u32, name: String, symbol: String) { - require_not_mainnet(&env); + gmx_keys::require_not_mainnet(&env, Error::MainnetNotAllowed as u32); // Issue #612: require the owner's own auth so a front-runner who // observes the deploy transaction (or predicts the deterministic // contract address) can't call initialize first with themselves as @@ -266,12 +259,6 @@ impl TestToken { } } -fn require_not_mainnet(env: &Env) { - if env.ledger().network_id() == BytesN::from_array(env, &MAINNET_NETWORK_ID) { - panic_with_error!(env, Error::MainnetNotAllowed); - } -} - fn get_owner(env: &Env) -> Address { env.storage() .instance() @@ -479,7 +466,7 @@ mod tests { fn initialize_rejects_mainnet_network_id() { let env = Env::default(); env.mock_all_auths(); - env.ledger().set_network_id(MAINNET_NETWORK_ID); + env.ledger().set_network_id(gmx_keys::MAINNET_NETWORK_ID); let owner = Address::generate(&env); let id = env.register(TestToken, ()); TestTokenClient::new(&env, &id).initialize( @@ -489,4 +476,233 @@ mod tests { &String::from_str(&env, "TWBTC"), ); } + + // ── Ported from market_token: allowance-expiration coverage (issue #362) ── + + /// Once the ledger sequence passes an approval's expiration_ledger, + /// allowance() must report 0 even though the underlying temporary entry + /// (if not yet TTL-evicted) still holds the original amount. + #[test] + fn allowance_reads_zero_after_expiration_ledger_passes() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let spender = Address::generate(&env); + + client.mint(&owner, &alice, &1000_0000); + let expiration = env.ledger().sequence() + 100; + client.approve(&alice, &spender, &500_0000, &expiration); + assert_eq!(client.allowance(&alice, &spender), 500_0000); + + env.ledger().set_sequence_number(expiration + 1); + assert_eq!( + client.allowance(&alice, &spender), + 0, + "allowance() must return 0 once expiration_ledger has passed" + ); + } + + #[test] + fn test_approve_and_transfer_from() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + let spender = Address::generate(&env); + + client.mint(&owner, &alice, &1000_0000); + client.approve( + &alice, + &spender, + &500_0000, + &(env.ledger().sequence() + 100), + ); + assert_eq!(client.allowance(&alice, &spender), 500_0000); + + client.transfer_from(&spender, &alice, &bob, &300_0000); + assert_eq!(client.balance(&alice), 700_0000); + assert_eq!(client.balance(&bob), 300_0000); + assert_eq!(client.allowance(&alice, &spender), 200_0000); + } + + /// transfer_from on an expired allowance must revert with AllowanceExpired. + #[test] + fn transfer_from_after_expiration_reverts_with_allowance_expired() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + let spender = Address::generate(&env); + + client.mint(&owner, &alice, &1000_0000); + let expiration = env.ledger().sequence() + 100; + client.approve(&alice, &spender, &500_0000, &expiration); + + env.ledger().set_sequence_number(expiration + 1); + + let result = client.try_transfer_from(&spender, &alice, &bob, &1_0000); + assert_eq!( + result, + Err(Ok(soroban_sdk::Error::from_contract_error( + Error::AllowanceExpired as u32 + ))) + ); + } + + #[test] + fn test_approve_and_burn_from() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let spender = Address::generate(&env); + + client.mint(&owner, &alice, &1000_0000); + assert_eq!(client.total_supply(), 1000_0000); + + client.approve( + &alice, + &spender, + &500_0000, + &(env.ledger().sequence() + 100), + ); + assert_eq!(client.allowance(&alice, &spender), 500_0000); + + client.burn_from(&spender, &alice, &300_0000); + assert_eq!(client.balance(&alice), 700_0000); + assert_eq!(client.allowance(&alice, &spender), 200_0000); + assert_eq!(client.total_supply(), 700_0000); + } + + /// burn_from on an expired allowance must revert with AllowanceExpired. + #[test] + fn burn_from_after_expiration_reverts_with_allowance_expired() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let spender = Address::generate(&env); + + client.mint(&owner, &alice, &1000_0000); + let expiration = env.ledger().sequence() + 100; + client.approve(&alice, &spender, &500_0000, &expiration); + + env.ledger().set_sequence_number(expiration + 1); + + let result = client.try_burn_from(&spender, &alice, &1_0000); + assert_eq!( + result, + Err(Ok(soroban_sdk::Error::from_contract_error( + Error::AllowanceExpired as u32 + ))) + ); + } + + // ── transfer_owner and pause/unpause owner gates ──────────────────────── + + #[test] + fn transfer_owner_moves_ownership() { + let (env, owner, client) = setup(); + let new_owner = Address::generate(&env); + let alice = Address::generate(&env); + + client.transfer_owner(&owner, &new_owner); + assert_eq!(client.owner(), new_owner); + + // old owner can no longer mint + assert!(client.try_mint(&owner, &alice, &1).is_err()); + // new owner can mint + client.mint(&new_owner, &alice, &1); + assert_eq!(client.balance(&alice), 1); + } + + #[test] + #[should_panic] + fn non_owner_cannot_transfer_owner() { + let (env, _owner, client) = setup(); + let attacker = Address::generate(&env); + let new_owner = Address::generate(&env); + client.transfer_owner(&attacker, &new_owner); + } + + #[test] + #[should_panic] + fn non_owner_cannot_pause() { + let (env, _owner, client) = setup(); + let attacker = Address::generate(&env); + client.pause(&attacker); + } + + #[test] + #[should_panic] + fn non_owner_cannot_unpause() { + let (env, owner, client) = setup(); + let attacker = Address::generate(&env); + client.pause(&owner); + client.unpause(&attacker); + } + + // ── Negative-amount rejection (require_non_negative guard) ────────────── + + #[test] + #[should_panic] + fn transfer_rejects_negative_amount() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + client.mint(&owner, &alice, &1000); + client.transfer(&alice, &bob, &-1); + } + + #[test] + #[should_panic] + fn approve_rejects_negative_amount() { + let (env, _, client) = setup(); + let alice = Address::generate(&env); + let spender = Address::generate(&env); + client.approve(&alice, &spender, &-1, &(env.ledger().sequence() + 100)); + } + + #[test] + #[should_panic] + fn mint_rejects_negative_amount() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + client.mint(&owner, &alice, &-1); + } + + #[test] + #[should_panic] + fn burn_rejects_negative_amount() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + client.mint(&owner, &alice, &1000); + client.burn(&alice, &-1); + } + + #[test] + #[should_panic] + fn transfer_from_rejects_negative_amount() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let bob = Address::generate(&env); + let spender = Address::generate(&env); + client.mint(&owner, &alice, &1000); + client.approve( + &alice, + &spender, + &1000, + &(env.ledger().sequence() + 100), + ); + client.transfer_from(&spender, &alice, &bob, &-1); + } + + #[test] + #[should_panic] + fn burn_from_rejects_negative_amount() { + let (env, owner, client) = setup(); + let alice = Address::generate(&env); + let spender = Address::generate(&env); + client.mint(&owner, &alice, &1000); + client.approve( + &alice, + &spender, + &1000, + &(env.ledger().sequence() + 100), + ); + client.burn_from(&spender, &alice, &-1); + } } diff --git a/libs/keys/src/lib.rs b/libs/keys/src/lib.rs index cf12faeb..8bac9440 100644 --- a/libs/keys/src/lib.rs +++ b/libs/keys/src/lib.rs @@ -13,6 +13,39 @@ pub const PERSISTENT_BUMP_TARGET: u32 = 518_400; /// Renew an entry's TTL once its remaining TTL drops below this threshold. pub const MIN_BUMP_THRESHOLD: u32 = 259_200; +// ─── Testnet guard (#567) ──────────────────────────────────────────────────── +// +// Single source of truth for the Stellar mainnet `network_id` (SHA-256 of the +// network passphrase) and the "must not run on mainnet" check previously +// duplicated byte-for-byte in `test_token` and `test_faucet`. Keeping exactly +// one definition ensures a future passphrase change or typo fix cannot be +// applied to one test-only contract while silently missing the other +// (issue #400 / #567). + +/// `network_id` for the Stellar public network. Test-only contracts must +/// refuse to initialize when `env.ledger().network_id()` equals this value. +pub const MAINNET_NETWORK_ID: [u8; 32] = [ + 0x7a, 0xc3, 0x39, 0x97, 0x54, 0x4e, 0x31, 0x75, 0xd2, 0x66, 0xbd, 0x02, 0x24, 0x39, 0xb2, 0x2c, + 0xdb, 0x16, 0x50, 0x8c, 0x01, 0x16, 0x3f, 0x26, 0xe5, 0xcb, 0x2a, 0x3e, 0x10, 0x45, 0xa9, 0x79, +]; + +/// Returns true if the current ledger's `network_id` is the Stellar mainnet. +pub fn is_mainnet(env: &Env) -> bool { + env.ledger().network_id() == BytesN::from_array(env, &MAINNET_NETWORK_ID) +} + +/// Panics with the given contract-error code if the current network is mainnet. +/// +/// `error_code` should be the `u32` discriminant of the caller's +/// `MainnetNotAllowed` variant (e.g. `Error::MainnetNotAllowed as u32`), so the +/// shared guard can panic with the caller's own typed error while keeping the +/// network-ID check in one place. +pub fn require_not_mainnet(env: &Env, error_code: u32) { + if is_mainnet(env) { + env.panic_with_error(soroban_sdk::Error::from_contract_error(error_code)); + } +} + // ─── Internal key builder ───────────────────────────────────────────────────── // // Each component is length-prefixed (2-byte BE length + bytes) so that