Implement EVM L2 cross-chain fee rebates#952
Open
mswilkison wants to merge 32 commits into
Open
Conversation
mswilkison
requested review from
Shadowfiend,
lrsaturnino,
lukasz-zimnoch,
nkuba,
pdyraga and
piotr-roslaniec
as code owners
Authenticating senders solely by transfer.fromAddress allows a contract deployed at the same address on any chain with a registered Wormhole Token Bridge to impersonate the authorized L2 redeemer. - Extend IWormhole with parseVM/VM to read the VAA header. - Add an optional Wormhole Core reference and a sender -> wormhole chain ID mapping; once the core is configured, every allowed sender must have a matching wormhole chain ID or redemptions revert. - Enforce the check in both requestRedemption and _completeTransfer. The check is gated on the core reference being set so existing deployments can migrate without breaking, but admins are forced to populate the mapping before the guard activates.
`rebates` grows permanently as stakers accumulate history. Copying the full array into the new staker makes `forceStakeTransfer` fail for long-lived accounts once the array exceeds the block gas limit. Only the active rolling window is semantically meaningful -- entries before `rollingWindowStartIndex` are never read -- so skip them and rebase the new array's start index to zero.
`msg.sender.code.length == 0` returns true while a contract is still executing its constructor, so an intermediate contract can mint a rebate for an address it controls by deploying and calling through the constructor in the same transaction. Replace the check with `tx.origin == msg.sender`, which rejects any nested call frame.
Upgradeable logic contracts deployed behind transparent proxies should not expose `initialize` on the implementation itself; otherwise anyone can take ownership of the logic contract and emit misleading events from it. Add an `_disableInitializers()` constructor to L2WormholeGateway, L2TBTC, and L2BTCRedeemerWormhole so only the proxy storage is ever initialised.
`.transfer` forwards only 2300 gas, which is not enough for smart-contract recipients such as multisigs or AA wallets. The owner must be able to recover native tokens to any address that can receive ETH, so switch to a low-level call with success checking.
Regression coverage for the audit fix: - setWormhole: owner-only, one-shot, rejects zero address - updateAllowedSenderWormholeChain: owner-only, event, overwrite - requestRedemption: backward-compatible when core is unset; reverts when sender has no wormhole chain configured; reverts on emitter mismatch; accepts on match.
Regression for the gas-bound fix: create a staker with three rebates, advance past the rolling window between the first and the remaining entries, then transfer and assert that only the in-window rebates survive, rebased to index 0, and that the derived available rebate reflects the post-copy state.
Add a small helper contract that makes the target call from its constructor and assert both `initializeDepositWithRebate` and `requestRedemptionWithRebate` revert with "Signed auth required". This pins the fix that replaced `code.length == 0` with `tx.origin == msg.sender`; without the fix the helper would pass because its own code length is zero while the constructor executes.
Deploy each of L2WormholeGateway, L2TBTC, and L2BTCRedeemerWormhole as a bare implementation (not behind a proxy) and assert that calling `initialize` reverts with the OpenZeppelin "already initialized" message. This regression would fail if the `_disableInitializers()` constructor were ever removed.
Three follow-ups driven by running the suites locally: - Mirror the new emitter-chain authentication state (`wormhole`, `allowedSenderWormholeChainIds`) and the `setWormhole` / `updateAllowedSenderWormholeChain` admin surface into MockL1BTCRedeemerWormhole, and call the guard from its `requestRedemption`. The existing test suite deploys the mock rather than the production contract; without this mirror the new describe blocks report "is not a function". - Revert the `_disableInitializers()` constructor on L2BTCRedeemerWormhole. Adding it pushed the bytecode past a boundary that triggers a known bytecode-unlinking bug in @openzeppelin/hardhat-upgrades 1.22.0 / upgrades-core 1.20.0 (link references from unrelated linkable contracts get spliced into this contract's bytecode, corrupting the version check and breaking every deployProxy call in the test suite). Documented inline; the lock should be re-added once the plugin is upgraded. - Deploy the EOA-guard fixtures behind a minimal TransparentUpgradeableProxy instead of going through the OZ plugin, so the regression test bypasses the same validation pitfall.
- Use distinct fee values in the forceStakeTransfer regression so the copied entries are uniquely identifiable. With identical fees the "rebase to index 0" assertion couldn't distinguish the in-window entry from the stale one. - Drop the "should clear the old staker's rebate array" expectation. The landed fix only rebases the copied window and zeroes out the old staker's balances; it does not delete `oldStake.rebates`, so the array retains its historical length. That residue is harmless because the old stake amount is reset to zero. - Prettier formatting on the new test files.
The audit-fix commits deliberately reach for `tx.origin`, a low-level
`.call{value:}`, and (in the test helper) a raw `call`+`assembly revert`.
Each one was already annotated for slither; mirror those with the
corresponding `solhint-disable-next-line` / file-level
`solhint-disable` markers so CI's `lint:sol` stays clean. No behaviour
change.
Follow-up to the VAA emitter-chain authentication landed earlier in this PR, responding to a multi-lens review: - Make `setWormhole` atomic: accept `(address, bytes32[] senders, uint16[] wormholeChainIds)` so the core reference and every sender-to-chain-ID binding land in the same transaction. Before this change, the interval between `setWormhole` and the first `updateAllowedSenderWormholeChain` call would silently DoS every redemption (fail-closed on unconfigured senders). The admin is still responsible for listing every currently-allowed sender, but forgetting one is now visible in the same transaction rather than hours later. - Clear `allowedSenderWormholeChainIds[sender]` when `updateAllowedSender` or `updateAllowedSenderWithSourceChain` revoke a sender. Previously the wormhole chain ID binding could outlive the revocation and be reused by mistake on a re-enable. - Distinguish "sender has no wormhole chain configured" (`SourceChainNotAuthorized`) from "sender configured but VAA arrived from the wrong chain" (`WormholeChainMismatch`). - Document the Wormhole chain ID 0 "not configured" sentinel inline and pin the upstream SDK commit on the new `IWormhole.VM` struct and `parseVM` signature, matching the pattern used elsewhere in `Wormhole.sol`. - Link tracking issue #954 from the `L2BTCRedeemerWormhole` `_disableInitializers` deferral comment so the toolchain-driven exception is auditable. Tests: - `setWormhole`: owner-only, zero address rejected, array length mismatch rejected, empty bindings accepted, idempotency enforced, atomic binding population emits one event per binding. - `updateAllowedSender` and `updateAllowedSenderWithSourceChain` revocation paths each clear the bound wormhole chain ID and emit the zero update. - `requestRedemption`: the VAA-mismatch case now asserts `WormholeChainMismatch` rather than the legacy error. `MockL1BTCRedeemerWormhole` mirrors the signature and behaviour so the existing proxied test suite continues to exercise the guard.
Three regression gaps flagged during review:
- L1BTCDepositorNtt.retrieveTokens native branch only had an ERC20
happy-path test. Add two cases that directly exercise the
`.transfer()` -> `.call{value:}` fix:
- forward native tokens to a contract recipient whose `receive()`
consumes well over the 2300 gas stipend (would have out-of-gassed
under `.transfer()`),
- surface the low-level-call failure when the recipient explicitly
reverts, asserting the new `require(success, ...)`.
Two new helper contracts: `GreedyReceiver` (writes two cold storage
slots in receive) and `RevertingReceiver` (always reverts).
- `setWormhole` atomic-binding end-to-end: previous tests exercised
the getter surface; add a scenario that calls
`setWormhole(core, [sender], [chainId])` in one transaction and
verifies the very next `requestRedemption` passes without any
follow-up `updateAllowedSenderWormholeChain`. This pins the atomic
contract of the signature change.
- `updateAllowedSender` revocation round-trip: re-enabling a revoked
sender must leave `allowedSenderWormholeChainIds[sender] == 0`, so
an operator re-registering at a different Wormhole chain cannot
silently reuse the stale binding.
Follow-up to #952 addressing findings from an automated Solidity security audit (fabro `audit-solidity` workflow, verified manually). Each commit is a single isolated fix or a targeted regression test. ## Fixed + tested ### High - **emitterChainId validation on the Wormhole bridge receiver** (d2bd813) `L1BTCRedeemerWormhole` authenticated senders solely by `transfer.fromAddress`, letting a contract deployed at the same address on any chain with a registered Wormhole Token Bridge impersonate the authorized L2 redeemer. Introduces a Wormhole Core reference and a `sender -> wormhole chain ID` mapping; once the core is set, every allowed sender must have a matching wormhole chain ID or redemptions revert. Additive, gated so existing deployments migrate without breaking. Regression (4286488, 6226404): `setWormhole` one-shot / zero / owner-only; `updateAllowedSenderWormholeChain` owner-only + event; `requestRedemption` backward-compatible when core unset, reverts on unconfigured sender, reverts on chain mismatch, accepts on match. ### Medium - **Bound gas cost in `forceStakeTransfer`** (f52e42c) `rebates[]` grows indefinitely; copying the full array eventually trips the block gas limit. Only the active rolling window is copied, rebased to index 0. Regression (17cabe5): three rebates spanning the window, transfer, assert only the in-window entries survive and the available rebate reflects the post-copy state. - **EOA guard hardening on L2 rebate paths** (5091ee8) `msg.sender.code.length == 0` is bypassable from a constructor; replaced with `tx.origin == msg.sender`. Regression (bb4d930): a helper contract performs the call from its constructor, asserts both `initializeDepositWithRebate` and `requestRedemptionWithRebate` revert with "Signed auth required". - **Lock implementation initializers on upgradeable L2 contracts** (4ca6f28) `_disableInitializers()` constructor on `L2WormholeGateway` and `L2TBTC`. Regression (ac6f770): bare implementations reject `initialize` with the OZ "already initialized" error. *Partial*: `L2BTCRedeemerWormhole` reverted (see "Tooling caveat" below). ### Low - **`.transfer()` -> low-level `.call{}` in `L1BTCDepositorNtt.retrieveTokens`** (05879f5) -- 2300 gas is not enough for smart-wallet owners. ## Tooling caveat (`_disableInitializers` on `L2BTCRedeemerWormhole`) Adding the `_disableInitializers()` constructor pushed the contract's creation bytecode past a size boundary that triggers a known bug in `@openzeppelin/hardhat-upgrades@1.22.0` / `@openzeppelin/upgrades-core@1.20.0`: the plugin's version-matching loop mis-applies link references from unrelated linkable contracts (e.g., `Bridge` linking `Wallets` at byte offset 7376) to this contract's bytecode, producing a corrupt placeholder that fails the hex-validity check. Every `deployProxy` call in the existing test suite was failing with `Error: Bytecode is not a valid hex string` until the constructor was reverted. Rather than downgrade the fix on the other two contracts or patch vendored plugin code, I documented the constraint inline and deferred this specific hardening until the plugin is upgraded. The `L2BTCRedeemerWormhole` mock + initializer lock regression test already exercises the guard for `L2WormholeGateway` and `L2TBTC`; a one-line revert will re-enable it on the redeemer when the tooling is bumped. ## Test results (locally, subset of suites I touched) ``` test/cross-chain/wormhole/L1BTCRedeemerWormhole.test.ts test/cross-chain/wormhole/L2BTCRedeemerWormhole.test.ts test/cross-chain/wormhole/L2DepositorWormholeAdapter.test.ts test/cross-chain/wormhole/L2RebateEOAGuard.test.ts (new) test/cross-chain/wormhole/L2ImplementationInitializerLock.test.ts (new) test/cross-chain/L2TBTC.test.ts test/cross-chain/L2WormholeGateway.test.ts 278 passing (16s) ``` `test/bridge/RebateStaking.test.ts`: my new regression context compiles and is structurally correct, but the local env's external-deploy setup pulls the mainnet Bridge artifact, which predates `applyForRebate` (added by #952 itself). In CI against the PR branch's Bridge it will execute; the logic is in 17cabe5. ## Deferred (rationale) | Finding | Sev | Why deferred | |---|---|---| | Yearn spot-price sandwich (Curve/Convex/Saddle) | High | Pre-existing; needs TWAP or caller `minOut`, belongs in a dedicated yearn PR. | | Yearn AMM deadline = `block.timestamp` | Med | Same scope. | | `LightRelay.proofLength` immediate update | Med | Pre-existing; adding a timelock is a governance-visible behavior change. | | `__gap` missing in `AbstractL1BTCDepositor` | Med | `BaseL1BitcoinDepositor` (and likely more) already on mainnet; adding a gap now would shift descendant storage on upgrade. Needs a no-add-state constraint or a coordinated migration. | | `Ownable` -> `Ownable2Step` (LightRelay/Bank/BridgeGovernance/bob L2TBTC) | Low | Behavior-breaking on non-upgradeable governance contracts; adds a storage slot for upgradeable extenders. Requires governance-tool coordination. |
lrsaturnino
previously approved these changes
Apr 23, 2026
6 tasks
Add explicit setWormhole step to the governance runbook so operators bind the Wormhole Core reference and emitter-chain IDs before enabling rebate-aware L2 redemptions. Without this step the V2 redeemer silently skips VAA emitterChainId authentication for backward compatibility.
The implementation skips rebate application (returning the original treasury fee without consuming the authorization nonce) when the calculated rebate falls below minCrossChainRebateSat. Promote this from an undecided implementation detail to a documented behavior so off-chain tooling can rely on it.
cancelCrossChainRebate is called by the Bridge on every cross-chain redemption timeout or veto. Previously the function emitted no event when the matching actionId was not in the rolling window (aged out, already canceled, or beneficiary never staked), which left off-chain indexers unable to distinguish a successful cancellation from a no-op. Emit CrossChainRebateCancelRequested unconditionally after the actionId validity check and before any storage reads, while keeping the existing CrossChainRebateCanceled (which carries the canceled amount) only on a matched-and-zeroed rebate. Indexers can diff the two streams to detect no-op cancel calls without any extra on-chain cost on the hit path.
Extend the cancel-rebate test to assert the new CrossChainRebateCancelRequested entry event alongside the existing CrossChainRebateCanceled emit, and add four regressions for the cases where cancelCrossChainRebate intentionally no-ops on storage: - zero actionId revert - actionId not present in the rebates array - beneficiary that never staked - rebate that has aged out of the rolling window Each miss case must emit the request event but not the canceled event so off-chain indexers can distinguish a real cancellation from a no-op without scanning storage.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reviewer Notes
notifyRedemptionVetonow cancels direct-L1 rebates as well as cross-chain rebates. This restores rebate capacity for vetoed direct redemptions and changes pre-PR main behavior by adding aRebateCanceledemission path on veto.runs: 1to stay below the EIP-170 size limit. Follow-up architectural refactoring can revisit runtime gas versus deployment size.Verification
Notes: Hardhat warns about unsupported Node v25.9.0. Build/test still pass. Bridge bytecode is 23.310 KB after the Bridge optimizer override and the strict contract-sizer gate passes.