feat(contracts): add rate limits, token whitelist, top-up, and expert cancel#244
Merged
Luluameh merged 1 commit intoMay 31, 2026
Conversation
… cancel Implements LightForgeHub#236–LightForgeHub#239: per-address ledger cooldowns, admin-managed payment token registry, seeker session top-ups, and expert-initiated cancellation with partial refund and stored reason CID.
📝 WalkthroughWalkthroughThis PR introduces admin-managed rate limiting and token whitelisting, expert session cancellation with partial refunds, and seeker top-up capability. New error types, event publishers, and contract methods integrate these features into the session lifecycle with comprehensive test coverage. ChangesAdmin Controls and Session Features
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/lib.rs (1)
4021-4067:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWhitelist
offer_tokenin the swap-backed start path too.Only
ask_tokenis checked right now. The contract still callstransferonoffer_tokenand hands it to the DEX, so this route still accepts an arbitrary token contract despite the new whitelist feature.Suggested fix
if let Err(e) = admin::require_token_whitelisted(&env, &ask_token) { return Err(e); } + if let Err(e) = admin::require_token_whitelisted(&env, &offer_token) { + return Err(e); + } if offer_amount <= 0 { return Err(Error::InvalidAmount); }🤖 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/lib.rs` around lines 4021 - 4067, In start_session_with_swap ensure the offer_token is validated by calling the same whitelist check used for ask_token (i.e., invoke admin::require_token_whitelisted(&env, &offer_token) and propagate Err(e) on failure) before performing the balance check and offer_client.transfer; update the logic in start_session_with_swap so the offer_token whitelist check occurs prior to creating offer_client/transfer to prevent unwhitelisted tokens from being accepted.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@contracts/src/disputes.rs`:
- Around line 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.
- Around line 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.
In `@contracts/src/lib.rs`:
- Around line 2203-2235: top_up_session currently performs token_client.transfer
before enforcing the protocol pause and token-whitelist guards; move or add
guard checks immediately after loading the session and before any balance
transfer so top-ups are blocked when the protocol is paused or the session.token
is not approved. Concretely: in top_up_session (after let mut session =
Self::get_session_or_error...) call the contract's pause guard and token
whitelist guard (the same helper used elsewhere, e.g.
ensure_not_paused/require_not_paused and
ensure_token_whitelisted/require_token_whitelisted or their actual names) and
only then perform token_client.transfer and update session.balance, save and
emit events.
---
Outside diff comments:
In `@contracts/src/lib.rs`:
- Around line 4021-4067: In start_session_with_swap ensure the offer_token is
validated by calling the same whitelist check used for ask_token (i.e., invoke
admin::require_token_whitelisted(&env, &offer_token) and propagate Err(e) on
failure) before performing the balance check and offer_client.transfer; update
the logic in start_session_with_swap so the offer_token whitelist check occurs
prior to creating offer_client/transfer to prevent unwhitelisted tokens from
being accepted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1e280fad-cda4-40f1-8b4c-f2d9ae8bc607
📒 Files selected for processing (5)
contracts/src/admin.rscontracts/src/disputes.rscontracts/src/errors.rscontracts/src/events.rscontracts/src/lib.rs
Comment on lines
+31
to
+36
| 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); | ||
| } |
There was a problem hiding this comment.
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.
Comment on lines
+63
to
+70
| 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; | ||
| } |
There was a problem hiding this comment.
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:
- Transferring dust amounts to a treasury/burn address, or
- Awarding the full balance to one party when both are below minimum, or
- 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.
Comment on lines
+2203
to
+2235
| pub fn top_up_session( | ||
| env: Env, | ||
| seeker: Address, | ||
| session_id: u64, | ||
| amount: i128, | ||
| ) -> Result<i128, Error> { | ||
| seeker.require_auth(); | ||
|
|
||
| if amount <= 0 { | ||
| return Err(Error::InvalidAmount); | ||
| } | ||
|
|
||
| let mut session = Self::get_session_or_error(&env, session_id)?; | ||
|
|
||
| if seeker != session.seeker { | ||
| return Err(Error::Unauthorized); | ||
| } | ||
| if session.status != SessionStatus::Active { | ||
| return Err(Error::SessionNotActive); | ||
| } | ||
|
|
||
| let token_client = token::Client::new(&env, &session.token); | ||
| if token_client.balance(&seeker) < amount { | ||
| return Err(Error::InsufficientBalance); | ||
| } | ||
| token_client.transfer(&seeker, &env.current_contract_address(), &amount); | ||
|
|
||
| session.balance = session.balance.saturating_add(amount); | ||
| let new_balance = session.balance; | ||
| Self::save_session(&env, &session); | ||
|
|
||
| events::publish_top_up(&env, session_id, amount, new_balance); | ||
| Ok(new_balance) |
There was a problem hiding this comment.
Apply the new pause/whitelist guards before accepting a top-up.
Line 2224 transfers fresh escrow without checking whether the protocol is paused or whether session.token is still approved. That lets callers keep funding sessions during an emergency pause and keep using a token after the admin has removed it from the whitelist.
Suggested fix
pub fn top_up_session(
env: Env,
seeker: Address,
session_id: u64,
amount: i128,
) -> Result<i128, Error> {
+ Self::ensure_protocol_active(&env)?;
seeker.require_auth();
if amount <= 0 {
return Err(Error::InvalidAmount);
}
let mut session = Self::get_session_or_error(&env, session_id)?;
+ admin::require_token_whitelisted(&env, &session.token)?;
if seeker != session.seeker {
return Err(Error::Unauthorized);
}
if session.status != SessionStatus::Active {📝 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
| pub fn top_up_session( | |
| env: Env, | |
| seeker: Address, | |
| session_id: u64, | |
| amount: i128, | |
| ) -> Result<i128, Error> { | |
| seeker.require_auth(); | |
| if amount <= 0 { | |
| return Err(Error::InvalidAmount); | |
| } | |
| let mut session = Self::get_session_or_error(&env, session_id)?; | |
| if seeker != session.seeker { | |
| return Err(Error::Unauthorized); | |
| } | |
| if session.status != SessionStatus::Active { | |
| return Err(Error::SessionNotActive); | |
| } | |
| let token_client = token::Client::new(&env, &session.token); | |
| if token_client.balance(&seeker) < amount { | |
| return Err(Error::InsufficientBalance); | |
| } | |
| token_client.transfer(&seeker, &env.current_contract_address(), &amount); | |
| session.balance = session.balance.saturating_add(amount); | |
| let new_balance = session.balance; | |
| Self::save_session(&env, &session); | |
| events::publish_top_up(&env, session_id, amount, new_balance); | |
| Ok(new_balance) | |
| pub fn top_up_session( | |
| env: Env, | |
| seeker: Address, | |
| session_id: u64, | |
| amount: i128, | |
| ) -> Result<i128, Error> { | |
| Self::ensure_protocol_active(&env)?; | |
| seeker.require_auth(); | |
| if amount <= 0 { | |
| return Err(Error::InvalidAmount); | |
| } | |
| let mut session = Self::get_session_or_error(&env, session_id)?; | |
| admin::require_token_whitelisted(&env, &session.token)?; | |
| if seeker != session.seeker { | |
| return Err(Error::Unauthorized); | |
| } | |
| if session.status != SessionStatus::Active { | |
| return Err(Error::SessionNotActive); | |
| } | |
| let token_client = token::Client::new(&env, &session.token); | |
| if token_client.balance(&seeker) < amount { | |
| return Err(Error::InsufficientBalance); | |
| } | |
| token_client.transfer(&seeker, &env.current_contract_address(), &amount); | |
| session.balance = session.balance.saturating_add(amount); | |
| let new_balance = session.balance; | |
| Self::save_session(&env, &session); | |
| events::publish_top_up(&env, session_id, amount, new_balance); | |
| Ok(new_balance) |
🤖 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/lib.rs` around lines 2203 - 2235, top_up_session currently
performs token_client.transfer before enforcing the protocol pause and
token-whitelist guards; move or add guard checks immediately after loading the
session and before any balance transfer so top-ups are blocked when the protocol
is paused or the session.token is not approved. Concretely: in top_up_session
(after let mut session = Self::get_session_or_error...) call the contract's
pause guard and token whitelist guard (the same helper used elsewhere, e.g.
ensure_not_paused/require_not_paused and
ensure_token_whitelisted/require_token_whitelisted or their actual names) and
only then perform token_client.transfer and update session.balance, save and
emit events.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements four session lifecycle and security hardening features for the SkillSphere Soroban contract:
DataKey::LastAction(Address)in temporary storage. Admin-configurable viaset_rate_limit_min_ledgers. Enforced onstart_sessionandheartbeat.Vec<Address>of approved payment tokens.start_session/start_session_with_swapreject unlisted tokens withError::TokenNotWhitelisted.cancel_session(session_id, reason_cid)on their own Active/Paused sessions. Accrued earnings go to the expert, remainder is refunded to the seeker, reason CID is stored, status becomesCancelledByExpert.top_up_session(session_id, amount)on Active sessions to add escrow funds without reopening the session. EmitsTopUpEvent(session/topup).New / changed files
contracts/src/admin.rscontracts/src/disputes.rscontracts/src/events.rscontracts/src/lib.rsSessionStatus::CancelledByExpert, testscontracts/src/errors.rsRateLimitExceeded,TokenNotWhitelisted, etc.New public API
set_rate_limit_min_ledgers(min_ledgers)get_rate_limit_min_ledgers()add_approved_token(token)remove_approved_token(token)is_token_approved(token)get_approved_tokens()cancel_session(expert, session_id, reason_cid)get_session_cancel_reason(session_id)top_up_session(seeker, session_id, amount)New errors
RateLimitExceededTokenNotWhitelistedTokenAlreadyWhitelistedTokenNotInWhitelistMigration notes
add_approved_tokenfor each accepted payment asset before new sessions can start.min_ledgers = 0). Enable via admin when ready.CancelledByExpertis treated as a terminal session state alongsideCompleted,Disputed, andResolved.Test plan
cargo testincontracts/— all existing + new tests passstart_session/heartbeatblocked within cooldown; allowed after ledger gapstart_sessionfails for non-whitelisted token; succeeds afteradd_approved_tokenCancelledByExpert, reason CID storedCloses #236
Closes #237
Closes #238
Closes #239
Summary by CodeRabbit
Release Notes