Deploy script fix and guardrails by franrolotti · Pull Request #119 · BreadchainCoop/saving-circles

franrolotti · 2026-02-16T10:57:49Z

Problem

Previous deployments could create a nested ProxyAdmin ownership chain (ProxyAdmin A owned by ProxyAdmin B, then owned by deployer EOA).
In practice, this made upgrade authority easy to misconfigure and operationally unsafe.

The root cause was that deployment accepted any ADMIN_ADDRESS without guardrails, including a ProxyAdmin contract address.

What This PR Changes

Adds deploy-time validation for ADMIN_ADDRESS:
- Rejects address(0)
- Rejects addresses that behave like ProxyAdmin (via owner() + UPGRADE_INTERFACE_VERSION() checks)
Updates deploy flow to match OZ v5 TransparentUpgradeableProxy behavior:
- Uses ADMIN_ADDRESS as initialOwner of the proxy’s internally created ProxyAdmin
Adds post-deploy assertions:
- Reads ERC-1967 admin slot and ensures admin is deployed/non-zero
- Confirms ProxyAdmin.owner() == ADMIN_ADDRESS
- Reads ERC-1967 implementation slot and confirms it matches deployed implementation
Adds explicit deployment logs for:
- Admin
- ProxyAdmin
- Proxy
- Implementation

Validation Performed

Deployed successfully on Gnosis with the updated scripts.
Verified onchain:
- signer from GNOSIS_PK equals ADMIN_ADDRESS
- ADMIN_ADDRESS has no code (EOA in current deployment)
- proxy ERC-1967 admin slot points to deployed ProxyAdmin
- ProxyAdmin.owner() equals ADMIN_ADDRESS
- proxy ERC-1967 implementation slot matches expected implementation
- upgradeAndCall authorization simulation via cast call succeeds from ADMIN_ADDRESS (no state change)

Notes

Existing broken deployments with nested admin chains are not repaired by this PR; this prevents recurrence on new deployments.

bagelface · 2026-02-16T23:24:42Z

script/Common.sol

