Repository navigation
Conversation
- M-01: cap fee at MAX_FEE_BPS, require minFee <= maxFee, clamp fee to amount - M-02: revert composed messages on limit failure instead of parking - M-03: separate inbound/outbound daily counters - M-05: require LayerZero Labs + Nethermind DVNs; add eids to production networks - L-01/L-02: adapter is the spender (approvalRequired true), MinterBurner burns only caller tokens with balance-delta check - L-03: onlyOperators checks the live Controller avatar - L-04: disable initializers on the MinterBurner implementation - L-06: fail closed when whitelist identity is unresolvable; emit NameServiceSet - L-07: validate setBridgeLimits - L-08: enforce limits on the dust-free amount - L-09: quoteOFT returns real min/max limits - upgradeOFT routes Avatar-owned upgrades through the DAO - add oft:verify post-deploy invariant task - remove dead generateGuid
![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 11 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="100-103" />
<code_context>
INameService public nameService;
+ /// @dev Inbound (receive) bridge daily limit tracking, separate so sends can't exhaust the receive budget
+ BridgeDailyLimit public bridgeReceiveDailyLimit;
+
+ /// @dev Inbound (receive) account daily limit tracking
+ mapping(address => AccountLimit) public accountsReceiveDailyLimit;
+
/// @dev Error for bridge limits violations
</code_context>
<issue_to_address>
**Inbound limits reset on upgrade**
When the adapter is upgraded while the previous shared counters contain receive usage from the current 24-hour window, the new receive counters start at zero and do not include that usage, so inbound checks grant another daily allowance and accept receives beyond the configured cap.
Preserve the prior inbound usage when introducing the receive counters so the upgrade does not grant a fresh allowance.
</issue_to_address>
### Comment 2
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="100-103" />
<code_context>
INameService public nameService;
+ /// @dev Inbound (receive) bridge daily limit tracking, separate so sends can't exhaust the receive budget
+ BridgeDailyLimit public bridgeReceiveDailyLimit;
+
+ /// @dev Inbound (receive) account daily limit tracking
+ mapping(address => AccountLimit) public accountsReceiveDailyLimit;
+
/// @dev Error for bridge limits violations
</code_context>
<issue_to_address>
**Upgrade validation rejects storage gap**
When the new implementation is checked against the deployed layout using OpenZeppelin upgrade validation, the two-slot `bridgeReceiveDailyLimit` and one-slot mapping are inserted before the unchanged 50-slot `__gap`, so OpenZeppelin’s upgrade validation sees the old gap displaced instead of reduced and rejects the implementation upgrade.
Reduce the adapter storage gap by the three slots consumed by the appended counters and validate the resulting layout before upgrading.
</issue_to_address>
### Comment 3
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol" line_range="70" />
<code_context>
* @return success True if the burn was successful
*/
function burn(address _from, uint256 _amount) external onlyOperators returns (bool) {
+ require(_from == msg.sender, 'can only burn own tokens');
+ uint256 balanceBefore = token.balanceOf(address(this));
require(token.transferFrom(_from, address(this), _amount), 'TRANSFER_FROM_FAILED');
</code_context>
<issue_to_address>
**MinterBurner upgrade breaks sends**
When the MinterBurner upgrade executes before the adapter upgrade, including when the DAO executes its proposal first, the old adapter calls `burn(user, amount)`, but the new `GoodDollarOFTMinterBurner.burn` requires `_from == msg.sender`; that call reverts and outbound sends fail until the adapter is upgraded.
Keep the new `burn` compatible with the old adapter, or ensure the two upgrades execute atomically so the old adapter cannot call the new implementation.
Also at `packages/bridge-contracts/deploy/upgradeOFT.ts:109`, `packages/bridge-contracts/deploy/upgradeOFT.ts:159`.
</issue_to_address>
### Comment 4
<location path="packages/bridge-contracts/scripts/oft/verify-invariants.ts" line_range="96-98" />
<code_context>
+ const raw = await endpoint.getConfig(adapterAddress, lib, remoteEid, ULN_CONFIG_TYPE);
+ const [uln] = ethers.utils.defaultAbiCoder.decode(ULN_CONFIG_ABI, raw);
+ check(
+ uln.requiredDVNCount >= MIN_REQUIRED_DVNS && uln.requiredDVNCount !== 255,
+ `${side} ULN requiredDVNCount ${uln.requiredDVNCount} >= ${MIN_REQUIRED_DVNS} (${uln.requiredDVNs.join(", ")})`
+ );
+ }
+
</code_context>
<issue_to_address>
**Untrusted DVNs pass verification**
When a send or receive ULN configuration has at least two required DVNs that are not the intended LayerZero Labs and Nethermind pair, `verifyOftInvariants` checks only `requiredDVNCount`, so configurations using unintended or untrusted providers pass and operators see verification succeed for the wrong security configuration.
Have `verifyOftInvariants` verify the required DVN identities as well as their count.
</issue_to_address>
### Comment 5
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="411-420" />
<code_context>
+ }
+ oftLimit = OFTLimit(bridgeLimits.minAmount, maxAmount);
+
+ // TODO: destination-chain bridge fee quoting is an open decision; fees are left out of the quote for now
+ oftFeeDetails = new OFTFeeDetail[](0);
+
+ (uint256 amountSentLD, uint256 amountReceivedLD) = _debitView(
+ _sendParam.amountLD,
+ _sendParam.minAmountLD,
+ _sendParam.dstEid
+ );
+ oftReceipt = OFTReceipt(amountSentLD, amountReceivedLD);
+ }
/**
</code_context>
<issue_to_address>
**Quotes overstate received tokens**
When a nonzero destination-chain bridge fee is configured and a caller uses `quoteOFT` to estimate destination proceeds, `quoteOFT` reports `_debitView`’s gross amount as `amountReceivedLD` and returns no fee details, while `_credit` deducts the destination fee; callers therefore display more tokens than the recipient receives and cannot derive accurate slippage.
Include the destination fee in the quote’s receipt amount and return the corresponding fee details.
</issue_to_address>
### Comment 6
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="395" />
<code_context>
+ returns (OFTLimit memory oftLimit, OFTFeeDetail[] memory oftFeeDetails, OFTReceipt memory oftReceipt)
+ {
+ uint256 maxAmount;
+ if (!isClosed) {
+ BridgeLimits memory limits = bridgeLimits;
+ uint256 dailyUsed = bridgeDailyLimit.lastTransferReset < block.timestamp - 1 days
</code_context>
<issue_to_address>
**Ineligible senders get positive quotes**
When whitelist mode is enabled and the caller is not eligible, including when Identity cannot be resolved, `quoteOFT` calculates a positive maximum without applying the eligibility check used by `_checkBridgeClosedAndWhitelisted`; the quote shows an available amount even though the send is rejected.
Apply the same whitelist and Identity eligibility check in `quoteOFT` that the send path uses.
Also at `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:396-409`.
</issue_to_address>
### Comment 7
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="210" />
<code_context>
function setBridgeFees(BridgeFees memory _fees) external onlyOwner {
- require(_fees.fee <= 10000, 'invalid fee');
+ require(_fees.fee <= MAX_FEE_BPS, 'invalid fee');
+ require(_fees.maxFee == 0 || _fees.minFee <= _fees.maxFee, 'minFee > maxFee');
bridgeFees = _fees;
emit BridgeFeesSet(_fees.minFee, _fees.maxFee, _fees.fee);
</code_context>
<issue_to_address>
**Stored fees exceed new cap**
When the proxy is upgraded with a previously configured `bridgeFees.fee` above 500 BPS, `setBridgeFees` caps only future updates, while `_takeFee` applies the stored value; receives therefore continue charging above the new maximum.
Ensure the fee applied by `_takeFee` cannot exceed the new cap for values already stored at upgrade.
</issue_to_address>
### Comment 8
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="209-210" />
<code_context>
*/
function setBridgeFees(BridgeFees memory _fees) external onlyOwner {
- require(_fees.fee <= 10000, 'invalid fee');
+ require(_fees.fee <= MAX_FEE_BPS, 'invalid fee');
+ require(_fees.maxFee == 0 || _fees.minFee <= _fees.maxFee, 'minFee > maxFee');
bridgeFees = _fees;
emit BridgeFeesSet(_fees.minFee, _fees.maxFee, _fees.fee);
</code_context>
<issue_to_address>
**Minimum fee bypasses percentage cap**
When the owner configures a `minFee` at least as large as a bridged amount, or above 5% of that amount with `maxFee` set to zero, `setBridgeFees` permits the minimum fee, and `_takeFee` can clamp it only to the transfer amount; the resulting fee exceeds the 5% cap and can consume the entire transfer.
Enforce the 5% cap on the final fee charged, including the `minFee` floor.
Also at `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:238-241`.
</issue_to_address>
### Comment 9
<location path="packages/bridge-contracts/test/GoodDollarOFT.test.ts" line_range="424" />
<code_context>
+ adapter
+ .connect(user)
+ .send({ ...makeSendParam(user.address, amount), minAmountLD: 0 }, messagingFee, user.address, { value: 0 })
+ ).to.not.be.revertedWithCustomError(adapter, "BRIDGE_LIMITS");
+
await expect(
</code_context>
<issue_to_address>
**Dust-free send test accepts failures**
When the first send reverts for any reason other than `BRIDGE_LIMITS`, including a missing adapter allowance, the assertion only excludes the `BRIDGE_LIMITS` custom error, so another revert passes and the test does not show that the dust-free amount passed the limit check.
Approve the adapter in the fixture and assert that the first send does not revert.
</issue_to_address>
### Comment 10
<location path="packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol" line_range="526" />
<code_context>
+ innerToken.safeTransferFrom(_from, address(this), amountSentLD);
+ require(innerToken.balanceOf(address(this)) - balanceBefore == amountSentLD, 'received amount mismatch');
+ innerToken.forceApprove(address(minterBurner), amountSentLD);
+ require(minterBurner.burn(address(this), amountSentLD), 'GoodDollarOFTAdapter: Burn failed');
}
</code_context>
<issue_to_address>
**Burn events lose the token owner**
When an integrator uses `TokensBurned.from` to attribute bridge sends, the adapter calls `minterBurner.burn(address(this), amountSentLD)`, so `TokensBurned.from` records the adapter rather than the user whose tokens were pulled; event consumers no longer see the originating account.
Preserve the originating sender in a separate event field or emit an adapter event that includes the original `_from` address.
</issue_to_address>
### Comment 11
<location path="packages/bridge-contracts/test/GoodDollarOFT.e2e.test.ts" line_range="336-352" />
<code_context>
});
it('Should enforce maxFee when calculated fee exceeds maxFee', async function () {
- // Set fee to 10% with maxFee of 50 tokens
+ // Set fee to 5% with maxFee of 20 tokens
await adapter.setBridgeFees({
minFee: ethers.utils.parseEther('0'),
- maxFee: ethers.utils.parseEther('50'),
- fee: 1000, // 10%
+ maxFee: ethers.utils.parseEther('20'),
+ fee: 500, // 5%
});
- // For 1000 tokens: 1000 * 1000 / 10000 = 100 tokens (above maxFee of 50)
</code_context>
<issue_to_address>
**Fee cap test ignores contract result**
`Should enforce maxFee when calculated fee exceeds maxFee` only computes `calculatedFee` and compares it with a local `maxFee`; it never checks the adapter’s fee. The test passes even if the adapter ignores `maxFee` and charges the full calculated fee.
Exercise the fee path and assert the fee recipient receives exactly `maxFee` and the recipient receives the net amount.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 10 findings to address first, and this changes the bridge's token-debit authorization, DAO-controlled upgrade path, whitelist behavior, DVN requirements, and inbound/outbound limit policies; a mistake could burn or release tokens, permit or block transfers, or leave production proxies running the wrong implementation. Reverting would not undo tokens already burned or messages already processed, and several changes are policy or trust-boundary decisions that take effect immediately for all users.
Blocking findings: packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:103, packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:103, packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol:70, packages/bridge-contracts/scripts/oft/verify-invariants.ts:98, packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:420, and 5 more
| BridgeDailyLimit public bridgeReceiveDailyLimit; | ||
|
|
||
| /// @dev Inbound (receive) account daily limit tracking | ||
| mapping(address => AccountLimit) public accountsReceiveDailyLimit; |
There was a problem hiding this comment.
🟡 Medium · Inbound limits reset on upgrade
When the adapter is upgraded while the previous shared counters contain receive usage from the current 24-hour window, the new receive counters start at zero and do not include that usage, so inbound checks grant another daily allowance and accept receives beyond the configured cap.
Preserve the prior inbound usage when introducing the receive counters so the upgrade does not grant a fresh allowance.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at lines 100-103:
**Inbound limits reset on upgrade**
When the adapter is upgraded while the previous shared counters contain receive usage from the current 24-hour window, the new receive counters start at zero and do not include that usage, so inbound checks grant another daily allowance and accept receives beyond the configured cap.
Preserve the prior inbound usage when introducing the receive counters so the upgrade does not grant a fresh allowance.| BridgeDailyLimit public bridgeReceiveDailyLimit; | ||
|
|
||
| /// @dev Inbound (receive) account daily limit tracking | ||
| mapping(address => AccountLimit) public accountsReceiveDailyLimit; |
There was a problem hiding this comment.
🟡 Medium · Upgrade validation rejects storage gap
When the new implementation is checked against the deployed layout using OpenZeppelin upgrade validation, the two-slot bridgeReceiveDailyLimit and one-slot mapping are inserted before the unchanged 50-slot __gap, so OpenZeppelin’s upgrade validation sees the old gap displaced instead of reduced and rejects the implementation upgrade.
Reduce the adapter storage gap by the three slots consumed by the appended counters and validate the resulting layout before upgrading.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at lines 100-103:
**Upgrade validation rejects storage gap**
When the new implementation is checked against the deployed layout using OpenZeppelin upgrade validation, the two-slot `bridgeReceiveDailyLimit` and one-slot mapping are inserted before the unchanged 50-slot `__gap`, so OpenZeppelin’s upgrade validation sees the old gap displaced instead of reduced and rejects the implementation upgrade.
Reduce the adapter storage gap by the three slots consumed by the appended counters and validate the resulting layout before upgrading.| * @return success True if the burn was successful | ||
| */ | ||
| function burn(address _from, uint256 _amount) external onlyOperators returns (bool) { | ||
| require(_from == msg.sender, 'can only burn own tokens'); |
There was a problem hiding this comment.
🟡 Medium · MinterBurner upgrade breaks sends
When the MinterBurner upgrade executes before the adapter upgrade, including when the DAO executes its proposal first, the old adapter calls burn(user, amount), but the new GoodDollarOFTMinterBurner.burn requires _from == msg.sender; that call reverts and outbound sends fail until the adapter is upgraded.
Keep the new burn compatible with the old adapter, or ensure the two upgrades execute atomically so the old adapter cannot call the new implementation.
Also at packages/bridge-contracts/deploy/upgradeOFT.ts:109, packages/bridge-contracts/deploy/upgradeOFT.ts:159.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTMinterBurner.sol` at line 70:
**MinterBurner upgrade breaks sends**
When the MinterBurner upgrade executes before the adapter upgrade, including when the DAO executes its proposal first, the old adapter calls `burn(user, amount)`, but the new `GoodDollarOFTMinterBurner.burn` requires `_from == msg.sender`; that call reverts and outbound sends fail until the adapter is upgraded.
Keep the new `burn` compatible with the old adapter, or ensure the two upgrades execute atomically so the old adapter cannot call the new implementation.
Also at `packages/bridge-contracts/deploy/upgradeOFT.ts:109`, `packages/bridge-contracts/deploy/upgradeOFT.ts:159`.| uln.requiredDVNCount >= MIN_REQUIRED_DVNS && uln.requiredDVNCount !== 255, | ||
| `${side} ULN requiredDVNCount ${uln.requiredDVNCount} >= ${MIN_REQUIRED_DVNS} (${uln.requiredDVNs.join(", ")})` | ||
| ); |
There was a problem hiding this comment.
🟡 Medium · Untrusted DVNs pass verification
When a send or receive ULN configuration has at least two required DVNs that are not the intended LayerZero Labs and Nethermind pair, verifyOftInvariants checks only requiredDVNCount, so configurations using unintended or untrusted providers pass and operators see verification succeed for the wrong security configuration.
Have verifyOftInvariants verify the required DVN identities as well as their count.
Prompt for AI agents
In `packages/bridge-contracts/scripts/oft/verify-invariants.ts` at lines 96-98:
**Untrusted DVNs pass verification**
When a send or receive ULN configuration has at least two required DVNs that are not the intended LayerZero Labs and Nethermind pair, `verifyOftInvariants` checks only `requiredDVNCount`, so configurations using unintended or untrusted providers pass and operators see verification succeed for the wrong security configuration.
Have `verifyOftInvariants` verify the required DVN identities as well as their count.| // TODO: destination-chain bridge fee quoting is an open decision; fees are left out of the quote for now | ||
| oftFeeDetails = new OFTFeeDetail[](0); | ||
|
|
||
| (uint256 amountSentLD, uint256 amountReceivedLD) = _debitView( | ||
| _sendParam.amountLD, | ||
| _sendParam.minAmountLD, | ||
| _sendParam.dstEid | ||
| ); | ||
| oftReceipt = OFTReceipt(amountSentLD, amountReceivedLD); | ||
| } |
There was a problem hiding this comment.
🟡 Medium · Quotes overstate received tokens
When a nonzero destination-chain bridge fee is configured and a caller uses quoteOFT to estimate destination proceeds, quoteOFT reports _debitView’s gross amount as amountReceivedLD and returns no fee details, while _credit deducts the destination fee; callers therefore display more tokens than the recipient receives and cannot derive accurate slippage.
Include the destination fee in the quote’s receipt amount and return the corresponding fee details.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at lines 411-420:
**Quotes overstate received tokens**
When a nonzero destination-chain bridge fee is configured and a caller uses `quoteOFT` to estimate destination proceeds, `quoteOFT` reports `_debitView`’s gross amount as `amountReceivedLD` and returns no fee details, while `_credit` deducts the destination fee; callers therefore display more tokens than the recipient receives and cannot derive accurate slippage.
Include the destination fee in the quote’s receipt amount and return the corresponding fee details.| function setBridgeFees(BridgeFees memory _fees) external onlyOwner { | ||
| require(_fees.fee <= 10000, 'invalid fee'); | ||
| require(_fees.fee <= MAX_FEE_BPS, 'invalid fee'); | ||
| require(_fees.maxFee == 0 || _fees.minFee <= _fees.maxFee, 'minFee > maxFee'); |
There was a problem hiding this comment.
🟡 Medium · Stored fees exceed new cap
When the proxy is upgraded with a previously configured bridgeFees.fee above 500 BPS, setBridgeFees caps only future updates, while _takeFee applies the stored value; receives therefore continue charging above the new maximum.
Ensure the fee applied by _takeFee cannot exceed the new cap for values already stored at upgrade.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at line 210:
**Stored fees exceed new cap**
When the proxy is upgraded with a previously configured `bridgeFees.fee` above 500 BPS, `setBridgeFees` caps only future updates, while `_takeFee` applies the stored value; receives therefore continue charging above the new maximum.
Ensure the fee applied by `_takeFee` cannot exceed the new cap for values already stored at upgrade.| require(_fees.fee <= MAX_FEE_BPS, 'invalid fee'); | ||
| require(_fees.maxFee == 0 || _fees.minFee <= _fees.maxFee, 'minFee > maxFee'); |
There was a problem hiding this comment.
🟡 Medium · Minimum fee bypasses percentage cap
When the owner configures a minFee at least as large as a bridged amount, or above 5% of that amount with maxFee set to zero, setBridgeFees permits the minimum fee, and _takeFee can clamp it only to the transfer amount; the resulting fee exceeds the 5% cap and can consume the entire transfer.
Enforce the 5% cap on the final fee charged, including the minFee floor.
Also at packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:238-241.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at lines 209-210:
**Minimum fee bypasses percentage cap**
When the owner configures a `minFee` at least as large as a bridged amount, or above 5% of that amount with `maxFee` set to zero, `setBridgeFees` permits the minimum fee, and `_takeFee` can clamp it only to the transfer amount; the resulting fee exceeds the 5% cap and can consume the entire transfer.
Enforce the 5% cap on the final fee charged, including the `minFee` floor.
Also at `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol:238-241`.| adapter | ||
| .connect(user) | ||
| .send({ ...makeSendParam(user.address, amount), minAmountLD: 0 }, messagingFee, user.address, { value: 0 }) | ||
| ).to.not.be.revertedWithCustomError(adapter, "BRIDGE_LIMITS"); |
There was a problem hiding this comment.
🟡 Medium · Dust-free send test accepts failures
When the first send reverts for any reason other than BRIDGE_LIMITS, including a missing adapter allowance, the assertion only excludes the BRIDGE_LIMITS custom error, so another revert passes and the test does not show that the dust-free amount passed the limit check.
Approve the adapter in the fixture and assert that the first send does not revert.
Prompt for AI agents
In `packages/bridge-contracts/test/GoodDollarOFT.test.ts` at line 424:
**Dust-free send test accepts failures**
When the first send reverts for any reason other than `BRIDGE_LIMITS`, including a missing adapter allowance, the assertion only excludes the `BRIDGE_LIMITS` custom error, so another revert passes and the test does not show that the dust-free amount passed the limit check.
Approve the adapter in the fixture and assert that the first send does not revert.| innerToken.safeTransferFrom(_from, address(this), amountSentLD); | ||
| require(innerToken.balanceOf(address(this)) - balanceBefore == amountSentLD, 'received amount mismatch'); | ||
| innerToken.forceApprove(address(minterBurner), amountSentLD); | ||
| require(minterBurner.burn(address(this), amountSentLD), 'GoodDollarOFTAdapter: Burn failed'); |
There was a problem hiding this comment.
⚪ Low · Burn events lose the token owner
When an integrator uses TokensBurned.from to attribute bridge sends, the adapter calls minterBurner.burn(address(this), amountSentLD), so TokensBurned.from records the adapter rather than the user whose tokens were pulled; event consumers no longer see the originating account.
Preserve the originating sender in a separate event field or emit an adapter event that includes the original _from address.
Prompt for AI agents
In `packages/bridge-contracts/contracts/oft/GoodDollarOFTAdapter.sol` at line 526:
**Burn events lose the token owner**
When an integrator uses `TokensBurned.from` to attribute bridge sends, the adapter calls `minterBurner.burn(address(this), amountSentLD)`, so `TokensBurned.from` records the adapter rather than the user whose tokens were pulled; event consumers no longer see the originating account.
Preserve the originating sender in a separate event field or emit an adapter event that includes the original `_from` address.| it('Should enforce maxFee when calculated fee exceeds maxFee', async function () { | ||
| // Set fee to 10% with maxFee of 50 tokens | ||
| // Set fee to 5% with maxFee of 20 tokens | ||
| await adapter.setBridgeFees({ | ||
| minFee: ethers.utils.parseEther('0'), | ||
| maxFee: ethers.utils.parseEther('50'), | ||
| fee: 1000, // 10% | ||
| maxFee: ethers.utils.parseEther('20'), | ||
| fee: 500, // 5% | ||
| }); | ||
|
|
||
| // For 1000 tokens: 1000 * 1000 / 10000 = 100 tokens (above maxFee of 50) | ||
| // So fee should be 50 tokens (maxFee) | ||
| // For 1000 tokens: 1000 * 500 / 10000 = 50 tokens (above maxFee of 20) | ||
| // So fee should be 20 tokens (maxFee) | ||
| const amount = ethers.utils.parseEther('1000'); | ||
| const calculatedFee = amount.mul(1000).div(10000); | ||
| const maxFee = ethers.utils.parseEther('50'); | ||
| const calculatedFee = amount.mul(500).div(10000); | ||
| const maxFee = ethers.utils.parseEther('20'); | ||
|
|
||
| // The actual fee should be maxFee (50) since calculated (100) > maxFee (50) | ||
| // The actual fee should be maxFee (20) since calculated (50) > maxFee (20) | ||
| expect(calculatedFee.gt(maxFee)).to.be.true; | ||
| }); |
There was a problem hiding this comment.
🟡 Medium · Fee cap test ignores contract result
Should enforce maxFee when calculated fee exceeds maxFee only computes calculatedFee and compares it with a local maxFee; it never checks the adapter’s fee. The test passes even if the adapter ignores maxFee and charges the full calculated fee.
Exercise the fee path and assert the fee recipient receives exactly maxFee and the recipient receives the net amount.
Prompt for AI agents
In `packages/bridge-contracts/test/GoodDollarOFT.e2e.test.ts` at lines 336-352:
**Fee cap test ignores contract result**
`Should enforce maxFee when calculated fee exceeds maxFee` only computes `calculatedFee` and compares it with a local `maxFee`; it never checks the adapter’s fee. The test passes even if the adapter ignores `maxFee` and charges the full calculated fee.
Exercise the fee path and assert the fee recipient receives exactly `maxFee` and the recipient receives the net amount.1 participant
Stacked on #14 (base:
fix/oft-audit-critical-high). Remaining medium/low findings from the OFT audit, plus scripts and tests.Fixes
Contracts
setBridgeFees:fee <= MAX_FEE_BPSandminFee <= maxFee(whenmaxFee != 0)._takeFeeclamps the fee to the amount, so aminFeelarger than the amount can't underflow._lzReceive: when limits fail on a composed message, it reverts withBRIDGE_LIMITS(reason)instead of parking it. The message stays retryable at the endpoint and the compose payload is not lost.bridgeReceiveDailyLimit,accountsReceiveDailyLimit), appended afternameService. Outbound counters are unchanged._checkBridgeLimitsand_resetDailyLimitsIfNeedednow take the counters as parameters.approvalRequired()returns true)._debitpulls withsafeTransferFrominto the adapter, callsforceApprove(minterBurner), thenminterBurner.burn(address(this)).MinterBurner.burnrequires_from == msg.sender._debitandMinterBurner.burn(received amount mismatch).onlyOperatorschecks the livedao.avatar()instead of the cached avatar.MinterBurnerconstructor calls_disableInitializers().onlyWhitelistedis on and the NameService or IDENTITY can't be resolved, transfers fail closed withidentity not set.setNameServiceemitsNameServiceSet.setBridgeLimitsrequiresminAmount <= txLimit <= dailyLimitandaccountDailyLimit <= dailyLimit._removeDust(amountLD).quoteOFToverride: min =minAmount; max = min(txLimit, remaining daily, remaining account daily formsg.sender), and 0 when closed.generateGuid/_toBytes32.Scripts / config
layerzero.config.tsnow requires 2 DVNs: LayerZero Labs + Nethermind. LZ metadata confirms both are live (v2, not deprecated) on Celo and XDC. Other DVNs live on both chains: Horizen, Canary, Deutsche Telekom.hardhat.config.ts: addedeidtoproduction-celoandproduction-xdc.deploy/upgradeOFT.ts: upgrades of Avatar-owned proxies (adapter after handover, and the MinterBurner) go throughController.genericCall.genericCallpayloads to submit as a DAO proposal.oft:verifytask (scripts/oft/verify-invariants.ts) checks:verifyOftDeploymentwith the owner required to be the Avatar;endpoint.delegates(adapter) == Avatar;bridgeLimitsmatchoft.config.json;G$.isMinter(minterBurner);requiredDVNCount >= 2.bridge-oft-token.ts: approves the adapter instead of the MinterBurner.Tests
test/helpers/deployMinterBurnerProxy.ts. TheMinterBurnerimplementation can't be initialized anymore, so tests use a GoodProtocolERC1967Proxy.exposed_debitharness for the L-01 burn path.sendburns through the adapter;MAX_FEE_BPS = 500(5%) was picked by me. Confirm or change it.quoteOFTreturns emptyoftFeeDetails(there is a TODO). The fee is taken on the destination, soamountReceivedLDin the receipt doesn't include it, and the UI/slippage handling needs a decision.lz:oapp:wire.oft:verifywas not run against a live network: the repo has no OFT deployment artifacts yet. Run it after deploy and wiring.Document-and-accept (no code change)
verifyOftDeployment.onlyWhitelistedonly after Identity is confirmed on both chains. The receive side checks the recipient, so a non-whitelisted recipient's message reverts and stays retryable.approveFailedRequestskips the daily counters on purpose; the owner/optimistic approval is the override.dailyLimitcan pass around a window boundary. SetdailyLimitto half the burst you can tolerate.setDelegateruns 3× duringinitialize(OApp, OFT and explicit). This is harmless.OwnableUpgradeable). The handover script asserts the owner and delegate afterwards.tokenagainstinnerToken. The deploy passes the same address, andverifyOftDeploymentcheckstoken().MAX_FEE_BPS), limits, pause, peers, DVNs, operators, approve/reject parked requests, and upgrade both proxies.Summary by Sourcery
Harden the OFT adapter and deployment workflow against remaining medium- and low-severity audit findings while adding verification tooling and regression coverage.
New Features:
Bug Fixes:
Enhancements:
Deployment:
Tests:
Chores: