Trading engine & API security assessment

Critical CDX-01 matching engine · order-service/cancel.go

Cancellation of a partially filled order releases the original locked amount

Summary

The cancel handler releases order.quantity × order.price. Fills have already consumed part of that lock. The difference is credited to the account as spendable quote-currency balance.

Root cause

Placement and cancellation each compute the lock independently from the order's original terms, so the two are symmetric by construction. Nothing tracks how much of the lock remains. The filled quantity is recorded on the order row but is not consulted when releasing.

// order-service/cancel.go
released := order.Quantity.Mul(order.Price)      // original notional
acct.Release(order.QuoteAsset, released)          // ← should be the remainder

Reproduction

Account funded with 50,000 USDC. No special privileges required.

1. POST /v1/orders   BUY 1.0 BTC-USDC @ 50,000 limit
   → 50,000 USDC locked, available = 0

2. Counterparty fills 0.4 BTC
   → 0.4 BTC credited, 20,000 USDC consumed from the lock
   → lock should now be 30,000

3. DELETE /v1/orders/{id}
   → 50,000 USDC released

4. GET /v1/balances
   → available = 50,000 USDC  and  0.4 BTC held
   → net gain 20,000 USDC

The cycle is repeatable without limit and the resulting balance passed POST /v1/withdrawals validation in staging.

Impact

Unbounded creation of withdrawable balance by any funded account. Exploitation is indistinguishable from ordinary trading in the order flow, and the only signal is a widening gap between the ledger and reserves. At the observed 1.4 cycles per second in staging, a single account reaches a six-figure fabricated balance inside an hour.

Remediation

Make the lock a tracked quantity with a single owner rather than a value recomputed at each step. Release exactly order.locked_amount, decrementing it on every fill.

- released := order.Quantity.Mul(order.Price)
+ released := order.LockedAmount              // decremented on each fill
+ if released.IsNegative() { return ErrLockUnderflow }
  acct.Release(order.QuoteAsset, released)
+ order.LockedAmount = decimal.Zero

Assert INV-02 after every lifecycle transition, in CI and in a production reconciliation job.

Retest — 30 June 2026

Closed. locked_amount is now authoritative and decremented on fill. The cycle above nets zero. The invariant assertion is present in CI and fails the build when we reintroduced the original expression.

High CDX-02 order API · order-service/place.go

Concurrent order placement passes a single balance check

Summary

Balance is read, validated and debited as three separate statements with no enclosing transaction and no row lock. Two placements arriving together both read the pre-debit balance and both pass validation.

Root cause

bal := repo.GetBalance(acct, asset)        // read
if bal.Available.LessThan(required) {      // check
    return ErrInsufficientBalance
}
repo.Lock(acct, asset, required)           // use — no lock held across the gap

Reproduction

Two requests issued in parallel, each for the entire available balance. Sequentially the second is correctly rejected; in parallel both are accepted.

$ contoso-fuzz --parallel 2 --order 'BUY 1.0 BTC-USDC @ 50,000' --balance 50000
  req A → 201 Created
  req B → 201 Created
  GET /v1/balances → available = -50,000 USDC

Impact

An account can hold more open exposure than it has funded. Negative balances break settlement assumptions downstream and, combined with a withdrawal in the same window, are directly monetisable.

Remediation

Serialise the read-check-debit sequence. Either SELECT ... FOR UPDATE on the balance row within one transaction, or an optimistic version column with a retry. Assert non-negativity after commit, not before.

Retest — 30 June 2026

Closed. The sequence now runs inside a transaction with FOR UPDATE. At 64 parallel placements exactly one succeeded and balance never went negative across 10,000 fuzz runs.

Medium CDX-03 ledger · ledger-service/fees.go

Taker fee is floored while maker rebate is ceiled

Summary

At tier 2 on ETH-USDC the taker fee rounds down to 8 decimal places and the maker rebate rounds up. An account on both sides of a fill nets a positive fraction of the smallest unit per trade.

Root cause

Fee and rebate are computed in different functions written at different times. Each rounds in a locally sensible direction; nothing checks that the pair cannot be combined profitably. Self-match prevention rejects an account crossing itself directly but does not prevent two sub-accounts under one owner from matching.

fee    := notional.Mul(takerBps).Div(10000).RoundDown(8)
rebate := notional.Mul(makerBps).Div(10000).RoundUp(8)   // ← direction

Reproduction

