Repository navigation
Conversation
- 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
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="20" />
<code_context>
* @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;
</code_context>
<issue_to_address>
**Legacy adapter storage shifts on upgrade**
When a proxy deployed with the pre-PR layout is upgraded through `upgradeOFT.ts`, `ReentrancyGuardUpgradeable` adds storage before `OFTCoreUpgradeable` and the adapter’s fields, so `upgradeOFT.ts` upgrades 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.
</issue_to_address>
### Comment 2
<location path="packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts" line_range="39-45" />
<code_context>
- 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`
</code_context>
<issue_to_address>
**Anyone can replace the LZ delegate**
When an untrusted account calls the inherited `setDelegate` before or after the ownership handover, the inherited `setDelegate` entry point is permissionless, so an attacker can set themselves as the endpoint delegate after this script sets the Avatar. They can then change the adapter’s LayerZero configuration despite the ownership handover, and can race the separate `transferOwnership` transaction so the script transfers ownership before detecting the wrong delegate.
Override `setDelegate` with an owner or governance authorization check, or otherwise prevent callers other than the intended governance from changing the endpoint delegate.
</issue_to_address>
### Comment 3
<location path="packages/bridge-contracts/deploy/deployOFT.ts" line_range="183" />
<code_context>
- } else {
- console.log('GoodDollarOFTAdapter already initialized');
- }
+ avatarAddress,
+ ]),
+ );
</code_context>
<issue_to_address>
**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.
</issue_to_address>
### Comment 4
<location path="packages/bridge-contracts/deploy/deployOFT.ts" line_range="149-150" />
<code_context>
+ 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) => {
</code_context>
<issue_to_address>
**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.
</issue_to_address>
### Comment 5
<location path="packages/bridge-contracts/scripts/oft/configure-oft.ts" line_range="75" />
<code_context>
+ 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);
</code_context>
<issue_to_address>
**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`.
</issue_to_address>
### Comment 6
<location path="packages/bridge-contracts/deploy/deployOFT.ts" line_range="142-145" />
<code_context>
+ );
+
+ 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`)),
+ );
</code_context>
<issue_to_address>
**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.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and this changes bridge authorization, LayerZero delegate control, proxy deployment and token-crediting behavior; an error could leave the bridge miswired, expose configuration control, or credit or strand tokens across deployments. Reverting the code would not undo deployed proxies, ownership or delegate transfers, or any cross-chain credits and access changes that already occurred.
Blocking findings: packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:20, packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts:45, packages/bridge-contracts/deploy/deployOFT.ts:183, packages/bridge-contracts/deploy/deployOFT.ts:150, packages/bridge-contracts/scripts/oft/configure-oft.ts:75
| * @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 { |
There was a problem hiding this comment.
🟠 High · Legacy adapter storage shifts on upgrade
When a proxy deployed with the pre-PR layout is upgraded through upgradeOFT.ts, ReentrancyGuardUpgradeable adds storage before OFTCoreUpgradeable and the adapter’s fields, so upgradeOFT.ts upgrades 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
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at line 20:
**Legacy adapter storage shifts on upgrade**
When a proxy deployed with the pre-PR layout is upgraded through `upgradeOFT.ts`, `ReentrancyGuardUpgradeable` adds storage before `OFTCoreUpgradeable` and the adapter’s fields, so `upgradeOFT.ts` upgrades 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.| 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); |
There was a problem hiding this comment.
🟠 High · Anyone can replace the LZ delegate
When an untrusted account calls the inherited setDelegate before or after the ownership handover, the inherited setDelegate entry point is permissionless, so an attacker can set themselves as the endpoint delegate after this script sets the Avatar. They can then change the adapter’s LayerZero configuration despite the ownership handover, and can race the separate transferOwnership transaction so the script transfers ownership before detecting the wrong delegate.
Override setDelegate with an owner or governance authorization check, or otherwise prevent callers other than the intended governance from changing the endpoint delegate.
Prompt for AI agents
In `packages/bridge-contracts/scripts/oft/steps/adapter-ownership.ts` at lines 39-45:
**Anyone can replace the LZ delegate**
When an untrusted account calls the inherited `setDelegate` before or after the ownership handover, the inherited `setDelegate` entry point is permissionless, so an attacker can set themselves as the endpoint delegate after this script sets the Avatar. They can then change the adapter’s LayerZero configuration despite the ownership handover, and can race the separate `transferOwnership` transaction so the script transfers ownership before detecting the wrong delegate.
Override `setDelegate` with an owner or governance authorization check, or otherwise prevent callers other than the intended governance from changing the endpoint delegate.| } else { | ||
| console.log('GoodDollarOFTAdapter already initialized'); | ||
| } | ||
| avatarAddress, |
There was a problem hiding this comment.
🟠 High · 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.
Prompt for AI agents
In `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.| const minterBurnerAddress: string = await proxyFactory.getDeploymentAddress(minterBurnerProxySalt, root.address); | ||
| const oftAdapterAddress: string = await proxyFactory.getDeploymentAddress(oftAdapterProxySalt, root.address); |
There was a problem hiding this comment.
🟠 High · 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.
Prompt for AI agents
In `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 [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.
🟠 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, 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.
Prompt for AI agents
In `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`.| const minterBurnerProxySalt = ethers.BigNumber.from( | ||
| ethers.utils.keccak256(ethers.utils.toUtf8Bytes(`${envPrefix}-GoodDollarOFTMinterBurnerV1`)), | ||
| ); | ||
| const oftAdapterProxySalt = ethers.BigNumber.from( |
There was a problem hiding this comment.
⚪ 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 agents
In `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.1 participant
Fixes the critical and high findings from the OFT security audit (
GoodDollarOFTAdapter,GoodDollarOFTMinterBurner, deploy/config scripts). These must be fixed before mainnet. The medium, low and informational findings come in a follow-up PR stacked on this branch.Contract changes (
GoodDollarOFTAdapter)approveFailedRequest. SuperGoodDollarmintcalls the recipient's ERC777tokensReceivedhook. A request could therefore be re-approved from inside_creditand minted N times. The fix clearsfailedbefore_creditand addsnonReentrant(viaReentrancyGuardUpgradeable).approveFailedRequestnow doesrequire(!isClosed). A new owner-onlyrejectFailedRequest(guid)(eventFailedReceiveRequestRejected) cancels forged or suspicious parked requests._creditreturn value._creditnow returnsrecipientAmount, soOFTReceivedand the receipt report the net amount after fees. Previously it returned the gross amount.Deploy / config scripts
H-02: deploy and initialize atomically.
deployOFT.tsnow uses GoodProtocolProxyFactory1967instead of Arachnid CREATE2 plus a separateinitializetx.getDeploymentAddress(salt, deployer). This breaks the MinterBurner ↔ adapter init cycle.deployProxy). The salt is bound to the deployer, so the proxies can't be front-run or hijacked.lz:oapp:wire, so this is fine.New
deploy/utils/verifyOftDeployment.ts. It checks:*_Implementationdeployments;owner()(deployer or Avatar),token(),minterBurner()andfeeRecipient() == Avatar;token()andoperators(adapter).It runs at the end of the deploy, at the start of
oft:configure(before wiring peers) and ingrant-minter-role(beforeaddMinter).L-05: fee recipient. The Avatar is the fee recipient at
initialize. Previously it was the deployer.H-01: LZ delegate handover.
adapter-ownership.tsnow callssetDelegate(Avatar)beforetransferOwnership(Avatar)and asserts that both owner andendpoint.delegates(adapter)are the Avatar.Tests
test/GoodDollarOFT.reentrancy.test.tsreplaces the PoC and keeps it as a regression test.ReentrancyGuard: reentrant call, and a normal approval mints exactly once.test/GoodDollarOFT.test.ts:_creditreturn value andOFTReceivednet amount with a 1% fee.test/GoodDollarOFT.e2e.test.ts: a Celo-fork test that mirrors the new deploy flow against the realProxyFactory1967and LZ EndpointV2.Full suite: 98 passing (34 pending, already skipped before this PR).
Needs manual verification
hardhat deploy --tags OFTondevelopment-celo/development-xdc. Confirm thatgoodprotocoldeployment.jsonhasProxyFactoryfor both networks: dev and prod Celo and XDC have it.transferOwnership(Avatar)is irreversible.adapter-ownership.tsnow asserts the Avatar address fromdeployment.json.Summary by Sourcery
Harden the OFT contracts and deployment flow against audited security findings before mainnet.
New Features:
Bug Fixes:
_creditandOFTReceived.Enhancements:
Deployment:
Tests: