code/cl8y-bridge-monorepo

Fork 0

security(evm): fail closed when guard and rate-limit bridge are unset #178

New issue

Open

opened 2026-09-12 11:53:17 +00:00 by PlasticDigits · 0 comments

PlasticDigits commented

2026-09-12 11:53:17 +00:00

Owner

Summary

EVM Bridge deposit/withdraw guard hooks and TokenRegistry withdraw rate-limit checks skip when their wiring addresses are zero. Both pointers are zero after initialize. Deploy scripts do not set them. Docs still treat wiring as a manual post-deploy step (OPERATIONAL_NOTES.md §8, docs/deployment-guide.md §6.1a).

This is not #176 (on-chain M-of-N operator approve). This is not #175 (Terra WithdrawApprove ignoring min_signatures). This is not #177 (timelock / cancel-window floor). This is not #135 (Terra admin handoff). Closed #122 / #121 are deploy orchestration, not fail-closed semantics. Closed #76 is test-suite breakage after rate-limit changes. Keyword overlap on “rate limit” / “guard” is not this bug.

Bundle (same ticket, do not split):

Fail closed for registered tokens until Bridge.guardBridge and TokenRegistry.rateLimitBridge are non-zero (or require non-zero addresses at initialize / first register).
Stop allowing owner setGuardBridge(address(0)) / setRateLimitBridge(address(0)) to silently disable enforcement on a live registry.
Tests that deposits and withdraw executes revert when either pointer is unset.
Docs and deploy scripts: wiring is a protocol invariant, not an optional checklist.

Founder-required contracts. No community autoland. Do not add ready.

Impact (today vs hypothetical)

Funds at risk today in source, and on any live chain whose owner skipped (or later cleared) the wiring step. Registered-token deposits and withdraw executes succeed with no GuardBridge modules (blacklist, TokenRateLimit modules, account checks) and no TokenRegistry withdraw min/max/period caps. Mainnet safety then depends on a missed owner call, not on the contract.

This is not a user-facing puzzle that needs an exploit recipe. Any holder of a registered token can deposit; any submitted+approved withdraw can execute. The defect is that those paths do not require the safety stack to be attached.

Hypothetical-only if every production proxy already has both pointers set and they cannot be cleared. Source still permits the gap at init and via address(0) setters. Docs already call an unset pointer a “critical” misconfiguration; the code still treats it as a no-op.

Do not publish a mainnet “deposit while unset” sequence or live RPC probe scripts.

EVM: `_checkDepositGuard` / `_checkWithdrawGuard` no-op on zero

Bridge.initialize takes admin, operator, fee recipient, wrapped native, registries, handlers, and thisChainId. It does not take a guard. guardBridge stays default address(0).

setGuardBridge is onlyOwner and documents address(0) as “disable”.

Called after fee math, before lock/burn:

depositNative
ERC-20 lock deposit
burn deposit

Called after decimal normalization, before unlock/mint:

withdrawExecuteUnlock
withdrawExecuteMint

Implementation:

if guardBridge != address(0) → IGuardBridge.checkDeposit / checkWithdraw
if tokenRegistry is set and tokenRegistry.rateLimitBridge() != address(0) → checkAndUpdateDepositRateLimit / checkAndUpdateWithdrawRateLimit
otherwise return

Zero on either pointer is success, not a revert.

TokenRegistry: withdraw limit returns on zero; deposit check is a no-op even when wired

TokenRegistry.initialize(admin, chainRegistry) does not take a bridge. rateLimitBridge stays address(0).

setRateLimitBridge is onlyOwner and documents address(0) as “disable”.

checkAndUpdateWithdrawRateLimit:

if rateLimitBridge == address(0) → return (no min/max/period)
else require msg.sender == rateLimitBridge, then _checkAndUpdateRateLimit

registerToken still writes default rateLimitConfigs from supply. Those configs never run until rateLimitBridge is the Bridge proxy.

checkAndUpdateDepositRateLimit is external pure and does nothing. Deposit-side registry caps are out of this ticket (do not invent them). Fail-closed here means: registered-token deposits still need guardBridge, and registered-token withdraw executes need both guardBridge and rateLimitBridge.

