Integer Division Rounding Error in Fee Distribution#80
Integer Division Rounding Error in Fee Distribution#80caxtonacollins wants to merge 28 commits intoTrustless-Work:single-release-developfrom
Conversation
…hance validation checks
…d error for non-existent milestones
…d validation checks
…idation functions
…nd enhance validation
…r non-existent milestones
… enhance validation
…ation and improve error handling
… fee distribution
📝 WalkthroughWalkthroughPlatform-authorized deployment added; observers persisted; milestone APIs converted to batch operations; dispute/withdraw flows refactored to precompute per-recipient fees and batch net transfers (safe arithmetic); new validators, errors, helper functions, and tests added. Changes
Sequence Diagram(s)sequenceDiagram
participant Resolver as Dispute Resolver
participant Contract as Escrow Contract
participant Validator as Validators
participant Storage as Escrow Storage
participant Token as Token Client
Resolver->>Contract: withdraw_remaining_funds(distributions, trustless_addr, resolver)
activate Contract
Contract->>Validator: validate_distributions_size(distributions)
Validator-->>Contract: Ok / Err
Contract->>Storage: load escrow
Storage-->>Contract: escrow
Contract->>Validator: validate_withdraw_remaining_funds_conditions(escrow, resolver, all_processed, balance, total)
Validator-->>Contract: Ok / Err
Note over Contract: compute per-recipient fee shares (safe_mul/div), accumulate actual_trustless/platform fees, build net_transfers Vec
Contract->>Token: transfer(trustless_work_address, actual_trustless_fees) (if >0)
Token-->>Contract: Ok
Contract->>Token: transfer(platform_address, actual_platform_fees) (if >0)
Token-->>Contract: Ok
loop for each (recipient, net_amount)
Contract->>Token: transfer(recipient, net_amount)
Token-->>Contract: Ok
end
Contract->>Storage: persist escrow (flags/balances)
Storage-->>Contract: Ok
Contract-->>Resolver: Ok
deactivate Contract
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 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 |
|
@armandocodecr please review, if there is anything please reachout. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/escrow/src/storage/types.rs (1)
37-45: Storage schema change risk: addingRoles.observerscan break deserialization of existing Escrow data.If this contract is already deployed with stored
Escrowentries, adding a field toRoleswill make older ledger data incompatible. Please add a migration/versioning strategy (e.g., a versionedEscrowV2or separate storage for observers) before release.
🧹 Nitpick comments (4)
contracts/escrow/src/core/dispute.rs (2)
64-67: Redundant check after validation.The
amount <= 0check inside the loop (lines 64-67) is redundant since the earlier validation loop (lines 43-47) already returns an error for non-positive amounts. However, usingcontinuehere is harmless and provides defense-in-depth.
58-108: Consider extracting shared fee distribution logic.The fee calculation and distribution logic in
withdraw_remaining_funds(lines 58-108) andresolve_dispute(lines 145-196) is nearly identical. Consider extracting this into a shared helper function to reduce duplication and ensure consistent behavior.♻️ Example helper signature
fn distribute_with_fees( e: &Env, token_client: &TokenClient, contract_address: &Address, trustless_work_address: &Address, platform_address: &Address, distributions: &Map<Address, i128>, total: i128, fee_result: &FeeResult, ) -> Result<(), ContractError>Also applies to: 145-196
contracts/escrow/src/core/milestone.rs (1)
35-38: Consider usingok_orfor defensive error handling.While the
.unwrap()on line 38 should be safe after index validation, using.ok_or(ContractError::...)would be more defensive and consistent with the validation pattern.♻️ Suggested defensive pattern
- let mut milestone_to_update = existing_escrow - .milestones - .get(idx) - .unwrap(); + let mut milestone_to_update = existing_escrow + .milestones + .get(idx) + .ok_or(ContractError::MilestoneToUpdateDoesNotExist)?;contracts/escrow/src/contract.rs (1)
178-191: Missing event publication forwithdraw_remaining_funds.This function does not publish an event after completing the withdrawal, which is inconsistent with other state-changing operations in this contract:
resolve_disputepublishesDisputeResolvedfund_escrowpublishesFundEscdispute_escrowpublishesEscrowDisputedFor auditability and observability of fund movements, consider adding a corresponding event.
♻️ Proposed fix to add event publication
Add a new event type (e.g.,
FundsWithdrawn) to your events module, then:pub fn withdraw_remaining_funds( e: Env, dispute_resolver: Address, trustless_work_address: Address, distributions: Map<Address, i128>, ) -> Result<(), ContractError> { - DisputeManager::withdraw_remaining_funds( + let escrow = DisputeManager::withdraw_remaining_funds( &e, dispute_resolver, trustless_work_address, distributions, )?; + // Publish appropriate event here, e.g.: + // FundsWithdrawn { escrow }.publish(&e); Ok(()) }
|
@armandocodecr pls review and merge |
|
@armandocodecr GM man, please review and merge, will appreciate some compliments |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@contracts/escrow/src/core/dispute.rs`:
- Around line 1-7: The code treats flags.disputed as a terminal processed state
(all_processed = true), allowing withdraw_remaining_funds to run while an active
dispute exists; change the logic in the place that sets all_processed (and/or
the validator validate_withdraw_remaining_funds_conditions) so that
all_processed is true only for terminal states like flags.released or
flags.resolved and explicitly disallow withdraw_remaining_funds when
flags.disputed is set (i.e., require !flags.disputed or require
flags.resolved/released before withdraw_remaining_funds/resolve_dispute). Also
fix the fee rounding leak: instead of computing per-recipient fees using integer
division that loses remainder ((amount * total_fee) / total) for each recipient,
compute per-recipient fees and track allocated_sum, then compute remainder =
total_fee - allocated_sum and add the remainder to one recipient (e.g., the
largest share or the resolver) so the sum of per-recipient fees equals the
previously-calculated total_fee; adjust symbols referenced: all_processed,
flags.released, flags.resolved, flags.disputed, withdraw_remaining_funds,
resolve_dispute, validate_withdraw_remaining_funds_conditions, total_fee,
basis_points, and the per-recipient fee calculation.
🧹 Nitpick comments (1)
contracts/escrow/src/core/dispute.rs (1)
131-167: Avoid double rounding by computing per‑recipient fees directly from basis points.
Currentamount * fee_result / totalrounds twice and can under‑collect fees more than necessary. Computing per‑recipient fees directly reduces leakage and keeps fee math closer to policy.♻️ Suggested refactor
- let fee_result = FeeCalculator::calculate_standard_fees(total, escrow.platform_fee)?; - let mut actual_trustless_fees = 0i128; let mut actual_platform_fees = 0i128; let mut net_distributions: Vec<(Address, i128)> = Vec::new(e); for (addr, amount) in distributions.iter() { - let recipient_trustless_fee = - BasicMath::safe_mul(amount, fee_result.trustless_work_fee)? - .checked_div(total) - .ok_or(ContractError::DivisionError)?; - let recipient_platform_fee = BasicMath::safe_mul(amount, fee_result.platform_fee)? - .checked_div(total) - .ok_or(ContractError::DivisionError)?; - - let total_recipient_fee = - BasicMath::safe_add(recipient_trustless_fee, recipient_platform_fee)?; - let net_amount = BasicMath::safe_sub(amount, total_recipient_fee)?; + let per_recipient_fees = + FeeCalculator::calculate_standard_fees(amount, escrow.platform_fee)?; + let recipient_trustless_fee = per_recipient_fees.trustless_work_fee; + let recipient_platform_fee = per_recipient_fees.platform_fee; + let net_amount = per_recipient_fees.receiver_amount;
| use soroban_sdk::token::Client as TokenClient; | ||
| use soroban_sdk::{Address, Env, Map}; | ||
| use soroban_sdk::{Address, Env, Map, Vec}; | ||
|
|
||
| use crate::core::escrow::EscrowManager; | ||
| use crate::core::validators::dispute::{ | ||
| validate_distributions_size, validate_withdraw_remaining_funds_conditions, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the validator for withdraw_remaining_funds
cat -n contracts/escrow/src/core/validators/dispute.rsRepository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 3632
🏁 Script executed:
# Let's also check the dispute.rs file to see the full context
cat -n contracts/escrow/src/core/dispute.rsRepository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 7531
🏁 Script executed:
# Search for all_processed and its usage
rg -n 'all_processed' contracts/escrow/src -g '*.rs' -B 2 -A 2Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 1718
🏁 Script executed:
# Search for disputed flag usage
rg -n 'disputed' contracts/escrow/src -g '*.rs' -B 1 -A 1Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 8627
🏁 Script executed:
# Check if there are any rounding correction mechanisms for fees
rg -n 'remainder|round|truncat' contracts/escrow/src/modules/fee.rs -B 2 -A 2Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 161
🏁 Script executed:
# Verify if there are tests that exercise withdraw_remaining_funds with disputed flag
rg -n 'withdraw_remaining_funds' contracts/escrow/src/tests/test.rs -B 5 -A 5Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 1009
🏁 Script executed:
# Find fee-related files
fd -t f 'fee' contracts/escrow/srcRepository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 68
🏁 Script executed:
# Look at the fee calculation implementation
cat -n contracts/escrow/src/modules/fee.rs 2>/dev/null || find contracts/escrow/src -name "*fee*" -type fRepository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 68
🏁 Script executed:
# Search for FeeCalculator trait and implementation
rg -n 'FeeCalculator|calculate_standard_fees' contracts/escrow/src -B 2 -A 5Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 3832
🏁 Script executed:
# Check the modules directory structure
ls -la contracts/escrow/src/modules/ 2>/dev/null || find contracts/escrow/src -type d -name modulesRepository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 354
🏁 Script executed:
# See the complete fee calculator implementation
cat -n contracts/escrow/src/modules/fee/calculator.rsRepository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 1823
🏁 Script executed:
# Check if test exercises withdraw_remaining_funds with disputed flag set
rg -n 'test_withdraw_remaining_funds' contracts/escrow/src/tests/test.rs -A 100 | head -150Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 4284
🏁 Script executed:
# Check if there's a test case for withdraw_remaining_funds with disputed=true
rg -n 'withdraw_remaining_funds' contracts/escrow/src/tests/test.rs -B 10 | grep -E 'disputed|withdraw_remaining_funds|fn test'Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 233
🏁 Script executed:
# Check if there's any test with disputed flag set to true during withdraw
rg -n 'disputed.*true' contracts/escrow/src/tests/test.rs -B 5 -A 15 | grep -A 15 'withdraw_remaining_funds'Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 68
🏁 Script executed:
# Look at the rounding issue more carefully - check if there's any remainder handling
cat -n contracts/escrow/src/core/dispute.rs | sed -n '140,167p'Repository: Trustless-Work/Trustless-Work-Smart-Escrow
Length of output: 1575
all_processed treats disputed as a terminal state, allowing withdrawals before resolution.
Lines 34–38 set all_processed = true when any of flags.released, flags.resolved, or flags.disputed is set. The validator (lines 19–42 of validators/dispute.rs) requires all_processed == true to proceed with withdraw_remaining_funds. This means a dispute_resolver can distribute funds via withdraw_remaining_funds while the escrow is disputed, before calling resolve_dispute. Confirm whether interim distributions during active disputes are intended.
Additionally, the fee calculation has a rounding loss issue. At line 140, total fees are calculated once using basis points. Then at lines 147–153, per-recipient fees are calculated using integer division: (amount * total_fee) / total for each recipient. The sum of these rounded values will be less than the pre-calculated total fees, leaking the remainder to the contract.
🤖 Prompt for AI Agents
In `@contracts/escrow/src/core/dispute.rs` around lines 1 - 7, The code treats
flags.disputed as a terminal processed state (all_processed = true), allowing
withdraw_remaining_funds to run while an active dispute exists; change the logic
in the place that sets all_processed (and/or the validator
validate_withdraw_remaining_funds_conditions) so that all_processed is true only
for terminal states like flags.released or flags.resolved and explicitly
disallow withdraw_remaining_funds when flags.disputed is set (i.e., require
!flags.disputed or require flags.resolved/released before
withdraw_remaining_funds/resolve_dispute). Also fix the fee rounding leak:
instead of computing per-recipient fees using integer division that loses
remainder ((amount * total_fee) / total) for each recipient, compute
per-recipient fees and track allocated_sum, then compute remainder = total_fee -
allocated_sum and add the remainder to one recipient (e.g., the largest share or
the resolver) so the sum of per-recipient fees equals the previously-calculated
total_fee; adjust symbols referenced: all_processed, flags.released,
flags.resolved, flags.disputed, withdraw_remaining_funds, resolve_dispute,
validate_withdraw_remaining_funds_conditions, total_fee, basis_points, and the
per-recipient fee calculation.
|
Hi @caxtonacollins! Thank you for your PR. 🚀 The branch you should clone and consequently send your changes to is called “single-release-develop-v2.” This is mentioned in the issue instructions. Please make the changes to continue with the review. Apologies for the delay. |
2 participants
closes #79
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.