Soroban zkCrossDex - ZKCross

After lock, the full swapped_amount is transmitted to the administrator immediately. If the cross-chain operation fails or stalls, users lack an on-chain mechanism to recover their funds. The current model depends entirely on off-chain processes and administrator discretion for refunds.

This situation can leave user funds permanently inaccessible if the relayer or destination chain encounters an issue, potentially undermining trust and leading to increased support and liability challenges for operators.

Introduce a pending state with an associated expiry: record lock_hash -> pending(amount, user, expiry). Require the administrator to release the pending transaction before expiry. If not released in time, the user may invoke refund(lock_hash) to recover the funds from the contract balance. Alternatively, implement an explicit refund function for administrators, accompanied by auditable events.

SOLVED: The zkCross team solved this issue in the following commit IDs:

• 8c6a4ef

• 856e9d2

• 7e3cbaf

• 0f0ecc8

• 55dcf30

• 2803f01

The team restructured the flow so user funds remain in the contract until confirmed processing. Specifically: after lock, funds are stored as a pending lock with a 24h TTL; a new atomic process_and_mark_lock moves funds to the admin and simultaneously records ProcessedLocks (only if not expired), preventing double-processing; an admin-auth refund path now returns funds to the user after expiry.

The contract does not include an owner- or administrator-controlled pause function to swiftly halt lock and/or release operations in response to emergencies such as security breaches, pricing anomalies, or integration failures.

This absence could allow continued inflows or outflows during critical incidents, thereby enlarging the incident impact and increasing operational losses before off-chain mitigation measures are implemented.

Add a global paused flag and restrict lock/release operations behind this flag, which can only be controlled by the owner or administrator through pause/unpause functions. Emit PauseEvent and UnpauseEvent to signal these state changes. Optionally, enable selective pausing, such as pausing only the release functionality. Consider implementing multi-signature or time-lock governance mechanisms to regulate pause state transitions.

SOLVED: The zkCross team addressed this issue through the following changes:

• A new Paused flag was added to storage

• New Functions:

  • pause() — an owner-restricted function implemented to halt operations

  • unpause() — an owner-restricted function implemented to resume operations

• Integration: Both lock() and release() now verify the paused state prior to execution

set_fee_percentage permits setting percentage to zero. In lock, min_amount is calculated as ceil(10_000 / fee_bps). If fee_bps == 0, this division causes a panic, rendering the lock function unusable for all users until the fee is modified by the owner.

This situation could lead to a complete denial of service for the lock function if the fee is inadvertently or maliciously set to zero.

Code Location

Code of the set_fee_percentage function:

pub fn set_fee_percentage(env: Env, percentage: u32) {
    let owner: Address = env.storage().instance().get(&DataKey::Owner).unwrap();
    owner.require_auth();
    
    if percentage > 2000 {
        env.panic_with_error(Error::from_type_and_code(
            ScErrorType::Contract,
            ScErrorCode::InvalidAction,
        ));
    }
    env.storage().instance().set(&DataKey::FeePercentage, &percentage);
    env.events().publish(("set_fee_percentage_event",), percentage);
}

Code snippet from the lock function:

let fee_bps: u32 = env
    .storage()
    .instance()
    .get(&DataKey::FeePercentage)
    .unwrap_or(30);

// 1) Compute raw fee in stroops
//    fee = floor(in_amount * fee_bps / 10_000)
let fee = in_amount
    .checked_mul(fee_bps as i128)
    .expect("overflow")
    / 10_000;

// 2) Enforce a minimum in_amount so that fee >= 1 stroop:
//    We need in_amount * fee_bps / 10_000 >= 1
//    ⟹ in_amount >= ceil(10_000 / fee_bps)
let min_amount: i128 = {
    let numerator = 10_000i128 + fee_bps as i128 - 1;
    numerator / (fee_bps as i128)
};