Docs and deploy still encode fail-open

packages/contracts-evm/OPERATIONAL_NOTES.md §8: “Guard disabled by default”; “when disabled, all guard checks are no-ops”; production unset is “critical” but the fix is “verify after deploy.”

docs/deployment-guide.md: Deploy.s.sol does not call TokenRegistry.setRateLimitBridge or Bridge.setGuardBridge. §6.1a / §9.3 are operator cast call checklists.

OPERATIONAL_NOTES.md §9: Guard–Bridge integration tests “not required.” GuardBridge.t.sol tests the module in isolation. Happy-path Bridge.t.sol fixtures can pass with both pointers still zero.

Why the new implementation is needed

A safety control that is optional at the opcode level is not a control. Checklists do not survive a new-chain deploy, a proxy clone, or an owner “disable for debugging” call.
registerToken already assumes limits exist (defaults from supply). Enforcement that returns on zero contradicts that invariant.
Tests today can lock in fail-open. Without a revert-when-unset fixture, the next deploy script change will not fail CI.

Constraints / guardrails

Do not remove GuardBridge modules, TokenRateLimit, blacklist, pause, cancelers, or (srcChain, nonce) replay. This ticket makes those controls mandatory to attach, not weaker.
Fail closed for registered tokens. Unregistered-token paths should keep their existing “not registered” reverts; do not invent a second error that leaks through registration.
initialize may keep a zero default if deploy order deploys GuardBridge after the proxy, provided every user-facing deposit/withdraw of a registered token reverts until both pointers are non-zero. If initialize/register instead require non-zero addresses, update Foundry scripts in the same change so local and parity replays still boot.
setGuardBridge(address(0)) / setRateLimitBridge(address(0)) must not restore fail-open on a populated registry. Revert on zero, or require pause and still revert registered-token transfers while unset. Owner pause remains the incident brake.
Do not require rateLimitBridge to enforce deposit-side registry caps (those are a documented no-op). Deposit fail-closed is the guard pointer; withdraw fail-closed is both pointers.
TokenRegistry rate-limit caller check stays: only the wired Bridge may consume the 24h window. Do not open checkAndUpdateWithdrawRateLimit to arbitrary callers.
UUPS / storage: do not break __gap / existing proxy layout. New errors and require-nonzero checks are enough; avoid a new initializer version unless a new field is actually required.
AccessManager role split for guard-stack admin (role 2 vs minter role 1) stays. This ticket does not retarget #98.
No community autoland. Do not add ready. No public mainnet unset-pointer recipe.

Relevant files

Path	Why
`packages/contracts-evm/src/Bridge.sol`	`guardBridge` unset at init; `_checkDepositGuard` / `_checkWithdrawGuard` skip on zero; `setGuardBridge` allows zero
`packages/contracts-evm/src/TokenRegistry.sol`	`rateLimitBridge` unset at init; `checkAndUpdateWithdrawRateLimit` returns on zero; `setRateLimitBridge` allows zero
`packages/contracts-evm/src/GuardBridge.sol`	Module dispatcher; never reached when `guardBridge == 0`
`packages/contracts-evm/src/TokenRateLimit.sol`	Guard-stack rate limit; never reached when guard is unwired
`packages/contracts-evm/script/Deploy.s.sol` (and DeployPart1 / local deploy scripts)	Do not call `setGuardBridge` / `setRateLimitBridge`
`packages/contracts-evm/test/Bridge.t.sol`	Happy-path deposits/withdraws must gain revert-when-unset + wired-success cases
`packages/contracts-evm/test/TokenRegistry.t.sol`	Rate-limit tests must not treat zero `rateLimitBridge` as a valid production state
`packages/contracts-evm/test/GuardBridge.t.sol`	Isolation tests stay; add Bridge integration coverage here or in `Bridge.t.sol`
`packages/contracts-evm/OPERATIONAL_NOTES.md`	§8 “disabled by default” / §9 “integration tests not required”
`docs/deployment-guide.md`	§6.1a / §9.3 checklist instead of a contract invariant

