Trail of Bits tBTC v2 AC Extension — security remediation#1003
Open
lrsaturnino wants to merge 17 commits into
Open
Trail of Bits tBTC v2 AC Extension — security remediation#1003lrsaturnino wants to merge 17 commits into
lrsaturnino wants to merge 17 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
lrsaturnino
changed the title
…st outstanding optimistic minting debt TBTCVault.finalizeUpgrade only blocked finalization while migration debt was outstanding, ignoring optimistic minting debt. Optimistic debt is per-depositor local vault state repaid from future sweep proceeds routed through the vault callback. Finalizing an upgrade while it is outstanding strands the debt on the old vault: a deposit revealed for the old vault cannot repay the debt on the fresh vault (sweep validation pins the recorded vault), and if the old vault is later untrusted the sweep proceeds bypass the repayment callback entirely, letting a depositor mint a second time for the same Bitcoin deposit. Add aggregate optimistic-minting-debt tracking: an `_outstandingOptimisticMintingDebtCount` counter incremented in finalizeOptimisticMint on the zero-to-nonzero transition and decremented in repayOptimisticMintingDebt when a depositor's debt returns to zero, exposed via a new `hasOutstandingOptimisticMintingDebt()` on ITBTCVaultMigrationDebt. finalizeUpgrade now reverts while that aggregate is nonzero. The test harness setter maintains the same invariant and the migration-debt mocks implement the new interface method.
…ult has optimistic minting debt setVaultStatus and rotateMigrationDebtVault only rejected vaults with outstanding migration debt, not outstanding optimistic minting debt. A vault untrusted (directly or via canonical rotation) while it still held per-depositor optimistic minting debt would silently reroute later sweep proceeds through Bank.increaseBalances, bypassing the vault's optimistic debt repayment callback. The depositor keeps their optimistic mint and gains a fresh Bank balance for the same satoshis, which can be minted a second time via the same vault, leaving unbacked TBTC. Add a fail-open `_hasOutstandingOptimisticMintingDebt` staticcall (mirroring `_hasOutstandingMigrationDebt`) and reject untrust in setVaultStatus and the previous-vault leg of rotateMigrationDebtVault when the vault reports outstanding optimistic minting debt. The BridgeVaultStatusHarness mirror is kept in sync with the production guards.
…ithout a configured reserve pullPendingSweepReserve returned early when a revealer had a pending migration sweep completion but no configured reserve, leaving the pending flag set. A reserve configured after the fact could then let a later notification consume that stale flag and emit a migration completion that is not tied to the original repayment event. pullPendingSweepReserve now drops the pending completion when it is consumed with no reserve configured, reporting the drop so the vault emits MigrationSweepCompletionPendingCleared for observability. A reserve configured afterwards no longer replays the elapsed completion.
…nter was removed The Sui TBTC `mint` function accepted any MinterCap as authorization and never checked the minters list. Because `remove_minter` cannot destroy a MinterCap that was already issued, a removed minter — or a gateway whose minter authority governance believed it had revoked — could keep minting canonical tBTC with its retained cap. The removal only updated the `minters` vector, which `mint` ignored. Require the cap's minter address to still be present in the active minters list before minting, adding the E_NOT_MINTER abort. Removing a minter now genuinely revokes its retained cap. The remove_minter comment is corrected to describe this enforcement, and a regression test asserts that minting with a removed minter's retained cap aborts.
lrsaturnino
force-pushed
the
fix/tob-tbtcacext-remediation
branch
from
74a9a4d to
aa3c389
Compare
lrsaturnino
requested review from
Shadowfiend,
lukasz-zimnoch,
nkuba,
pdyraga and
piotr-roslaniec
as code owners
lrsaturnino
force-pushed
the
fix/tob-tbtcacext-remediation
branch
from
aa3c389 to
e471e27
Compare
miquelcabot
reviewed
Jul 7, 2026
Comment on lines
+281
to
+283
| for (uint256 j = 0; j < postUpgradeWallets.length; j++) { | ||
| self.walletRegistrationOrder.push(postUpgradeWallets[j]); | ||
| } |
There was a problem hiding this comment.
This reconstruction appends the on-chain postUpgradeWallets snapshot after the caller-supplied preUpgradeWallets, with no dedup, but the runbook in 86_deploy_tip109_hotfix.ts tells the operator to recompute the full event-ordered list (computeWalletRegistrationOrder scans NewWalletRegistered from Bridge-deploy -> head), which already includes any post-upgrade wallet.
So if a wallet completes DKG in the upgrade->seed window (registration is permissionless) and the operator follows the runbook literally, that wallet ends up twice in walletRegistrationOrder. selectMovingFundsTargetWallets then returns a target set with a repeated entry, which a strictly-ascending commitment can never match -> submitMovingFundsCommitment reverts permanently for the affected counts.
The defensive non-empty branch was added exactly to survive an in-window registration, but as written it produces the corruption it means to prevent. Suggest deduping against the existing on-chain order before appending, e.g.:
for (uint256 i = 0; i < preUpgradeWallets.length; i++) {
self.walletRegistrationOrder.push(preUpgradeWallets[i]);
}
for (uint256 j = 0; j < postUpgradeWallets.length; j++) {
bytes20 w = postUpgradeWallets[j];
bool seen = false;
for (uint256 k = 0; k < preUpgradeWallets.length; k++) {
if (preUpgradeWallets[k] == w) { seen = true; break; }
}
if (!seen) self.walletRegistrationOrder.push(w);
}
(alternatively, keep the seed dedup-free and change the runbook to pass only wallets not already on-chain). Either way, also fix the runbook/NatSpec text that claims the seed "requires an empty order" / reverts with "Wallet registration order already populated". The actual guard is walletRegistrationOrderSeeded / "already seeded" and it does accept a non-empty order.
…ges can still mature finalizeWalletClosing transitioned a wallet from Closing to Closed without checking for unresolved fraud challenges. A challenge submitted in the last fraudChallengeDefeatTimeout window of the closing period could not be resolved once the wallet became Closed: notifyWalletFraudChallengeDefeatTimeout rejects Closed wallets, stranding the challenger's ETH deposit and letting the wallet operators avoid slashing. Both notifyWalletClosingPeriodElapsed and submitFraudChallenge are permissionless, so the conflict is a matter of timing. The TIP-109 hotfix re-enables fraud challenges, making the window exploitable. Track unresolved fraud challenges per wallet via a walletPendingFraudChallenges mapping (added to BridgeState.Storage by consuming one reserved __gap slot): incremented in submitFraudChallenge and decremented when a challenge is defeated or times out. finalizeWalletClosing now reverts while the counter is nonzero, so the wallet stays in Closing — where the timeout and defeat paths remain reachable — until every challenge resolves. The decrement tolerates challenges opened before the counter existed so their resolution cannot underflow. The counter only protects challenges opened after the upgrade that added it. A fraud challenge already open at upgrade time is never counted, so its wallet counter stays zero and finalizeWalletClosing cannot keep the wallet in Closing; the wallet can still reach Closed with that challenge maturing. Handle this pre-upgrade case in notifyWalletFraudChallengeDefeatTimeout by accepting Closed wallets alongside Terminated ones: both have already had their ECDSA registry entry deleted, so neither can be slashed, but the timeout still resolves the challenge and refunds the challenger instead of reverting. Counted challenges keep the wallet in Closing where slashing still applies, so the Closed branch runs only for uncounted pre-upgrade challenges.
…s-chain redemption refunds A cross-chain redemption submitted through the L1 redeemer records the L1 redeemer contract as the Bridge redeemer. When such a redemption times out, the Bridge refunds the Bank balance to request.redeemer — the L1 contract — not the originating L2 user. Bank balance is a distinct asset from tBTC tokens, so the existing rescueTbtc cannot recover it, and there was no other path to extract it, permanently stranding the refund absent a contract upgrade. Add rescueBankBalance to AbstractBTCRedeemer, letting the owner forward Bank balance held by the contract to the user or a designated recovery address, mirroring rescueTbtc. Expose balanceOf and transferBalance on IBank (both already present on the Bank contract) and implement them in MockBank.
…ption funds from the watchtower A cross-chain redemption records the L1 redeemer contract as the Bridge redeemer. When such a redemption is vetoed, RedemptionWatchtower lets only veto.redeemer withdraw the non-penalty Bank balance via withdrawVetoedFunds. For cross-chain requests that redeemer is the L1 contract, which had no function to call the watchtower or forward the recovered balance, stranding the funds until a contract upgrade. Add withdrawVetoedRedemptionFunds to AbstractBTCRedeemer: the owner supplies the watchtower and vetoed redemption key, the contract withdraws the balance (as the recorded redeemer) and forwards only the freshly recovered amount to a recovery recipient, leaving any unrelated Bank balance for separate recovery. Add a minimal IRedemptionWatchtower interface and a mock for tests.
…r Closing and Closed target wallets Moved-funds sweep target wallets are validated as Live only when the commitment is submitted, not when the moved-funds sweep request is registered. If a target wallet enters Closing or Closed after commitment but before its sweep completes, the request became stuck: the sweep path accepts only Live or MovingFunds, and notifyWalletMovedFundsSweepTimeout rejected Closing and Closed, so the timeout could neither slash nor clear the request. Accept Closing and Closed in notifyWalletMovedFundsSweepTimeout. Closing wallets still hold their ECDSA registry metadata, so they are slashed and terminated like Live/MovingFunds. Closed wallets have had their registry entry deleted by finalizeWalletClosing, so seizing is impossible; the request is cleared without slashing, like Terminated. The stuck request can now always be resolved.
…llenge escrow seed calldata The hotfix script computed the open pre-upgrade fraud-challenge escrow at script-run time and saved the resulting seedFraudChallengeEscrow calldata as an executable post-upgrade action in the deployment summary. Because the script runs before the governance timelock executes the upgrade, new pre-upgrade challenges can still be opened during the delay. Executing the stale saved calldata would seed openFraudChallengeEscrow below the true amount of unresolved challenger deposits, exposing those deposits to recoverETH and underflowing later challenge resolution. The seed is one-shot, so the understatement could not be corrected without another upgrade. The persisted postUpgradeActions entry no longer contains executable calldata. It carries a requiresRecomputation flag, the function signature, the event scan start block, and the earlier computation only as clearly-labeled reference values, instructing the operator to recompute the open escrow immediately before executing the seed action.
…nsfer governance before seeding fraud escrow The upgraded Bridge blocks all new fraud challenges until seedFraudChallengeEscrow succeeds, but the hotfix script routed that seed call at the existing mainnet BridgeGovernance, which predates the function and has no fallback. The call would revert, fraudChallengeEscrowSeeded would stay false, and every new fraud challenge would revert with FraudChallengeEscrowNotSeeded after the upgrade. Deploy a fresh BridgeGovernance built from this tree (which forwards seedFraudChallengeEscrow to the Bridge), preserving the current governance delay read on-chain. Generate the ordered governance actions: hand the new BridgeGovernance to the Council Safe, transfer Bridge governance from the legacy BridgeGovernance to the new one (begin then finalize after the delay), and route the post-upgrade seed at the new BridgeGovernance instead of the legacy one. Print the generated runbook in execution order so the governance transfer lands before the Bridge upgrade: STEP 1 transfers Bridge governance to the new BridgeGovernance, STEP 2 runs the timelock upgrades, and STEP 3 seeds the fraud escrow immediately after. Running the transfer first shrinks the window where the upgraded Bridge has fraud challenges disabled from the full governance-transfer delay down to the gap between the upgrade and the seed. A deploy test captures the printed runbook and asserts the transfer precedes the upgrade, which precedes the seed.
…TBTCVault in the hotfix The migration-debt flow routes deposits through TBTCVault, and the upgraded Bridge only accepts a canonical migrationDebtVault that exposes the migration-debt read interface. The hotfix script upgraded the Bridge but never deployed or upgraded TBTCVault, so the existing mainnet vault (a direct deployment predating the interface) would be rejected by setMigrationDebtVault with MigrationDebtVaultInterfaceMissing, leaving migration deposits unactivatable and migration debt unrepayable. Deploy a fresh TBTCVault built from this tree, wired to the existing Bank, TBTC token, and Bridge. Generate the ordered activation actions: the TBTCVault owner initiates the upgrade and, after the 24h vault delay, finalizes it (moving TBTC ownership and Bank balance to the new vault); the Council Safe then trusts the new vault and sets it as the canonical migration debt vault through the new BridgeGovernance.
…y from migration-tagged deposits submitDepositSweepProof passed every swept depositor to the migration sweep completion callback. A migration revealer's regular (non-migration) deposit, routed to the canonical migration debt vault, repays their migration debt in receiveBalanceIncrease and then had its completion consumed by the same regular sweep's callback, emitting a migration completion tied to a normal sweep transaction hash before the intended migration sweep. Track migration-tagged depositors separately during sweep proof processing (a per-input array aligned with depositors, populated only when the deposit extraData is a migration reveal, regular slots left as the zero address which the vault callback ignores) and drive notifyMigrationSweepCallback from that list. Regular deposits no longer complete migration callbacks; genuine migration deposits still do.
…back retry completion The migration sweep callback is intentionally fail-open: a reverting downstream notifier does not block a proven sweep, and the pending completion is preserved for a later retry, with MigrationSweepCallbackFailed and MigrationSweepCallbackRetryFailed emitted as actionable alerts. Add an end-to-end test that reverts both the batch and per-revealer hooks, asserts the sweep stays fail-open and both alert events fire with no downstream notification, then clears the failure and retries, verifying the downstream state transition completes on the operational retry.
…rget selection submitMovingFundsCommitment accepted any sorted subset of Live wallets that matched the expected count, so a single source-wallet member could front-run the honest commitment with a different valid subset and permanently pin the fund-routing decision to wallets where they hold more signing influence. The Bridge now records wallet registration order in an append-only list and reconstructs the canonical target set on-chain inside submitMovingFundsCommitment: the newest Live wallets other than the source, matching the selection computed off-chain. Any submission that differs is rejected. The deterministic selection lives in the MovingFunds and Wallets libraries to keep the Bridge within its contract size limit. Wallets registered before the upgrade that introduced the order are absent from it, so governance backfills them once via seedWalletRegistrationOrder, a one-shot onlyGovernance entry mirroring seedFraudChallengeEscrow that requires the order to still be empty so the backfilled wallets form the correct oldest-first prefix. The hotfix deployment script generates this backfill action alongside the fraud escrow seed. Until the order is backfilled it cannot reconstruct the canonical set for pre-upgrade wallets, so submitMovingFundsCommitment rejects those commitments rather than falling back to the permissive subset check that this finding exploits; the backfill is therefore a required precondition for resuming moving-funds commitments after the upgrade. Bridge optimizer runs are lowered to keep the new governance entry point under the EIP-170 deployed bytecode limit.
…legacy redemptions The legacy redemption path burned fungible TBTC and forwarded Bank balance to the Bridge without notifying Account Control, so redeeming AC-origin TBTC left the reserve's minted exposure unchanged and overstated cross-route supply accounting. TBTCVault now exposes an optional, owner-configured IAccountControlRedemptionNotifier and reconciles through it on every vault path that burns fungible TBTC back into Bank balance. Both _unmint and _unmintAndRedeem call a shared _reconcileAccountControlRedemption helper before the burn: _unmint matters because the Bank balance it returns can itself be redeemed directly through the Bridge, consuming BTC without ever touching AC, so reconciling only the bundled _unmintAndRedeem left that unmint-then-redeem sequence as an open bypass. When a notifier is set it identifies the AC reserve behind the redeemer and decrements its minted exposure, reverting the whole operation if the accounting update cannot be made, and no-ops for redeemers with no AC exposure so regular unminting and redemption are unaffected. Reserve identification is delegated to the notifier because the legacy path does not record which reserve minted a given fungible balance. An owner-set accountControlReconciliationRequired flag lets an Account Control deployment make reconciliation mandatory: once enabled, a legacy unmint or redemption with no notifier configured reverts instead of silently bypassing AC accounting. It defaults to false so non-AC deployments keep the existing legacy behavior, and is independent of the notifier so the requirement cannot be bypassed by unsetting the notifier.
…ud challenges The covenant signer produces a normal Bitcoin SIGHASH_ALL signature over a covenant active UTXO with the tBTC wallet key. A covenant active UTXO is neither a swept deposit, a spent main UTXO, nor a processed moved-funds sweep, so defeatFraudChallenge could never recognize the spend as honest: any observer could reconstruct the BIP-143 preimage from the broadcast migration transaction, open a fraud challenge, and slash the wallet for a covenant migration it was asked to sign. The keep-core covenant signer currently fails closed (bridgeCovenantFraudDefenseConfirmed defaults to false) precisely because this bridge-side defense path did not exist. The Bridge now exposes defeatFraudChallengeWithCovenantSpend, which recognizes the covenant spend through the account-control migration flow itself: the caller provides the covenant migration transaction, whose single input must be the challenged outpoint and whose outputs must include at least one swept migration-tagged deposit of the Bridge. Migration-tagged deposits can only be revealed through the guarded migration reveal path bound to the canonical migration debt vault, and their swept state is set only by an SPV-proven deposit sweep, which also proves the migration transaction was confirmed on the Bitcoin chain. The challenged input value that does not end up in swept migration-tagged deposit outputs — the transaction fee plus the anyone-can-spend fee-bumping anchor output — is capped at depositTxMaxFee. Without that bound, a rogue wallet colluding with a registered migration revealer could launder a theft of any wallet-controlled UTXO into a defeatable challenge by attaching a dust migration deposit to the stealing transaction. Covenant migration transaction plans must therefore keep the fee plus anchor value within depositTxMaxFee, and the funded migration deposit must be swept before the challenge defeat timeout for the defense to be available.
lrsaturnino
force-pushed
the
fix/tob-tbtcacext-remediation
branch
from
af46c08 to
e4e9696
Compare
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.
Applies the remediation for the Trail of Bits "tBTC v2 AC Extension" security assessment. Every in-scope finding whose target code lives in this repository is fixed in its own commit, tagged with its public report ID (
TOB-TBTCACEXT-<n>) so each fix maps 1:1 to a finding, with regression test(s) added alongside each fix.Base / stacking
This branch was created off the frozen review commit
adefd664, which is the head offeat/psbt-covenant-bridge-port(the reviewed PR, #957). The PR therefore targets that branch so the diff is exactly the remediation (17 commits) rather than PR #957's own changes. Retarget tomainonce #957 lands.Findings remediated (16)
TBTCVaultin the hotfix), 18 (deploy newBridgeGovernanceand transfer governance before seeding fraud escrow), 23 (Sui: reject minting with aMinterCapwhose minter was removed)Closing/Closedtarget wallets)Finding 37 is a pre-existing issue outside the reviewed diff; it is included because the scoped TIP-109 hotfix re-enables the fraud-challenge path it affects.
Finding 1 is targeted at
keep-core/pkg/tbtc/covenant_signer.goby the report's Target field, but the report's recommended remediation is a bridge-level fix in this repository: a covenant-active-UTXO defense path forFraud.defeatFraudChallenge. That defense is implemented here asdefeatFraudChallengeWithCovenantSpend, anchored to swept migration-tagged deposits and bounded bydepositTxMaxFee. The keep-core covenant signer keeps its fail-closed stopgap (keep-core PR #4126,bridgeCovenantFraudDefenseConfirmeddefaulting tofalse) until this defense path is deployed and confirmed.Verification
yarn buildpasses and the full Solidity suite is green (3092 passing, 0 failing, 31 pending). The Sui Move change is covered bysui move test(mint-authorization tests). Each finding commit carries its own regression test(s).