Repository navigation
fix(oft): audit fixes C-01, H-01, H-02, H-03, M-01a, L-05 #14
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,29 @@ | ||
| /*** | ||
| * Hardhat-deploy script for GoodDollar OFT (Omnichain Fungible Token) contracts | ||
| * | ||
| * Deploys (same pattern as MessageBridge: deterministic proxy + implementation + execute initialize): | ||
| * Deploys: | ||
| * 1. GoodDollarOFTMinterBurner - DAO-upgradeable contract that handles minting and burning of GoodDollar tokens for OFT | ||
| * 2. GoodDollarOFTAdapter - Upgradeable LayerZero OFT adapter that wraps GoodDollar token for cross-chain transfers | ||
| * | ||
| * Steps: | ||
| * 1. Deploy ERC1967Proxy (deterministic) for GoodDollarOFTMinterBurner, deploy implementation | ||
| * then execute initialize(nameService, adapter) once the adapter proxy address is known | ||
| * 2. Deploy ERC1967Proxy (deterministic) for GoodDollarOFTAdapter, deploy implementation (constructor: token, lzEndpoint), execute initialize(token, minterBurner, owner, feeRecipient) | ||
| * 1. Deploy both implementations (deterministic, salt = bytecode hash) | ||
| * 2. Predict both proxy addresses via GoodProtocol ProxyFactory1967.getDeploymentAddress(salt, deployer) | ||
| * 3. Deploy + initialize each proxy atomically via ProxyFactory1967.deployProxy(salt, impl, initData) | ||
| * - MinterBurner.initialize(nameService, adapter) | ||
| * - Adapter.initialize(token, minterBurner, deployer, Avatar as feeRecipient) | ||
| * 4. verifyOftDeployment: impl slots, owner, token, minterBurner, feeRecipient, operators(adapter) | ||
| * | ||
| * Note: GoodDollarOFTMinterBurner.initialize wires the adapter as operator. | ||
| * Note: proxy addresses depend on the per-chain ProxyFactory address, so they differ between chains. | ||
| */ | ||
|
|
||
| import { DeployFunction } from 'hardhat-deploy/types'; | ||
| import { ethers } from 'hardhat'; | ||
| import { BigNumber } from 'ethers'; | ||
| import Contracts from '@gooddollar/goodprotocol/releases/deployment.json'; | ||
| import { getImplementationAddress } from '@openzeppelin/upgrades-core'; | ||
| import { verifyContract } from './utils/verifyContract'; | ||
| import { getLzEndpoint } from './utils/getLzEndpoint'; | ||
| import { verifyOftDeployment } from './utils/verifyOftDeployment'; | ||
|
|
||
| const func: DeployFunction = async function (hre) { | ||
| const { deployments, network } = hre; | ||
|
|
@@ -103,22 +108,6 @@ const func: DeployFunction = async function (hre) { | |
| const oftAdapterArtifact = await hre.artifacts.readArtifact('GoodDollarOFTAdapter'); | ||
| const oftAdapterImplSalt = ethers.utils.keccak256(oftAdapterArtifact.bytecode); | ||
|
|
||
| // --- GoodDollarOFTMinterBurner (hardhat-deploy: deterministic proxy + implementation + execute initialize) --- | ||
| const minterBurnerProxySalt = ethers.utils.keccak256( | ||
| ethers.utils.toUtf8Bytes( | ||
| isDevelopment ? 'Development-GoodDollarOFTMinterBurnerV1' : 'Production-GoodDollarOFTMinterBurnerV1' | ||
| ), | ||
| ); | ||
| const minterBurnerProxyDeploy = await deployments.deterministic('GoodDollarOFTMinterBurner', { | ||
| contract: 'ERC1967Proxy', | ||
| from: root.address, | ||
| salt: minterBurnerProxySalt, | ||
| log: true, | ||
| }); | ||
| const minterBurnerProxy = await minterBurnerProxyDeploy.deploy(); | ||
| const minterBurnerAddress = minterBurnerProxy.address; | ||
| console.log('GoodDollarOFTMinterBurner proxy', minterBurnerAddress); | ||
|
|
||
| const minterBurnerImpl = await deployments.deploy('GoodDollarOFTMinterBurner_Implementation', { | ||
| contract: 'GoodDollarOFTMinterBurner', | ||
| from: root.address, | ||
|
|
@@ -127,54 +116,6 @@ const func: DeployFunction = async function (hre) { | |
| }); | ||
| console.log('GoodDollarOFTMinterBurner implementation', minterBurnerImpl.address); | ||
|
|
||
| const minterBurnerContract = await ethers.getContractAt('GoodDollarOFTMinterBurner', minterBurnerAddress); | ||
| const minterBurnerInitialized = await minterBurnerContract | ||
| .token() | ||
| .then((addr: string) => addr !== ethers.constants.AddressZero) | ||
| .catch(() => false); | ||
|
|
||
| if (!minterBurnerInitialized) { | ||
| console.log('GoodDollarOFTMinterBurner not initialized yet; will initialize after deploying adapter...'); | ||
| } else { | ||
| console.log('GoodDollarOFTMinterBurner already initialized'); | ||
| } | ||
|
|
||
| // Verify GoodDollarOFTMinterBurner implementation (no constructor args) on non-local networks (skip: 'hardhat', 'localhost') | ||
| if (!['hardhat', 'localhost'].includes(networkName)) { | ||
| await verifyContract(hre as any, minterBurnerImpl.address, [], 'GoodDollarOFTMinterBurner'); | ||
| } | ||
|
|
||
| // --- GoodDollarOFTAdapter (hardhat-deploy: deterministic proxy + implementation with constructor + execute initialize) --- | ||
| const oftAdapterProxySalt = ethers.utils.keccak256( | ||
| ethers.utils.toUtf8Bytes(isDevelopment ? 'Development-GoodDollarOFTAdapterV1' : 'Production-GoodDollarOFTAdapterV1'), | ||
| ); | ||
| const oftAdapterProxyDeploy = await deployments.deterministic('GoodDollarOFTAdapter', { | ||
| contract: 'ERC1967Proxy', | ||
| from: root.address, | ||
| salt: oftAdapterProxySalt, | ||
| log: true, | ||
| }); | ||
| const oftAdapterProxy = await oftAdapterProxyDeploy.deploy(); | ||
| const oftAdapterAddress = oftAdapterProxy.address; | ||
| console.log('GoodDollarOFTAdapter proxy', oftAdapterAddress); | ||
|
|
||
| // Initialize minter/burner after we know the adapter address | ||
| if (!minterBurnerInitialized) { | ||
| console.log('Initializing GoodDollarOFTMinterBurner with adapter address...'); | ||
| const minterBurnerInitData = minterBurnerContract.interface.encodeFunctionData('initialize', [ | ||
| nameServiceAddress, | ||
| oftAdapterAddress, | ||
| ]); | ||
| await deployments.execute( | ||
| 'GoodDollarOFTMinterBurner', | ||
| { from: root.address }, | ||
| 'initialize', | ||
| minterBurnerImpl.address, | ||
| minterBurnerInitData, | ||
| ); | ||
| console.log('GoodDollarOFTMinterBurner initialized'); | ||
| } | ||
|
|
||
| const oftAdapterImpl = await deployments.deploy('GoodDollarOFTAdapter_Implementation', { | ||
| contract: 'GoodDollarOFTAdapter', | ||
| from: root.address, | ||
|
|
@@ -184,35 +125,70 @@ const func: DeployFunction = async function (hre) { | |
| }); | ||
| console.log('GoodDollarOFTAdapter implementation', oftAdapterImpl.address); | ||
|
|
||
| const oftAdapterContract = await ethers.getContractAt('GoodDollarOFTAdapter', oftAdapterAddress); | ||
| const oftAdapterInitialized = await oftAdapterContract | ||
| .minterBurner() | ||
| .then((addr: string) => addr !== ethers.constants.AddressZero) | ||
| .catch(() => false); | ||
| // ProxyFactory1967 deploys and initializes in one tx, with a CREATE2 salt bound to the deployer (no init front-running) | ||
| const proxyFactoryAddress = goodProtocolContracts.ProxyFactory; | ||
| if (!proxyFactoryAddress) { | ||
| throw new Error(`ProxyFactory address not found in GoodProtocol deployment for network ${networkName}`); | ||
| } | ||
| const proxyFactory = await ethers.getContractAt( | ||
| [ | ||
| 'function deployProxy(uint256 _salt, address _logic, bytes _data) returns (address)', | ||
| 'function getDeploymentAddress(uint256 _salt, address _sender) view returns (address)', | ||
| ], | ||
| proxyFactoryAddress, | ||
| ); | ||
|
|
||
| const envPrefix = isDevelopment ? 'Development' : 'Production'; | ||
| const minterBurnerProxySalt = ethers.BigNumber.from( | ||
| ethers.utils.keccak256(ethers.utils.toUtf8Bytes(`${envPrefix}-GoodDollarOFTMinterBurnerV1`)), | ||
| ); | ||
| const oftAdapterProxySalt = ethers.BigNumber.from( | ||
|
Comment on lines
+142
to
+145
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ Low · Proxy addresses misdescribed When operators use the configuring guide to predict or compare proxy addresses across chains, readers of the configuring guide are told that the proxy salt gives a fixed address per environment, but these salts are passed to each chain’s ProxyFactory and the factory address affects the resulting proxy address. Operators therefore expect the Celo and XDC proxies to share an address when they do not. Update the guide to state that proxy addresses depend on the per-chain ProxyFactory and deployer, and differ between chains. Prompt for AI agentsIn `packages/bridge-contracts/deploy/deployOFT.ts` at lines 142-145:
**Proxy addresses misdescribed**
When operators use the configuring guide to predict or compare proxy addresses across chains, readers of the configuring guide are told that the proxy salt gives a fixed address per environment, but these salts are passed to each chain’s ProxyFactory and the factory address affects the resulting proxy address. Operators therefore expect the Celo and XDC proxies to share an address when they do not.
Update the guide to state that proxy addresses depend on the per-chain ProxyFactory and deployer, and differ between chains. |
||
| ethers.utils.keccak256(ethers.utils.toUtf8Bytes(`${envPrefix}-GoodDollarOFTAdapterV1`)), | ||
| ); | ||
| // Both proxy addresses are known before deployment, which breaks the MinterBurner <-> adapter init cycle | ||
| const minterBurnerAddress: string = await proxyFactory.getDeploymentAddress(minterBurnerProxySalt, root.address); | ||
| const oftAdapterAddress: string = await proxyFactory.getDeploymentAddress(oftAdapterProxySalt, root.address); | ||
|
Comment on lines
+149
to
+150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High · Reruns create a second proxy pair When a different signer resumes or reruns deployment after the original proxies have been deployed, Reuse existing deployment records on reruns, and do not replace them with proxies predicted from a different signer. Prompt for AI agentsIn `packages/bridge-contracts/deploy/deployOFT.ts` at lines 149-150:
**Reruns create a second proxy pair**
When a different signer resumes or reruns deployment after the original proxies have been deployed, `getDeploymentAddress` predicts addresses from the new `root.address`, so `deployProxyIfMissing` deploys a second pair and `deployments.save` replaces the local records without disabling the original pair. Later scripts target the new pair while the original can remain authorized or wired.
Reuse existing deployment records on reruns, and do not replace them with proxies predicted from a different signer. |
||
|
|
||
| const deployProxyIfMissing = async (name: string, address: string, salt: BigNumber, logic: string, initData: string) => { | ||
| if ((await ethers.provider.getCode(address)) !== '0x') { | ||
| console.log(`${name} proxy already deployed`, address); | ||
| } else { | ||
| const tx = await proxyFactory.deployProxy(salt, logic, initData); | ||
| await tx.wait(); | ||
| console.log(`${name} proxy deployed and initialized`, address, tx.hash); | ||
| } | ||
| await deployments.save(name, { address, abi: (await hre.artifacts.readArtifact(name)).abi }); | ||
| }; | ||
|
|
||
| const minterBurnerInterface = (await ethers.getContractFactory('GoodDollarOFTMinterBurner')).interface; | ||
| await deployProxyIfMissing( | ||
| 'GoodDollarOFTMinterBurner', | ||
| minterBurnerAddress, | ||
| minterBurnerProxySalt, | ||
| minterBurnerImpl.address, | ||
| minterBurnerInterface.encodeFunctionData('initialize', [nameServiceAddress, oftAdapterAddress]), | ||
| ); | ||
|
|
||
| if (!oftAdapterInitialized) { | ||
| console.log('Initializing GoodDollarOFTAdapter...'); | ||
| const oftAdapterInitData = oftAdapterContract.interface.encodeFunctionData('initialize', [ | ||
| // Deployer owns the adapter until oft:configure hands owner + LZ delegate to the Avatar; fees go to the Avatar | ||
| const oftAdapterInterface = (await ethers.getContractFactory('GoodDollarOFTAdapter')).interface; | ||
| await deployProxyIfMissing( | ||
| 'GoodDollarOFTAdapter', | ||
| oftAdapterAddress, | ||
| oftAdapterProxySalt, | ||
| oftAdapterImpl.address, | ||
| oftAdapterInterface.encodeFunctionData('initialize', [ | ||
| tokenAddress, | ||
| minterBurnerAddress, | ||
| root.address, | ||
| root.address, | ||
| ]); | ||
| await deployments.execute( | ||
| 'GoodDollarOFTAdapter', | ||
| { from: root.address }, | ||
| 'initialize', | ||
| oftAdapterImpl.address, | ||
| oftAdapterInitData, | ||
| ); | ||
| console.log('GoodDollarOFTAdapter initialized'); | ||
| console.log('Fee recipient:', avatarAddress); | ||
| } else { | ||
| console.log('GoodDollarOFTAdapter already initialized'); | ||
| } | ||
| avatarAddress, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High · Fee-bearing receives revert When the Avatar has no ERC777 recipient implementer registered and bridge fees are positive, Register the Avatar as an ERC777 recipient or route fees to a recipient that can receive ERC777 tokens. Prompt for AI agentsIn `packages/bridge-contracts/deploy/deployOFT.ts` at line 183:
**Fee-bearing receives revert**
When the Avatar has no ERC777 recipient implementer registered and bridge fees are positive, `_credit` sends the fee to the Avatar through `minterBurner.mint`; SuperGoodDollar rejects the mint, so the entire receive reverts and incoming transfers remain undelivered.
Register the Avatar as an ERC777 recipient or route fees to a recipient that can receive ERC777 tokens. |
||
| ]), | ||
| ); | ||
|
|
||
| // Verify GoodDollarOFTAdapter implementation (constructor: tokenAddress, lzEndpoint) on non-local networks (skip: 'hardhat', 'localhost') | ||
| await verifyOftDeployment(networkName, [root.address, avatarAddress]); | ||
|
|
||
| // Verify implementations on non-local networks (skip: 'hardhat', 'localhost') | ||
| if (!['hardhat', 'localhost'].includes(networkName)) { | ||
| await verifyContract(hre as any, minterBurnerImpl.address, [], 'GoodDollarOFTMinterBurner'); | ||
| await verifyContract(hre as any, oftAdapterImpl.address, [tokenAddress, lzEndpoint], 'GoodDollarOFTAdapter'); | ||
| } | ||
|
|
||
|
|
@@ -234,7 +210,8 @@ const func: DeployFunction = async function (hre) { | |
| console.log(' Implementation:', oftAdapterImplAddress); | ||
| } | ||
| console.log('Token:', tokenAddress); | ||
| console.log('OFT Adapter Owner (Avatar):', avatarAddress); | ||
| console.log('OFT Adapter Owner (deployer, until oft:configure):', root.address); | ||
| console.log('Fee recipient (Avatar):', avatarAddress); | ||
| console.log('LayerZero Endpoint:', lzEndpoint); | ||
| console.log('========================\n'); | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import { deployments, ethers } from 'hardhat'; | ||
| import Contracts from '@gooddollar/goodprotocol/releases/deployment.json'; | ||
|
|
||
| const EIP1967_IMPLEMENTATION_SLOT = '0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc'; | ||
|
|
||
| async function readProxyImplementation(proxy: string): Promise<string> { | ||
| const raw = await ethers.provider.getStorageAt(proxy, EIP1967_IMPLEMENTATION_SLOT); | ||
| return ethers.utils.getAddress(ethers.utils.hexDataSlice(raw, 12)); | ||
| } | ||
|
|
||
| function expectEqual(label: string, actual: string, expected: string) { | ||
| if (actual.toLowerCase() !== expected.toLowerCase()) { | ||
| throw new Error(`OFT deployment check failed: ${label} is ${actual}, expected ${expected}`); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Verifies the OFT proxies point at our implementations and were initialized with the expected wiring. | ||
| * Must pass before granting MINTER_ROLE or setting LZ peers. | ||
| * @param allowedOwners adapter owners accepted at this stage (deployer before handover, Avatar after) | ||
| */ | ||
| export async function verifyOftDeployment(networkName: string, allowedOwners: string[]) { | ||
| const gp = Contracts[networkName as keyof typeof Contracts] as any; | ||
| if (!gp?.GoodDollar || !gp?.Avatar) throw new Error(`Missing GoodDollar/Avatar for ${networkName}`); | ||
|
|
||
| const minterBurnerAddress = (await deployments.get('GoodDollarOFTMinterBurner')).address; | ||
| const adapterAddress = (await deployments.get('GoodDollarOFTAdapter')).address; | ||
|
|
||
| expectEqual( | ||
| 'GoodDollarOFTAdapter implementation', | ||
| await readProxyImplementation(adapterAddress), | ||
| (await deployments.get('GoodDollarOFTAdapter_Implementation')).address, | ||
| ); | ||
| expectEqual( | ||
| 'GoodDollarOFTMinterBurner implementation', | ||
| await readProxyImplementation(minterBurnerAddress), | ||
| (await deployments.get('GoodDollarOFTMinterBurner_Implementation')).address, | ||
| ); | ||
|
|
||
| const adapter = await ethers.getContractAt('GoodDollarOFTAdapter', adapterAddress); | ||
| const owner: string = await adapter.owner(); | ||
| if (!allowedOwners.some((o) => o.toLowerCase() === owner.toLowerCase())) { | ||
| throw new Error(`OFT deployment check failed: adapter owner ${owner} not in [${allowedOwners.join(', ')}]`); | ||
| } | ||
| expectEqual('adapter.token()', await adapter.token(), gp.GoodDollar); | ||
| expectEqual('adapter.minterBurner()', await adapter.minterBurner(), minterBurnerAddress); | ||
| expectEqual('adapter.feeRecipient()', await adapter.feeRecipient(), gp.Avatar); | ||
|
|
||
| const minterBurner = await ethers.getContractAt('GoodDollarOFTMinterBurner', minterBurnerAddress); | ||
| expectEqual('minterBurner.token()', await minterBurner.token(), gp.GoodDollar); | ||
| if (!(await minterBurner.operators(adapterAddress))) { | ||
| throw new Error('OFT deployment check failed: adapter is not a MinterBurner operator'); | ||
| } | ||
|
|
||
| console.log(`OFT deployment verified on ${networkName} (adapter owner ${owner})`); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import { spawnSync } from "child_process"; | |
| import path from "path"; | ||
| import { task } from "hardhat/config"; | ||
| import type { HardhatRuntimeEnvironment } from "hardhat/types"; | ||
| import Contracts from "@gooddollar/goodprotocol/releases/deployment.json"; | ||
|
|
||
| export type ConfigureFlags = { | ||
| skipMinter: boolean; | ||
|
|
@@ -62,10 +63,17 @@ export async function configureOft(hre: HardhatRuntimeEnvironment, flags: Config | |
| const { main: grantMinterRole } = await import("./steps/grant-minter-role"); | ||
| const { main: setBridgeLimits } = await import("./steps/set-bridge-limits"); | ||
| const { main: transferAdapterOwnership } = await import("./steps/adapter-ownership"); | ||
| const { verifyOftDeployment } = await import("../../deploy/utils/verifyOftDeployment"); | ||
|
|
||
| const networkName = hre.network.name; | ||
| console.log(`oft:configure ${networkName}`, flags); | ||
|
|
||
| // Never wire peers or grant MINTER_ROLE to proxies we can't prove are ours | ||
| const [signer] = await hre.ethers.getSigners(); | ||
| const avatar = (Contracts[networkName as keyof typeof Contracts] as any)?.Avatar; | ||
| if (!avatar) throw new Error(`Avatar not found for ${networkName}`); | ||
| await verifyOftDeployment(networkName, [signer.address, avatar]); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟠 High · Configuration mutates before delegate check When the adapter is already Avatar-owned, its endpoint delegate is not the Avatar, and configuration runs with minter or limit steps enabled, Check the endpoint delegate during the initial preflight and abort before any configuration mutations when it is inconsistent. Also at Prompt for AI agentsIn `packages/bridge-contracts/scripts/oft/configure-oft.ts` at line 75:
**Configuration mutates before delegate check**
When the adapter is already Avatar-owned, its endpoint delegate is not the Avatar, and configuration runs with minter or limit steps enabled, `verifyOftDeployment` accepts the Avatar as owner without checking the endpoint delegate, so `configureOft` can grant the minter role and set limits before `adapter-ownership` detects the bad delegate and throws. The command fails with the bridge partly configured and the delegate mismatch unresolved.
Check the endpoint delegate during the initial preflight and abort before any configuration mutations when it is inconsistent.
Also at `packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts:26`. |
||
|
|
||
| if (!flags.skipWire) wireLayerZero(networkName); | ||
| if (!flags.skipMinter) await grantMinterRole(); | ||
| if (!flags.skipLimits) await setBridgeLimits(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 High · Legacy adapter storage shifts on upgrade
When a proxy deployed with the pre-PR layout is upgraded through
upgradeOFT.ts,ReentrancyGuardUpgradeableadds storage beforeOFTCoreUpgradeableand the adapter’s fields, soupgradeOFT.tsupgrades a pre-PR proxy with a shifted layout; its token wiring, limits, and failed-request records are then read from the wrong slots, breaking the bridge and making the old state inaccessible.Keep the old storage layout for in-place upgrades or use a fresh proxy and explicitly migrate state instead of upgrading a pre-PR proxy to this implementation.
Prompt for AI agents