Recommended direction

Transfer-time invariant: in _checkDepositGuard / _checkWithdrawGuard, if the token is registered, revert when guardBridge == address(0). On withdraw, also revert when rateLimitBridge() == address(0) before calling into the registry. Dedicated errors (e.g. GuardBridgeNotSet / RateLimitBridgeNotSet used as a real revert, not a silent skip).
Setter invariant: setGuardBridge(0) and setRateLimitBridge(address(0)) revert (or only allowed while paused and transfers still revert). Update natspec; remove “address(0) to disable.”
Optional init tightening: if deploy order allows, pass non-zero guard/bridge into initialize or a post-init wireSafetyStack that must run before unpause. Pause-at-init until wired is acceptable.
Deploy scripts: after GuardBridge + TokenRateLimit + modules exist, call setGuardBridge and setRateLimitBridge(bridgeProxy) in the same broadcast path as register-token. Parity replay / MegaETH scripts must not complete “success” with zeros.
Docs: replace “verify or you are unsafe” with “transfers revert until wired.” Delete the “integration tests not required” row for Guard–Bridge.

Acceptance criteria

AC1. After initialize, with a registered token and guardBridge == 0, depositNative / lock deposit / burn deposit revert. They succeed only after setGuardBridge to a live GuardBridge.
AC2. Withdrawn execute (unlock and mint) of a registered token reverts while guardBridge == 0 or rateLimitBridge == 0. Both must be non-zero for execute to reach lock/mint logic.
AC3. setGuardBridge(address(0)) and setRateLimitBridge(address(0)) cannot return the proxy to a transferring fail-open state.
AC4. When both pointers are set, existing guard modules and withdraw min/max/period still revert on violation (blacklist, per-tx max, period max). Wiring is not a bypass.
AC5. checkAndUpdateDepositRateLimit may remain a no-op; deposit safety is GuardBridge, not a new TokenRegistry deposit window, unless product explicitly adds one in this same change.
AC6. Docs/scripts no longer describe unset pointers as a supported production mode. OPERATIONAL_NOTES §9 does not excuse missing integration tests.

Test plan (functional paths)

#	Path	Expect
T1	Init, register token, deposit, `guardBridge == 0`	Revert (`GuardBridgeNotSet` or equivalent)
T2	Init, register, `setGuardBridge(gb)`, deposit	Guard `checkDeposit` runs; happy path succeeds
T3	Approve withdraw, execute, `rateLimitBridge == 0`	Revert even if `guardBridge` is set
T4	Both pointers set, execute after cancel window	Unlock/mint succeed; registry window updates
T5	`setGuardBridge(0)` after T2	Revert (or pause + deposits still revert)
T6	`setRateLimitBridge(0)` after wiring	Revert (or pause + execute still revert)
T7	Unregistered token deposit	Existing not-registered revert (unchanged)
T8	Wired + `setRateLimit` maxPerTx, execute above max	Existing `RateLimitExceededPerTx`
T9	Deploy script / local deploy fixture	Post-broadcast `guardBridge` and `rateLimitBridge` non-zero, or the first user deposit reverts until a documented wire step that tests also run

Test plan (attack, hack, and abuse)

Non-exploitative. Local Forge only. Do not use these as a mainnet recipe.

#	Vector	Expect
A1	Skip `setGuardBridge` after deploy, deposit registered token	Revert; no lock/burn
A2	Skip `setRateLimitBridge`, submit+approve, execute	Revert; no unlock/mint
A3	Owner sets pointer back to zero after going live	Cannot silently disable; transfers stay closed or paused
A4	Non-bridge caller hits `checkAndUpdateWithdrawRateLimit` once wired	Still `RateLimitBridgeNotSet` / caller check
A5	Empty `GuardBridge` (zero modules) vs unset pointer	Unset still reverts. Empty module set is a separate ops gap; do not treat it as “wired” if product requires at least one deposit/withdraw module — document the choice.
A6	Pause then unset (if that exception exists)	Unpause without re-wire still fail-closed

