feat(l1+precompiles): add L1-backed zone storage provider#626
feat(l1+precompiles): add L1-backed zone storage provider#6260xrusowsky wants to merge 17 commits into
Conversation
0xrusowsky
changed the title
7e797c0 to
6e20d87
Compare
|
cyclops audit |
d96421e to
5ba8103
Compare
|
cyclops audit |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #626 adds mutation-aware L1 state-cache barriers, shares the cache with the subscriber/EVM/engine, and prunes it after accepted L1-backed zone blocks. I found three actionable issues in the integration; see inline comments for details. VULNERABILITY_FOUND
Reviewer Callouts
- ⚡ ZonePortal raw storage surface: Review every ZonePortal storage slot reachable through
TempoStateand ensure each is either exact-diffed by subscriber events or protected by conservative portal invalidation barriers. - ⚡ Consensus vs RPC cache boundary: Decide which execution modes may populate consensus-critical node state; read-only RPC currently writes into the cache pruned on the block-production path.
- ⚡ Historical execution contract: If historical replay/RPC is supported after pruning, enforce hash-qualified L1 lookups, bounded errors, and archival RPC requirements.
- ⚡ Sequencer privacy lookup: Workers flagged a pre-existing sensitive boundary where
ZoneTip20Tokenprivate-read authorization usesSequencerExt::latest_sequencer()rather than the zone-committedtempoBlockNumber; this was not consolidated as a PR #626 finding but merits separate review.
|
cyclops audit note="verify whether the proposed fix addresses the concern about 3rd party attacks via eth_call #626 (comment)" |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
PR #626 adds mutation barriers and a floor-driven lazy-pruning scheme for the raw L1 state cache. The forward execution path looks mostly sound, but two cache issues remain actionable: untrusted eth_call traffic can still grow the shared cache without a real bound, and pruned invalidation history can make below-floor historical reads return stale L1 storage.
Reviewer Callouts
- ⚡ Floor ownership on non-sequencer/RPC roles:
advance_floor()is driven byZoneEngine::advance, while the engine is only spawned whensequencer_configis present. If non-sequencer nodes are intended to expose RPC, confirm how their sharedL1StateCacheis bounded. - ⚡ RPC-to-system-contract boundary: private RPC authorization controls
from, but public system contracts such asZoneConfigcan still invoke theTempoStateprecompile as an allowed immediate caller. Treat EVM simulations as untrusted writers unless cache admission is separated by execution context.
0xrusowsky
changed the title
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
Holistic pass after the stacked changes were consolidated into #626. I found four approval blockers and one follower-lifetime issue; inline details below. The previously discussed global cache-cardinality issue remains accepted follow-up work.
| sequencer.clone(), | ||
| )) | ||
| } else if *address == TIP_FEE_MANAGER_ADDRESS { | ||
| Some(execution::create_l1_backed_precompile( |
There was a problem hiding this comment.
[P1] Apply the L1 overlay to protocol fee collection — This wraps only external calls to the FeeManager address. Tempo transaction handling invokes ProtocolFeeManager::collect_fee_pre_tx directly under the ordinary EVM storage provider, so that path never reaches this lookup or ZonePrecompileStorageProvider. Zone tokens keep local transferPolicyId = 1; consequently a sender blacklisted by the anchored L1 policy can still pay the fee-token transfer and execute a transaction whose body does not call TIP-20. Please route the internal fee hooks through the same anchored overlay and add an empty-transaction regression test for an L1-blacklisted sender.
There was a problem hiding this comment.
pending upstream tempoxyz/tempo#6879 to unlock the fix
There was a problem hiding this comment.
tentatively fixed with 1e130d4 (this PR) which assumes upstream PR gets merged as is
| extend_zone_precompiles( | ||
| precompiles, | ||
| &cfg, | ||
| self.l1_provider.clone(), |
There was a problem hiding this comment.
[P1] Do not install the dead L1 reader in executable CLI configs — new_without_l1() supplies 127.0.0.1:1; this provider now reaches every migrated L1-backed precompile through this argument, while get_storage() retries forever. import, stage, or re-execute will hang on the first replayed block with a TIP-20/TIP-403 storage read. Please give maintenance execution a usable historical L1 source or fail before execution instead of installing an indefinitely retrying reader.
| self.check_balance_read(decoded.account, call.caller) | ||
| }) | ||
| } | ||
| _ => Ok(()), |
There was a problem hiding this comment.
[P1] Preserve privacy for reward getters — Both userRewardInfo(address) and getPendingRewards(address) now fall through this default branch, allowing an authenticated outsider to query another account reward recipient, index, and pending balance. The previous wrapper gated both using the balance-read rule, and the associated tests were removed. Add both selectors through decode_and_check(... check_balance_read) and restore owner/sequencer/outsider coverage.
| // Populate raw L1 state: policy 5 = WHITELIST, Alice is in the set, Bob is not. | ||
| seed_raw_tip403_policy( | ||
| zone.l1_state_cache(), | ||
| 1, |
There was a problem hiding this comment.
[P1] Seed before advancing the cache floor — This test waits until tempoBlockNumber == 3 and then seeds at block 1. L1StateCacheInner::set discards writes below block_floor (now 3), so the first registry call misses and the local dummy L1 reader retries forever. The same ordering appears in the blacklist, compound, and block-versioned tests. Seed versions before advancing the floor or use the current accepted anchor; the helper should also surface rejected insertions.
| } | ||
|
|
||
| /// Seed the absence of an address-level TIP-403 receive policy for the next fixture block. | ||
| pub(crate) fn seed_no_receive_policy(&self, recipient: Address) { |
There was a problem hiding this comment.
[P1] Seed receive-policy slots for all transfer fixtures — Only the migrated policy test calls this helper. The unchanged transfer suite (test_deposit_then_transfer, sequential transfers, events, and memo) executes T6+ transfers to Bob or Charlie without seeding their receive-policy slots; estimates or execution miss the cache and retry the dummy RPC forever. Seed recipients before estimate/send, or make fixture misses finite and explicit.
| // The engine has committed this L1 height, so advance the forward-cache floor. | ||
| // Individual slot and mutation histories are compacted lazily when next touched, so | ||
| // RPC-created slots cannot amplify engine work. | ||
| self.l1_state_cache |
There was a problem hiding this comment.
[P2] Advance the floor on follower imports too — This is only called from ZoneEngine::advance, but that engine is spawned only when sequencer_config is present. The subscriber still runs on non-sequencer nodes and adds invalidations and slot versions while the floor remains zero, so follower memory grows for the lifetime of the process. Drive the floor from the common canonical import/commit path for every node role and initialize it from local TempoState on startup.
mattsse
left a comment
mattsse
left a comment
There was a problem hiding this comment.
Follow-up holistic review at 2ec6aab0. The bounded retry, fixture seeding, receive-policy seeding, and canonical follower-floor fixes look good. Two P1 approval blockers remain; inline comments include concrete implementation suggestions.
| sequencer.clone(), | ||
| )) | ||
| } else if *address == TIP_FEE_MANAGER_ADDRESS { | ||
| Some(execution::create_l1_backed_precompile( |
There was a problem hiding this comment.
[P1] Apply the anchored L1 overlay to internal fee collection
This lookup only wraps external calls to TIP_FEE_MANAGER_ADDRESS. Tempo's transaction handler invokes TipFeeManager::collect_fee_pre_tx and collect_fee_post_tx directly against the ordinary journal-backed StorageCtx, so those fee-token transfers never see ZonePrecompileStorageProvider. An L1-blacklisted fee payer can therefore submit an otherwise empty transaction and still execute it.
Suggested implementation: factor the anchored policy-storage setup out of create_l1_backed_precompile and make it available to ZoneBlockExecutor/the Tempo fee hooks. Resolve the TempoState anchor once for the transaction, then run both pre- and post-fee collection through the same ZonePrecompileStorageProvider (preserving local balances/allowances while sourcing TIP-403 policy slots from L1). Add an end-to-end empty-call transaction test with the sender blacklisted at the anchor; it should be rejected before the body executes.
There was a problem hiding this comment.
pending upstream tempoxyz/tempo#6879 to unlock the fix
There was a problem hiding this comment.
tentatively fixed with 1e130d4 (this PR) which assumes upstream PR gets merged as is
| self.check_balance_read(decoded.account, call.caller) | ||
| }) | ||
| } | ||
| ITIP20::allowanceCall::SELECTOR => { |
There was a problem hiding this comment.
[P1] Restore privacy checks for reward getters
userRewardInfo(address) and getPendingRewards(address) still fall through to upstream dispatch without applying the account/sequencer authorization used by the other private reads. The previous wrapper protected both selectors, so this exposes per-account reward state to arbitrary callers.
This compiles against the current ITIP20 bindings:
| ITIP20::allowanceCall::SELECTOR => { | |
| ITIP20::userRewardInfoCall::SELECTOR => { | |
| decode_and_check::<ITIP20::userRewardInfoCall>(args, |decoded| { | |
| self.check_balance_read(decoded.account, call.caller) | |
| }) | |
| } | |
| ITIP20::getPendingRewardsCall::SELECTOR => { | |
| decode_and_check::<ITIP20::getPendingRewardsCall>(args, |decoded| { | |
| self.check_balance_read(decoded.account, call.caller) | |
| }) | |
| } | |
| ITIP20::allowanceCall::SELECTOR => { |
Please also restore owner/sequencer/outsider coverage for both selectors; outsiders should receive Unauthorized.
There was a problem hiding this comment.
i did it on purpose cause rewards are disabled on TIP20s now but the selectors were preserved as a no-op.
IMO fine to leave as is (cause they won't like anything and simply return 0), and simply update the spec.
wdyt @0xKitsune @mattsse?
|
cyclops audit note="we decided we will address unbounded cache growth in a follow-up PR, so don't report this issue" |
|
cc @0xrusowsky Cyclops audit event published. View workflow run Config: config: |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
tempoxyz-bot
left a comment
tempoxyz-bot
left a comment
There was a problem hiding this comment.
👁️ Cyclops Review
Head drift detected: the audit reports targeted 1e130d4acd7c, while the current PR head is d4db039ac9e148f3d239ca19a48757623b886287. I rechecked the candidate findings against the current head and diff; the actionable findings below still apply.
PR #626 refactors zone precompiles to execute TIP-20/TIP-403/FeeManager logic over a finalized L1-backed storage overlay. The implementation has multiple security regressions in the new TIP-20 rule layer and L1 integration: private reads can be bypassed or use the wrong L1 anchor, reward views lost their privacy gate, receive-policy recovery is omitted, and cached policy IDs can become stale after subscriber reconnects.
Reviewer Callouts
- ⚡ Dynamic precompile lookup subset:
set_precompile_lookupreplaces Tempo's upstream dynamic lookup. Any upstream precompile needed by forwarded TIP-20/TIP-403/FeeManager code must be explicitly re-registered by the zone. - ⚡ L1 storage hash anchoring:
TempoStatestores an L1 hash and number, but the L1 storage reader API passes only a block number to RPC. Please confirm that finality/trusted-RPC assumptions make number-only reads acceptable for proof/replay requirements. - ⚡ Decode symmetry: Any rule layer that pre-decodes calldata with stricter validation than the executor it guards can fail open. Review other
abi_decode_raw_validategates in front of non-validating upstream dispatch.
| ) -> CallCheckResult { | ||
| match C::abi_decode_raw_validate(args) { | ||
| Ok(decoded) => check(decoded), | ||
| Err(_) => Ok(()), |
There was a problem hiding this comment.
🚨 [SECURITY] TIP-20 privacy checks fail open on non-canonical ABI encoding
decode_and_check returns Ok(()) on strict ABI decode failure. That maps to CallCheck::Continue, so the original calldata is forwarded to upstream TIP-20. Upstream dispatch uses non-validating decoding, so dirty high bytes in an address word can skip the zone owner/sequencer check while still decoding upstream to the victim account, leaking balanceOf, allowance, or hasRole data.
Recommended Fix:
Fail closed for decode errors on privacy-gated selectors, matching the previous decode_or_revert! behavior, or decode with the same ABI semantics upstream uses and enforce the check on those decoded values.
| self.check_balance_read(decoded.account, call.caller) | ||
| }) | ||
| } | ||
| _ => Ok(()), |
There was a problem hiding this comment.
🚨 [SECURITY] zTIP20 reward views bypass the owner/sequencer privacy gate
The catch-all admits selectors that are not explicitly listed. The upstream account-scoped userRewardInfo(address) and getPendingRewards(address) selectors were gated before this PR, but now fall through and are forwarded for any caller, exposing per-account reward state and potentially balance-derived information.
Recommended Fix:
Add explicit arms for ITIP20::userRewardInfoCall::SELECTOR and ITIP20::getPendingRewardsCall::SELECTOR that decode account and call check_balance_read(account, call.caller), with regression tests for unrelated callers.
|
|
||
| fn is_sequencer(&self, caller: Address) -> bool { | ||
| self.sequencer | ||
| fn check_sequencer(&self, caller: Address) -> CallCheckResult { |
There was a problem hiding this comment.
🚨 [SECURITY] TIP-20 sequencer privacy gate uses latest L1 state instead of the TempoState anchor
This local rule authorizes sequencer-visible private reads before ZonePrecompileStorageProvider resolves the finalized TempoState anchor, and it ultimately calls latest_sequencer(). The production provider reads the portal sequencer at the cache/RPC latest L1 head, not the L1 block being executed. Around handovers, a future sequencer can read private state before the handover block is imported, and historical replay can depend on current L1 head.
Recommended Fix:
Move sequencer-dependent privacy checks into the L1-backed phase, or add an anchored sequencer_at(block_number) path that reads the portal sequencer slot through the same TempoState anchor used by the storage overlay.
| } else if *address == ACCOUNT_KEYCHAIN_ADDRESS { | ||
| Some(AccountKeychain::create_precompile(&tempo_env)) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
🚨 [SECURITY] L1-backed receive policies can lock TIP-20 funds because ReceivePolicyGuard is not registered
This replacement dynamic lookup falls through to None for upstream dynamic precompiles that the forwarded TIP-20 implementation can depend on, including RECEIVE_POLICY_GUARD_ADDRESS. Upstream TIP-20 can route blocked inbound transfers/mints into the guard and create claim receipts, but the zone then has no callable ReceivePolicyGuard::claim/burn recovery entrypoint, so funds can remain stuck.
Recommended Fix:
Register RECEIVE_POLICY_GUARD_ADDRESS for the relevant hardforks using an L1-backed execution environment consistent with TIP-20 policy reads, and audit other omitted upstream dynamic precompiles for reachable dependencies.
| { | ||
| policy_events.push(event); | ||
| // TODO: Remove once Tempo has migrated policy IDs to the TIP-403 registry. | ||
| mutated_accounts.insert(addr); |
There was a problem hiding this comment.
🚨 [SECURITY] TIP-20 transfer-policy IDs can remain stale after subscriber reconnects
The mutation barrier for a TIP-20 TransferPolicyUpdate is only recorded inside the tracked_tokens branch. Portal-enabled tokens are added to the ephemeral tracked_tokens list but are not persisted into policy_cache, and reconnects reset tracked_tokens from that cache. After reconnect, a token can be dropped, future policy-update logs skip invalidation, and L1StateCache can keep serving a stale transferPolicyId indefinitely.
Recommended Fix:
Make invalidation coverage match overlay read coverage: record a mutation barrier for any TIP-20-prefixed address emitting TransferPolicyUpdate, or persist/reseed portal-enabled tokens so updates cannot be skipped after reconnect.
4 participants
High-level goal
before this PR stack, Zones had two representations of Tempo L1 state:
PolicyCache/PolicyProvider, populated from typed TIP-403 and TIP-20 events with RPC fallback. CustomZoneTip20TokenandZoneTip403ProxyRegistryimplementations used it to reproduce upstream policy behavior.(address, slot), used byTempoStateand Zone system contracts for portal storage reads.this duplicated policy semantics and left two L1-state paths that had to remain aligned in block anchoring, event decoding, invalidation, RPC fallback, and upstream behavior. The goal is to remove the semantic policy implementation from the EVM path and execute upstream Tempo precompiles against an L1-aware storage provider. The provider wraps the normal EVM storage provider and:
TempoStatetransferPolicyIdfield in TIP-20’s packed slot, preserving Zone-local fieldsfrom the upstream precompile's perspective these remain ordinary storage operations, but selected slots are virtualized to finalized L1 state. This makes upstream Tempo code the source of execution semantics rather than maintaining Zone-specific policy implementations.
Raw L1 cache design
the existing generic cache has this shape:
(address, storage slot) → BTreeMap<L1 block number, storage value>. A lookup at blockNreturns the latest cached value at or beforeN. The intended benefit is that mirrored policy state changes infrequently, so one fetched value can serve many later Tempo blocks without another synchronous L1 RPC call.I added contract-level mutation barriers, which basically track when the cache for a given contract is invalidated: A value fetched at block
Vmay serve blockNonly if there is no barrier in(V, N]. The subscriber installs barriers for ZonePortal logs, TIP-403 registry logs, and tracked TIP-20 policy updates. Exact sequencer-slot updates are applied after the portal barrier, so same-block values represent post-block state.the cache now has two independent progress markers:
anchor: latest confirmed L1 block observed by the subscriber; it can run ahead while blocks are queuedblock_floor: latest L1 block consumed by the Zone enginethe engine advances
block_floorin O(1). Slot and barrier histories are pruned lazily when that specific slot/address is next mutated: retain the latest pre-floor baseline, retain entries at or above the floor, and remove older redundant history. Reads below the floor bypass inherited cache state because the pruned barrier history can no longer answer historical inheritance safely. Historical fallback results are not inserted back into the forward cache. Reorgs clear values, barriers, and the subscriber anchor while preserving the monotonic engine floor.Precompile Execution
storage.rsaddsZonePrecompileStorageProvider, a wrapper around Tempo's normal EVM precompile storage provider. It resolves the finalized L1 block once from localTempoState, serves TIP-403 storage reads from that exact L1 block, and overlays only the L1-ownedtransferPolicyIdfield in TIP-20's packed slot while preserving Zone-local fields. All other storage remains local. Mirrored reads still perform the localSLOADfirst so warming, gas accounting, and storage-action recording are preserved, and writes to L1-owned state are rejected.execution.rscentralizes local and L1-backed precompile execution. It installs the appropriate storage provider, preserves the active composed Tempo hardfork and storage accounting, and appliesCallRulesin two phases:CallRulesalso centralizes direct-call enforcement, fixed-gas handling, calldata gas, and forwarding of the original calldata and caller.Together these changes let us drop the Zone-specific L1 precompile implementations from the execution path. TIP-20, TIP-403, and
TipFeeManagernow execute the upstream Tempo implementations directly against anchored L1-aware storage; the remaining Zone-specific behavior is expressed only asCallRulesaround that upstream execution.Follow-up work
PolicyProvideris no longer used by EVM precompile execution, but it is still retained for encrypted-deposit authorization in the engine/payload-builder path. To remove it completely, that path must be migrated to perform the same checks from the block-anchored raw L1 storage provider. Once that cutover is complete, we can remove thePolicyProvider/PolicyCachewiring from the node and subscriber, the policy prefetch task and typed policy-cache event plumbing, and the associated legacy tests and documentation.