Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions contracts/src/admin.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
//! Admin-managed configuration: rate-limit cooldowns (#236) and the
//! approved-token registry (#239).

use soroban_sdk::{Address, Env, Vec};

use crate::{DataKey, Error};

/// Default rate-limit cooldown: 0 disables per-address throttling.
const DEFAULT_RATE_LIMIT_MIN_LEDGERS: u32 = 0;

/// Reads the configured minimum ledger gap between rate-limited calls.
pub fn rate_limit_min_ledgers(env: &Env) -> u32 {
env.storage()
.instance()
.get(&DataKey::RateLimitMinLedgers)
.unwrap_or(DEFAULT_RATE_LIMIT_MIN_LEDGERS)
}

/// Persists the minimum ledger gap between rate-limited calls.
pub fn set_rate_limit_min_ledgers(env: &Env, min_ledgers: u32) {
env.storage()
.instance()
.set(&DataKey::RateLimitMinLedgers, &min_ledgers);
}

/// Enforces a per-address cooldown of at least `min_ledgers` ledgers
/// between successive calls. The last action ledger is stored under
/// `DataKey::LastAction(address)` in **temporary** storage so it
/// auto-expires and does not accumulate rent.
pub fn rate_limit(env: &Env, caller: &Address, min_ledgers: u32) -> Result<(), Error> {
if min_ledgers == 0 {
return Ok(());
}

let key = DataKey::LastAction(caller.clone());
let current = env.ledger().sequence();

if let Some(last) = env
.storage()
.temporary()
.get::<DataKey, u32>(&key)
{
if current.saturating_sub(last) < min_ledgers {
return Err(Error::RateLimitExceeded);
}
}

env.storage().temporary().set(&key, &current);

// Keep the tombstone alive through the cooldown window.
let extend_to = min_ledgers.saturating_add(10);
env.storage()
.temporary()
.extend_ttl(&key, min_ledgers, extend_to);

Ok(())
}

/// Returns the admin-maintained list of approved payment tokens.
pub fn approved_tokens(env: &Env) -> Vec<Address> {
env.storage()
.persistent()
.get(&DataKey::ApprovedTokens)
.unwrap_or_else(|| Vec::new(env))
}

fn save_approved_tokens(env: &Env, tokens: &Vec<Address>) {
env.storage()
.persistent()
.set(&DataKey::ApprovedTokens, tokens);
}

/// Returns `true` when `token` is present in the approved-token registry.
pub fn is_token_whitelisted(env: &Env, token: &Address) -> bool {
let tokens = approved_tokens(env);
for i in 0..tokens.len() {
if tokens.get(i).unwrap() == *token {
return true;
}
}
false
}

/// Rejects `token` unless it appears in the approved-token registry.
pub fn require_token_whitelisted(env: &Env, token: &Address) -> Result<(), Error> {
if is_token_whitelisted(env, token) {
Ok(())
} else {
Err(Error::TokenNotWhitelisted)
}
}

/// Appends `token` to the approved-token registry.
pub fn add_approved_token(env: &Env, token: Address) -> Result<(), Error> {
let mut tokens = approved_tokens(env);
for i in 0..tokens.len() {
if tokens.get(i).unwrap() == token {
return Err(Error::TokenAlreadyWhitelisted);
}
}
tokens.push_back(token);
save_approved_tokens(env, &tokens);
Ok(())
}

/// Removes `token` from the approved-token registry.
pub fn remove_approved_token(env: &Env, token: Address) -> Result<(), Error> {
let tokens = approved_tokens(env);
let mut next = Vec::new(env);
let mut found = false;
for i in 0..tokens.len() {
let entry = tokens.get(i).unwrap();
if entry == token {
found = true;
} else {
next.push_back(entry);
}
}
if !found {
return Err(Error::TokenNotInWhitelist);
}
save_approved_tokens(env, &next);
Ok(())
}
98 changes: 98 additions & 0 deletions contracts/src/disputes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
//! Expert-initiated session cancellation with partial refund (#238).

use soroban_sdk::{token, Address, Env, String};

use crate::{
events, DataKey, Error, SessionStatus, SkillSphereContract, MIN_SESSION_ESCROW,
};

