Skip to content

[simplex]: guard sends, rebalancing, top-ups and bids against bad inputs and unverified reads - #1418

Open
seunlanlege wants to merge 4 commits into
fix/simplex-delegation-correctnessfrom
fix/simplex-small-guards
Open

seunlanlege wants to merge 4 commits into
fix/simplex-delegation-correctnessfrom
fix/simplex-small-guards

Conversation

@seunlanlege

@seunlanlege seunlanlege commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Stacked on #1415 (stack #1416). Review only the commits above it.

Ten small guards from the 2026-10-15 simplex audit (c19, c27, c29, c31, c35, c45, c48, c50, c64, and the round-2 unpayable-bid low). Each refuses a bad input or an unverified read before money or gas is spent.

  • Send recipients (TokenSender.send). Refused, case-insensitively:

    • the zero address;
    • the token's own address;
    • a vault configured on that chain;
    • the filler's own wallet, for native and ERC-20 sends only. A vault-share redeem into the wallet stays allowed, since that's how a specific amount is withdrawn from a vault.

    Library callers are covered, not just /api/send.

  • Rebalancing config. A shared validateRebalancingConfig runs in validateConfig, RebalanceController.set and PUT /api/rebalancing.

    • It is skipped when triggerPercentage is undefined, since rebalancing is then off.
    • triggerPercentage must be finite and strictly between 0 and 1. A numeric string such as "0.5" is accepted, because the rebalancer coerces it; "abc" is not.
    • Each base balance must parse, as getBaseBalance parses it (parseFloat), to a finite number that is not negative. A base of 0 or "0" is accepted.
  • Rebalance transfers don't race a nonce. Each chain is a lane, and a leg waits on every chain it sends from: its source, and for CCTP also its destination, where BridgeKit mints from the same wallet. USDT0 and Binance legs send only from the source. Different chains still run in parallel, and a failed leg doesn't hold up the next one in its lane. A lane is held for the whole leg (CCTP burn, attestation and mint; Binance deposit credit and withdrawal; USDT0 delivery wait). Lanes cover rebalance legs only; fills, sweeps, top-ups and operator sends share the EOA's nonce without them.

  • Block-time tolerance. A measured block time below half the chain's declared one (declared/2) is discarded with a warning and the declared value is used, so one lying RPC can't stretch bid validity past twice its window. A chain that declares none has no reference to judge a sample against, so it only gets the flat MIN_PLAUSIBLE_BLOCK_TIME_SECONDS (0.1 s) floor. A sample below it is replaced by FALLBACK_BLOCK_TIME_SECONDS (2 s); anything above is used as measured (BSC Chapel's 0.45 s included).

  • Wizard funding reads are chain-verified.

    • Each RPC URL's chain id is checked with the bounded fetchChainId probe. A wrong chain id fails the check, and an unreachable URL is left out.
    • The read then runs through a fallback over only the URLs that passed.
    • So a wrong-chain URL can't fill the process-wide fee-token and Permit2-support caches.
  • Top-up outcomes need the chain.

    • reclaimed needs isOrderRefunded on chain and a recorded commitment, and the indexer is used only for the tx hash and time. If the indexer says REFUNDED but the chain read fails, recovery retries within [simplex]: pay a top-up reclaim's relayer fee in the fee token and bound its retry #1408's budget instead of sending a cancel.
    • An empty escrow is also what a fill leaves, so filledByOther reads BSC's _filled[commitment] first. It counts as a fill only when non-zero and not the order's creator (order.user), because a destination-side cancel, the top-up's own or a third party's, sets _filled to the creator too. A fill moves the record to delivering, not reclaimed. refunded(), live-order tracking and the reconcile of an unfilled record all apply it, and a failed _filled read throws, so neither outcome is recorded from an unread value.
    • delivered needs an on-chain receipt. DESTINATION is kept for the timeline only.
    • A cancelled order is not reclaimed by itself. When the executor reports CANCELLED, refunded() decides: refunded → reclaimed; escrow still held → unfilled, so the normal reclaim recovers it. Anyone may cancel on BSC once the deadline has passed, and the source escrow is refunded only when that cancel's RefundEscrow is relayed. A failed read of the escrow or of _filled records neither and is retried.
    • A fill the indexer hasn't reached is found on BSC without it. findFill reads the gateway's OrderFilled log for the commitment from where the order became fillable (its deadline less the order window in blocks, less evidenceMarginBlocks) to the deadline, capped at BSC's head. It reads in chunks, keeps the existing match rules, and a failed range throws so the caller retries instead of reporting "no fill".
  • The top-up balance check includes the solver fee. start() quotes it with quoteOrderFees and checks amountIn plus the fee in the fee token, summed when they're the same token, net of the reserve, before anything is persisted. What the SDK places is unchanged.

  • Source-gateway release before a cross-chain partial. partialEligible also runs assertGatewayRelease on the source gateway, caching only a supported release. An old or unreadable source rules out the partial and nothing else.

  • Wallet paymaster reserve is 5 tokens. PAYMASTER_RESERVE_TOKENS is now RECOMMENDED_AMOUNT_USD, because an Ethereum prefund can be 2–5 tokens. This updates the 2026-08-19 decision, which was based on L2 prefunds. The wizard and Chains copy now say the 5 are kept for gas and fills spend what's above them.

  • No bid nothing can pay for.

    • When paymaster selection returns none, the bid is refused if EntryPoint deposit + native < baseGas × maxFeePerGas. That is checked against the gas the op is signed with, right after its preVerificationGas estimate. A failed read lets the bid through.
    • This closes the gap the 2026-09-01 and 2026-09-07 decisions left open, and both docs are updated.

Behaviour changes:

  • Every paymaster token the wallet holds keeps 5 back on a paymaster chain.
  • Same-destination CCTP legs now run one after another.
  • A rebalancing config outside 0 < triggerPercentage < 1 no longer boots.
  • A top-up the executor reports cancelled is unfilled, not reclaimed, until the escrow is refunded.
  • Without the indexer, a late-found fill is searched over the order's whole life on BSC (about 10 getLogs pages), not a few minutes around now.

Validation

  • Simplex lint exit 0; test:data 146, test:unit 284 and test:filler 71 passed (5 skipped).
  • bridge-top-up, token-sender, UserOpSender and DelegationService*: 236 passed, 1 skipped.
  • Full offline suite: 1728 passed. The only failures are the 10 live/credential tests in six files (rebalancers binance, cctp, usdt0; strategies fx, stable, vault .testnet).
  • Typecheck: tsc shows only the 14 errors already on main (3 of them in the ox dependency), none in changed files.
  • New and updated tests cover each guard:
    • the four recipient refusals, and redeem-to-self allowed;
    • validateConfig and the PUT route;
    • rebalance lanes: same-destination CCTP legs don't overlap, a USDT0 leg isn't blocked, opposite CCTP legs don't overlap;
    • the block-time tolerance and floor;
    • wizard reads: wrong chain id → rejected with the cache empty, unreachable + good URL → read through the good one;
    • top-up reclaim and delivery on chain only, including the refunded-read-failure retry budget and the _filled rule;
    • CANCELLED with the escrow held → unfilled, with it empty and _filled == order.user → reclaimed, and a failed read → neither;
    • findFill locating a fill from long before now, stopping at the deadline or head, and throwing on a failed range;
    • the fee-inclusive balance check;
    • an old source gateway;
    • the reserve;
    • unpayable bids at and below the threshold.

Not covered: no mainnet run.

@seunlanlege
seunlanlege added this pull request to stack #1416 October 16, 2026 14:24

@Wizdave97 Wizdave97 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: approve with minor comments. I reviewed only this PR's commit on top of #1415.

Most of the guards check out:

  • The send exemption uses the same isVaultShare test as send, so a plain ERC-20 send to the wallet can't use it.
  • The unpayable-bid check prices the full prefund (EntryPoint v0.7 and later) against the signer's EntryPoint deposit plus its native balance, on the configured EntryPoint.
  • The top-up fee check handles a fee token that differs from the pay token, with its own reserve.
  • The wizard probe throws before any read if every URL fails.
  • Only a supported source-gateway release is cached.
  1. validateRebalancingConfig refuses configs that boot today (filler-toml.ts:278-297, called at :406).

    • Base balances with no triggerPercentage (for example commented out to switch rebalancing off) boot today, because boot.ts:369 only starts rebalancing when it is fully configured. Now validateConfig throws got undefined, and the CLI, init and setup-api all refuse the config.
    • A base of "0" (a donor chain that keeps nothing) works today and is now rejected.
    • So "parsed as getBaseBalance parses them" isn't quite true: getBaseBalance treats "" and 0 as not configured.
    • The JSDoc's reasoning is also inverted. With the threshold base × (1 − p), p = 0 fires whenever balance ≤ base, and only p > 1 never fires.
    • Suggestion: skip validation when triggerPercentage is undefined (matching boot), accept a base of 0 or document that it's refused, and fix the comment.
  2. The block-time floor limits a lying RPC but doesn't stop it (ContractInteractionService.ts:105, :1039-1041).

    • The direction is right: a smaller measured block time is what stretches a bid, since validity in blocks = seconds ÷ block time.
    • But any sample between declared/4 and declared is accepted. On Ethereum (12 s declared), a 3.01 s sample turns a 330 s window into about 22 minutes. The new test even accepts 3.5 s.
    • On chains that declare nothing (Chapel, Amoy and Sepolia in viem 2.47), a sample under 0.1 s is replaced by 0.1 s, which is the most-stretching value still allowed: about 20× on a 2 s chain.
    • Suggestion: use a tighter or per-chain tolerance, and fall back to FALLBACK_BLOCK_TIME_SECONDS on chains with no declared time. Otherwise reword "can't stretch bid validity".
  3. Rebalance lanes can't deadlock, but they're held for the whole leg and don't cover other senders (RebalancingService.ts:99, :436-446).

    • Lanes are chained in plan order, so they can't deadlock, and they are released when a leg fails.
    • But a lane stays held through CCTP attestation and mint, a Binance deposit credit plus withdrawal (up to about 30 min), and USDT0 waits. A surplus chain feeding several deficits now runs those legs one after another, and a BridgeKit leg with no timeout blocks the rest of its run.
    • The nonce race also exists with every other native tx from the same wallet, which the lanes don't cover: the TokenSender native fallback, Permit2 approve, EntryPoint deposit top-ups and top-up cancels.
    • Suggestion: a viem nonceManager on the shared account in ChainClientManager would serialise nonces for the whole process without holding lanes through bridge waits. If the lanes stay, document that they last the whole leg.
  4. isOrderRefunded can't tell a refund from a fill's escrow release (BridgeTopUp.ts:1508). This predates the PR.

    • IntentsBase._withdraw empties the escrow for a redeem as well as for a refund.
    • Scenario: the executor misses a fill and the operator reclaims. Once the redeem lands, the top-up is marked reclaimed, and the BRIDGE delivery is never self-relayed.
    • Suggestion: check isOrderFilled on BSC inside refunded() before recording reclaimed.

Docs:

  • The decision docs are edited in place, which is good, but a few history phrases remain: 2026-08-19's "It was first 2, picked from…" and 2026-09-07's "was left out…; it now exists". Reword them as the current decision.
  • The new JSDoc was inserted between validateOrderbookConfig's docblock and that function, so the old docblock now sits above validateRebalancingConfig.

Cross-PR: lanes only serialise legs within one run. #1410's "one rebalance pass at a time" is what stops overlapping runs, and longer-held lanes make overlaps more likely, so the two should land together.

seunlanlege added a commit that referenced this pull request Oct 19, 2026
- validateRebalancingConfig skips configs with no triggerPercentage
  (rebalancing is off, as in boot) and accepts a base of 0 or "0"; the
  JSDoc states the threshold `base x (1 - p)` correctly and now sits on
  the right function, with validateOrderbookConfig's docblock restored.
- The measured block-time tolerance is half the reference (the declared
  block time, or FALLBACK_BLOCK_TIME_SECONDS when none is declared)
  instead of a quarter / a flat 0.1 s, and the docs say it limits how far
  one RPC can stretch a bid rather than preventing it.
- BridgeTopUp.refunded() checks gateway.isOrderFilled on BSC before
  recording `reclaimed`: a fill's escrow release empties the escrow too,
  so a filled order moves to `delivering` and resumes delivery.
- Document that a rebalance lane is held for the whole leg and that
  lanes serialise rebalance legs only (a nonce manager is a possible
  follow-up); reword the history phrases in the 2026-08-19 and 2026-09-07
  decision notes as the current decision.
seunlanlege added a commit that referenced this pull request Oct 19, 2026
@seunlanlege
seunlanlege force-pushed the fix/simplex-small-guards branch from ac56d10 to 63475f8 Compare October 19, 2026 13:45
seunlanlege added a commit that referenced this pull request Oct 19, 2026
- validateRebalancingConfig skips configs with no triggerPercentage
  (rebalancing is off, as in boot) and accepts a base of 0 or "0"; the
  JSDoc states the threshold `base x (1 - p)` correctly and now sits on
  the right function, with validateOrderbookConfig's docblock restored.
- The measured block-time tolerance is half the reference (the declared
  block time, or FALLBACK_BLOCK_TIME_SECONDS when none is declared)
  instead of a quarter / a flat 0.1 s, and the docs say it limits how far
  one RPC can stretch a bid rather than preventing it.
- BridgeTopUp.refunded() checks gateway.isOrderFilled on BSC before
  recording `reclaimed`: a fill's escrow release empties the escrow too,
  so a filled order moves to `delivering` and resumes delivery.
- Document that a rebalance lane is held for the whole leg and that
  lanes serialise rebalance legs only (a nonce manager is a possible
  follow-up); reword the history phrases in the 2026-08-19 and 2026-09-07
  decision notes as the current decision.
seunlanlege added a commit that referenced this pull request Oct 19, 2026
@seunlanlege
seunlanlege force-pushed the fix/simplex-small-guards branch from 63475f8 to 7d9894e Compare October 19, 2026 13:53
seunlanlege added a commit that referenced this pull request Oct 19, 2026
- validateRebalancingConfig skips configs with no triggerPercentage
  (rebalancing is off, as in boot) and accepts a base of 0 or "0"; the
  JSDoc states the threshold `base x (1 - p)` correctly and now sits on
  the right function, with validateOrderbookConfig's docblock restored.
- The measured block-time tolerance is half the reference (the declared
  block time, or FALLBACK_BLOCK_TIME_SECONDS when none is declared)
  instead of a quarter / a flat 0.1 s, and the docs say it limits how far
  one RPC can stretch a bid rather than preventing it.
- BridgeTopUp.refunded() checks gateway.isOrderFilled on BSC before
  recording `reclaimed`: a fill's escrow release empties the escrow too,
  so a filled order moves to `delivering` and resumes delivery.
- Document that a rebalance lane is held for the whole leg and that
  lanes serialise rebalance legs only (a nonce manager is a possible
  follow-up); reword the history phrases in the 2026-08-19 and 2026-09-07
  decision notes as the current decision.
seunlanlege added a commit that referenced this pull request Oct 19, 2026
@seunlanlege
seunlanlege force-pushed the fix/simplex-small-guards branch from 7d9894e to e8532c0 Compare October 19, 2026 15:08

@Wizdave97 Wizdave97 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. All four round-1 findings are fixed or answered with a reason that holds:

  • A config with no trigger, or with a base balance of 0, is accepted.
  • The block-time tolerance is now half the declared block time. Chains that declare none fall back to 2 s.
  • The lanes' scope is documented.
  • reclaimed and delivering now check _filled against order.user.

Non-blocking follow-ups:

  • CANCELLED still goes straight to reclaimed (BridgeTopUp.ts:1393-1395), with no isOrderRefunded check.
    • Scenario: a third party cancels on the destination while simplex is down. The executor yields CANCELLED, and the top-up is closed.
    • But the source escrow is only refunded if the canceller's RefundEscrow gets relayed.
    • Mapping CANCELLED to unfilled and letting refunded() decide matches the rule the PR states.
    • The test at bridge-top-up.test.ts:2474 doesn't cover this, because its resumeBest mock yields nothing.
  • A fill found late may never be located without the indexer. In that case findFill only searches about 3 minutes around the current time (BridgeTopUp.ts:1196-1210). The real fill happened before the deadline, outside that window. Searching from the placement block to the deadline would find it.
  • PR description is stale. It still says declared/4 and > 0 base balances, and doesn't mention the _filled check.

seunlanlege added a commit that referenced this pull request Oct 20, 2026
- validateRebalancingConfig skips configs with no triggerPercentage
  (rebalancing is off, as in boot) and accepts a base of 0 or "0"; the
  JSDoc states the threshold `base x (1 - p)` correctly and now sits on
  the right function, with validateOrderbookConfig's docblock restored.
- The measured block-time tolerance is half the reference (the declared
  block time, or FALLBACK_BLOCK_TIME_SECONDS when none is declared)
  instead of a quarter / a flat 0.1 s, and the docs say it limits how far
  one RPC can stretch a bid rather than preventing it.
- BridgeTopUp.refunded() checks gateway.isOrderFilled on BSC before
  recording `reclaimed`: a fill's escrow release empties the escrow too,
  so a filled order moves to `delivering` and resumes delivery.
- Document that a rebalance lane is held for the whole leg and that
  lanes serialise rebalance legs only (a nonce manager is a possible
  follow-up); reword the history phrases in the 2026-08-19 and 2026-09-07
  decision notes as the current decision.
seunlanlege added a commit that referenced this pull request Oct 20, 2026
@seunlanlege
seunlanlege force-pushed the fix/simplex-small-guards branch from e8532c0 to f2ca706 Compare October 20, 2026 11:55
seunlanlege added a commit that referenced this pull request Oct 20, 2026
- validateRebalancingConfig skips configs with no triggerPercentage
  (rebalancing is off, as in boot) and accepts a base of 0 or "0"; the
  JSDoc states the threshold `base x (1 - p)` correctly and now sits on
  the right function, with validateOrderbookConfig's docblock restored.
- The measured block-time tolerance is half the reference (the declared
  block time, or FALLBACK_BLOCK_TIME_SECONDS when none is declared)
  instead of a quarter / a flat 0.1 s, and the docs say it limits how far
  one RPC can stretch a bid rather than preventing it.
- BridgeTopUp.refunded() checks gateway.isOrderFilled on BSC before
  recording `reclaimed`: a fill's escrow release empties the escrow too,
  so a filled order moves to `delivering` and resumes delivery.
- Document that a rebalance lane is held for the whole leg and that
  lanes serialise rebalance legs only (a nonce manager is a possible
  follow-up); reword the history phrases in the 2026-08-19 and 2026-09-07
  decision notes as the current decision.
@seunlanlege
seunlanlege force-pushed the fix/simplex-small-guards branch from f2ca706 to 928c9c9 Compare October 20, 2026 12:35

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants