feat: zones can restrict deposit/withdrawal and callback targets#709
feat: zones can restrict deposit/withdrawal and callback targets#7090xalpharush wants to merge 5 commits into
Conversation
0xalpharush
changed the title
|
cyclops audit |
|
cc @0xalpharush Cyclops audit event published. View workflow run Config: config: |
0xalpharush
changed the title
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review — VULNERABILITY_FOUND
PR #709 adds mutable zone access/gateway enforcement and expands the factory/create-zone ABI. Current head has partially addressed the earlier initializer-event gap by emitting initial mode/member events, but verified issues remain around L1-state cache completeness, fee accounting for closed-loop/refund paths, encrypted-deposit TIP-403 parity, and native factory rollout.
Body-only/non-diff finding: crates/l1/src/block.rs:105). Plaintext deposits require both Recipient and MintRecipient, while decrypted encrypted deposits are checked only as MintRecipient in the host builder / zone mint path. Compound policies can therefore reject a plaintext recipient but accept the same recipient via encrypted deposit. Fix: after decryption, require both roles deterministically in zone execution and bounce unauthorized encrypted deposits.
Reviewer Callouts
- ⚡ Head drift note: Commit
73a782baadded initializer events, so the original no-initializer-event subcase is partially stale; the remaining cache issues are floor-extrapolated RPC snapshots, followers/restarts without complete event history, and default-false mapping reads. - ⚡ Factory rollout: Tooling still targets fixed/default factory addresses; release must include a compatible native/precompile deployment, not only updated Solidity artifacts.
- ⚡ Gateway/router economics: Production gateways and routers need post-portal-fee minimum net output, and Enforced zones need explicit gateway registration during deployment.
- ⚡ Scope nuance: The bounce-back fee and encrypted-deposit policy issues appear to predate parts of this PR, but remain reachable on current head and are relevant to the deposit/refund semantics touched here.
| /// @notice Check account membership in the portal's admin-managed closed-loop allowlist. | ||
| /// @notice Read account allowlist enforcement from the dedicated packed mode slot. | ||
| function accessMode() public view returns (ZoneAccessMode) { | ||
| bytes32 value = tempoState.readTempoStorageSlot(tempoPortal, PORTAL_ACCESS_MODE_SLOT); |
There was a problem hiding this comment.
🚨 [SECURITY] Stale floor-cache L1 storage can make Closed/Enforced execute as Open/Open
This new mode read is consensus-critical, but the underlying L1StateProvider still stores exact historical RPC fallback values in a floor-lookup cache. Followers skip the portal-event subscriber and restarted/RPC-only nodes can miss intervening mode/member events, so an older Open/Open value can be reused for a later Tempo height and make replicas execute withdrawals differently. Current head emits initializer events, but it does not add a cache-completeness boundary or follower observation path.
Recommended Fix: Do not floor-serve RPC snapshots across unknown gaps. Store exact-block RPC results separately or require proven complete event history through the requested block; run an observation-only subscriber/seeded baseline for followers and restarts.
|
|
||
| /// @notice Check whether an account is authorized under the portal's access mode. | ||
| function isAllowedAccount(address account) external view returns (bool) { | ||
| if (accessMode() == ZoneAccessMode.Open) return true; |
There was a problem hiding this comment.
When access is Closed, ordinary negative allowedAccount lookups have no event-derived cache entry. The cache does not synthesize default-zero mapping values, so rejecting a non-member falls through to historical eth_getStorageAt; the synchronous execution path retries indefinitely if that historical RPC is unavailable/pruned. The initializer-event fix covers initial true entries, not arbitrary false keys.
Recommended Fix: Add default-zero semantics backed by a known complete portal-history range, or maintain per-slot completeness ranges and test empty Closed membership plus unregistered gateway lookups without relying on historical RPC availability.
| // intended result. Open access imposes no source-deposit invariant: callback value may go | ||
| // to another zone or leave the zone system entirely. | ||
| if ( | ||
| accessMode() == ZoneAccessMode.Closed |
There was a problem hiding this comment.
🚨 [SECURITY] Mutable deposit fees can drain closed-loop callback returns while only queue movement is checked
The Closed-access callback success condition only checks that the source deposit queue hash changed. A registered gateway/router can append a deposit with zero or tiny net amount if the sequencer raises zoneGasRate just before processWithdrawal(): the gateway deposits the gross callback amount, the portal pays it out as a deposit fee, the queue hash changes, and the withdrawal is marked successful instead of bouncing.
Recommended Fix: Bind callback success to post-fee value preservation. Snapshot/waive/cap portal deposit fees for gateway returns, and pass/enforce minNetAmount or maxDepositFee through callback data.
| internal | ||
| returns (uint128 fee, uint128 netAmount) | ||
| { | ||
| // FIXME(closed-loop-fees): Fix sequencer fees to zero and enforce onchain. |
There was a problem hiding this comment.
🚨 [SECURITY] Processing-time bouncebackGas can confiscate failed-deposit refunds
The deposit path checks the current bounce-back fee but does not store a cap or reserve. If the deposit later fails/rejects, _processDepositBounceBack() recomputes the fee from the current sequencer-controlled bouncebackGas and can cap it to the entire refund amount, paying the user’s failed-deposit refund to the sequencer.
Recommended Fix: Snapshot a user-approved maximum bounce-back fee or escrow the reserved fee at deposit time. If the current fee exceeds that cap, park/refund the principal instead of silently paying the excess to the sequencer.
| } | ||
| struct CreateZoneParams { | ||
| address initialToken; | ||
| ZoneAccessMode accessMode; |
There was a problem hiding this comment.
The local ABI now sends expanded CreateZoneParams to the fixed TIP-1091 factory address, but the protocol factory is a native Tempo precompile. The pinned/available native implementation still exposes the old selector/metadata and initializes a portal storage mirror that stops before the new mode and membership slots. Calls against the old native ABI fail, and a partial runtime-only rollout can leave requested Closed/Enforced zones initialized as zero-slot Open/Open.
Recommended Fix: Coordinate an atomic Tempo protocol update for native factory ABI/dispatch, ZoneCreated/zones() encoding, verifier()/messenger(), overlap validation, and slots 19-21 initialization; gate old Moderato defaults until that target is live.
2 participants
Stack 3 of 3; based on #707.
Makes account allowlist and callback gateway enforcement independently mutable and open by default:
ZoneConfigand RPC, with event-driven L1 cache updatesValidated with the full Forge suite (628 tests), Rust L1 tests and checks, mutable-mode and cross-zone E2Es, both former hang regressions, and storage/genesis parity.
L1 cache history coverage and follower freshness are intentionally deferred to #698 and remain required before merge.
The factory ABI, portal storage, and bundled bytecode changed, so this requires a fresh ZoneFactory deployment. No Moderato deployment is included.