Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .github/workflows/upgrade-safety-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
name: Upgrade Safety Check

on:
pull_request:
branches:
- dev

jobs:
upgrade-safety:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4
with:
fetch-depth: 0
submodules: recursive

- name: Install Foundry
uses: foundry-rs/foundry-toolchain@v1

- name: Install Node dependencies
run: yarn --frozen-lockfile --network-concurrency 1

- name: Fetch dev branch
run: git fetch origin dev:refs/remotes/origin/dev

- name: Check SavingCircles upgrade safety
run: ./scripts/check_upgrade_safety.sh origin/dev HEAD src/contracts/SavingCircles.sol:SavingCircles
74 changes: 65 additions & 9 deletions script/Common.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ pragma solidity 0.8.28;
import {ProxyAdmin} from '@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol';
import {TransparentUpgradeableProxy} from '@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol';
import {Script} from 'forge-std/Script.sol';
import {console2} from 'forge-std/console2.sol';

import {DelegatedSavingCircles} from '../src/contracts/DelegatedSavingCircles.sol';
import {SavingCircles} from '../src/contracts/SavingCircles.sol';
Expand All @@ -16,35 +17,90 @@ import {SavingCirclesViewer} from '../src/contracts/SavingCirclesViewer.sol';
* @dev This contract is intended for use in Scripts and Integration Tests
*/
contract Common is Script {
bytes32 internal constant _ERC1967_IMPLEMENTATION_SLOT =
0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc;
bytes32 internal constant _ERC1967_ADMIN_SLOT = 0xb53127684a568b3173ae13b9f8a6016e243e63b6e8ee1178d6a717850b5d6103;
bytes4 internal constant _OWNER_SELECTOR = bytes4(keccak256('owner()'));
bytes4 internal constant _UPGRADE_INTERFACE_VERSION_SELECTOR = bytes4(keccak256('UPGRADE_INTERFACE_VERSION()'));

error InvalidAdminAddress();
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copilot uses AI. Check for mistakes.
error InvalidAdminProxyAdmin(address admin);
error ProxyAdminOwnerMismatch(address proxyAdmin, address expectedOwner, address actualOwner);
error ProxyAdminNotDeployed(address proxy);
error ProxyImplementationSlotMismatch(address proxy, address expectedImplementation, address actualImplementation);

function setUp() public virtual {}

function _deploySavingCircles() internal returns (SavingCircles) {
return new SavingCircles();
}

function _deployProxyAdmin(address _admin) internal returns (ProxyAdmin) {
return new ProxyAdmin(_admin);
}

function _deployTransparentProxy(
address _implementation,
address _proxyAdmin,
address _adminOwner,
bytes memory _initData
) internal returns (TransparentUpgradeableProxy) {
return new TransparentUpgradeableProxy(_implementation, _proxyAdmin, _initData);
return new TransparentUpgradeableProxy(_implementation, _adminOwner, _initData);
}

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;
}
Comment on lines 46 to 67
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Deployment fails when _admin is address(0)
  2. Deployment fails when _admin is a ProxyAdmin contract address
  3. Deployment succeeds with a valid EOA address
  4. Post-deployment assertions correctly validate the proxy state
  5. 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 uses AI. Check for mistakes.

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

@bagelface bagelface Feb 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

function _assertDeployment(
address _proxy,
address _implementation,
address _expectedAdminOwner
) internal view returns (address proxyAdmin) {
proxyAdmin = _readAddressFromSlot(_proxy, _ERC1967_ADMIN_SLOT);
if (proxyAdmin == address(0)) revert ProxyAdminNotDeployed(_proxy);

address actualOwner = ProxyAdmin(proxyAdmin).owner();
if (actualOwner != _expectedAdminOwner) {
revert ProxyAdminOwnerMismatch(proxyAdmin, _expectedAdminOwner, actualOwner);
}

address actualImplementation = _readAddressFromSlot(_proxy, _ERC1967_IMPLEMENTATION_SLOT);
if (actualImplementation != _implementation) {
revert ProxyImplementationSlotMismatch(_proxy, _implementation, actualImplementation);
}
}

function _readAddressFromSlot(address _contract, bytes32 _slot) internal view returns (address) {
return address(uint160(uint256(vm.load(_contract, _slot))));
}

function _isProxyAdmin(address _candidate) internal view returns (bool) {
if (_candidate.code.length == 0) return false;

(bool ownerCallSuccess,) = _candidate.staticcall(abi.encodeWithSelector(_OWNER_SELECTOR));
if (!ownerCallSuccess) return false;

(bool versionCallSuccess,) = _candidate.staticcall(abi.encodeWithSelector(_UPGRADE_INTERFACE_VERSION_SELECTOR));
return versionCallSuccess;
}
}
61 changes: 61 additions & 0 deletions script/UpgradeExecute.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.28;

import {ProxyAdmin} from '@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol';
import {ITransparentUpgradeableProxy} from '@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol';
import {console2} from 'forge-std/console2.sol';

import {Common} from 'script/Common.sol';

