Conversation
Clarifies that minFeeBps/maxFeeBps are per-capture (not aggregate) bounds and that integer division floors both minFee and maxFee. Includes a worked fragmentation example and states this as pre-existing accepted risk under the current operator-trust model. Addresses audit follow-up to #90. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
Collaborator
🟡 Heimdall Review Status
|
…c0 migration Two audit follow-ups on merged PR #90: 1. Example 4 previously showed feeAmount=0 with feeReceiver=address(0) as valid when PaymentInfo.feeReceiver is a fixed non-zero address. _validateFee actually reverts with InvalidFeeReceiver in that case (receiver matching is unconditional, not gated on feeAmount>0). Fix the example to require the configured recipient and add a call-out note distinguishing fixed vs. flexible recipient semantics. 2. Add an 'Event Migration' section documenting the topic0 changes on PaymentCharged and PaymentCaptured. Includes canonical old/new signatures, the four topic0 hashes (verified with cast and independently reproduced), a warning that ABIs from before the migration decode the new logs silently-incorrectly (feeBps vs feeAmount semantics at the same log data offset), and a migration checklist for integrators. Co-authored-by: OpenCode <opencode-noreply@coinbase.com>
amiecorso
approved these changes
Jul 21, 2026
Collaborator
|
Review Error for amiecorso @ 2026-07-21 21:07:42 UTC |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Docs-only follow-up addressing three audit findings on merged PR #90. All three are documentation clarifications; the deployed contract at
0xBdEA0D1bcC5966192B070Fdf62aB4EF5b4420cffis unchanged and no code, ABI, tests, or CI are affected.Findings addressed
1. Per-capture rounding and fragmentation risk (commit 8026382)
Documents that
minFeeBps/maxFeeBpsare per-capture rate bounds (not aggregate payment-level guarantees) and that integer division floors bothminFeeandmaxFee. This enables a fragmentation edge case where an operator with capture-amount discretion can split a payment into many small captures whose individualminFeevalues each round to zero.This behavior is pre-existing — the old
feeBpsAPI used the sameamount * bps / 10_000per-capture formula. PR #90 preserved the semantics while moving the concrete fee amount off-chain, so this is not a regression from the absolute-fee migration.Accepted-risk rationale:
2. Example 4 contradicted
_validateFee(commit f6c2618)docs/Fees.mdExample 4 previously listedfeeAmount: 0, feeReceiver: address(0)as valid whenPaymentInfo.feeReceiveris a fixed non-zero address. The actual_validateFeelogic (src/AuthCaptureEscrow.sol:511–530) reverts withInvalidFeeReceiverin that case — receiver matching is unconditional, not gated onfeeAmount > 0. Receiver skipping only applies under the flexible-recipient branch (PaymentInfo.feeReceiver == address(0), Example 3).Fix: remove the incorrect row, add it as an explicit failure case, and add a callout note distinguishing fixed vs. flexible recipient semantics. Docs are now aligned with deployed behavior.
3.
topic0migration forPaymentChargedandPaymentCaptured(commit f6c2618)The
uint16 feeBps→uint256 feeAmountchange altered the Solidity event signatures, producing brand-newtopic0values for bothPaymentChargedandPaymentCaptured. Off-chain indexers filtering by the oldtopic0will silently observe zero matching logs from a new deployment — no error surfaces. Worse, an old ABI applied to the new logs decodes without error but misinterprets the fee (e.g. a25e6USDC absolute fee is read as25,000,000 bps).The current deployment (
0xBdEA0D1bcC5966192B070Fdf62aB4EF5b4420cff) is immutable and continues to emit the old signatures. Any future redeployment will emit the new signatures. This can't be avoided in Solidity (topic0 is derived from the canonical signature), so this is purely a release-management/integrator-communication concern.Fix: add a new "Event Migration:
topic0Changes" section publishing both sets of canonical signatures and the fourtopic0hashes, with a migration checklist recommending(address, topic0)routing during the coexistence period.topic0values (verified withcast keccakand independently reproduced with a fresh Python Keccak-256 impl — byte-identical):topic0PaymentCharged0x943ae4341dd799d7aeedc501f616cd26b134639e0bc2ec059581ba3ebbf1e7d0PaymentCharged0x137b0e73e4453f43c2e1ad2552980a0e0c7e988764619974ca26d37a7159940cPaymentCaptured0xe749f7bbd01b49bb05abf26ca492cb4dfdea6bedeada8a40fdccd478c73a74e2PaymentCaptured0xac1e0db1957daedbc6944bcb4c8dc947ff8101d710671ac2ec26309f7f38d58eChanges
docs/Fees.mdonly:feeAmount: 0, feeReceiver: address(0)valid-row with an explicit failure case, plus a callout note on fixed vs. flexible recipient semantics.topic0Changes forPaymentChargedandPaymentCaptured" section: canonical old/new signatures,topic0values, ABI-misdecode warning, integrator migration checklist.No code changes
Docs-only. No impact on contract bytecode, ABI, tests, or CI.