ENG-472 Bridge controller pointer and MintBurnGuard guardrails#918
Open
piotr-roslaniec wants to merge 79 commits intomainfrom
Open
ENG-472 Bridge controller pointer and MintBurnGuard guardrails#918piotr-roslaniec wants to merge 79 commits intomainfrom
piotr-roslaniec wants to merge 79 commits intomainfrom
Conversation
…ocal env; add Sepolia env example
…only, verify library bytecodes before linking
…_PK or named account)
…ack to use governanceDelays(0)
…OZ upgrades plugin + Sepolia env
…es; format; make format script run Prettier only to satisfy CI
This reverts commit 766a6d0.
piotr-roslaniec
requested review from
Shadowfiend,
lukasz-zimnoch,
nkuba and
pdyraga
as code owners
lrsaturnino
previously approved these changes
Dec 3, 2025
Member
lrsaturnino
left a comment
lrsaturnino
left a comment
There was a problem hiding this comment.
Took a deep dive into this and it LGTM. The separation between Bridge (who can mint) and MintBurnGuard (caps, pause, rate limits) is clean and makes the security model easy to reason about. I initially flagged a few things as concerns but after actually tracing through the code, they're all intentional - the Bank's handling of zero-address, the batch validation, test coverage - it all checks out. Nice touch on tracking mints AND burns together in totalMinted - that way the cap actually reflects real exposure, not just minting volume.
Comment on lines
+10
to
+11
| solidity/.env | ||
| solidity/.env.sepolia |
There was a problem hiding this comment.
Suggested change
| solidity/.env | |
| solidity/.env.sepolia | |
| solidity/.env* |
Comment on lines
+39
to
+49
| function mintToBank(address recipient, uint256 tbtcAmount) external; | ||
|
|
||
| /// @notice Burns TBTC bank balance via the underlying Bank and reduces | ||
| /// global net exposure. | ||
| /// @param from Source address for burns that operate on balances. | ||
| /// @param tbtcAmount Amount in TBTC satoshis (1e8) to burn from the Bank. | ||
| function burnFromBank(address from, uint256 tbtcAmount) external; | ||
|
|
||
| /// @notice Unmints TBTC via the underlying Vault and reduces global net exposure. | ||
| /// @param tbtcAmount Amount in TBTC satoshis (1e8) to unmint. | ||
| function unmintFromVault(uint256 tbtcAmount) external; |
There was a problem hiding this comment.
Suggested change
| function mintToBank(address recipient, uint256 tbtcAmount) external; | |
| /// @notice Burns TBTC bank balance via the underlying Bank and reduces | |
| /// global net exposure. | |
| /// @param from Source address for burns that operate on balances. | |
| /// @param tbtcAmount Amount in TBTC satoshis (1e8) to burn from the Bank. | |
| function burnFromBank(address from, uint256 tbtcAmount) external; | |
| /// @notice Unmints TBTC via the underlying Vault and reduces global net exposure. | |
| /// @param tbtcAmount Amount in TBTC satoshis (1e8) to unmint. | |
| function unmintFromVault(uint256 tbtcAmount) external; | |
| function mintToBank(address recipient, uint256 tbtcAmountSats) external; | |
| /// @notice Burns TBTC bank balance via the underlying Bank and reduces | |
| /// global net exposure. | |
| /// @param from Source address for burns that operate on balances. | |
| /// @param tbtcAmount Amount in TBTC satoshis (1e8) to burn from the Bank. | |
| function burnFromBank(address from, uint256 tbtcAmountSats) external; | |
| /// @notice Unmints TBTC via the underlying Vault and reduces global net exposure. | |
| /// @param tbtcAmount Amount in TBTC satoshis (1e8) to unmint. | |
| function unmintFromVault(uint256 tbtcAmountSats) external; |
There was a problem hiding this comment.
the name tbtcAmountSats is clear
| uint256 public lastBurnAmount; | ||
|
|
||
| function decreaseBalance(uint256 amount) external { | ||
| lastBurnAmount = amount; |
There was a problem hiding this comment.
What about adding a new event here to help with debugging the tests?
…, and modify IBank.transferBalanceFrom() signature,
miquelcabot
reviewed
Jan 7, 2026
| uint256 newTotal = _increaseTotalMintedInternal(amount); | ||
|
|
||
| emit BankMintExecuted(controller, recipient, amount, newTotal); | ||
| bridge.controllerIncreaseBalance(recipient, _toTbtcBaseUnits(amount)); |
There was a problem hiding this comment.
@piotr-roslaniec Possible Issue: incorrectly calling bridge.controllerIncreaseBalance(recipient, _toTbtcBaseUnits(amount)), converting satoshis (1e8) to base units (1e18) before passing to the Bridge. However, Bridge.controllerIncreaseBalance() → Bank.increaseBalance() expects satoshis (1e8), not base units (1e18)
Contributor
Author
There was a problem hiding this comment.
Good catch, @miquelcabot. Please include that in your changes as well. Thanks
…ounts instead of tbtc (1e18)
… state management library
…n & tests (#921) ## Summary This PR completes the ENG-472 workstream by implementing the `MintBurnGuard` contract with full integration support for Account Control, comprehensive testing, deployment scripts, and Bridge governance wiring. The focus is to establish a robust controller-driven mint/burn path with global exposure tracking, rate limiting, and pause controls while maintaining clean separation between the guard's enforcement logic and Account Control's reserve management. ## Scope - **Bridge controller authorization**: Implemented `setMintingController()` governance function and renamed `IMintingAuthorization` --> `IBridgeController` for clarity. - **Operator terminology standardization**: Unified naming from "controller" to "operator" throughout MintBurnGuard to distinguish from Bridge's "minting controller" role. - **Core burn/unmint functionality**: Implemented `unmintAndBurnFrom()` and `burnFrom()` methods for AccountControl integration, with SafeERC20 usage and comprehensive error handling. - **Interface refinement**: Updated `IMintBurnGuard` with new burn/unmint methods; extended `IBank` and `ITBTCVault` interfaces for integration requirements. - **Deployment infrastructure**: Created deployment and configuration scripts (`44_deploy_mint_burn_guard.ts`, `45_configure_mint_burn_guard.ts`, `46_authorize_mint_burn_guard_in_bridge.ts`). - **Comprehensive test coverage**: Expanded from 28 to 38 tests, including 10 new integration tests with `MockAccountControl` simulating realistic AccountControl usage patterns. - **Mock contracts for testing**: Created `MockAccountControl`, `MockBridgeController`, `MockBurnBank`, and `MockBurnVault` to support integration testing. ## Motivation ENG-472 introduced a least-privilege operator pattern with global mint/burn guardrails. This PR builds on that foundation by: - Completing the MintBurnGuard implementation with all required burn/unmint methods - Establishing clear terminology (operator vs. controller) to avoid confusion - Providing comprehensive integration testing that validates AccountControl usage patterns - Creating production-ready deployment scripts with governance authorization flow - Ensuring the design is aligned with Bridge governance and Account Control expectations The completed implementation enables AccountControl to manage per-reserve accounting while MintBurnGuard enforces system-wide caps, rate limits, and pause controls. ## Notes - This PR is the completion of the initial ENG-472 MintBurnGuard workstream. - **Breaking changes**: Interface renames (`IMintingAuthorization` --> `IBridgeController`, `controller` --> `operator` in MintBurnGuard) require migration for any external integrations. - **Test coverage**: All 38 tests passing with comprehensive coverage of mint/burn flows, operator authorization, cap enforcement, rate limiting, and multi-reserve scenarios.
…ation (#936) # Summary PR#933 upgraded the Sepolia Bridge proxy to repair rebateStaking (using reinitializer(5)), overwriting a prior deployment from new/bank-decreaser that had introduced MintBurnGuard controller support. The current on-chain Bridge is missing: - setMintingController() / getMintingController() - controllerIncreaseBalance() / controllerIncreaseBalances() This PR adds the tooling to re-upgrade the Sepolia Bridge proxy to the new/bank-decreaser implementation, restoring those methods. No reinitializer is called — the mintingController slot defaults to address(0) and is set post-upgrade via governance. ## Changes - solidity/deploy/84_upgrade_bridge_mint_burn_controller.ts — new deploy script that reads the ProxyAdmin from the EIP-1967 admin slot, deploys a fresh Bridge implementation linked to the existing Sepolia libraries, and calls ProxyAdmin.upgrade() directly. Avoids dependency on the OZ network manifest. - solidity/deploy/utils/library-resolution.ts — fix verifyLibraryBytecodes to use FQN contracts/bridge/${name}.sol:${name} to avoid HH701 ambiguity on the Wallets artifact. - solidity/test/bridge/Bridge.MintBurnControllerUpgrade.test.ts — fork test validating the upgrade against a Sepolia fork. Requires Anvil (--network system_tests); also activates when HARDHAT_NETWORK=system_tests so FORKING_URL is not required. - solidity/scripts/snapshot.sh — pre-upgrade snapshot script: records baseline on-chain state and validates abort conditions. - solidity/scripts/verify-upgrade.sh — post-upgrade verification script: asserts implementation changed, new methods exist, rebateStaking preserved, governance and ProxyAdmin unchanged. - solidity/.env.example — documents all env vars required for the upgrade workflow. - docs/bridge-mintburn-controller-upgrade-runbook.md — end-to-end 8-phase runbook covering snapshot, build, standard tests, fork test, dry run with real keys, live upgrade, post-upgrade verification, and skip guard restore. ## Runbook status - [x] Phase 1 — Pre-upgrade snapshot (snapshot.sh) - [x] Phase 2 — Build and lint - [x] Phase 3 — Standard test suite - [x] Phase 4 — Fork test (4/4 passing against Sepolia fork on Anvil) - [x] Phase 5 — Dry run with real keys against Anvil fork (--network system_tests) - [x] **Phase 6 — Execute the live upgrade on Sepolia** - [x] Phase 7 — Post-upgrade verification (verify-upgrade.sh) - [x] Phase 8 — Re-enable func.skip and commit
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
7 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
Motivation
Changes
setControllerBalanceIncreaser(address)(zero clears); controller-guardedcontrollerIncreaseBalanceand batch variant; exposecontrollerBalanceIncreaser(); storage gap adjusted.setControllerBalanceIncreaser(address)to manage the Bridge controller pointer.reduceExposureAndBurn; added owner-onlysetTotalMinted(cap-checked, rate window reset); enforced cap vs rate-limit consistency, pause/rate checks, zero-address guards, custom errors, and safe addition (unchecked removed); event naming aligned toamount.