From dcde0a32457064e891f7366b5230bdfed2b05df8 Mon Sep 17 00:00:00 2001 From: sirpy Date: Sun, 11 Oct 2026 16:32:08 +0300 Subject: [PATCH] fix(oft): audit C-01, H-01, H-02, H-03, M-01a, L-05 - C-01: approveFailedRequest clears the failed flag before _credit and is nonReentrant (G$ ERC777 mint hook re-entry) - H-03: approveFailedRequest reverts while the bridge is closed; owner-only rejectFailedRequest - M-01a: _credit returns the net amount credited (OFTReceived/OFTReceipt) - H-02: deploy proxies via ProxyFactory1967 with atomic initialize; verifyOftDeployment before wiring/minter grant - L-05: Avatar is the fee recipient at initialize - H-01: setDelegate(Avatar) before transferOwnership and assert owner + delegate - tests: reentrancy regression (mock + Celo fork with real SuperGoodDollar), H-03/M-01a regressions, fork deploy/handover --- .../contracts/oft/GoodDollarOFTAdapter.sol | 24 ++- .../contracts/test/ReentrancyPoC.sol | 13 ++ packages/bridge-contracts/deploy/deployOFT.ts | 163 ++++++++---------- .../deploy/utils/verifyOftDeployment.ts | 56 ++++++ .../scripts/oft/configure-oft.ts | 8 + .../scripts/oft/steps/adapter-ownership.ts | 27 ++- .../scripts/oft/steps/grant-minter-role.ts | 4 + .../test/GoodDollarOFT.e2e.test.ts | 70 ++++++++ .../test/GoodDollarOFT.reentrancy.poc.test.ts | 75 -------- .../test/GoodDollarOFT.reentrancy.test.ts | 108 ++++++++++++ .../test/GoodDollarOFT.test.ts | 100 +++++++++++ 11 files changed, 472 insertions(+), 176 deletions(-) create mode 100644 packages/bridge-contracts/deploy/utils/verifyOftDeployment.ts delete mode 100644 packages/bridge-contracts/test/GoodDollarOFT.reentrancy.poc.test.ts create mode 100644 packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts diff --git a/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol b/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol index f7dc926..7486656 100644 --- a/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol +++ b/packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol @@ -6,6 +6,7 @@ import "@layerzerolabs/oft-evm-upgradeable/contracts/oft/OFTCoreUpgradeable.sol" import { IMintableBurnable } from "@layerzerolabs/oft-evm/contracts/interfaces/IMintableBurnable.sol"; import { INameService } from "@gooddollar/goodprotocol/contracts/utils/DAOUpgradeableContract.sol"; import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol"; +import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; interface IIdentity { function isWhitelisted(address) external view returns (bool); @@ -16,7 +17,7 @@ interface IIdentity { * @notice Upgradeable OFT adapter that uses mint/burn mechanisms for cross-chain transfers * @dev Inherits from OFTCoreUpgradeable (which already includes OwnableUpgradeable) and implements mint/burn logic similar to MintBurnOFTAdapter */ -contract GoodDollarOFTAdapter is UUPSUpgradeable, OFTCoreUpgradeable { +contract GoodDollarOFTAdapter is UUPSUpgradeable, ReentrancyGuardUpgradeable, OFTCoreUpgradeable { using OFTMsgCodec for bytes; using OFTMsgCodec for bytes32; /// @dev Struct for storing bridge fees @@ -122,6 +123,9 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, OFTCoreUpgradeable { /// @dev Event emitted when a failed receive request is approved event FailedReceiveRequestApproved(bytes32 indexed guid); + /// @dev Event emitted when a failed receive request is rejected by the owner + event FailedReceiveRequestRejected(bytes32 indexed guid); + /** * @dev Constructor for the upgradeable implementation; token is used for decimals, init is disabled. * @param _token The underlying ERC20 token (used to get decimals for parent) @@ -154,6 +158,7 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, OFTCoreUpgradeable { require(IERC20Metadata(_token).decimals() == innerTokenDecimals, 'token decimals mismatch'); __UUPSUpgradeable_init(); + __ReentrancyGuard_init(); __Ownable_init(); __OAppSender_init(_owner); __OAppReceiver_init(_owner); @@ -251,15 +256,26 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, OFTCoreUpgradeable { /** * @notice Approves a failed receive request with optimistic window or by owner */ - function approveFailedRequest(bytes32 _guid) external { + function approveFailedRequest(bytes32 _guid) external nonReentrant { + require(!isClosed, 'closed'); FailedReceiveRequest memory request = failedReceiveRequests[_guid]; require(request.timestamp + OPTIMISTIC_WINDOW < block.timestamp || msg.sender == owner(), 'optimistic period not ended or not owner'); require(request.failed, 'request not failed'); - _credit(request.toAddress, request.amount, request.srcEid); + // G$ mint calls the recipient's ERC777 hook, so state must be final before _credit failedReceiveRequests[_guid].failed = false; + _credit(request.toAddress, request.amount, request.srcEid); emit FailedReceiveRequestApproved(_guid); } + /** + * @notice Rejects a failed receive request so it can never be approved (e.g. a forged or suspicious message) + */ + function rejectFailedRequest(bytes32 _guid) external onlyOwner { + require(failedReceiveRequests[_guid].failed, 'request not failed'); + failedReceiveRequests[_guid].failed = false; + emit FailedReceiveRequestRejected(_guid); + } + /** * @notice Bridge closed / whitelist check only (no limit checks). * @dev Revert on this path does not store to failedReceiveRequests. @@ -409,7 +425,7 @@ contract GoodDollarOFTAdapter is UUPSUpgradeable, OFTCoreUpgradeable { emit FeeCollected(feeRecipient, fee); } - return _amountLD; + return recipientAmount; } /** diff --git a/packages/bridge-contracts/contracts/test/ReentrancyPoC.sol b/packages/bridge-contracts/contracts/test/ReentrancyPoC.sol index 6ce0085..e7c9052 100644 --- a/packages/bridge-contracts/contracts/test/ReentrancyPoC.sol +++ b/packages/bridge-contracts/contracts/test/ReentrancyPoC.sol @@ -18,6 +18,10 @@ interface IFailedRequestApprover { function approveFailedRequest(bytes32 guid) external; } +interface IERC1820RegistryLike { + function setInterfaceImplementer(address account, bytes32 interfaceHash, address implementer) external; +} + /** * @dev Mimics SuperGoodDollar/SuperToken._mint: the recipient's ERC777 tokensReceived hook is invoked after * balances are updated (SuperToken resolves the implementer via ERC1820; here a contract recipient is its own implementer). @@ -46,6 +50,15 @@ contract FailedRequestReentrancyAttacker { constructor(IFailedRequestApprover _adapter) { adapter = _adapter; + // Registers as its own ERC777 recipient so the real SuperGoodDollar (fork tests) calls tokensReceived + address registry = 0x1820a4B7618BdE71Dce8cdc73aAB6C95905faD24; + if (registry.code.length > 0) { + IERC1820RegistryLike(registry).setInterfaceImplementer( + address(this), + keccak256('ERC777TokensRecipient'), + address(this) + ); + } } function attack(bytes32 _guid, uint256 _maxReentries) external { diff --git a/packages/bridge-contracts/deploy/deployOFT.ts b/packages/bridge-contracts/deploy/deployOFT.ts index a7c3889..c35a8bd 100644 --- a/packages/bridge-contracts/deploy/deployOFT.ts +++ b/packages/bridge-contracts/deploy/deployOFT.ts @@ -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( + 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); + + 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, + ]), + ); - // 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'); }; diff --git a/packages/bridge-contracts/deploy/utils/verifyOftDeployment.ts b/packages/bridge-contracts/deploy/utils/verifyOftDeployment.ts new file mode 100644 index 0000000..8dd0593 --- /dev/null +++ b/packages/bridge-contracts/deploy/utils/verifyOftDeployment.ts @@ -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 { + 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})`); +} diff --git a/packages/bridge-contracts/scripts/oft/configure-oft.ts b/packages/bridge-contracts/scripts/oft/configure-oft.ts index bfe8a79..1096b2a 100644 --- a/packages/bridge-contracts/scripts/oft/configure-oft.ts +++ b/packages/bridge-contracts/scripts/oft/configure-oft.ts @@ -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]); + if (!flags.skipWire) wireLayerZero(networkName); if (!flags.skipMinter) await grantMinterRole(); if (!flags.skipLimits) await setBridgeLimits(); diff --git a/packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts b/packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts index 86be3fb..34a9026 100644 --- a/packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts +++ b/packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts @@ -13,10 +13,22 @@ export const main = async () => { const avatarAddress = goodProtocolContracts.Avatar as string; const { GoodDollarOFTAdapter: oftAdapterAddress } = getOftDeploymentAddresses(networkName); const oftAdapter = await ethers.getContractAt("GoodDollarOFTAdapter", oftAdapterAddress); + const endpoint = await ethers.getContractAt( + ["function delegates(address) view returns (address)"], + await oftAdapter.endpoint() + ); + const isAvatar = (addr: string) => addr.toLowerCase() === avatarAddress.toLowerCase(); const currentOwner = await oftAdapter.owner(); + const currentDelegate = await endpoint.delegates(oftAdapterAddress); - if (currentOwner.toLowerCase() === avatarAddress.toLowerCase()) { - console.log("adapter ownership: already Avatar"); + if (isAvatar(currentOwner)) { + // The LZ delegate can rewrite DVN/executor config, so it must be the Avatar too + if (!isAvatar(currentDelegate)) { + throw new Error( + `Adapter owner is Avatar but LZ delegate is ${currentDelegate}; setDelegate(Avatar) must go through a DAO proposal` + ); + } + console.log("adapter ownership: already Avatar (owner + LZ delegate)"); return; } @@ -24,12 +36,19 @@ export const main = async () => { throw new Error(`Signer ${signer.address} is not owner ${currentOwner}`); } + if (!isAvatar(currentDelegate)) { + const delegateTx = await oftAdapter.setDelegate(avatarAddress); + await delegateTx.wait(); + console.log(`adapter LZ delegate: set to Avatar (${delegateTx.hash})`); + } + const tx = await oftAdapter.transferOwnership(avatarAddress); await tx.wait(); const newOwner = await oftAdapter.owner(); - if (newOwner.toLowerCase() !== avatarAddress.toLowerCase()) { - throw new Error(`Ownership transfer failed (tx ${tx.hash})`); + const newDelegate = await endpoint.delegates(oftAdapterAddress); + if (!isAvatar(newOwner) || !isAvatar(newDelegate)) { + throw new Error(`Ownership handover failed: owner ${newOwner}, delegate ${newDelegate} (tx ${tx.hash})`); } console.log(`adapter ownership: transferred to Avatar (${tx.hash})`); }; diff --git a/packages/bridge-contracts/scripts/oft/steps/grant-minter-role.ts b/packages/bridge-contracts/scripts/oft/steps/grant-minter-role.ts index ec4251b..992380a 100644 --- a/packages/bridge-contracts/scripts/oft/steps/grant-minter-role.ts +++ b/packages/bridge-contracts/scripts/oft/steps/grant-minter-role.ts @@ -1,6 +1,7 @@ import { network, ethers } from "hardhat"; import Contracts from "@gooddollar/goodprotocol/releases/deployment.json"; import { getOftDeploymentAddresses } from "../../../deploy/utils/getOftDeploymentAddresses"; +import { verifyOftDeployment } from "../../../deploy/utils/verifyOftDeployment"; export const main = async () => { const networkName = network.name; @@ -17,6 +18,9 @@ export const main = async () => { } const { GoodDollarOFTMinterBurner: minterBurnerAddress } = getOftDeploymentAddresses(networkName); + const [signer] = await ethers.getSigners(); + await verifyOftDeployment(networkName, [signer.address, avatarAddress]); + const token = await ethers.getContractAt( "contracts/oft/interfaces/ISuperGoodDollar.sol:ISuperGoodDollar", tokenAddress diff --git a/packages/bridge-contracts/test/GoodDollarOFT.e2e.test.ts b/packages/bridge-contracts/test/GoodDollarOFT.e2e.test.ts index 025eed0..59efa27 100644 --- a/packages/bridge-contracts/test/GoodDollarOFT.e2e.test.ts +++ b/packages/bridge-contracts/test/GoodDollarOFT.e2e.test.ts @@ -380,5 +380,75 @@ describe('GoodDollarOFTAdapter Fork Tests (Celo Production)', () => { expect(expectedFee).to.equal(amount); }); }); + + // Mirrors deploy/deployOFT.ts + scripts/oft/steps/adapter-ownership.ts against the real Celo ProxyFactory/endpoint + describe('Atomic deploy via ProxyFactory1967 and Avatar handover (H-01/H-02)', () => { + it('deploys both proxies initialized in one tx each and hands owner + LZ delegate to the Avatar', async function () { + const avatar = celoContracts.Avatar; + const proxyFactory = await ethers.getContractAt( + [ + 'function deployProxy(uint256 _salt, address _logic, bytes _data) returns (address)', + 'function getDeploymentAddress(uint256 _salt, address _sender) view returns (address)', + ], + celoContracts.ProxyFactory, + ); + const endpoint = await ethers.getContractAt( + ['function delegates(address) view returns (address)'], + CELO_LZ_ENDPOINT, + ); + + const minterBurnerImpl = await (await ethers.getContractFactory('GoodDollarOFTMinterBurner')).deploy(); + const AdapterFactory = await ethers.getContractFactory('GoodDollarOFTAdapter'); + const adapterImpl = await AdapterFactory.deploy(celoContracts.GoodDollar, CELO_LZ_ENDPOINT); + + const mbSalt = ethers.BigNumber.from(ethers.utils.id('Test-GoodDollarOFTMinterBurnerV1')); + const adapterSalt = ethers.BigNumber.from(ethers.utils.id('Test-GoodDollarOFTAdapterV1')); + const mbAddress = await proxyFactory.getDeploymentAddress(mbSalt, owner.address); + const adapterAddress = await proxyFactory.getDeploymentAddress(adapterSalt, owner.address); + // salt is bound to the deployer, so another sender gets a different address + expect(await proxyFactory.getDeploymentAddress(adapterSalt, user.address)).to.not.equal(adapterAddress); + + await proxyFactory.deployProxy( + mbSalt, + minterBurnerImpl.address, + minterBurnerImpl.interface.encodeFunctionData('initialize', [celoContracts.NameService, adapterAddress]), + ); + await proxyFactory.deployProxy( + adapterSalt, + adapterImpl.address, + AdapterFactory.interface.encodeFunctionData('initialize', [ + celoContracts.GoodDollar, + mbAddress, + owner.address, + avatar, + ]), + ); + + const implSlot = '0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc'; + const implOf = async (proxy: string) => + ethers.utils.getAddress(ethers.utils.hexDataSlice(await ethers.provider.getStorageAt(proxy, implSlot), 12)); + expect(await implOf(adapterAddress)).to.equal(adapterImpl.address); + expect(await implOf(mbAddress)).to.equal(minterBurnerImpl.address); + + const adapter = await ethers.getContractAt('GoodDollarOFTAdapter', adapterAddress); + const mb = await ethers.getContractAt('GoodDollarOFTMinterBurner', mbAddress); + expect(await adapter.owner()).to.equal(owner.address); + expect(await adapter.token()).to.equal(celoContracts.GoodDollar); + expect(await adapter.minterBurner()).to.equal(mbAddress); + expect(await adapter.feeRecipient()).to.equal(avatar); + expect(await mb.token()).to.equal(celoContracts.GoodDollar); + expect(await mb.operators(adapterAddress)).to.equal(true); + await expect( + adapter.initialize(celoContracts.GoodDollar, mbAddress, user.address, user.address), + ).to.be.revertedWith('Initializable: contract is already initialized'); + + // H-01: delegate must move with ownership, otherwise the deployer keeps control of DVN/executor config + expect(await endpoint.delegates(adapterAddress)).to.equal(owner.address); + await adapter.setDelegate(avatar); + await adapter.transferOwnership(avatar); + expect(await adapter.owner()).to.equal(avatar); + expect(await endpoint.delegates(adapterAddress)).to.equal(avatar); + }); + }); }); diff --git a/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.poc.test.ts b/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.poc.test.ts deleted file mode 100644 index 802adff..0000000 --- a/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.poc.test.ts +++ /dev/null @@ -1,75 +0,0 @@ -import { ethers, upgrades } from 'hardhat'; -import { expect } from 'chai'; -import { time } from '@nomicfoundation/hardhat-network-helpers'; - -// PoC: GoodDollarOFTAdapter.approveFailedRequest updates `failed` only after _credit(), and G$ (SuperGoodDollar) -// calls the recipient's ERC777 tokensReceived hook on mint, so the recipient can re-enter and mint repeatedly. -const DECIMAL_CONVERSION_RATE = ethers.BigNumber.from(10).pow(12); - -function encodeOFTMessage(toAddress: string, amountLD: ethers.BigNumber): string { - const toBytes32 = ethers.utils.hexZeroPad(toAddress, 32); - const amountHex = ethers.utils.hexZeroPad(amountLD.div(DECIMAL_CONVERSION_RATE).toHexString(), 8); - return toBytes32 + amountHex.slice(2); -} - -describe('PoC: approveFailedRequest reentrancy', () => { - it('mints a single failed request multiple times via tokensReceived re-entry', async () => { - const [owner, feeRecipient, avatar] = await ethers.getSigners(); - - const nameService = await (await ethers.getContractFactory('NameServiceMock')).deploy(); - const controller = await (await ethers.getContractFactory('ControllerMock')).deploy(avatar.address); - const token = await (await ethers.getContractFactory('MockGoodDollarWithMintHook')).deploy(); - await nameService.setAddress('CONTROLLER', controller.address); - await nameService.setAddress('GOODDOLLAR', token.address); - - const endpoint = await (await ethers.getContractFactory('LayerZeroEndpointMock')).deploy(); - const minterBurner = await (await ethers.getContractFactory('GoodDollarOFTMinterBurner')).deploy(); - const harness = await upgrades.deployProxy( - await ethers.getContractFactory('GoodDollarOFTAdapterHarness'), - [token.address, minterBurner.address, owner.address, feeRecipient.address], - { - kind: 'uups', - constructorArgs: [token.address, endpoint.address], - unsafeAllow: ['constructor', 'state-variable-immutable', 'duplicate-initializer-call', 'missing-initializer'], - }, - ); - await minterBurner.initialize(nameService.address, harness.address); - - // production limits from scripts/oft/oft.config.json - await harness.setBridgeLimits({ - dailyLimit: ethers.utils.parseEther('1000000'), - txLimit: ethers.utils.parseEther('100000'), - accountDailyLimit: ethers.utils.parseEther('50000'), - minAmount: ethers.utils.parseEther('10'), - onlyWhitelisted: false, - }); - - const attacker = await (await ethers.getContractFactory('FailedRequestReentrancyAttacker')).deploy(harness.address); - - // Inbound transfer above the destination accountDailyLimit is parked as a failed request - const amountLD = ethers.utils.parseEther('60000'); - const guid = ethers.utils.formatBytes32String('poc-guid'); - const origin = { srcEid: 1, sender: ethers.constants.HashZero, nonce: 1 }; - await harness.exposed_lzReceive( - origin, - guid, - encodeOFTMessage(attacker.address, amountLD), - ethers.constants.AddressZero, - '0x', - ); - expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); - - await time.increase(3 * 24 * 60 * 60 + 1); - - const reentries = 30; - await attacker.attack(guid, reentries); - - const received = await token.balanceOf(attacker.address); - console.log( - ` bridged: ${ethers.utils.formatEther(amountLD)} G$, minted to attacker: ${ethers.utils.formatEther( - received, - )} G$`, - ); - expect(received).to.equal(amountLD.mul(reentries + 1)); - }); -}); diff --git a/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts b/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts new file mode 100644 index 0000000..ffc14cf --- /dev/null +++ b/packages/bridge-contracts/test/GoodDollarOFT.reentrancy.test.ts @@ -0,0 +1,108 @@ +import { ethers, upgrades } from 'hardhat'; +import { expect } from 'chai'; +import * as networkHelpers from '@nomicfoundation/hardhat-network-helpers'; +import contracts from '@gooddollar/goodprotocol/releases/deployment.json'; + +// Regression for C-01: G$ (SuperGoodDollar) calls the recipient's ERC777 tokensReceived hook on mint, +// so approveFailedRequest must not be re-enterable from inside _credit. +const DECIMAL_CONVERSION_RATE = ethers.BigNumber.from(10).pow(12); +const OPTIMISTIC_WINDOW = 3 * 24 * 60 * 60; +const CELO_CHAIN_URL = 'https://forno.celo.org'; + +function encodeOFTMessage(toAddress: string, amountLD: ethers.BigNumber): string { + const toBytes32 = ethers.utils.hexZeroPad(toAddress, 32); + const amountHex = ethers.utils.hexZeroPad(amountLD.div(DECIMAL_CONVERSION_RATE).toHexString(), 8); + return toBytes32 + amountHex.slice(2); +} + +async function deployHarness(tokenAddress: string, nameServiceAddress: string, endpointAddress: string) { + const [owner, feeRecipient] = await ethers.getSigners(); + const minterBurner = await (await ethers.getContractFactory('GoodDollarOFTMinterBurner')).deploy(); + const harness = await upgrades.deployProxy( + await ethers.getContractFactory('GoodDollarOFTAdapterHarness'), + [tokenAddress, minterBurner.address, owner.address, feeRecipient.address], + { + kind: 'uups', + constructorArgs: [tokenAddress, endpointAddress], + unsafeAllow: ['constructor', 'state-variable-immutable', 'duplicate-initializer-call', 'missing-initializer'], + }, + ); + await minterBurner.initialize(nameServiceAddress, harness.address); + + // production limits from scripts/oft/oft.config.json + await harness.setBridgeLimits({ + dailyLimit: ethers.utils.parseEther('1000000'), + txLimit: ethers.utils.parseEther('100000'), + accountDailyLimit: ethers.utils.parseEther('50000'), + minAmount: ethers.utils.parseEther('10'), + onlyWhitelisted: false, + }); + return { harness, minterBurner }; +} + +// Parks a transfer above accountDailyLimit for a hook-enabled recipient, then tries to re-enter on approval +async function assertReentrancyBlocked(harness: any, token: any) { + const attacker = await (await ethers.getContractFactory('FailedRequestReentrancyAttacker')).deploy(harness.address); + + const amountLD = ethers.utils.parseEther('60000'); + const guid = ethers.utils.formatBytes32String('reentrancy-guid'); + const origin = { srcEid: 1, sender: ethers.constants.HashZero, nonce: 1 }; + await harness.exposed_lzReceive(origin, guid, encodeOFTMessage(attacker.address, amountLD), ethers.constants.AddressZero, '0x'); + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); + + await networkHelpers.time.increase(OPTIMISTIC_WINDOW + 1); + + await expect(attacker.attack(guid, 30)).to.be.revertedWith('ReentrancyGuard: reentrant call'); + expect(await token.balanceOf(attacker.address)).to.equal(0); + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(true); + + // the hook still runs on a legitimate approval, but the request is credited exactly once + await attacker.attack(guid, 0); + expect(await token.balanceOf(attacker.address)).to.equal(amountLD); + expect((await harness.failedReceiveRequests(guid)).failed).to.equal(false); + await expect(harness.approveFailedRequest(guid)).to.be.revertedWith('request not failed'); +} + +describe('GoodDollarOFTAdapter approveFailedRequest reentrancy (C-01)', () => { + it('blocks re-entry through a mint hook (mock token)', async () => { + const [, , avatar] = await ethers.getSigners(); + const nameService = await (await ethers.getContractFactory('NameServiceMock')).deploy(); + const controller = await (await ethers.getContractFactory('ControllerMock')).deploy(avatar.address); + const token = await (await ethers.getContractFactory('MockGoodDollarWithMintHook')).deploy(); + await nameService.setAddress('CONTROLLER', controller.address); + await nameService.setAddress('GOODDOLLAR', token.address); + const endpoint = await (await ethers.getContractFactory('LayerZeroEndpointMock')).deploy(); + + const { harness } = await deployHarness(token.address, nameService.address, endpoint.address); + await assertReentrancyBlocked(harness, token); + }); + + describe('Celo fork (real SuperGoodDollar + ERC1820)', () => { + const celo = (contracts as any)['production-celo']; + + before(async () => { + await networkHelpers.reset(CELO_CHAIN_URL); + }); + + after(async () => { + await networkHelpers.reset(); + }); + + it('blocks re-entry through the ERC777 tokensReceived hook', async () => { + const token = await ethers.getContractAt( + 'contracts/oft/interfaces/ISuperGoodDollar.sol:ISuperGoodDollar', + celo.GoodDollar, + ); + const endpoint = await (await ethers.getContractFactory('LayerZeroEndpointMock')).deploy(); + const { harness, minterBurner } = await deployHarness(celo.GoodDollar, celo.NameService, endpoint.address); + + await networkHelpers.impersonateAccount(celo.Avatar); + await networkHelpers.setBalance(celo.Avatar, ethers.utils.parseEther('10')); + await token.connect(await ethers.getSigner(celo.Avatar)).addMinter(minterBurner.address); + await networkHelpers.stopImpersonatingAccount(celo.Avatar); + + const erc20 = await ethers.getContractAt('@openzeppelin/contracts/token/ERC20/IERC20.sol:IERC20', celo.GoodDollar); + await assertReentrancyBlocked(harness, erc20); + }); + }); +}); diff --git a/packages/bridge-contracts/test/GoodDollarOFT.test.ts b/packages/bridge-contracts/test/GoodDollarOFT.test.ts index 44c2422..61a36d2 100644 --- a/packages/bridge-contracts/test/GoodDollarOFT.test.ts +++ b/packages/bridge-contracts/test/GoodDollarOFT.test.ts @@ -604,6 +604,94 @@ describe("OFT (unit, no fork)", () => { await expect(harness.approveFailedRequest(fakeGuid)).to.be.revertedWith("request not failed"); }); + + // H-03: a paused bridge must stop failed-request approvals, and the owner must be able to reject them + async function parkFailedRequest(harness: any, to: string, label: string) { + await harness.setBridgeLimits({ + dailyLimit: ethers.utils.parseEther("1000"), + txLimit: ethers.utils.parseEther("50"), + accountDailyLimit: ethers.utils.parseEther("1000"), + minAmount: ethers.utils.parseEther("1"), + onlyWhitelisted: false, + }); + const guid = ethers.utils.formatBytes32String(label); + const message = encodeOFTMessage(to, toSD(ethers.utils.parseEther("100"))); + await harness.exposed_lzReceive(origin(), guid, message, ethers.constants.AddressZero, "0x"); + expect((await harness.failedReceiveRequests(guid)).failed).to.be.true; + return guid; + } + + it("approveFailedRequest: reverts while the bridge is closed, even for the owner", async () => { + const { harness, user, token } = await loadFixture(harnessFixture); + const guid = await parkFailedRequest(harness, user.address, "guid-closed-approve"); + + await harness.pauseBridge(true); + await time.increase(3 * 24 * 60 * 60 + 1); + + await expect(harness.approveFailedRequest(guid)).to.be.revertedWith("closed"); + await expect(harness.connect(user).approveFailedRequest(guid)).to.be.revertedWith("closed"); + expect(await token.balanceOf(user.address)).to.equal(0); + + await harness.pauseBridge(false); + await harness.connect(user).approveFailedRequest(guid); + expect(await token.balanceOf(user.address)).to.equal(ethers.utils.parseEther("100")); + }); + + it("rejectFailedRequest: owner can reject, and a rejected request cannot be approved", async () => { + const { harness, user, token } = await loadFixture(harnessFixture); + const guid = await parkFailedRequest(harness, user.address, "guid-reject"); + + await expect(harness.rejectFailedRequest(guid)) + .to.emit(harness, "FailedReceiveRequestRejected") + .withArgs(guid); + expect((await harness.failedReceiveRequests(guid)).failed).to.be.false; + + await time.increase(3 * 24 * 60 * 60 + 1); + await expect(harness.connect(user).approveFailedRequest(guid)).to.be.revertedWith("request not failed"); + await expect(harness.approveFailedRequest(guid)).to.be.revertedWith("request not failed"); + expect(await token.balanceOf(user.address)).to.equal(0); + }); + + it("rejectFailedRequest: non-owner cannot reject", async () => { + const { harness, user } = await loadFixture(harnessFixture); + const guid = await parkFailedRequest(harness, user.address, "guid-reject-non-owner"); + + await expect(harness.connect(user).rejectFailedRequest(guid)).to.be.revertedWith("Ownable: caller is not the owner"); + expect((await harness.failedReceiveRequests(guid)).failed).to.be.true; + }); + + it("rejectFailedRequest: reverts for non-existent or already approved request", async () => { + const { harness, user } = await loadFixture(harnessFixture); + await expect(harness.rejectFailedRequest(ethers.utils.formatBytes32String("nonexistent"))).to.be.revertedWith( + "request not failed" + ); + + const guid = await parkFailedRequest(harness, user.address, "guid-reject-approved"); + await harness.approveFailedRequest(guid); + await expect(harness.rejectFailedRequest(guid)).to.be.revertedWith("request not failed"); + }); + + it("emits OFTReceived with the net amount after fees", async () => { + const { harness, user, token } = await loadFixture(harnessFixture); + await harness.setBridgeLimits({ + dailyLimit: ethers.utils.parseEther("1000"), + txLimit: ethers.utils.parseEther("1000"), + accountDailyLimit: ethers.utils.parseEther("1000"), + minAmount: ethers.utils.parseEther("1"), + onlyWhitelisted: false, + }); + await harness.setBridgeFees({ minFee: 0, maxFee: 0, fee: 100 }); + + const amountLD = ethers.utils.parseEther("100"); + const netAmount = amountLD.sub(amountLD.mul(100).div(10000)); + const guid = ethers.utils.formatBytes32String("guid-net-received"); + const message = encodeOFTMessage(user.address, toSD(amountLD)); + + await expect(harness.exposed_lzReceive(origin(), guid, message, ethers.constants.AddressZero, "0x")) + .to.emit(harness, "OFTReceived") + .withArgs(guid, 1, user.address, netAmount); + expect(await token.balanceOf(user.address)).to.equal(netAmount); + }); }); // ───────────────────────────────────────────────────────────────────────── @@ -687,6 +775,18 @@ describe("OFT (unit, no fork)", () => { expect(await token.balanceOf(feeRecipient.address)).to.equal(expectedFee); }); + it("returns the net amount credited to the recipient", async () => { + const { harness, user } = await loadFixture(feeHarnessFixture); + const amount = ethers.utils.parseEther("1000"); + + expect(await harness.callStatic.exposed_credit(user.address, amount, 1)).to.equal(amount); + + await harness.setBridgeFees({ minFee: 0, maxFee: 0, fee: 100 }); + expect(await harness.callStatic.exposed_credit(user.address, amount, 1)).to.equal( + amount.sub(amount.mul(100).div(10000)) + ); + }); + it("enforces minFee when calculated fee is below minimum", async () => { const { harness, user, token, feeRecipient } = await loadFixture(feeHarnessFixture);