/// Cancels an active or paused session on behalf of the expert.
///
/// Accrued (claimable) earnings are paid to the expert; the remaining
/// escrow balance is refunded to the seeker. The cancellation reason
/// CID is stored for transparency and the session status becomes
/// `CancelledByExpert`.
pub fn cancel_session_by_expert(
env: &Env,
expert: Address,
session_id: u64,
reason_cid: String,
) -> Result<(i128, i128), Error> {
SkillSphereContract::assert_not_locked(env)?;
SkillSphereContract::set_reentrancy_lock(env, true);

expert.require_auth();

if !SkillSphereContract::is_valid_ipfs_cid(&reason_cid) {
SkillSphereContract::set_reentrancy_lock(env, false);
return Err(Error::InvalidCid);
}

let mut session = SkillSphereContract::get_session_or_error(env, session_id)?;

if expert != session.expert {
SkillSphereContract::set_reentrancy_lock(env, false);
return Err(Error::Unauthorized);
}
Comment on lines +31 to +36
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Reentrancy lock not released when get_session_or_error fails.

If get_session_or_error returns an error, the ? operator propagates immediately without calling set_reentrancy_lock(env, false). This leaves the contract in a locked state, blocking all subsequent operations that check reentrancy.

🐛 Proposed fix
-    let mut session = SkillSphereContract::get_session_or_error(env, session_id)?;
+    let mut session = match SkillSphereContract::get_session_or_error(env, session_id) {
+        Ok(s) => s,
+        Err(e) => {
+            SkillSphereContract::set_reentrancy_lock(env, false);
+            return Err(e);
+        }
+    };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let mut session = SkillSphereContract::get_session_or_error(env, session_id)?;
if expert != session.expert {
SkillSphereContract::set_reentrancy_lock(env, false);
return Err(Error::Unauthorized);
}
let mut session = match SkillSphereContract::get_session_or_error(env, session_id) {
Ok(s) => s,
Err(e) => {
SkillSphereContract::set_reentrancy_lock(env, false);
return Err(e);
}
};
if expert != session.expert {
SkillSphereContract::set_reentrancy_lock(env, false);
return Err(Error::Unauthorized);
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/src/disputes.rs` around lines 31 - 36, The reentrancy lock may
remain set if SkillSphereContract::get_session_or_error(env, session_id) returns
an error because the `?` returns before calling
SkillSphereContract::set_reentrancy_lock(env, false); change the flow to ensure
the lock is always cleared on early return: call get_session_or_error and handle
its Result explicitly (e.g., match or map_err) so that on Err you first call
SkillSphereContract::set_reentrancy_lock(env, false) and then return the error;
alternatively implement a small RAII guard that calls set_reentrancy_lock(env,
false) in Drop and use it around this function, referencing
get_session_or_error, set_reentrancy_lock, SkillSphereContract, session_id and
expert to locate the code.


if !matches!(
session.status,
SessionStatus::Active | SessionStatus::Paused
) {
SkillSphereContract::set_reentrancy_lock(env, false);
return Err(Error::InvalidSessionState);
}

let now = env.ledger().timestamp();
let effective_time = SkillSphereContract::bounded_time(&session, now);
let claimable = SkillSphereContract::claimable_amount_for_session(&session, effective_time);
let remaining = session.balance.saturating_sub(claimable);

session.balance = 0;
session.accrued_amount = 0;
session.last_settlement_timestamp = effective_time as u32;
session.status = SessionStatus::CancelledByExpert;
SkillSphereContract::save_session(env, &session);

env.storage()
.persistent()
.set(&DataKey::SessionCancelReason(session_id), &reason_cid);

let token_client = token::Client::new(env, &session.token);

let mut expert_payout = claimable;
let mut seeker_refund = remaining;
if expert_payout < MIN_SESSION_ESCROW {
expert_payout = 0;
}
if seeker_refund < MIN_SESSION_ESCROW {
seeker_refund = 0;
}
Comment on lines +63 to +70
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Potential fund lock when both payouts fall below MIN_SESSION_ESCROW.

If both claimable and remaining are below MIN_SESSION_ESCROW, both payouts are zeroed but session.balance was already set to 0 on line 51. These funds become permanently locked in the contract with no recovery path.

Consider either:

  1. Transferring dust amounts to a treasury/burn address, or
  2. Awarding the full balance to one party when both are below minimum, or
  3. Only applying the threshold when the individual amount is non-zero but below minimum
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@contracts/src/disputes.rs` around lines 63 - 70, The current logic zeroes
both expert_payout and seeker_refund when each is below MIN_SESSION_ESCROW,
risking locked funds because session.balance was already set to 0; update the
payout logic (around expert_payout, seeker_refund, MIN_SESSION_ESCROW and
session.balance) to handle the “both below threshold” case explicitly — e.g., if
expert_payout < MIN_SESSION_ESCROW and seeker_refund < MIN_SESSION_ESCROW and
session.balance > 0 then route the leftover (session.balance or sum of
claimable+remaining) to a recovery destination (award full balance to one party
or transfer to a treasury/burn address) instead of leaving both zero, or
alternatively only apply the threshold to an amount that is non-zero so at least
one party receives the funds. Ensure the chosen branch updates session.balance
and emits the appropriate transfer to avoid locked funds.


if expert_payout > 0 {
token_client.transfer(
&env.current_contract_address(),
&session.expert,
&expert_payout,
);
}
if seeker_refund > 0 {
token_client.transfer(
&env.current_contract_address(),
&session.seeker,
&seeker_refund,
);
}

events::publish_expert_cancel(
env,
session_id,
expert,
expert_payout,
seeker_refund,
reason_cid,
);

SkillSphereContract::set_reentrancy_lock(env, false);
Ok((expert_payout, seeker_refund))
}
6 changes: 6 additions & 0 deletions contracts/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,10 @@ pub enum Error {
SessionFrozen = 48,
SwapFailed = 49,

// #236 / #239 / #237 / #238
RateLimitExceeded = 50,
TokenNotWhitelisted = 51,
TokenAlreadyWhitelisted = 52,
TokenNotInWhitelist = 53,

}
32 changes: 32 additions & 0 deletions contracts/src/events.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//! Centralised event publishing helpers for session lifecycle events.

use soroban_sdk::{symbol_short, Env};

/// Emitted when a seeker tops up an active session escrow (#237).
pub fn publish_top_up(env: &Env, session_id: u64, amount: i128, new_balance: i128) {
env.events().publish(
(symbol_short!("session"), symbol_short!("topup")),
(session_id, amount, new_balance),
);
}

/// Emitted when an expert cancels their session (#238).
pub fn publish_expert_cancel(
env: &Env,
session_id: u64,
expert: soroban_sdk::Address,
expert_payout: i128,
seeker_refund: i128,
reason_cid: soroban_sdk::String,
) {
env.events().publish(
(symbol_short!("session"), symbol_short!("expcncl")),
(
session_id,
expert,
expert_payout,
seeker_refund,
reason_cid,
),
);
}
Loading
Loading