From 6f435779d1f5b79fb44137d1610d1413588ac351 Mon Sep 17 00:00:00 2001 From: rudrasatani13 Date: Sat, 12 Sep 2026 22:23:32 +0530 Subject: [PATCH] fix(asserter-consumer): pin trusted addresses and gate create_assertion_as_self create_assertion_as_self had no require_auth() at all, and built its authorize_as_current_contract pre-authorization from caller-supplied tholos_id and token_id. An attacker could pass a token this contract holds plus a Tholos instance they control, and the pre-authorization would then satisfy the auth requirement for a transfer out of this contract's own balance, repeatable. The trusted instance and token are now stored once by a new admin-gated initialize and read from storage, so no caller input reaches the pre-authorization at all; bond_amount is the only remaining caller-supplied value and is bounded by this contract's own balance. The call itself is admin-gated, because it spends the contract's funds. The admin is pinned by __constructor rather than taken as an initialize argument: a caller-supplied admin in initialize is front-runnable, which is the hole contracts/tholos moved its own pinning out of initialize to close. Two error variants are added to tholos_client::Error for the consumer's lifecycle states. Discriminants 1-4 are unchanged. Closes #157 --- contracts/asserter-consumer/src/lib.rs | 155 ++++++++++--- contracts/asserter-consumer/src/test.rs | 275 ++++++++++++++---------- contracts/tholos-client/src/lib.rs | 12 ++ 3 files changed, 302 insertions(+), 140 deletions(-) diff --git a/contracts/asserter-consumer/src/lib.rs b/contracts/asserter-consumer/src/lib.rs index 0223110..5df57ff 100644 --- a/contracts/asserter-consumer/src/lib.rs +++ b/contracts/asserter-consumer/src/lib.rs @@ -2,45 +2,115 @@ //! Second integration example: this contract's own address as the asserter, //! demonstrating the "Your contract's own address as asserter" pattern from -//! INTEGRATION.md. See `demo-consumer` for the simpler, recommended default -//! (end user as asserter) instead. +//! INTEGRATION.md. See `demo-consumer` for the simpler, recommended +//! default (end user as asserter) instead. use soroban_sdk::{ auth::{ContractContext, InvokerContractAuthEntry, SubContractInvocation}, - contract, contractimpl, Address, Env, IntoVal, Symbol, Vec, + contract, contractimpl, contracttype, Address, Env, IntoVal, Symbol, Vec, }; use tholos_client::{tholos, Error}; +const DAY_IN_LEDGERS: u32 = 17280; +const INSTANCE_BUMP_AMOUNT: u32 = 30 * DAY_IN_LEDGERS; +const INSTANCE_LIFETIME_THRESHOLD: u32 = INSTANCE_BUMP_AMOUNT - DAY_IN_LEDGERS; + +#[contracttype] +pub enum DataKey { + Admin, + TholosId, + TokenId, +} + #[contract] pub struct AsserterConsumer; #[contractimpl] impl AsserterConsumer { + /// Pins the admin atomically at deploy time. + /// + /// This is a `__constructor` rather than an `admin` argument to + /// `initialize` on purpose. A caller-supplied admin in `initialize` + /// is front-runnable: the first caller wins and chooses the admin, which + /// for this contract would mean choosing who may spend its balance. + /// `contracts/tholos` moved its own admin pinning into a constructor + /// for exactly this reason; this follows that pattern. + pub fn __constructor(env: Env, admin: Address) { + admin.require_auth(); + + env.storage().instance().set(&DataKey::Admin, &admin); + Self::touch_instance_ttl(&env); + } + + /// Stores the trusted Tholos instance and token addresses once, after + /// which they can never be changed. + /// + /// Both are fixed for the life of the contract because they are what + /// `create_assertion_as_self` pre-authorizes a transfer with. If either + /// could be supplied per call, the caller could name a token this contract + /// holds and a destination it controls, and the pre-authorization would + /// satisfy the auth requirement for moving this contract's own balance. + /// Keeping them in storage removes that input entirely rather than + /// validating it. + /// + /// Requires the signature of the admin pinned at deploy time. Fails with + /// `Error::AlreadyInitialized` if called twice. + pub fn initialize(env: Env, tholos_id: Address, token_id: Address) -> Result<(), Error> { + let admin = Self::admin(&env)?; + admin.require_auth(); + + if env.storage().instance().has(&DataKey::TholosId) { + return Err(Error::AlreadyInitialized); + } + + env.storage().instance().set(&DataKey::TholosId, &tholos_id); + env.storage().instance().set(&DataKey::TokenId, &token_id); + Self::touch_instance_ttl(&env); + + Ok(()) + } + /// Posts an assertion with this contract's own address as the asserter, so - /// the bond pools under this contract rather than an end user. `token_id` - /// and `bond_amount` must match the Tholos instance's actual configuration - /// at `tholos_id`: there's no way to query them from Tholos ahead of the - /// call, so the caller (or this contract's own deployer) has to already - /// know them, exactly as INTEGRATION.md describes. + /// the bond pools under this contract rather than an end user. + /// + /// Both the token being spent and the Tholos instance being paid are read + /// from storage, never from arguments: the caller cannot influence the + /// transfer this contract authorizes. `bond_amount` is the only + /// remaining caller-supplied value, and it is bounded by this contract's + /// own balance. The call is admin-gated, because it spends the contract's + /// funds. /// /// Soroban only auto-grants a contract's implicit self-authorization one - /// call deep. This call chain is two deep (this contract -> Tholos -> - /// the token's `transfer`), so the deeper call needs to be explicitly - /// pre-authorized with `authorize_as_current_contract` before invoking - /// Tholos, specifying the exact token contract, `transfer` args, and - /// amount Tholos will end up calling. + /// call deep. This call chain is two deep (this contract -> Tholos -> the + /// token's `transfer`), so the deeper call is explicitly pre-authorized + /// with `authorize_as_current_contract` before invoking Tholos, + /// specifying the exact token contract, `transfer` args, and amount + /// Tholos will end up calling. /// - /// Returns `Error::TholosNotInitialized` or `Error::TholosPaused` if the - /// Tholos instance at `tholos_id` can't currently accept an assertion, or - /// `Error::InvalidTholosId` if `tholos_id` doesn't resolve to an - /// invokable Tholos instance at all. + /// Returns `Error::NotInitialized` if `initialize` has not run, + /// `Error::TholosNotInitialized` or `Error::TholosPaused` if the + /// trusted Tholos instance cannot currently accept an assertion, or + /// `Error::InvalidTholosId` if it does not resolve to an invokable + /// Tholos instance at all. pub fn create_assertion_as_self( env: Env, - tholos_id: Address, - token_id: Address, bond_amount: i128, outcome: bool, ) -> Result { + let admin = Self::admin(&env)?; + admin.require_auth(); + + let tholos_id: Address = env + .storage() + .instance() + .get(&DataKey::TholosId) + .ok_or(Error::NotInitialized)?; + let token_id: Address = env + .storage() + .instance() + .get(&DataKey::TokenId) + .ok_or(Error::NotInitialized)?; + let curr_contract = env.current_contract_address(); env.authorize_as_current_contract(Vec::from_array( @@ -53,7 +123,7 @@ impl AsserterConsumer { &env, [ curr_contract.into_val(&env), - tholos_id.into_val(&env), + tholos_id.clone().into_val(&env), bond_amount.into_val(&env), ], ), @@ -63,20 +133,53 @@ impl AsserterConsumer { )); let client = tholos::Client::new(&env, &tholos_id); - Error::from_tholos_call(client.try_assert_outcome(&curr_contract, &outcome)) + let result = Error::from_tholos_call(client.try_assert_outcome(&curr_contract, &outcome)); + + if result.is_ok() { + Self::touch_instance_ttl(&env); + } + + result } /// Forwards a read of an assertion's current state. See INTEGRATION.md for - /// why `Assertion.outcome` is the *claimed* outcome, not necessarily the - /// final one if the assertion was disputed and overturned. + /// why `Assertion.outcome` is the *claimed* outcome, not necessarily + /// the final one if the assertion was disputed and overturned. + /// + /// This keeps taking `tholos_id` rather than reading the configured + /// instance: it moves no funds, so an arbitrary instance here is a read the + /// caller could already make directly, and narrowing it would widen this + /// change without closing anything. /// - /// Returns `Error::AssertionNotFound` if no assertion exists under `id` - /// on the Tholos instance at `tholos_id`, or `Error::InvalidTholosId` if - /// `tholos_id` doesn't resolve to an invokable Tholos instance at all. + /// Returns `Error::AssertionNotFound` if no assertion exists under + /// `id` on the Tholos instance at `tholos_id`, or + /// `Error::InvalidTholosId` if `tholos_id` does not resolve to an + /// invokable Tholos instance at all. pub fn get_status(env: Env, tholos_id: Address, id: u64) -> Result { let client = tholos::Client::new(&env, &tholos_id); Error::from_tholos_call(client.try_get_assertion_state(&id)) } + + /// The admin pinned by the constructor. + /// + /// `NotInitialized` here is defensive rather than reachable in + /// practice: every live instance has already run its constructor by the + /// time any call reaches this, and the constructor is the only writer of + /// this key. + fn admin(env: &Env) -> Result { + env.storage() + .instance() + .get(&DataKey::Admin) + .ok_or(Error::NotInitialized) + } + + /// Renews instance storage TTL, so the trusted addresses and the admin + /// cannot be archived out from under a contract that is still in use. + fn touch_instance_ttl(env: &Env) { + env.storage() + .instance() + .extend_ttl(INSTANCE_LIFETIME_THRESHOLD, INSTANCE_BUMP_AMOUNT); + } } mod test; diff --git a/contracts/asserter-consumer/src/test.rs b/contracts/asserter-consumer/src/test.rs index 118caa7..b6f1494 100644 --- a/contracts/asserter-consumer/src/test.rs +++ b/contracts/asserter-consumer/src/test.rs @@ -4,94 +4,17 @@ use super::*; use soroban_sdk::testutils::{Address as _, MockAuth, MockAuthInvoke}; use soroban_sdk::{token, IntoVal}; -#[test] -fn test_asserter_consumer_can_assert_as_itself_through_tholos() { - let env = Env::default(); - - // Deliberately not using blanket auth mocking past construction: this - // test exists specifically to prove authorize_as_current_contract - // grants the real nested auth Tholos's assert_outcome needs for its - // token transfer, without blanket auth mocking papering over a bug in - // that mechanism. A brief mock_all_auths_allowing_non_root_auth() - // covers only __constructor's admin.require_auth() (admin is now - // pinned atomically at deploy, #158, so there's no way to narrow-mock - // it before a contract id exists to address a MockAuthInvoke at; the - // non-root variant is needed because registering from imported WASM - // records the constructor's auth as non-root). Every call after that, - // including initialize, mocks only the one specific signature it needs. - let admin = Address::generate(&env); - env.mock_all_auths_allowing_non_root_auth(); - let tholos_id = env.register(tholos::WASM, (admin.clone(),)); - let tholos_client = tholos::Client::new(&env, &tholos_id); - - let token_admin = Address::generate(&env); - let token_contract = env.register_stellar_asset_contract_v2(token_admin.clone()); - let token_id = token_contract.address(); - let token_asset_client = token::StellarAssetClient::new(&env, &token_id); - - let resolvers = Vec::from_array( - &env, - [ - Address::generate(&env), - Address::generate(&env), - Address::generate(&env), - ], - ); - let bond_amount: i128 = 100; - - env.mock_auths(&[MockAuth { - address: &admin, - invoke: &MockAuthInvoke { - contract: &tholos_id, - fn_name: "initialize", - args: ( - token_id.clone(), - bond_amount, - 3600u64, - resolvers.clone(), - 0u32, - ) - .into_val(&env), - sub_invokes: &[], - }, - }]); - tholos_client.initialize(&token_id, &bond_amount, &3600, &resolvers, &0u32); - - let consumer_id = env.register(AsserterConsumer, ()); - let consumer_client = AsserterConsumerClient::new(&env, &consumer_id); - - // The bond comes from this contract's own balance, not an end user's. - env.mock_auths(&[MockAuth { - address: &token_admin, - invoke: &MockAuthInvoke { - contract: &token_id, - fn_name: "mint", - args: (consumer_id.clone(), 1_000i128).into_val(&env), - sub_invokes: &[], - }, - }]); - token_asset_client.mint(&consumer_id, &1_000); - - let id = consumer_client.create_assertion_as_self(&tholos_id, &token_id, &bond_amount, &true); - - let state = consumer_client.get_status(&tholos_id, &id); - assert!(state.outcome); - assert_eq!(state.asserter, consumer_id); - assert_eq!( - token::Client::new(&env, &token_id).balance(&consumer_id), - 900 - ); -} - -/// A registered but uninitialized token, resolver committee, and Tholos -/// instance, plus an AsserterConsumer pointed at it. Shared setup for the -/// error-path tests below, which don't need the full funded/authorized flow -/// the happy-path test above exercises. +/// A Tholos instance, a token, and an AsserterConsumer whose admin is pinned +/// at deploy, all sharing one ledger. struct Fixture { env: Env, + consumer_admin: Address, + token_admin: Address, tholos_id: Address, tholos_client: tholos::Client<'static>, token_id: Address, + token_asset_client: token::StellarAssetClient<'static>, + consumer_id: Address, consumer_client: AsserterConsumerClient<'static>, resolvers: Vec
, bond_amount: i128, @@ -101,21 +24,22 @@ impl Fixture { fn new() -> Self { let env = Env::default(); - // Covers __constructor's admin.require_auth(): admin is pinned - // atomically at deploy now (#158), so it applies to registration - // itself, before initialize_tholos()'s own mock_all_auths() runs. - // Registering from imported WASM (rather than the native Rust - // type) records the constructor's require_auth as non-root, so - // the non-root variant is required here specifically. + // Covers both constructors' admin.require_auth(): each admin is now + // pinned atomically at deploy, so it applies to registration itself, + // before anything else runs. Registering Tholos from imported WASM + // records its constructor's auth as non-root, so the non-root variant + // is needed here specifically. env.mock_all_auths_allowing_non_root_auth(); - let admin = Address::generate(&env); - let tholos_id = env.register(tholos::WASM, (admin.clone(),)); + + let tholos_admin = Address::generate(&env); + let tholos_id = env.register(tholos::WASM, (tholos_admin,)); let tholos_client = tholos::Client::new(&env, &tholos_id); let token_admin = Address::generate(&env); let token_id = env - .register_stellar_asset_contract_v2(token_admin) + .register_stellar_asset_contract_v2(token_admin.clone()) .address(); + let token_asset_client = token::StellarAssetClient::new(&env, &token_id); let resolvers = Vec::from_array( &env, @@ -126,14 +50,19 @@ impl Fixture { ], ); - let consumer_id = env.register(AsserterConsumer, ()); + let consumer_admin = Address::generate(&env); + let consumer_id = env.register(AsserterConsumer, (consumer_admin.clone(),)); let consumer_client = AsserterConsumerClient::new(&env, &consumer_id); Fixture { env, + consumer_admin, + token_admin, tholos_id, tholos_client, token_id, + token_asset_client, + consumer_id, consumer_client, resolvers, bond_amount: 100, @@ -150,21 +79,143 @@ impl Fixture { &0u32, ); } + + /// Points the consumer at a specific instance and token. Used by the + /// error-path tests, which need the consumer ready against something other + /// than a healthy Tholos instance. + fn initialize_consumer_at(&self, tholos_id: &Address, token_id: &Address) { + self.env.mock_all_auths(); + self.consumer_client.initialize(tholos_id, token_id); + } + + fn initialize_consumer(&self) { + self.initialize_consumer_at(&self.tholos_id, &self.token_id); + } + + fn fund_consumer(&self, amount: i128) { + self.env.mock_all_auths(); + self.token_asset_client.mint(&self.consumer_id, &amount); + } +} + +/// End-to-end happy path, deliberately narrow-mocking auth past construction. +/// +/// This test exists specifically to prove that `authorize_as_current_contract` +/// grants the real nested auth Tholos's `assert_outcome` needs for its token +/// transfer, without a blanket `mock_all_auths()` papering over a bug in that +/// mechanism. Only the mint and the consumer admin's own signature are mocked; +/// the transfer out of this contract is authorized by the pre-authorization. +#[test] +fn test_asserter_consumer_can_assert_as_itself_through_tholos() { + let f = Fixture::new(); + f.initialize_tholos(); + f.initialize_consumer(); + + // The bond comes from this contract's own balance, not an end user's. + f.env.mock_auths(&[MockAuth { + address: &f.token_admin, + invoke: &MockAuthInvoke { + contract: &f.token_id, + fn_name: "mint", + args: (f.consumer_id.clone(), 1_000i128).into_val(&f.env), + sub_invokes: &[], + }, + }]); + f.token_asset_client.mint(&f.consumer_id, &1_000); + + // Only the configured admin signs. Nothing here names a destination. + f.env.mock_auths(&[MockAuth { + address: &f.consumer_admin, + invoke: &MockAuthInvoke { + contract: &f.consumer_id, + fn_name: "create_assertion_as_self", + args: (f.bond_amount, true).into_val(&f.env), + sub_invokes: &[], + }, + }]); + + let id = f + .consumer_client + .create_assertion_as_self(&f.bond_amount, &true); + + let state = f.consumer_client.get_status(&f.tholos_id, &id); + assert!(state.outcome); + assert_eq!(state.asserter, f.consumer_id); + + let token = token::Client::new(&f.env, &f.token_id); + assert_eq!(token.balance(&f.consumer_id), 900); + // The bond landed on the *configured* Tholos instance, and it is the only + // destination the pre-authorization can name: there is no argument left + // that could redirect it, which is what closes the drain. + assert_eq!(token.balance(&f.tholos_id), f.bond_amount); +} + +#[test] +fn test_create_assertion_as_self_before_initialize_is_rejected() { + let f = Fixture::new(); + f.initialize_tholos(); + + // Consumer never initialized: there is no trusted instance or token yet. + assert_eq!( + f.consumer_client + .try_create_assertion_as_self(&f.bond_amount, &true), + Err(Ok(Error::NotInitialized)) + ); +} + +#[test] +fn test_initialize_cannot_run_twice() { + let f = Fixture::new(); + f.initialize_consumer(); + + // The trusted addresses are write-once, so a later caller cannot re-point + // the contract at an instance or token of their choosing. + let other = Address::generate(&f.env); + assert_eq!( + f.consumer_client.try_initialize(&other, &other), + Err(Ok(Error::AlreadyInitialized)) + ); +} + +/// With no auth mocked at all, the pinned admin's signature is the only thing +/// that could satisfy `initialize`. +#[test] +#[should_panic] +fn test_initialize_requires_the_configured_admin() { + let f = Fixture::new(); + f.env.set_auths(&[]); + + f.consumer_client.initialize(&f.tholos_id, &f.token_id); +} + +/// The same for the entrypoint that spends the contract's balance: an address +/// that is not the configured admin cannot trigger the self-authorized +/// transfer at all. +#[test] +#[should_panic] +fn test_create_assertion_as_self_requires_the_configured_admin() { + let f = Fixture::new(); + f.initialize_tholos(); + f.initialize_consumer(); + f.fund_consumer(1_000); + + f.env.set_auths(&[]); + + f.consumer_client + .create_assertion_as_self(&f.bond_amount, &true); } #[test] fn test_create_assertion_as_self_fails_against_uninitialized_tholos() { let f = Fixture::new(); - // No initialize() call: Tholos rejects with NotInitialized before ever - // reaching the token transfer, so this doesn't need mocked auths either. + // Consumer ready, but its Tholos instance never initialized. Tholos + // rejects with NotInitialized before ever reaching the token transfer. + f.initialize_consumer(); + assert_eq!( - f.consumer_client.try_create_assertion_as_self( - &f.tholos_id, - &f.token_id, - &f.bond_amount, - &true - ), + f.consumer_client + .try_create_assertion_as_self(&f.bond_amount, &true), Err(Ok(Error::TholosNotInitialized)) ); } @@ -173,17 +224,14 @@ fn test_create_assertion_as_self_fails_against_uninitialized_tholos() { fn test_create_assertion_as_self_fails_when_tholos_paused() { let f = Fixture::new(); f.initialize_tholos(); + f.initialize_consumer(); f.env.mock_all_auths(); f.tholos_client.set_paused(&true); assert_eq!( - f.consumer_client.try_create_assertion_as_self( - &f.tholos_id, - &f.token_id, - &f.bond_amount, - &true - ), + f.consumer_client + .try_create_assertion_as_self(&f.bond_amount, &true), Err(Ok(Error::TholosPaused)) ); } @@ -192,18 +240,17 @@ fn test_create_assertion_as_self_fails_when_tholos_paused() { fn test_create_assertion_as_self_fails_for_invalid_tholos_id() { let f = Fixture::new(); - // An address with no contract registered at all: the call can't even + // An address with no contract registered at all: the call cannot even // reach Tholos's own error handling, so this exercises the // Err(Err(InvokeError)) -> InvalidTholosId path, not Err(Ok(_)). let not_a_tholos_instance = Address::generate(&f.env); + f.initialize_consumer_at(¬_a_tholos_instance, &f.token_id); - let result = f.consumer_client.try_create_assertion_as_self( - ¬_a_tholos_instance, - &f.token_id, - &f.bond_amount, - &true, + assert_eq!( + f.consumer_client + .try_create_assertion_as_self(&f.bond_amount, &true), + Err(Ok(Error::InvalidTholosId)) ); - assert_eq!(result, Err(Ok(Error::InvalidTholosId))); } #[test] diff --git a/contracts/tholos-client/src/lib.rs b/contracts/tholos-client/src/lib.rs index bd398f0..843e0f8 100644 --- a/contracts/tholos-client/src/lib.rs +++ b/contracts/tholos-client/src/lib.rs @@ -30,6 +30,18 @@ pub enum Error { /// No assertion exists under the given id on the Tholos instance at /// `tholos_id`. AssertionNotFound = 4, + /// The consuming contract's own one-time `initialize` has already run. + /// This is the consumer's lifecycle state, not a Tholos-side condition: it + /// lives here rather than in a second error type because the consumer + /// entry points already return this enum, and a duplicate definition is + /// exactly what this crate exists to avoid. Discriminants 1-4 are + /// unchanged. + AlreadyInitialized = 5, + /// The consuming contract has not been initialized yet, so the trusted + /// instance and token addresses it would need do not exist. The admin is + /// pinned by `__constructor`, so in practice only the + /// trusted addresses can be missing. + NotInitialized = 6, } impl Error {