`calculate_split` ceiling division can extract >100% of payment value with zero downstream validation

[therealjhay]

Files: settlement_contract/src/lib.rs:706-727, 539-546, 609-618

The Problem in One Sentence: Both fees use ceiling division which rounds UP independently, so their sum can exceed the payment amount — and the merchant_amount becomes negative — but neither store_payment_reference nor calculate_fee_split checks for this before storing the result in an immutable on-chain record.

Full Explanation:

The core fee calculation at lines 706-727:

fn calculate_split(amount: i128, rule: &SettlementRule) -> FeeSplit {
    let platform_fee_amount =
        (amount * (rule.platform_fee_bps as i128) + BPS_DENOMINATOR - 1) / BPS_DENOMINATOR;
    let network_fee_amount =
        (amount * (rule.network_fee_bps as i128) + BPS_DENOMINATOR - 1) / BPS_DENOMINATOR;
    let merchant_amount = amount - platform_fee_amount - network_fee_amount;
    FeeSplit { gross_amount: amount, platform_fee_amount, network_fee_amount, merchant_amount }
}

The ceiling division formula (n * bps + 9999) / 10000 means each individual fee rounds UP to the nearest integer. When both fees round up, their sum can exceed the original amount.

Concrete exploit with realistic values:

With platform_fee_bps = 5000 (50%), network_fee_bps = 5000 (50%), amount = 1:

platform_fee = (1 * 5000 + 9999) / 10000 = 14999/10000 = 1
network_fee = (1 * 5000 + 9999) / 10000 = 14999/10000 = 1
merchant    = 1 - 1 - 1 = -1

Total extracted: 2 units from a 1-unit payment = 200% extraction rate.

With platform_fee_bps = 100 (1%), network_fee_bps = 100 (1%), amount = 199:

platform_fee = (199 * 100 + 9999) / 10000 = 29899/10000 = 2
network_fee = (199 * 100 + 9999) / 10000 = 29899/10000 = 2
merchant    = 199 - 2 - 2 = 195

Actual fee rate: 4/199 = 2.01% (higher than the nominal 2%).

Where this negative value gets permanently stored:

store_payment_reference at lines 541-552 creates a PaymentRecord with the split results and writes it to storage:

let record = PaymentRecord {
    merchant: merchant.clone(),
    amount,
    platform_fee_amount: split.platform_fee_amount,  // Could be > amount
    network_fee_amount: split.network_fee_amount,    // Could be > amount
    merchant_amount: split.merchant_amount,           // Could be NEGATIVE
    // ...
};
env.storage().persistent().set(&payment_key, &record);

The calculate_fee_split public function at lines 609-618 also returns a FeeSplit with a negative merchant amount — with no validation:

pub fn calculate_fee_split(env: Env, merchant: Address, amount: i128) -> FeeSplit {
    // Checks registration and amount > 0, but NOT merchant_amount >= 0
    let rule = read_rule_or_default(&env, merchant);
    calculate_split(amount, &rule)  // Negative merchant_amount flows through unchecked
}

Why an external settlement system would act on this negative value:

The PaymentRecord is the canonical source of truth for off-chain settlement processes. An automated settlement system that reads merchant_amount and executes a transfer would see a negative value. Depending on how the settlement system is implemented, this could:

• Cause a transfer attempt of a negative amount (protocol error or revert)
• Be misinterpreted as the merchant OWING money to the platform (reversed payment direction)
• Trigger an assertion failure in the settlement pipeline, blocking all downstream payments

The Deeper Problem:

The code's own comment at line 718-719 acknowledges this: "the sum of rounded-up fees can exceed the gross amount, resulting in a negative merchant payout." But there is zero runtime protection. The contract:

1. Does not check merchant_amount >= 0 in calculate_split
2. Does not check merchant_amount >= 0 in store_payment_reference before storing
3. Does not check merchant_amount >= 0 in calculate_fee_split before returning
4. Does not cap platform_fee_amount or network_fee_amount to amount

Attack Scenario:

1. Admin sets a rule with platform_fee_bps = 5000, network_fee_bps = 5000 (100% total)
2. Merchant calls store_payment_reference with amount = 1 (which passes the MIN_PAYMENT_AMOUNT = 100 check? No — 1 < 100, so this is rejected)
3. Merchant calls with amount = 100 (minimum allowed):
   • platform_fee = (100 * 5000 + 9999) / 10000 = 509999/10000 = 50
   • network_fee = same = 50
   • merchant = 100 - 50 - 50 = 0 (zero, not negative)
4. With platform_fee_bps = 100, network_fee_bps = 9900 (total 10000), amount = 150:
   • platform_fee = (150 * 100 + 9999) / 10000 = 24999/10000 = 2
   • network_fee = (150 * 9900 + 9999) / 10000 = 1489999/10000 = 149 (ceiling of 148.9999)
   • merchant = 150 - 2 - 149 = -1 (NEGATIVE)

The merchant processed a payment and now owes 1 unit to the system. This is fundamentally broken.

The Fix:

Add validation in calculate_split or in both callers. The cleanest fix is in store_payment_reference and calculate_fee_split:

In store_payment_reference (after line 540):

let split = calculate_split(amount, &rule);
if split.merchant_amount < 0 {
    panic_with_error!(&env, SettlementError::InvalidFeeSplit);
}

Add InvalidFeeSplit to SettlementError.

In calculate_fee_split (after line 616):

let rule = read_rule_or_default(&env, merchant);
let split = calculate_split(amount, &rule);
if split.merchant_amount < 0 {
    panic_with_error!(&env, SettlementError::InvalidFeeSplit);
}
split

For a more mathematically complete fix, change the model so that fees are subtracted from amount with FLOOR division (always rounding in favor of the merchant), so the sum of fees can never exceed the amount:

fn calculate_split(amount: i128, rule: &SettlementRule) -> FeeSplit {
    // Floor division: fees round down, merchant gets the remainder
    // This guarantees platform_fee + network_fee + merchant == amount
    let platform_fee_amount =
        (amount * (rule.platform_fee_bps as i128)) / BPS_DENOMINATOR;
    let network_fee_amount =
        (amount * (rule.network_fee_bps as i128)) / BPS_DENOMINATOR;
    let merchant_amount = amount - platform_fee_amount - network_fee_amount;
    FeeSplit { gross_amount: amount, platform_fee_amount, network_fee_amount, merchant_amount }
}

This guarantees merchant_amount >= 0 for any amount > 0 and any fee BPS whose sum ≤ 10000.

[ebubeb683-ship-it]

Please assign this issue to me. i checked it out and im confident i will solve it
my eta is 4 hours

[Robotron2]

I'd like to work on this issue.

I'll review the fee calculation flow to ensure invalid fee splits can never propagate into stored PaymentRecords or the public calculate_fee_split API. My plan is to add validation around merchant_amount and introduce an appropriate contract error for invalid splits, while evaluating whether the rounding strategy itself should be adjusted to guarantee the total allocation never exceeds the payment amount. I'll also add tests covering edge cases around rounding and maximum fee configurations.

I have experience with Soroban contracts and financial calculations, so I'm comfortable implementing this carefully to preserve accounting correctness and prevent invalid on chain state. I should be able to have a well tested PR ready within a day.

[Adecom16]

I will like to jump on this issue.

I understand the root cause is that independent ceiling rounding can produce a negative merchant_amount when the combined rounded fees exceed the payment amount. I'll implement a safe fix by validating the computed FeeSplit before it's persisted or returned, introduce an appropriate SettlementError for invalid fee splits, add regression tests covering the edge cases, and ensure the existing settlement behavior remains unchanged for valid fee configurations.