review(pr-965): follow-up fixes from code review#966
Open
piotr-roslaniec wants to merge 22 commits into
Open
Conversation
Introduces the on-chain machinery that lets a migration vault settle deposit obligations against the Bridge in lockstep with the Bitcoin sweep flow. The mechanism enables a one-way migration channel where a designated vault accumulates "migration debt" as deposits are revealed under a tagged extra-data byte string, and that debt is repaid when the matching sweep transaction is proven on-chain. Bridge changes: - Adds a canonical migration debt vault pointer maintained by governance via setMigrationDebtVault and an atomic rotateMigrationDebtVault that refuses to leave the prior canonical vault holding outstanding debt. - Tightens setVaultStatus so the canonical vault cannot be untrusted while it is the canonical pointer, and so any vault that still reports outstanding migration debt cannot be untrusted (fail-open staticcall; vaults not implementing the interface are unaffected). - Adds a reveal-time guard in Deposit.revealDeposit: deposits whose extra-data carries the migration tag must come from a registered migration revealer in the per-deposit vault and must have outstanding debt to repay; conversely, deposits from a registered migration revealer that lack the tag are rejected. - Exposes explicit failure reasons via MigrationRevealRejected with reason codes covering missing debt, missing tag, mismatched revealer, unauthorised revealer, vault call failure and bad vault response. - Reduces __gap from 48 to 47 to introduce migrationDebtVault while preserving storage layout continuity for proxy upgrades; documented in docs/BRIDGE_STORAGE_UPGRADE_NOTES.md. DepositSweep changes: - After a sweep is proven, notifies the canonical migration vault via ITBTCVaultMigrationSweepHook with a fail-open batch call followed by bounded per-revealer fallback retries so a single bad entry cannot strand a completion batch. Vault changes: - TBTCOptimisticMinting now implements ITBTCVaultMigrationDebt with per-revealer migration debt tracking, a registration counter that enforces hasOutstandingMigrationDebt across rotations, and a TBTCMigrationDebtOperations library that isolates the debt state transitions (register, repay, residual clear). - Adds MIN_OPTIMISTIC_MINTING_FEE_DIVISOR = 10 to bound the maximum optimistic minting fee rate at 10%. - Adds MAX_MIGRATION_SWEEP_BATCH_SIZE = 100 to cap per-sweep callback fan-out. - Wires migrationSweepNotifier and migrationSweepReserve so a downstream contract can be informed exactly once when a revealer's migration debt reaches zero, with pending-completion bookkeeping that survives partial batch outcomes. Test infrastructure: - Adds harness contracts (BridgeForVaultHarness, TBTCVaultHarness, DepositRevealGuardHarness, DepositSweepCallbackHarness, plus mocks for migration debt vault, sweep notifier, sweep vault and a regular comparison vault) so the migration paths can be exercised in isolation from the full Bridge fixture. - Adds a NatSpec verification script that asserts the required documentation surface for the migration debt interface remains present. Deploy: - Resolves wallet registry and reimbursement pool against deployment artifacts when present so downstream networks can wire the new debt vault pointer through the bridge governance flow.
Bridge deployed bytecode exceeded the EIP-170 24,576-byte limit. Two public reinitializers — initializeV2_FixVaultZeroDeposit and initializeV5_RepairRebateStaking — were permanently consumed on every live proxy: slot 50 _initialized reads 0x05 on mainnet and Sepolia, with Initialized events at mainnet blocks 24282346 and 24800704. OpenZeppelin Initializable does not decrement, so the functions could never run again. Removing both reinitializers plus a Bridge-only optimizer override (runs 500) brings deployed bytecode to 23,522 bytes — 1,054 under the limit. Storage layout is unchanged. The DepositVaultFixed and RebateStakingRepaired event declarations are retained so historical logs decode through the current ABI. Deploy scripts that targeted the removed selectors are hard-disabled (func.skip = true) with on-chain evidence captured in the deprecation headers. The exported encoder helpers in script 85 remain importable as historical calldata-format regression coverage. The Bridge.RebateRecovery test is deleted — it exercised the removed reinitializer through helpers.upgrades.upgradeProxy. Incidental: prettier collapsed the PreviousMigrationDebtVaultMismatch declaration into a single line.
- Chunk migration sweep callback batches at the vault's 100-revealer limit to avoid the O(N) per-revealer fallback path on oversized sweeps (DepositSweep.notifyMigrationSweepCallback, MAX_MIGRATION_SWEEP_BATCH_SIZE). - Reject EOA / non-contract addresses at setMigrationSweepNotifier set time so misconfiguration surfaces before sweep callbacks fire; address(0) remains a valid disable state. - Guard registerMigrationDebt against re-registration while a pending sweep completion is unresolved, closing a state-machine gap where a stale callback could clear a fresh reserve. - Document the side effect of zeroing setMigrationSweepReserve (clears pendingMigrationSweepCompletion) via NatSpec. - Simplify registerDebt in TBTCMigrationDebtOperations to a direct assign; the existing require(migrationDebt == 0) makes the prior addition equivalent. - Consolidate the duplicate isMigrationReveal helper into MigrationExtraData; update callers in Deposit, TBTCOptimisticMinting, and DepositRevealGuardHarness. Tests: 4 new in TBTCVault.MigrationDebt (EOA notifier reverts, zero disable, pending-sweep guard, re-register after pending clears) and 2 new in DepositSweep.MigrationCallback (chunking happy path, chunked fallback). Local yarn build clean; all 48 migration-tagged tests pass. Pre-existing unrelated failures (cross-chain/Wormhole, OM divisor-math) remain untouched.
…ivisor Restores divisor=0 as the explicit "fee disabled" governance state alongside the existing non-zero minimum bound. The previous guard rejected zero, but the fee formula at TBTCOptimisticMinting.sol still retained the `divisor > 0 ? amount / divisor : 0` ternary — leaving the zero branch as permanently dead code while three test contexts that asserted truncation- to-zero with `divisor = type(uint32).max` empirically failed (e.g. 199_900_000_000_000 / 4_294_967_295 = 46_542, not 0). Both setter guards (begin and finalize) now accept `divisor == 0 || divisor >= MIN_OPTIMISTIC_MINTING_FEE_DIVISOR`, aligning the setter with the fee formula's existing zero-handling branch. The non-zero floor of 10 (capping non-zero fee rate at 10%) is preserved, so divisors in [1, 9] remain blocked. Test changes: - Two zero-fee test contexts restored to their pre-port `divisor = 0` baseline; arithmetic comments and labels updated. - The setter test that asserted `divisor = 0` reverts is replaced with a boundary test asserting `divisor = 1` reverts, matching the new guard shape. - AbstractBTCDepositor and the OM fee formula already handle `divisor = 0` correctly via `divisor > 0 ? ... : 0`; no other code paths needed changes. NatSpec on MIN_OPTIMISTIC_MINTING_FEE_DIVISOR and on the finalize setter documents that 0 is a valid out-of-band sentinel meaning "no fee". Local CI: yarn build clean, yarn lint 0 errors, 101/101 tests pass in TBTCVault.OptimisticMinting.test.ts (was 98/101 with 3 failures). TBTCVault contract size 19.449 KB (well under EIP-170).
Mechanical prettier reformatting on PR-#132 ported migration files plus targeted slither-disable directives with one-line rationale on the intentional fail-open vault.call sites and the bounded migration sweep loop. Slither suppressions: - TBTCOptimisticMinting.registerMigrationDebt -> registerDebt: unused-return is intentional; the library returns the registered amount which equals the caller's input by invariant. - TBTCOptimisticMinting.repayDebt counter decrement: costly-loop suppressed with rationale that per-revealer atomicity is the contract's guarantee. - TBTCOptimisticMinting notifier dispatch + emit: calls-loop and reentrancy-events suppressed; notifier is governance-controlled, the array is bounded by MAX_MIGRATION_SWEEP_BATCH_SIZE, partial failure is the intended fail-open semantic. - DepositSweep.notifyMigrationSweepCallback batch + per-revealer fallback emits: reentrancy-events suppressed; the sweep proof is finalized before these informational retry signals fire. Format: - yarn format:fix applied across migration files; multi-line return tuples and call expressions reformatted to prettier defaults. - One whitespace fix in test/verify-migration-debt-natspec.sh. No behavior changes. yarn build, yarn format, and the full test suite pass: 2973 passing / 0 failing / 31 pending. Bridge bytecode 23,522 bytes (1,054 under EIP-170). 44 lint warnings remain (function ordering and max-states-count) but yarn format exits 0 — they are non-blocking and pre-existing across the repo, not specific to this port.
…ry NatSpec Add a @dev note to setMigrationDebtVault explaining it skips the outstanding-debt guard (use rotateMigrationDebtVault for live rotations). Add @notice/@param NatSpec to setRevealer and setSweepReserve in TBTCMigrationDebtOperations, consistent with registerDebt above them.
The migration grant path in _revealDeposit authorized against reveal.vault directly, so any trusted vault implementing ITBTCVaultMigrationDebt could authorize a migration-tagged reveal - not just the canonical migrationDebtVault. Pin reveal.vault to self.migrationDebtVault inside the migration branch so the grant path and the post-sweep callback (DepositSweep) share a single source of truth, and migration reveals are rejected while migration is disabled at the Bridge level (canonical address zero). Adds tests for: trusted non-canonical vault, non-canonical migration-debt vault with registered revealer + debt, and unset canonical vault.
Four independently-scoped fixes from an audit pass on the migration debt subsystem: 1. Bridge ETH rescue (Blocker) submitFraudChallenge accepts ETH that can be orphaned when the downstream Fraud library refunds via a bounded-gas call whose return value is not checked: the challenge is marked resolved regardless of payout success, leaving funds custodied in the Bridge with no exit path. Adds recoverETH(payable, uint256) onlyGovernance with a checked low-level transfer, plus a forwarder on BridgeGovernance. 2. Canonical migration debt vault setter (Must-fix) setMigrationDebtVault now (a) probes the target's ITBTCVaultMigrationDebt interface conformance fail-closed -- the deposit-reveal guard fail-closes on staticcall failure to the canonical vault, so a non-conforming pointer would brick the pipeline -- and (b) rejects overwriting a canonical vault that still has outstanding migration debt, forcing governance to use rotateMigrationDebtVault (which atomically untrusts the previous vault). Emergency disable to address(0) still works because the previous-debt check uses a fail-open staticcall. The corresponding harness mirrors the same logic. 3. TBTCVault upgrade in-flight guard (Must-fix) finalizeUpgrade transferred TBTC ownership and the full Bank balance to a fresh new-vault contract that starts with empty migrationDebt, pendingMigrationSweepCompletion, migrationSweepReserve, isMigrationRevealer, and _outstandingMigrationDebtCount. Reverts when this vault is the canonical migration debt vault and hasOutstandingMigrationDebt is true so governance must drain debt before rotating ownership. Also widens hasOutstandingMigrationDebt from external to public so subclasses can read it without an external self-call. 4. Sweep solvency invariant on deposit parameters (Must-fix) BridgeState.updateDepositParameters only enforced dust > txMaxFee, but DepositSweep performs amount - treasuryFee - txFee and treasury fee is amount / treasuryFeeDivisor. A misconfigured divisor combined with the existing rule could create revealed deposits that always underflow at sweep time. Now also enforces dust > dust/treasuryFeeDivisor + txMaxFee when the divisor is non-zero, matching the worst-case sweep arithmetic. Bytecode budget: trimmed Bridge.sol require strings to custom errors to fit the EIP-170 limit after adding the rescue function and the canonical-vault setter logic.
The slither-disable-next-line directive was followed by a separate solhint-disable-next-line line, so slither's "next-line" applied to the solhint comment and the low-level-calls detector still fired on recipient.call. Inlines the solhint suppression on the call line and keeps slither-disable-next-line directly above it.
Bridge was below the bytecode ceiling by only 0.063 KiB, so replace remaining Bridge revert strings with custom errors and share the migration-debt staticcall helper across strict and fail-open paths. Rotation now probes non-zero target vaults for ITBTCVaultMigrationDebt, while setMigrationDebtVault fail-closes only on regular non-zero overwrites and preserves the emergency-disable lane. Fraud challenge ETH is tracked as open escrow and recoverETH is capped to the non-escrowed balance so governance cannot drain live challenge refund backing.
Probe canonical migration debt vault targets for every static selector used by Bridge reveal paths, while keeping outgoing-debt checks fail-closed and the zero-address disable lane. Add one-shot fraud challenge escrow seeding for upgraded deployments, gate rescue and new challenges until seeded, and skip legacy challenge decrements before the seed. Track whether newly submitted challenges are counted so seeded legacy challenges and post-seed challenges both release escrow without underflow. Update the hotfix deploy script to compute the seed from fraud challenge events and fail closed when a direct transaction value is insufficient evidence. Cover partial migration vault rejection, seeded legacy resolution/rescue bounds, and post-seed fraud challenge tracking.
The hotfix script previously linked the new Bridge implementation to the legacy mainnet Deposit, DepositSweep, and Fraud libraries. Those libraries' source was modified earlier in this PR chain, so the migration-debt reveal guard, post-sweep callbacks, and fraud-challenge escrow accounting would all be unreachable at runtime even though the new Bridge bytecode references them. Deploy fresh DepositTIP109Hotfix, DepositSweepTIP109Hotfix, and FraudTIP109Hotfix copies alongside the existing RedemptionTIP109Hotfix, and link the new Bridge implementation to the fresh addresses for every modified linked library. Add a deploy-script test that asserts the linkage: each modified library resolves to a freshly-deployed address rather than the legacy mainnet deployment, and the Bridge implementation libraries map matches.
Authorizes rebate accounting on callback redemptions via a per-pair mapping in RebateStaking and binds the decoded redeemer to the actual TBTC burner at the TBTCVault layer. Background. Bridge.receiveBalanceApproval authenticates only the Bank as caller, while Redemption.requestRedemption decodes the redeemer from caller-controlled bytes. The current code applies the redemption rebate to that decoded redeemer, which lets any Bank balance owner debit an arbitrary staker's rebate cap by naming them in redemptionData. Fix. Three coordinated changes ship together: - RebateStaking gains a many-to-one mapping rebateAuthorizations[redeemer][balanceOwner], a staker-only setter setRebateAuthorization, an isRebateAuthorized view, and a RebateAuthorizationSet event. __gap shrinks from 49 to 48 to absorb the new slot; no existing slot is reordered. - Redemption gates rebate application on isRebateAuthorizedForRedemption (a soft-fail view that returns true for direct redemptions and for callback redemptions where the redeemer has explicitly authorized the balance owner). Unauthorized callback redemptions proceed with the full treasury fee and no rebate is applied, preserving canonical vault UX. Apply/cancel symmetry on request.redeemer is preserved because cancelRebate is a no-op when no matching rebate exists. - TBTCVault.\_unmintAndRedeem decodes the first ABI element of redemptionData and requires it to equal the actual TBTC burner. This blocks the public-vault spoof where an attacker would name an opted-in staker as redeemer while burning their own TBTC. Tests. Bridge.RedemptionVaultRebate covers soft-fail and authorized paths, multi-tenant authorization, and a direct attacker-spoof attempt that consumes no victim rebate. RebateStaking unit tests cover setRebateAuthorization happy path, NotAStaker revert, zero balanceOwner revert, multi-staker concurrent authorization, and revocation. TBTCVault.Redemption adds a caller-binding negative case. Full suite: 3040 unit passing, 27 integration passing. Storage upgrade is appended-only; rollback path is the prior RebateStaking implementation. Bridge and TBTCVault upgrades are logic-only.
Adds two assertions per spec section 6: - Strengthen the unmintAndRedeem mismatch test to also assert no TBTC burn, no vault Bank balance movement, no bridge Bank balance movement, and no pending redemption entry after the revert. - Add a receiveApproval path test that routes through TBTC.approveAndCall with the decoded redeemer differing from the authenticated `from`, asserting the same revert reason. These close coverage gaps flagged in independent review while preserving the existing happy-path tests untouched.
…heck CI Slither flagged `stakes[msg.sender].stakedAmount == 0` as a dangerous strict equality in `setRebateAuthorization`. The rest of RebateStaking already avoids this warning by loading a local `Stake storage` reference before the equality check (see `setDelegatee`, `applyForRebate`, `cancelRebate`, `finalizeUnstaking`, `forceStakeTransfer`). Restructured `setRebateAuthorization` to match: load `Stake storage stakeInfo = stakes[msg.sender]` first, then check `stakeInfo.stakedAmount == 0`. Pure refactor — no behavior change, no storage change. All unit and integration tests still pass locally.
Add a `getStake(redeemer) == 0` early-return to `isRebateAuthorized`, making stale authorizations inert after `finalizeUnstaking` or `forceStakeTransfer` fully zeroes a staker's stake. Change `getStake` visibility from `external` to `public` so the function can be called internally without a delegatecall frame. ABI selector is unchanged (`getStake(address)` → same 4-byte digest). Closes the stale-authorization + delegation rebate primitive (F-1): an attacker naming a fully-unstaked address as `redeemer` can no longer cause the callback gate to forward to `applyForRebate`, even if that address was previously authorized and a current staker has since delegated to it. Add four regression tests in Bridge.RedemptionVaultRebate.test.ts: - stale-auth path after full unstake + victim delegation - forceStakeTransfer lifecycle: old stale auth inert for new staker - victim-delegates-to-former-staker end-to-end attack chain - canonical direct-path delegation still credited to delegating staker
`notifyRedemptionVeto` deletes the pending redemption and detains the full requested amount to the watchtower, but never calls `RebateStaking.cancelRebate`. A vetoed redeemer that consumed rebate capacity at request time therefore loses both the redeemed amount and the rebate-cap slot until the rolling window ages out. `notifyRedemptionTimeout` already mirrors the apply path with a guarded `cancelRebate` call. The NatSpec on the rebate flow asserts apply/cancel symmetry on `request.redeemer`; the veto path silently broke it. Mirror the timeout pattern: snapshot the request into memory so reads survive `delete`, then call `cancelRebate` (guarded by `rebateStaking != address(0)`) between the storage cleanup and the watchtower transfer. `cancelRebate` is a safe no-op when no rebate was applied. Adds vault-path veto tests in `Bridge.RedemptionVaultRebate.test.ts` covering both the authorized-rebate path (asserts `RebateCanceled(redeemer, requestedAt)`) and the no-rebate path (asserts no `RebateCanceled`, request still cleared, watchtower still receives funds).
Convert three fs.*Sync calls to fs.promises equivalents, wrap the build-info JSON.parse in try/catch, and tighten the Formatter monkey-patch type from any to Record<string,unknown>/TransactionResponse. Add .ubsignore to exclude deploy scripts from UBS: they are one-off Hardhat tools that intentionally use console.log for deployment output and are never bundled to a client. Also add a warning field to postUpgradeActions so the seedFraudChallengeEscrow timing constraint travels with the Safe calldata artifact, not only console output.
… and TBTCVault Add maxMigrationSweepBatchSize() getter to DepositSweepCallbackHarness to expose the internal library constant, then assert it equals TBTCVault.MAX_MIGRATION_SWEEP_BATCH_SIZE in the MigrationDebt test suite. If the two constants drift, the Bridge passes oversized sweep batches that the vault hook rejects.
piotr-roslaniec
requested review from
Shadowfiend,
lrsaturnino,
lukasz-zimnoch,
nkuba and
pdyraga
as code owners
…mat batch test Two CI fixes: - 86_deploy_tip109_hotfix.test.ts: commit 55daa27 migrated the hotfix deploy script from fs.writeFileSync/mkdirSync to fs.promises.writeFile/ mkdir, but the test still patched the sync counterparts. capturedSummary was never assigned, so the deployment-summary assertion saw undefined. Patch fs.promises.writeFile/mkdir instead. - TBTCVault.MigrationDebt.test.ts: split an overlong harness deploy line to satisfy prettier (printWidth 80) for contracts-format.
piotr-roslaniec
changed the base branch from
fix/redemption-rebate-redeemer-spoof
to
main
Contributor
Author
|
@lrsaturnino Let me know if we should close this |
Member
We can close this PR. |
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
2 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
Follow-up to #965. Four small fixes surfaced during code review.
warningfield topostUpgradeActions[0]in the hotfix deployment artifact so theseedFraudChallengeEscrowtiming constraint travels with the Safe calldata, not only console output. Also fix pre-existing syncfs.*Synccalls, a bareJSON.parse, and ananyannotation in the deploy script.@devnote tosetRebateAuthorizationdocumenting that authorizations persist across zero-stake periods and silently reactivate on re-stake.maxMigrationSweepBatchSize()getter toDepositSweepCallbackHarnessand a test assertingDepositSweep.MAX_MIGRATION_SWEEP_BATCH_SIZE == TBTCVault.MAX_MIGRATION_SWEEP_BATCH_SIZE. Catches drift before it causes the Bridge to pass oversized sweep batches.releaseFraudChallengeEscrowexplaining thefraudChallengeEscrowSeededbranch for pre-upgrade challenges.Test plan
DepositSweep.MigrationCallbacktests still passMAX_MIGRATION_SWEEP_BATCH_SIZEparity test passes inTBTCVault.MigrationDebtsolhintandeslintclean on changed files