Verification criteria

Forge: new tests for T1–T6 and A1–A4 in Bridge.t.sol / TokenRegistry.t.sol. Existing happy-path tests updated to wire the stack in setUp (or they now expect revert — do not leave skip-on-zero as the default fixture).
forge test in packages/contracts-evm green, including Bridge.inv.t.sol.
Grep: _checkDepositGuard / _checkWithdrawGuard / checkAndUpdateWithdrawRateLimit no longer return / skip on address(0) for registered tokens.
Docs: §8/§6.1a language matches revert-until-wired. No “no-op skip” production mode.
Do not verify by depositing on a production RPC.

Out of scope

Terra / Solana rate-limit or guard modules (EVM-only fail-open).
Operator M-of-N (#176), Terra min_signatures (#175), cancel-window timelock (#177).
Adding deposit-side TokenRegistry windows (unless done as an explicit extra in this change).
AccessManager role-id remap (#98).
Live owner calls to wire or unwire production proxies (ops, not this issue).

First-pass model recommendation

Recommendation: grok-high

Rationale: Security class plus founder-required contracts (Bridge + TokenRegistry + GuardBridge + deploy scripts). Composer is disallowed (High/security; contracts/auth/keys; fail-open → fail-closed is a protocol invariant, not a local three-file edit). A wrong exception (unset still succeeds, or address(0) setter restores skip) leaves registered tokens uncapped. Verify with Forge revert/success fixtures, not a production deposit.

## Summary EVM `Bridge` deposit/withdraw guard hooks and `TokenRegistry` withdraw rate-limit checks **skip when their wiring addresses are zero**. Both pointers are zero after `initialize`. Deploy scripts do not set them. Docs still treat wiring as a **manual post-deploy** step (`OPERATIONAL_NOTES.md` §8, `docs/deployment-guide.md` §6.1a). This is **not** [#176](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/176) (on-chain M-of-N operator approve). This is **not** [#175](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/175) (Terra `WithdrawApprove` ignoring `min_signatures`). This is **not** [#177](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/177) (timelock / cancel-window floor). This is **not** [#135](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/135) (Terra admin handoff). Closed [#122](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/122) / [#121](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/121) are deploy orchestration, not fail-closed semantics. Closed [#76](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/76) is test-suite breakage after rate-limit changes. Keyword overlap on “rate limit” / “guard” is not this bug. Bundle (same ticket, do not split): 1. Fail closed for **registered** tokens until `Bridge.guardBridge` and `TokenRegistry.rateLimitBridge` are non-zero (or require non-zero addresses at initialize / first register). 2. Stop allowing owner `setGuardBridge(address(0))` / `setRateLimitBridge(address(0))` to silently disable enforcement on a live registry. 3. Tests that deposits and withdraw executes **revert** when either pointer is unset. 4. Docs and deploy scripts: wiring is a protocol invariant, not an optional checklist. Founder-required contracts. No community autoland. Do not add `ready`. ## Impact (today vs hypothetical) **Funds at risk today in source, and on any live chain whose owner skipped (or later cleared) the wiring step.** Registered-token deposits and withdraw executes succeed with **no** `GuardBridge` modules (blacklist, `TokenRateLimit` modules, account checks) and **no** `TokenRegistry` withdraw min/max/period caps. Mainnet safety then depends on a missed owner call, not on the contract. This is not a user-facing puzzle that needs an exploit recipe. Any holder of a registered token can deposit; any submitted+approved withdraw can execute. The defect is that those paths do not require the safety stack to be attached. Hypothetical-only if every production proxy already has both pointers set **and** they cannot be cleared. Source still permits the gap at init and via `address(0)` setters. Docs already call an unset pointer a “critical” misconfiguration; the code still treats it as a no-op. Do not publish a mainnet “deposit while unset” sequence or live RPC probe scripts. ### EVM: `_checkDepositGuard` / `_checkWithdrawGuard` no-op on zero `Bridge.initialize` takes admin, operator, fee recipient, wrapped native, registries, handlers, and `thisChainId`. It does **not** take a guard. `guardBridge` stays default `address(0)`. `setGuardBridge` is `onlyOwner` and documents `address(0)` as “disable”. Called after fee math, before lock/burn: - `depositNative` - ERC-20 lock deposit - burn deposit Called after decimal normalization, before unlock/mint: - `withdrawExecuteUnlock` - `withdrawExecuteMint` Implementation: - if `guardBridge != address(0)` → `IGuardBridge.checkDeposit` / `checkWithdraw` - if `tokenRegistry` is set **and** `tokenRegistry.rateLimitBridge() != address(0)` → `checkAndUpdateDepositRateLimit` / `checkAndUpdateWithdrawRateLimit` - otherwise **return** Zero on either pointer is success, not a revert. ### TokenRegistry: withdraw limit returns on zero; deposit check is a no-op even when wired `TokenRegistry.initialize(admin, chainRegistry)` does not take a bridge. `rateLimitBridge` stays `address(0)`. `setRateLimitBridge` is `onlyOwner` and documents `address(0)` as “disable”. `checkAndUpdateWithdrawRateLimit`: - if `rateLimitBridge == address(0)` → **return** (no min/max/period) - else require `msg.sender == rateLimitBridge`, then `_checkAndUpdateRateLimit` `registerToken` still writes default `rateLimitConfigs` from supply. Those configs never run until `rateLimitBridge` is the Bridge proxy. `checkAndUpdateDepositRateLimit` is `external pure` and does nothing. Deposit-side registry caps are out of this ticket (do not invent them). Fail-closed here means: registered-token **deposits still need `guardBridge`**, and registered-token **withdraw executes need both** `guardBridge` and `rateLimitBridge`. ### Docs and deploy still encode fail-open `packages/contracts-evm/OPERATIONAL_NOTES.md` §8: “Guard disabled by default”; “when disabled, all guard checks are no-ops”; production unset is “critical” but the fix is “verify after deploy.” `docs/deployment-guide.md`: `Deploy.s.sol` does **not** call `TokenRegistry.setRateLimitBridge` or `Bridge.setGuardBridge`. §6.1a / §9.3 are operator `cast call` checklists. `OPERATIONAL_NOTES.md` §9: Guard–Bridge **integration tests** “not required.” `GuardBridge.t.sol` tests the module in isolation. Happy-path `Bridge.t.sol` fixtures can pass with both pointers still zero. ## Why the new implementation is needed 1. A safety control that is optional at the opcode level is not a control. Checklists do not survive a new-chain deploy, a proxy clone, or an owner “disable for debugging” call. 2. `registerToken` already assumes limits exist (defaults from supply). Enforcement that returns on zero contradicts that invariant. 3. Tests today can lock in fail-open. Without a revert-when-unset fixture, the next deploy script change will not fail CI. ## Constraints / guardrails - Do not remove `GuardBridge` modules, `TokenRateLimit`, blacklist, pause, cancelers, or `(srcChain, nonce)` replay. This ticket makes those controls **mandatory to attach**, not weaker. - Fail closed for **registered** tokens. Unregistered-token paths should keep their existing “not registered” reverts; do not invent a second error that leaks through registration. - `initialize` may keep a zero default if deploy order deploys `GuardBridge` after the proxy, **provided** every user-facing deposit/withdraw of a registered token reverts until both pointers are non-zero. If initialize/register instead require non-zero addresses, update Foundry scripts in the same change so local and parity replays still boot. - `setGuardBridge(address(0))` / `setRateLimitBridge(address(0))` must not restore fail-open on a populated registry. Revert on zero, or require pause **and** still revert registered-token transfers while unset. Owner pause remains the incident brake. - Do not require `rateLimitBridge` to enforce deposit-side registry caps (those are a documented no-op). Deposit fail-closed is the **guard** pointer; withdraw fail-closed is **both** pointers. - `TokenRegistry` rate-limit caller check stays: only the wired Bridge may consume the 24h window. Do not open `checkAndUpdateWithdrawRateLimit` to arbitrary callers. - UUPS / storage: do not break `__gap` / existing proxy layout. New errors and require-nonzero checks are enough; avoid a new initializer version unless a new field is actually required. - `AccessManager` role split for guard-stack admin (role `2` vs minter role `1`) stays. This ticket does not retarget [#98](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/98). - No community autoland. Do not add `ready`. No public mainnet unset-pointer recipe. ## Relevant files | Path | Why | | --- | --- | | `packages/contracts-evm/src/Bridge.sol` | `guardBridge` unset at init; `_checkDepositGuard` / `_checkWithdrawGuard` skip on zero; `setGuardBridge` allows zero | | `packages/contracts-evm/src/TokenRegistry.sol` | `rateLimitBridge` unset at init; `checkAndUpdateWithdrawRateLimit` returns on zero; `setRateLimitBridge` allows zero | | `packages/contracts-evm/src/GuardBridge.sol` | Module dispatcher; never reached when `guardBridge == 0` | | `packages/contracts-evm/src/TokenRateLimit.sol` | Guard-stack rate limit; never reached when guard is unwired | | `packages/contracts-evm/script/Deploy.s.sol` (and DeployPart1 / local deploy scripts) | Do not call `setGuardBridge` / `setRateLimitBridge` | | `packages/contracts-evm/test/Bridge.t.sol` | Happy-path deposits/withdraws must gain revert-when-unset + wired-success cases | | `packages/contracts-evm/test/TokenRegistry.t.sol` | Rate-limit tests must not treat zero `rateLimitBridge` as a valid production state | | `packages/contracts-evm/test/GuardBridge.t.sol` | Isolation tests stay; add Bridge integration coverage here or in `Bridge.t.sol` | | `packages/contracts-evm/OPERATIONAL_NOTES.md` | §8 “disabled by default” / §9 “integration tests not required” | | `docs/deployment-guide.md` | §6.1a / §9.3 checklist instead of a contract invariant | ## Recommended direction 1. **Transfer-time invariant:** in `_checkDepositGuard` / `_checkWithdrawGuard`, if the token is registered, revert when `guardBridge == address(0)`. On withdraw, also revert when `rateLimitBridge() == address(0)` before calling into the registry. Dedicated errors (e.g. `GuardBridgeNotSet` / `RateLimitBridgeNotSet` used as a real revert, not a silent skip). 2. **Setter invariant:** `setGuardBridge(0)` and `setRateLimitBridge(address(0))` revert (or only allowed while paused **and** transfers still revert). Update natspec; remove “address(0) to disable.” 3. **Optional init tightening:** if deploy order allows, pass non-zero guard/bridge into initialize or a post-init `wireSafetyStack` that must run before `unpause`. Pause-at-init until wired is acceptable. 4. **Deploy scripts:** after GuardBridge + TokenRateLimit + modules exist, call `setGuardBridge` and `setRateLimitBridge(bridgeProxy)` in the same broadcast path as register-token. Parity replay / MegaETH scripts must not complete “success” with zeros. 5. **Docs:** replace “verify or you are unsafe” with “transfers revert until wired.” Delete the “integration tests not required” row for Guard–Bridge. ## Acceptance criteria - AC1. After `initialize`, with a registered token and `guardBridge == 0`, `depositNative` / lock deposit / burn deposit revert. They succeed only after `setGuardBridge` to a live `GuardBridge`. - AC2. Withdrawn execute (unlock and mint) of a registered token reverts while `guardBridge == 0` **or** `rateLimitBridge == 0`. Both must be non-zero for execute to reach lock/mint logic. - AC3. `setGuardBridge(address(0))` and `setRateLimitBridge(address(0))` cannot return the proxy to a transferring fail-open state. - AC4. When both pointers are set, existing guard modules and withdraw min/max/period still revert on violation (blacklist, per-tx max, period max). Wiring is not a bypass. - AC5. `checkAndUpdateDepositRateLimit` may remain a no-op; deposit safety is `GuardBridge`, not a new TokenRegistry deposit window, unless product explicitly adds one in this same change. - AC6. Docs/scripts no longer describe unset pointers as a supported production mode. `OPERATIONAL_NOTES` §9 does not excuse missing integration tests. ## Test plan (functional paths) | # | Path | Expect | | --- | --- | --- | | T1 | Init, register token, deposit, `guardBridge == 0` | Revert (`GuardBridgeNotSet` or equivalent) | | T2 | Init, register, `setGuardBridge(gb)`, deposit | Guard `checkDeposit` runs; happy path succeeds | | T3 | Approve withdraw, execute, `rateLimitBridge == 0` | Revert even if `guardBridge` is set | | T4 | Both pointers set, execute after cancel window | Unlock/mint succeed; registry window updates | | T5 | `setGuardBridge(0)` after T2 | Revert (or pause + deposits still revert) | | T6 | `setRateLimitBridge(0)` after wiring | Revert (or pause + execute still revert) | | T7 | Unregistered token deposit | Existing not-registered revert (unchanged) | | T8 | Wired + `setRateLimit` maxPerTx, execute above max | Existing `RateLimitExceededPerTx` | | T9 | Deploy script / local deploy fixture | Post-broadcast `guardBridge` and `rateLimitBridge` non-zero, or the first user deposit reverts until a documented wire step that tests also run | ## Test plan (attack, hack, and abuse) Non-exploitative. Local Forge only. Do not use these as a mainnet recipe. | # | Vector | Expect | | --- | --- | --- | | A1 | Skip `setGuardBridge` after deploy, deposit registered token | Revert; no lock/burn | | A2 | Skip `setRateLimitBridge`, submit+approve, execute | Revert; no unlock/mint | | A3 | Owner sets pointer back to zero after going live | Cannot silently disable; transfers stay closed or paused | | A4 | Non-bridge caller hits `checkAndUpdateWithdrawRateLimit` once wired | Still `RateLimitBridgeNotSet` / caller check | | A5 | Empty `GuardBridge` (zero modules) vs unset pointer | Unset still reverts. Empty module set is a separate ops gap; do not treat it as “wired” if product requires at least one deposit/withdraw module — document the choice. | | A6 | Pause then unset (if that exception exists) | Unpause without re-wire still fail-closed | ## Verification criteria - Forge: new tests for T1–T6 and A1–A4 in `Bridge.t.sol` / `TokenRegistry.t.sol`. Existing happy-path tests updated to wire the stack in `setUp` (or they now expect revert — do not leave skip-on-zero as the default fixture). - `forge test` in `packages/contracts-evm` green, including `Bridge.inv.t.sol`. - Grep: `_checkDepositGuard` / `_checkWithdrawGuard` / `checkAndUpdateWithdrawRateLimit` no longer `return` / skip on `address(0)` for registered tokens. - Docs: §8/§6.1a language matches revert-until-wired. No “no-op skip” production mode. - Do not verify by depositing on a production RPC. ## Out of scope - Terra / Solana rate-limit or guard modules (EVM-only fail-open). - Operator M-of-N (#176), Terra `min_signatures` (#175), cancel-window timelock (#177). - Adding deposit-side `TokenRegistry` windows (unless done as an explicit extra in this change). - AccessManager role-id remap (#98). - Live owner calls to wire or unwire production proxies (ops, not this issue). ## First-pass model recommendation Recommendation: grok-high Rationale: Security class plus founder-required contracts (Bridge + TokenRegistry + GuardBridge + deploy scripts). Composer is disallowed (High/security; contracts/auth/keys; fail-open → fail-closed is a protocol invariant, not a local three-file edit). A wrong exception (unset still succeeds, or `address(0)` setter restores skip) leaves registered tokens uncapped. Verify with Forge revert/success fixtures, not a production deposit.

PlasticDigits added the

high-risk

security

labels

2026-09-12 11:53:19 +00:00

PlasticDigits referenced this issue

2026-09-12 12:24:14 +00:00

security(evm): stop recoverAsset from sweeping native ETH deposits #179

PlasticDigits referenced this issue

2026-09-12 13:07:44 +00:00

security(evm): reject fee-on-transfer ERC20 deposits that deliver less than recorded #189

PlasticDigits referenced this issue