Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

infra Checker does not check expectedConfig.proxyAdmin.owner #5509

Open
ltyu opened this issue Feb 18, 2025 · 0 comments
Open

infra Checker does not check expectedConfig.proxyAdmin.owner #5509

ltyu opened this issue Feb 18, 2025 · 0 comments

Comments

@ltyu
Copy link
Contributor

ltyu commented Feb 18, 2025

Problem

Currently, in some of the infra Getters, most notably Renzo's we have a config that specifies the existingProxyAdmins:

const existingProxyAdmins: ChainMap<{ address: string; owner: string }> = {
ethereum: {
address: '0x4f4671Ce69c9af15e33eB7Cf6D1358d1B39Af3bF',
owner: '0xD1e6626310fD54Eceb5b9a51dA2eC329D6D4B68A',
},
zircuit: {
address: '0x8b789B4A56675240c9f0985B467752b870c75711',
owner: '0x8410927C286A38883BC23721e640F31D3E3E79F8',
},
};
export const getRenzoPZETHWarpConfig = getRenzoWarpConfigGenerator({
chainsToDeploy: pzEthChainsToDeploy,
validators: pzEthValidators,
safes: pzEthSafes,
xERC20Addresses: pzEthAddresses,
xERC20Lockbox: pzEthProductionLockbox,
tokenPrices: pzEthTokenPrices,
existingProxyAdmins: existingProxyAdmins,

When we run yarn tsx ./scripts/check/check-deploy.ts, the Checker will detect if expectedConfig.proxyAdmin.address has been set, and compare the expected config with onchain address.

Otherwise, it will read the proxyAdmin.owner from onchain and compare to the top-level expectedConfig.owner or ownerOverrides.proxyAdmin.owner.

In short, infra does not validate expectedConfig.proxyAdmin.owner

if (expectedProxyAdminAddress) {
// config defines an expected ProxyAdmin address, we therefore check if the actual ProxyAdmin address matches the expected one
if (
!eqAddress(actualProxyAdminAddress, expectedProxyAdminAddress)
) {
this.addViolation({
type: ViolationType.ProxyAdmin,
chain,
name,
expected: expectedProxyAdminAddress,
actual: actualProxyAdminAddress,
proxyAddress: contract.address,
} as ProxyAdminViolation);
}
} else {
// config does not define an expected ProxyAdmin address, this means that checkOwnership will not be able to check the ownership of the ProxyAdmin contract
// as it is not explicitly defined in the config. We therefore check the ownership of the ProxyAdmin contract here.
const actualProxyAdminContract = ProxyAdmin__factory.connect(
actualProxyAdminAddress,
provider,
);
const actualProxyAdminOwner =
await actualProxyAdminContract.owner();
const expectedOwner = this.getOwner(
owner,
'proxyAdmin',
ownableOverrides,
);
if (!eqAddress(actualProxyAdminOwner, expectedOwner)) {
const violation: OwnerViolation = {
chain,
name: 'proxyAdmin',
type: ViolationType.Owner,
actual: actualProxyAdminOwner,
expected: expectedOwner,
contract: actualProxyAdminContract,
};
this.addViolation(violation);
}
}

Solution

  • Consider adding the check into Infra to also validate expectedConfig.proxyAdmin.owner

Nice to have

  • Since we have many such cases of proxyAdmin in the config (config.proxyAdmin and config.ownerOverrides.proxyAdmin) that are used in the infra or CLI exclusively, figure out how to unify these.
@ltyu ltyu changed the title fix: infra Checker does not check expectedConfig.proxyAdmin.owner infra Checker does not check expectedConfig.proxyAdmin.owner Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

No branches or pull requests

1 participant