+
+  function _assertValidAdmin(address _admin) internal view {
+    if (_admin == address(0)) revert InvalidAdminAddress();
+    if (_isProxyAdmin(_admin)) revert InvalidAdminProxyAdmin(_admin);


This error is sort of confusingly named imo. Maybe revert AddressAlreadyProxyAdmin? Do we want to have a way to deploy a proxy reusing a proxy admin contract? Like a ProxyAdmin address is technically a valid address. You can reuse ProxyAdmins for multiple proxy contracts. The script just needs to know not to redeploy a new ProxyAdmin contract with the existing ProxyAdmin contract as the owner.

bagelface

see comment https://github.com/BreadchainCoop/saving-circles/pull/119/changes#r2814358437

Copilot

Pull request overview

This PR adds deployment guardrails to prevent nested ProxyAdmin ownership chains by validating the admin address before deployment and asserting correct proxy configuration afterward. The changes align the deployment flow with OpenZeppelin v5's TransparentUpgradeableProxy pattern where the initialOwner parameter creates an internal ProxyAdmin, replacing the previous manual ProxyAdmin creation.

Changes:

Added pre-deployment validation to reject address(0) and ProxyAdmin contracts as admin addresses
Added post-deployment assertions to verify proxy admin slot, ProxyAdmin ownership, and implementation slot
Updated deployment flow to use OZ v5 pattern with initialOwner parameter instead of manually creating ProxyAdmin

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-18T18:48:26Z

script/Common.sol

  function _deployContracts(address _admin) internal returns (TransparentUpgradeableProxy) {
+    _assertValidAdmin(_admin);
+
+    SavingCircles implementation = _deploySavingCircles();
    TransparentUpgradeableProxy proxy = _deployTransparentProxy(
-      address(_deploySavingCircles()),
-      address(_deployProxyAdmin(_admin)),
-      abi.encodeWithSelector(SavingCircles.initialize.selector, _admin)
+      address(implementation), _admin, abi.encodeWithSelector(SavingCircles.initialize.selector, _admin)
    );

    // Deploy auxiliary contracts that reference the SavingCircles proxy
    new DelegatedSavingCircles(address(proxy));
    new SavingCirclesViewer(address(proxy));

+    address proxyAdmin = _assertDeployment(address(proxy), address(implementation), _admin);
+
+    console2.log('Deployer', msg.sender);
+    console2.log('Admin', _admin);
+    console2.log('ProxyAdmin', proxyAdmin);
+    console2.log('Proxy', address(proxy));
+    console2.log('Implementation', address(implementation));
+
    return proxy;
  }


The new validation logic in _assertValidAdmin and _assertDeployment lacks test coverage. Given that this is a security-critical feature designed to prevent deployment misconfigurations (specifically nested ProxyAdmin ownership chains), comprehensive tests should be added to verify:

Deployment fails when _admin is address(0)

Deployment fails when _admin is a ProxyAdmin contract address

Deployment succeeds with a valid EOA address

Post-deployment assertions correctly validate the proxy state

Edge cases in _isProxyAdmin detection (e.g., contracts with owner() but not UPGRADE_INTERFACE_VERSION())

The repository demonstrates comprehensive test coverage for other functionality (unit and integration tests exist), so this new validation logic should follow the same pattern.

Copilot · 2026-02-18T18:48:26Z

script/Common.sol

+  bytes4 internal constant _OWNER_SELECTOR = bytes4(keccak256('owner()'));
+  bytes4 internal constant _UPGRADE_INTERFACE_VERSION_SELECTOR = bytes4(keccak256('UPGRADE_INTERFACE_VERSION()'));
+
+  error InvalidAdminAddress();


For consistency with other error definitions in this contract, consider including the invalid address as a parameter in the InvalidAdminAddress error. While the error is only triggered when _admin == address(0), including the parameter would make the error interface more consistent and could be helpful if the validation logic is extended in the future.

Example: error InvalidAdminAddress(address admin); and revert InvalidAdminAddress(_admin);

…dmin control

…n/implementation guardrails

franrolotti · 2026-03-09T10:19:46Z

Upgrade Summary (SavingCircles) - Gnosis (chainId 100) - 2026-03-09

Upgrade completed successfully with pre/post safety checks.

Old implementation: 0x58Ad17Fe5Db2a4228942991BCA1bB7A1ff5e54a1
New implementation: 0x702F9Fd0157a2eA3C98daC71E2a998FBf567783E
Upgrade tx: 0x5de15b6b4d58f11a78b73bf9b7dc27252e271ebb6b260b7b5d81cd982d257bb8

Viewer rollout:

Old viewer: 0x30142762922fa1594eA0b9e2e9a3b167F5FF31B0
New viewer: 0x54Ff2199880724F1a78A322980221196362F0764
Viewer deploy tx: 0xdf450149983619455bcbcea168063a0840f2a5a4b4ffae83aeed34d9fe80ea9b

Final addresses:

Network: Gnosis (chainId 100)
SavingCircles Proxy (frontend): 0x7bC9212FA4Ad242D5Cee496e0570b312446877E3
SavingCircles Implementation: 0x702F9Fd0157a2eA3C98daC71E2a998FBf567783E
ProxyAdmin: 0x498C6b190130CEb23F4Fab1C3d19388F5D55b67b
DelegatedSavingCircles: 0x6DA3dAFa3d1372458eefa5707dfb1ED0cE19ab4d
SavingCirclesViewer: 0x54Ff2199880724F1a78A322980221196362F0764
Allowed token (BREAD): 0xa555d5344f6FB6c65da19e403Cb4c1eC4a1a5Ee3

Checks completed:

Storage layout compatibility check passed vs baseline commit 8be323992bc8755e77f27839b327422a07cfa37f.
Pre-upgrade validation passed (UpgradeValidate).
Upgrade execution succeeded (UpgradeExecute).
Post-upgrade wiring validation passed (UpgradePostValidate):

…on for PRs to dev

franrolotti requested review from RonTuretzky and bagelface February 16, 2026 10:58

franrolotti changed the base branch from 104-assess-time-based-rounds-derive-currentindex-from-time-define-behavior-for-unclaimed-rounds to dev February 16, 2026 17:07

bagelface reviewed Feb 16, 2026

View reviewed changes

RonTuretzky requested a review from Copilot February 18, 2026 18:41

Copilot started reviewing on behalf of RonTuretzky February 18, 2026 18:42 View session

Copilot AI reviewed Feb 18, 2026

View reviewed changes

franrolotti self-assigned this Feb 23, 2026

fix(deploy): prevent nested ProxyAdmin ownership and enforce direct a…

76a2640

…dmin control

franrolotti force-pushed the 104-deploy-guardrails branch from 3a017d6 to 76a2640 Compare March 9, 2026 08:50

feat(script): add read-only proxy upgrade validation script with admi…

740e532

…n/implementation guardrails

franrolotti added 7 commits March 9, 2026 11:22

feat(script): add upgrade execute and post-upgrade validation scripts

f13baa5

chore(script): add canonical storage layout compatibility checker

a7ceaff

ci: add PR storage layout compatibility check for SavingCircles

68c1869

ci: compare SavingCircles storage layout against dev for all PRs

ff77fd0

ci: run storage layout check on pull requests to dev

981c117

ci: use yarn install in storage layout workflow

5a33376

ci: replace storage layout diff check with OZ upgrade safety validati…

044e600

…on for PRs to dev

franrolotti mentioned this pull request Mar 9, 2026

Improve upgrade safety and deployment guardrail workflows #129

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deploy script fix and guardrails#119

Deploy script fix and guardrails#119
franrolotti wants to merge 9 commits intodevfrom
104-deploy-guardrails

franrolotti commented Feb 16, 2026

Uh oh!

bagelface Feb 16, 2026 •

edited

Loading

Uh oh!

bagelface left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 18, 2026

Uh oh!

Copilot AI Feb 18, 2026

Uh oh!

franrolotti commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

franrolotti commented Feb 16, 2026

Problem

What This PR Changes

Validation Performed

Notes

Uh oh!

bagelface Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bagelface left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

Uh oh!

franrolotti commented Mar 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bagelface Feb 16, 2026 •

edited

Loading