notional = 1,000.000000005 USDC   taker 2bps   maker rebate 1bps
  fee    = 0.20000000  (floored from 0.2000000001)
  rebate = 0.10000001  (ceiled  from 0.10000000005)
  net to owner = +0.00000001 USDC per matched pair

Driven from two sub-accounts at the platform rate limit, the extraction rate in staging was roughly 0.4 USDC per hour per pair — small, but it is real value leaving the exchange, it scales with pairs and accounts, and the same asymmetry would be material at a higher rebate tier.

Remediation

Round both quantities in the exchange's favour: fee up, rebate down. Add INV-05 as a property test over a round-trip loop rather than a single call. Extend self-match prevention to the owner level, not the account level.

Retest — 30 June 2026

Closed. Rounding directions corrected and the round-trip property test passes at 1,000,000 fuzz runs. Owner-level self-match prevention deployed.

Medium CDX-04 contracts · TokenVesting.sol

Cliff is enforced in the view but not on the claim path

Summary

claimable() returns zero before the cliff, but claim() computes its transfer from _vestedAmount(), which applies only linear accrual and never consults cliff. A beneficiary can withdraw accrued tokens before the cliff by calling claim() directly. The front end reads claimable(), so the UI shows zero and the behaviour is not visible in normal use.

Root cause

function claimable(address b) public view returns (uint256) {
    if (block.timestamp < cliff) return 0;            // gate is here
    return _vestedAmount(b) - claimed[b];
}

function claim() external nonReentrant {
    uint256 amount = _vestedAmount(msg.sender) - claimed[msg.sender];  // ← not here
    require(amount > 0, "nothing to claim");
    claimed[msg.sender] += amount;
    token.safeTransfer(msg.sender, amount);
}

Proof of concept

// forge test --match-test testClaimBeforeCliff -vvv
function testClaimBeforeCliff() public {
    uint256 start    = block.timestamp;
    uint256 cliff    = start + 180 days;
    uint256 duration = start + 730 days;

    vesting = new TokenVesting(address(token), start, cliff, duration);
    vesting.setAllocation(beneficiary, 1_000_000e18);
    token.transfer(address(vesting), 1_000_000e18);

    // one second before the cliff
    vm.warp(cliff - 1);
    assertEq(vesting.claimable(beneficiary), 0, "view correctly reports zero");

    vm.prank(beneficiary);
    vesting.claim();                       // succeeds — should revert

    assertGt(token.balanceOf(beneficiary), 0, "tokens released before cliff");
    // actual: 246,575.34e18 released at cliff - 1
}

[FAIL] testClaimBeforeCliff() — 246575342465753424657534 tokens released before cliff
  ↳ expected revert: CliffNotReached()

Impact

Approximately 24.6% of an allocation is reachable one second before the cliff on the schedule tested. For a public sale this is a disclosure and market-integrity problem as much as a technical one: the vesting terms published to investors are not the terms the contract enforces.

Remediation

Gate the cliff inside _vestedAmount() so every caller inherits it, rather than in one view function.

function _vestedAmount(address b) internal view returns (uint256) {
+   if (block.timestamp < cliff) return 0;
    if (block.timestamp >= duration) return allocation[b];
    return allocation[b] * (block.timestamp - start) / (duration - start);
}

Add the boundary cases to the test suite explicitly: cliff - 1, cliff, cliff + 1, and the final claim at duration.

Retest — 30 June 2026

Closed. Gate moved into _vestedAmount(). The PoC now reverts with CliffNotReached(). Boundary tests added and an invariant run of 5,000 sequences with time warping found no path to an early transfer.

Low CDX-05 API auth · api-gateway/middleware/scope.go

Read-scoped API key accepted for order cancellation

Summary

The scope middleware is registered on POST /v1/orders but not on DELETE /v1/orders/{id}. A key issued with read scope can cancel any open order belonging to its own account.

Reproduction

$ curl -X DELETE https://staging.contoso.example/v1/orders/8812 \
       -H "X-API-Key: $READ_ONLY_KEY" -H "X-Signature: ..."
  204 No Content        # expected 403

Impact

Limited: the key cannot place orders, move funds or reach another account. But read-only keys are the ones shared with analytics tools and third-party dashboards, so the blast radius of a leaked read key is larger than its holder expects. For a market maker, silent cancellation is a trading loss.

Remediation

Apply the scope middleware at the router group rather than per route, so a new endpoint is protected by default and omission is a deliberate act. Add a test asserting every mutating route rejects a read-scoped key.

Retest — 30 June 2026

Closed. Middleware moved to the group. A table-driven test now enumerates all 41 routes and asserts scope enforcement.