Over-Withdrawal via Repeated Early Withdrawal Invocations

[lukepuplett]

Again, contract out of scope on HackerOne.

---

Over-Withdrawal via Repeated Early Withdrawal Invocations

Severity

Critical - Allows extraction of funds exceeding payment amounts, violating core contract invariants.

Summary

The earlyWithdrawByArbiter function lacks validation to prevent withdrawnAmount from exceeding payment.amount across multiple invocations. This allows over-withdrawal when the same paymentID is processed multiple times with different signature parameters.

Vulnerability Details

Root Cause:
The function uses += to increment withdrawnAmount but only validates individual withdrawal amounts, not cumulative totals:

function earlyWithdrawByArbiter(...) external onlyArbiter {
    // ...
    for (uint256 i = 0; i < paymentIDs.length; ++i) {
        uint256 paymentID = paymentIDs[i];
        uint256 withdrawalAmount = withdrawalAmounts[i];
        Payment memory paymentCopy = payments[paymentID];

        if (withdrawalAmount > payment.amount) {  // ← Only checks THIS withdrawal
            revert InvalidWithdrawalAmount(paymentID, withdrawalAmount);
        }
        // ...
        payments[paymentID].withdrawnAmount += withdrawalAmount;  // ← Cumulative issue
    }
}

Missing Invariant Check:
The contract never validates that withdrawnAmount + withdrawalAmount <= payment.amount.

Attack Scenarios

Scenario 1: Buggy Arbiter Application

• Arbiter UI has a bug allowing sellers to request multiple early withdrawals on the same payment
• Each request generates a new signature (different salt or expiry)
• Contract processes each as valid, depleting beyond payment amount

Scenario 2: Legitimate Multiple Partial Withdrawals

• Payment: 100 USDC
• Week 1: Seller requests early withdrawal of 40 USDC (arbiter approves, withdrawnAmount = 40)
• Week 2: Seller requests early withdrawal of 40 USDC (arbiter approves, withdrawnAmount = 80)
• Week 3: Seller requests early withdrawal of 40 USDC (arbiter approves, withdrawnAmount = 120) ❌

Result: 120 USDC withdrawn from a 100 USDC payment.

Scenario 3: Defense-in-Depth Failure

Even if the arbiter is non-malicious, contract should enforce invariants to prevent:

• Arbiter application bugs
• Arbiter key compromise
• Race conditions in arbiter processing logic

Proof of Concept

// Payment created: 100 USDC to seller, paymentID = 0

// Early withdrawal #1: seller signs for 60 USDC, expiry = T1, salt = 1
earlyWithdrawByArbiter(
    [0], [60], 5, T1, 1, seller, v1, r1, s1
);
// withdrawnAmount[0] = 60, seller receives 55 USDC (5 fee)

// Early withdrawal #2: seller signs for 60 USDC, expiry = T2, salt = 2
earlyWithdrawByArbiter(
    [0], [60], 5, T2, 2, seller, v2, r2, s2
);
// withdrawnAmount[0] = 120, seller receives 55 USDC (5 fee)

// Total extracted: 110 USDC from a 100 USDC payment
// Over-withdrawal: 10 USDC + 10 USDC in fees

Secondary Impact: Bricked Normal Withdrawals

Once over-withdrawn, normal withdraw() becomes permanently unusable for that payment:

function withdraw(uint256[] calldata paymentIDs) external {
    // ...
    totalAmount += paymentCopy.amount - paymentCopy.withdrawnAmount;
    //           = 100 - 120
    //           = underflow → revert (Solidity ^0.8.24)
}

While this prevents further damage, the payment is now in a corrupt state. The underflow protection is defensive but doesn't prevent the initial over-withdrawal.

Recommended Fix

Add cumulative validation before incrementing withdrawnAmount:

function earlyWithdrawByArbiter(...) external onlyArbiter {
    // ...
    for (uint256 i = 0; i < paymentIDs.length; ++i) {
        uint256 paymentID = paymentIDs[i];
        uint256 withdrawalAmount = withdrawalAmounts[i];
        Payment memory paymentCopy = payments[paymentID];

        // Validate individual withdrawal doesn't exceed payment
        if (withdrawalAmount > paymentCopy.amount) {
            revert InvalidWithdrawalAmount(paymentID, withdrawalAmount);
        }
        
        // ADD THIS: Validate cumulative withdrawals don't exceed payment
        if (paymentCopy.withdrawnAmount + withdrawalAmount > paymentCopy.amount) {
            revert InvalidWithdrawalAmount(paymentID, withdrawalAmount);
        }
        
        // ... rest of logic
        payments[paymentID].withdrawnAmount += withdrawalAmount;
    }
}

Affected Components

• earlyWithdrawByArbiter() - primary vulnerability
• withdraw() - secondary impact (becomes unusable for over-withdrawn payments)

Impact Assessment

• Funds at Risk: Unlimited (proportional to payment volumes and arbiter processing frequency)
• Likelihood: Medium (requires arbiter error or compromise, but partial withdrawals are legitimate use case)
• Defense-in-Depth: Contract should enforce invariants regardless of arbiter behavior