Skip to content
Open
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
49 changes: 34 additions & 15 deletions contracts/payment-vault-contract/src/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,23 @@ pub fn unpause(env: &Env) -> Result<(), VaultError> {
Ok(())
}

pub fn set_fee(env: &Env, new_fee_bps: u32) -> Result<(), VaultError> {
let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?;
admin.require_auth();
if new_fee_bps > 2000 {
return Err(VaultError::FeeTooHigh);
}
storage::set_fee_bps(env, new_fee_bps);
Ok(())
}

pub fn set_treasury(env: &Env, treasury: &Address) -> Result<(), VaultError> {
let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?;
admin.require_auth();
storage::set_treasury(env, treasury);
Ok(())
}

pub fn set_my_rate(env: &Env, expert: &Address, rate_per_second: i128) -> Result<(), VaultError> {
expert.require_auth();

Expand Down Expand Up @@ -221,32 +238,34 @@ pub fn finalize_session(
return Err(VaultError::BookingNotPending);
}

// 4. Calculate payments.
// rate_per_second is stored in atomic units of the payment token, so this
// multiplication is safe for any token precision as long as the product fits i128.
let expert_pay = booking
.rate_per_second
.checked_mul(actual_duration as i128)
.ok_or(VaultError::Overflow)?;
let refund = booking.total_deposit - expert_pay;
// 4. Calculate payments
let gross_expert_pay = booking.rate_per_second * (actual_duration as i128);
let refund = booking.total_deposit - gross_expert_pay;

// Ensure calculations are valid
if expert_pay < 0 || refund < 0 {
if gross_expert_pay < 0 || refund < 0 {
return Err(VaultError::InvalidAmount);
}

let fee_bps = storage::get_fee_bps(env);
let fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000;
let expert_net_pay = gross_expert_pay - fee_amount;
Comment on lines +241 to +251
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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== finalize_session settlement math ==="
sed -n '241,252p' contracts/payment-vault-contract/src/contract.rs

echo
echo "=== overflow-check settings in Cargo profiles ==="
fd -HI 'Cargo.toml|config.toml' . -t f | while read -r file; do
  matches=$(rg -n '^\[profile\.release\]|overflow-checks' "$file" || true)
  if [ -n "$matches" ]; then
    echo "--- $file ---"
    printf '%s\n' "$matches"
  fi
done

Repository: LightForgeHub/SkillSphere-Contracts

Length of output: 1187


Use checked arithmetic for graceful error handling in finalize_session.

Lines 242–250 use raw i128 arithmetic. While the workspace enables overflow-checks = true in all Cargo profiles (which causes panics on overflow rather than silent wrapping), it's better to handle potential overflows explicitly and return VaultError::Overflow instead of crashing. This improves user experience and gives the contract control over error handling.

Proposed fix
     // 4. Calculate payments
-    let gross_expert_pay = booking.rate_per_second * (actual_duration as i128);
-    let refund = booking.total_deposit - gross_expert_pay;
+    let gross_expert_pay = booking
+        .rate_per_second
+        .checked_mul(actual_duration as i128)
+        .ok_or(VaultError::Overflow)?;
+    let refund = booking
+        .total_deposit
+        .checked_sub(gross_expert_pay)
+        .ok_or(VaultError::Overflow)?;
 
-    if gross_expert_pay < 0 || refund < 0 {
+    if refund < 0 {
         return Err(VaultError::InvalidAmount);
     }
 
     let fee_bps = storage::get_fee_bps(env);
-    let fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000;
-    let expert_net_pay = gross_expert_pay - fee_amount;
+    let fee_amount = gross_expert_pay
+        .checked_mul(fee_bps as i128)
+        .ok_or(VaultError::Overflow)?
+        / 10_000;
+    let expert_net_pay = gross_expert_pay
+        .checked_sub(fee_amount)
+        .ok_or(VaultError::Overflow)?;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/payment-vault-contract/src/contract.rs` around lines 241 - 251, In
finalize_session replace raw i128 math for gross_expert_pay, refund, fee_amount
and expert_net_pay with checked arithmetic (checked_mul, checked_sub,
checked_add, checked_div where appropriate) and propagate any None to
Err(VaultError::Overflow); specifically compute gross_expert_pay using
checked_mul, compute refund via checked_sub on booking.total_deposit and
gross_expert_pay, compute fee_amount by converting fee_bps to i128 and using
checked_mul followed by checked_div (or checked_mul then checked_div with
checks), then compute expert_net_pay via checked_sub, and return
Err(VaultError::Overflow) whenever a checked_* yields None instead of allowing a
panic.


// 5. Get token contract
let token_address = storage::get_token(env);
let token_client = token::Client::new(env, &token_address);
let contract_address = env.current_contract_address();

// 6. Execute transfers
// Pay expert
if expert_pay > 0 {
token_client.transfer(&contract_address, &booking.expert, &expert_pay);
if fee_amount > 0 {
if let Some(treasury) = storage::get_treasury(env) {
token_client.transfer(&contract_address, &treasury, &fee_amount);
}
}
Comment on lines +259 to +263
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

Potential issue: Fee amount may be silently retained in contract when treasury is not set.

When fee_amount > 0 but get_treasury() returns None, the fee is deducted from the expert's pay but never transferred out. These tokens will remain locked in the contract with no mechanism to retrieve them.

Consider one of these approaches:

  1. Skip fee deduction if treasury is not set (treat as 0% fee)
  2. Require treasury to be set before allowing non-zero fee
  3. Fail the transaction if fee would be collected but treasury is missing
🔧 Option 1: Skip fee if no treasury (recommended)
     let fee_bps = storage::get_fee_bps(env);
-    let fee_amount = (gross_expert_pay * fee_bps as i128) / 10_000;
-    let expert_net_pay = gross_expert_pay - fee_amount;
+    let treasury_opt = storage::get_treasury(env);
+    let fee_amount = if treasury_opt.is_some() {
+        (gross_expert_pay * fee_bps as i128) / 10_000
+    } else {
+        0
+    };
+    let expert_net_pay = gross_expert_pay - fee_amount;
 
     // ...
 
     if fee_amount > 0 {
-        if let Some(treasury) = storage::get_treasury(env) {
+        if let Some(treasury) = treasury_opt {
             token_client.transfer(&contract_address, &treasury, &fee_amount);
         }
     }
🔧 Option 2: Require treasury when fee is set

In set_fee, enforce that treasury must already be configured:

 pub fn set_fee(env: &Env, new_fee_bps: u32) -> Result<(), VaultError> {
     let admin = storage::get_admin(env).ok_or(VaultError::NotInitialized)?;
     admin.require_auth();
     if new_fee_bps > 2000 {
         return Err(VaultError::FeeTooHigh);
     }
+    if new_fee_bps > 0 && storage::get_treasury(env).is_none() {
+        return Err(VaultError::TreasuryNotSet); // New error variant needed
+    }
     storage::set_fee_bps(env, new_fee_bps);
     Ok(())
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@contracts/payment-vault-contract/src/contract.rs` around lines 174 - 178, The
code currently deducts fee_amount but only transfers it when
storage::get_treasury(env) returns Some, causing tokens to remain in the
contract if treasury is None; update the payment flow in the function that
computes and applies fee (where fee_amount, contract_address and
token_client.transfer are used) to handle the None case: if
storage::get_treasury(env) is None then treat fee_amount as zero (i.e., do not
deduct from the expert payout and do not call token_client.transfer), otherwise
proceed with the transfer to the treasury; alternatively (if you prefer stricter
behavior) enforce in set_fee that a treasury must be configured before allowing
non-zero fees by validating in set_fee and returning an error when treasury is
missing.


if expert_net_pay > 0 {
token_client.transfer(&contract_address, &booking.expert, &expert_net_pay);
}

// Refund user
if refund > 0 {
token_client.transfer(&contract_address, &booking.user, &refund);
}
Expand All @@ -255,7 +274,7 @@ pub fn finalize_session(
storage::update_booking_status(env, booking_id, BookingStatus::Complete);

// 8. Emit SessionFinalized event
events::session_finalized(env, booking_id, actual_duration, expert_pay);
events::session_finalized(env, booking_id, actual_duration, expert_net_pay, fee_amount);

Ok(())
}
Expand Down
1 change: 1 addition & 0 deletions contracts/payment-vault-contract/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ pub enum VaultError {
ExpertNotVerified = 10,
SessionAlreadyStarted = 11,
Overflow = 12,
FeeTooHigh = 13,
BookingNotDisputed = 13,
RemainderAlreadyRecovered = 14,
}
4 changes: 2 additions & 2 deletions contracts/payment-vault-contract/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,9 @@ pub fn booking_created(
}

/// Emitted when a session is finalized
pub fn session_finalized(env: &Env, booking_id: u64, actual_duration: u64, total_cost: i128) {
pub fn session_finalized(env: &Env, booking_id: u64, actual_duration: u64, expert_pay: i128, fee_amount: i128) {
let topics = (symbol_short!("finalized"), booking_id);
env.events().publish(topics, (actual_duration, total_cost));
env.events().publish(topics, (actual_duration, expert_pay, fee_amount));
}

pub fn session_reclaimed(env: &Env, booking_id: u64, amount: i128) {
Expand Down
8 changes: 8 additions & 0 deletions contracts/payment-vault-contract/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@ impl PaymentVaultContract {
contract::unpause(&env)
}

pub fn set_fee(env: Env, new_fee_bps: u32) -> Result<(), VaultError> {
contract::set_fee(&env, new_fee_bps)
}

pub fn set_treasury(env: Env, treasury: Address) -> Result<(), VaultError> {
contract::set_treasury(&env, &treasury)
}

/// Transfer admin rights to a new address (Admin-only)
/// Old admin instantly loses all privileges
pub fn transfer_admin(env: Env, new_admin: Address) -> Result<(), VaultError> {
Expand Down
23 changes: 23 additions & 0 deletions contracts/payment-vault-contract/src/storage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ pub enum DataKey {
BookingCounter, // Counter for generating unique booking IDs
UserBookings(Address), // User Address -> Vec<u64> of booking IDs
ExpertBookings(Address), // Expert Address -> Vec<u64> of booking IDs
FeeBps,
Treasury,
IsPaused, // Circuit breaker flag
// ── Indexed User Booking List ──────────────────────────────────────────
// Replaces the old Vec<u64> approach with O(1) per-write composite keys.
Expand Down Expand Up @@ -231,3 +233,24 @@ pub fn get_expert_rate(env: &Env, expert: &Address) -> Option<i128> {
.persistent()
.get(&DataKey::ExpertRate(expert.clone()))
}

// --- Fee BPS ---
pub fn set_fee_bps(env: &Env, fee_bps: u32) {
env.storage().instance().set(&DataKey::FeeBps, &fee_bps);
}

pub fn get_fee_bps(env: &Env) -> u32 {
env.storage()
.instance()
.get(&DataKey::FeeBps)
.unwrap_or(0)
}

// --- Treasury ---
pub fn set_treasury(env: &Env, treasury: &Address) {
env.storage().instance().set(&DataKey::Treasury, treasury);
}

pub fn get_treasury(env: &Env) -> Option<Address> {
env.storage().instance().get(&DataKey::Treasury)
}
Loading
Loading