contract UpgradeExecute is Common {
error NewImplementationHasNoCode(address implementation);
error NewImplementationMatchesCurrent(address implementation);
error AlreadyUpgraded(address implementation);

function run() public {
address proxy = vm.envAddress('PROXY_ADDRESS');
address expectedAdminOwner = vm.envAddress('EXPECTED_ADMIN_OWNER');
address expectedCurrentImplementation = vm.envAddress('EXPECTED_CURRENT_IMPLEMENTATION');
address newImplementation = vm.envAddress('NEW_IMPLEMENTATION');
bytes memory upgradeCalldata = _upgradeCalldataOrEmpty();

address currentImplementation = _readAddressFromSlot(proxy, _ERC1967_IMPLEMENTATION_SLOT);
if (currentImplementation == newImplementation) revert AlreadyUpgraded(newImplementation);

address proxyAdmin = _assertDeployment(proxy, expectedCurrentImplementation, expectedAdminOwner);

if (newImplementation.code.length == 0) revert NewImplementationHasNoCode(newImplementation);
if (newImplementation == expectedCurrentImplementation) {
revert NewImplementationMatchesCurrent(newImplementation);
}

console2.log('Executing upgrade');
console2.log('Proxy', proxy);
console2.log('ProxyAdmin', proxyAdmin);
console2.log('AdminOwner', expectedAdminOwner);
console2.log('CurrentImplementation', currentImplementation);
console2.log('NewImplementation', newImplementation);
console2.log('CalldataLength');
console2.log(upgradeCalldata.length);

vm.startBroadcast();
ProxyAdmin(proxyAdmin)
.upgradeAndCall(ITransparentUpgradeableProxy(payable(proxy)), newImplementation, upgradeCalldata);
vm.stopBroadcast();

_assertDeployment(proxy, newImplementation, expectedAdminOwner);

console2.log('Upgrade successful');
console2.log('Proxy', proxy);
console2.log('ProxyAdmin', proxyAdmin);
console2.log('CurrentImplementation', _readAddressFromSlot(proxy, _ERC1967_IMPLEMENTATION_SLOT));
}

function _upgradeCalldataOrEmpty() internal view returns (bytes memory data) {
try vm.envBytes('UPGRADE_CALLDATA') returns (bytes memory envData) {
return envData;
} catch {
return bytes('');
}
}
}
50 changes: 50 additions & 0 deletions script/UpgradePostValidate.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.28;

import {ProxyAdmin} from '@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol';
import {console2} from 'forge-std/console2.sol';

import {Common} from 'script/Common.sol';
import {SavingCircles} from 'src/contracts/SavingCircles.sol';
import {SavingCirclesViewer} from 'src/contracts/SavingCirclesViewer.sol';

contract UpgradePostValidate is Common {
error ViewerProxyMismatch(address viewer, address expectedProxy, address actualProxy);

function run() public view {
address proxy = vm.envAddress('PROXY_ADDRESS');
address expectedAdminOwner = vm.envAddress('EXPECTED_ADMIN_OWNER');
address expectedImplementation = vm.envAddress('EXPECTED_IMPLEMENTATION');

address proxyAdmin = _assertDeployment(proxy, expectedImplementation, expectedAdminOwner);
uint256 nextId = SavingCircles(proxy).nextId();

console2.log('Post-upgrade validation successful');
console2.log('Proxy', proxy);
console2.log('ProxyAdmin', proxyAdmin);
console2.log('AdminOwner', ProxyAdmin(proxyAdmin).owner());
console2.log('Implementation', expectedImplementation);
console2.log('SmokeCheck.nextId');
console2.log(nextId);

address viewer = _viewerAddressOrZero();
if (viewer == address(0)) return;

address viewerProxy = address(SavingCirclesViewer(viewer).SAVING_CIRCLES());
if (viewerProxy != proxy) revert ViewerProxyMismatch(viewer, proxy, viewerProxy);

uint256 totalBalance = SavingCirclesViewer(viewer).getTotalBalance(expectedAdminOwner);
console2.log('Viewer', viewer);
console2.log('Viewer.SAVING_CIRCLES', viewerProxy);
console2.log('ViewerSmoke.totalBalance(owner)');
console2.log(totalBalance);
}

function _viewerAddressOrZero() internal view returns (address viewer) {
try vm.envAddress('VIEWER_ADDRESS') returns (address envViewer) {
return envViewer;
} catch {
return address(0);
}
}
}
39 changes: 39 additions & 0 deletions script/UpgradeValidate.s.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.28;

import {ProxyAdmin} from '@openzeppelin/contracts/proxy/transparent/ProxyAdmin.sol';
import {console2} from 'forge-std/console2.sol';

import {Common} from 'script/Common.sol';
import {SavingCircles} from 'src/contracts/SavingCircles.sol';

contract UpgradeValidate is Common {
error NewImplementationHasNoCode(address implementation);
error NewImplementationMatchesCurrent(address implementation);

function run() public view {
address proxy = vm.envAddress('PROXY_ADDRESS');
address expectedAdminOwner = vm.envAddress('EXPECTED_ADMIN_OWNER');
address expectedCurrentImplementation = vm.envAddress('EXPECTED_CURRENT_IMPLEMENTATION');
address newImplementation = vm.envAddress('NEW_IMPLEMENTATION');

address proxyAdmin = _assertDeployment(proxy, expectedCurrentImplementation, expectedAdminOwner);

if (newImplementation.code.length == 0) revert NewImplementationHasNoCode(newImplementation);
if (newImplementation == expectedCurrentImplementation) {
revert NewImplementationMatchesCurrent(newImplementation);
}

// Lightweight smoke check against proxy to confirm implementation responds.
uint256 nextId = SavingCircles(proxy).nextId();

console2.log('Validation successful');
console2.log('Proxy', proxy);
console2.log('ProxyAdmin', proxyAdmin);
console2.log('AdminOwner', ProxyAdmin(proxyAdmin).owner());
console2.log('CurrentImplementation', expectedCurrentImplementation);
console2.log('NewImplementation', newImplementation);
console2.log('SmokeCheck.nextId');
console2.log(nextId);
}
}
Loading
Loading