Repository navigation
[simplex]: guard sends, rebalancing, top-ups and bids against bad inputs and unverified reads - #1418
seunlanlege wants to merge 4 commits into
Conversation
Wizdave97
left a comment
There was a problem hiding this comment.
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
isVaultSharetest assend, 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.
-
validateRebalancingConfigrefuses 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. NowvalidateConfigthrowsgot undefined, and the CLI,initand 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
getBaseBalanceparses them" isn't quite true:getBaseBalancetreats""and0as 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
triggerPercentageis undefined (matching boot), accept a base of 0 or document that it's refused, and fix the comment.
- Base balances with no
-
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_SECONDSon chains with no declared time. Otherwise reword "can't stretch bid validity".
-
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
nonceManageron the shared account inChainClientManagerwould serialise nonces for the whole process without holding lanes through bridge waits. If the lanes stay, document that they last the whole leg.
-
isOrderRefundedcan't tell a refund from a fill's escrow release (BridgeTopUp.ts:1508). This predates the PR.IntentsBase._withdrawempties 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
isOrderFilledon BSC insiderefunded()before recordingreclaimed.
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 abovevalidateRebalancingConfig.
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.
- 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.
ac56d10 to
63475f8
Compare
- 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.
63475f8 to
7d9894e
Compare
- 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.
7d9894e to
e8532c0
Compare
Wizdave97
left a comment
There was a problem hiding this comment.
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.
reclaimedanddeliveringnow check_filledagainstorder.user.
Non-blocking follow-ups:
CANCELLEDstill goes straight toreclaimed(BridgeTopUp.ts:1393-1395), with noisOrderRefundedcheck.- 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
RefundEscrowgets relayed. - Mapping
CANCELLEDtounfilledand lettingrefunded()decide matches the rule the PR states. - The test at
bridge-top-up.test.ts:2474doesn't cover this, because itsresumeBestmock yields nothing.
- Scenario: a third party cancels on the destination while simplex is down. The executor yields
- A fill found late may never be located without the indexer. In that case
findFillonly 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
> 0base balances, and doesn't mention the_filledcheck.
- 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.
e8532c0 to
f2ca706
Compare
…uts and unverified reads
- 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.
f2ca706 to
928c9c9
Compare
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:Library callers are covered, not just
/api/send.Rebalancing config. A shared
validateRebalancingConfigruns invalidateConfig,RebalanceController.setandPUT /api/rebalancing.triggerPercentageis undefined, since rebalancing is then off.triggerPercentagemust 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.getBaseBalanceparses it (parseFloat), to a finite number that is not negative. A base of0or"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 byFALLBACK_BLOCK_TIME_SECONDS(2 s); anything above is used as measured (BSC Chapel's 0.45 s included).Wizard funding reads are chain-verified.
fetchChainIdprobe. A wrong chain id fails the check, and an unreachable URL is left out.Top-up outcomes need the chain.
reclaimedneedsisOrderRefundedon 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.filledByOtherreads 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_filledto the creator too. A fill moves the record todelivering, notreclaimed.refunded(), live-order tracking and the reconcile of anunfilledrecord all apply it, and a failed_filledread throws, so neither outcome is recorded from an unread value.deliveredneeds an on-chain receipt.DESTINATIONis kept for the timeline only.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'sRefundEscrowis relayed. A failed read of the escrow or of_filledrecords neither and is retried.findFillreads the gateway'sOrderFilledlog for the commitment from where the order became fillable (its deadline less the order window in blocks, lessevidenceMarginBlocks) 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 withquoteOrderFeesand checksamountInplus 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.
partialEligiblealso runsassertGatewayReleaseon 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_TOKENSis nowRECOMMENDED_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.
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.Behaviour changes:
0 < triggerPercentage < 1no longer boots.unfilled, notreclaimed, until the escrow is refunded.getLogspages), not a few minutes around now.Validation
lintexit 0;test:data146,test:unit284 andtest:filler71 passed (5 skipped).bridge-top-up,token-sender,UserOpSenderandDelegationService*: 236 passed, 1 skipped..testnet).tscshows only the 14 errors already on main (3 of them in theoxdependency), none in changed files.validateConfigand the PUT route;_filledrule;CANCELLEDwith the escrow held →unfilled, with it empty and_filled == order.user→reclaimed, and a failed read → neither;findFilllocating a fill from long before now, stopping at the deadline or head, and throwing on a failed range;Not covered: no mainnet run.