Disallow percentage == 0 by enforcing a valid range 1..=2000, or handle the specific case when percentage is zero to skip min_amount calculation and set fee = 0 safely.

SOLVED: The zkCross team solved this issue by handling the case where percentage equals zero; the min_amount calculation was skipped and fee = 0 was applied safely.

The contract hardcodes a minimum USDC input check of 2_000_000, implicitly assuming 6 decimal places ("2 USDC"). However, on Stellar, issued assets such as USDC are represented with 7 decimal places; therefore, 2_000_000 corresponds to only 0.2 USDC, not 2.0 USDC. As a result, the intended minimum threshold is ineffective on Stellar.

This discrepancy could lead to acceptance of USDC amounts below the intended minimum (e.g., 0.2 USDC instead of 2.0), thereby undermining the intended economic protections.

Retrieve the token's decimals metadata from the token contract and calculate the minimum amount dynamically.

SOLVED: The zkCross team solved this issue through setting the minimum locked amount for USDC to 20,000,000 stroops, corresponding to 2 USDC on the Stellar network.

AccumulatedRevenue is initialized to 0 in set_revenue_address but is neither updated nor accessed thereafter. Since the fee is computed within the lock function, this key should be incremented by the fee on each invocation to enable proper on-chain accounting and support deferred sweeping when Revenue is not yet configured. Currently, fees are transferred directly to revenue_address (if set) or remain stranded within the contract without a ledger of what is owed.

This structure risks missing on-chain records of collected fees, complicating reconciliation and obscuring stranded balances when Revenue is unset.

Either remove AccumulatedRevenue entirely, or implement it correctly:

• Increment an accumulated counter during lock by the computed fee.

• If Revenue is unset, record the fee in the accumulator instead of silently discarding it; when Revenue is set, allow an owner- or admin-only sweep_accumulated_revenue(token) function that transfers the accumulated amount to revenue_address and resets the counter.

• Prefer per-token accounting (e.g., AccumulatedRevenue(Address)) keyed by token to prevent mixing assets within a single i128.

• Emit events upon accumulation and sweep actions to facilitate auditability and off-chain reconciliation.

SOLVED: The zkCross team solved this issue by Implementing an accurate Per-Token revenue tracking:

• Data Structure: Introduced AccumulatedRevenue(address) mapping, keyed by token address, to store collected fees per token.

• Functionality:

  • Revenue is now accurately tracked per token during lock() operations.

  • Added sweep_accumulated_revenue(token) function, enabling the contract owner to withdraw accumulated fees for a specific token.

  • Emitted relevant events to ensure transparency and support auditability.

The set_revenue_address() function is permanently disabled after its initial execution through the RevenueSet flag. If the initially configured revenue account is lost, compromised, or requires rotation for operational reasons, the contract does not provide a mechanism to update it.

This limitation could lead to irrevocable loss of future protocol revenue or result in ransom scenarios if the associated key is compromised.

Code Location

Code snippet from the set_revenue_address() function:

pub fn set_revenue_address(env: Env, revenue_address: Address) {
    if env.storage().instance().has(&DataKey::RevenueSet) {
        env.panic_with_error(Error::from_type_and_code(
            ScErrorType::Contract,
            ScErrorCode::InvalidAction,
        ));
    }
    // ...
}

Replace the existing one-time pattern with an owner-only update_revenue_address() function that emits an event and enforces explicit authentication.

SOLVED: The zkCross team solved this issue in the specified commit ID.

After each successful release, the contract records a LockHashStatus { processed: true } entry under the corresponding lock-hash key. If these markers are retained indefinitely, the storage set will grow unboundedly, leading to increased rent costs that the administrator must continually cover. Conversely, manual removal of these entries via remove_old_lock_hash reuses the same identifiers, which compromises the bridge’s critical replay protection.

This vulnerability could lead to double-release of funds if a deleted hash is replayed, or result in a denial-of-service condition if rising rent costs cause the contract to be evicted due to insufficient balance replenishment by the administrator.

Deprecate remove_old_lock_hash in favor of setting a conservative automatic TTL (e.g., 90 days) when a hash is marked processed. This approach relies on Soroban's expiry mechanism to garbage-collect the hash after the replay window has safely closed. Maintain the extend_lock_hash_ttl helper (admin-only) to allow operators to extend the TTL of individual hashes if downstream finality requires additional time. Emit an event on any TTL modification to enhance auditability.

SOLVED: The zkCross team solved this issue in the specified commit ID.

Both lock and release functions perform external calls to token contracts:

• lock transfers tokens from the user to the contract, then to the administrator, and emits a LockEvent.

• release transfers tokens from the administrator to the user, then marks lock_hash as processed and emits a ReleaseEvent.

These functions follow a check-effects-interactions pattern, which could, in theory, be vulnerable to reentrancy if the token contract is malicious or exotic. However, lock requires user_address.require_auth() and release requires admin.require_auth(), ensuring that reentrant calls triggered by token contracts cannot satisfy these authorization checks to re-enter state-changing functions.

This design may lead to confusing event ordering or unexpected control flow when interacting with non-standard tokens, but it effectively prevents double-locking or double-releasing under normal circumstances, due to the authorization constraints enforced for users and administrators.

As part of defense-in-depth measures: (1) update the state before external calls where feasible (for example, set processed=true prior to executing the transfer in release, relying on transaction atomicity), (2) consider adding a simple reentrancy guard as an optional safeguard.

SOLVED: The zkCross team solved this issue in the specified commit ID.

In lock, both from_token: Address and src_token: Address refer to Stellar addresses. This redundancy for the source-side lock can lead to confusion. Allowing both fields to be specified enables users to provide conflicting values, which do not affect on-chain enforcement but may cause issues for indexers and downstream systems.

Remove one of the redundant parameters, src_token or from_token, and utilize only the remaining parameter to represent the source token address throughout the function.

SOLVED: The zkCross team solved this issue by removing the redundant src_token parameter.

Event names exhibit inconsistency: some utilize CamelCase with an Event suffix (e.g., AdminSetEvent, AdminUpdatedEvent, RevenueAddressSetEvent, UsdcTokenAddressSetEvent), while others do not (initialize) or employ snake_case (set_fee_percentage_event). Off-chain indexers and analytics systems typically depend on strict naming conventions to function correctly.

This inconsistency may lead to missed event subscriptions, unreliable queries, or monitoring blind spots when tools expect uniform naming patterns.

Standardize naming conventions by adopting a consistent approach. For example, rename events to InitializeEvent and SetFeePercentageEvent. Ensure these names remain stable in future implementations.

SOLVED: The zkCross team solved this issue in the specified commit ID.

In lock, the code calculates fee before verifying that in_amount >= min_amount. Although the impact is minimal, performing this arithmetic prior to validation may lead to unnecessary computations and slightly reduce code readability. This order also obscures the original validation intent.

This could result in inefficient execution and reduced clarity, although it does not introduce any security vulnerabilities.

Perform the min_amount validation first, then compute fee and swapped_amount. Ensure the validation process remains efficient and free of unnecessary calculations.

SOLVED: The zkCross team solved this issue in the specified commit ID.

When constructing LockEventData, several fields clone their values (e.g., dest_token.clone(), from_token.clone(), src_token.clone()), which could often be replaced with field-init shorthand if these original values are not used elsewhere. Additionally, swapped_amount: swapped_amount redundantly repeats the field name and could be simplified to swapped_amount using the shorthand.

This approach may lead to minor, avoidable CPU and memory overhead, and adds unnecessary noise to the code without providing any functional benefit.

When ownership permits, assign values directly and utilize field-initializer shorthand; clone only when necessary. Apply these practices consistently in event construction to enhance clarity and enable minor optimizations.

SOLVED: The zkCross team solved this issue in the specified commit ID.