Add clarifying comments for potential security review false positives#208
Merged
joshuahannan merged 1 commit intomainfrom Apr 3, 2026
Merged
Conversation
Addresses items raised during a security audit that were determined to be non-issues but lacked sufficient inline documentation: - FlowEVMBridge.cdc: explain why force-casts to CrossVMNFT.EVMNFT in handleUpdatedBridgedNFTToEVM are safe (bridge-defined NFTs always implement EVMNFT by template contract construction) - FlowEVMBridge.cdc: document the routing invariant for handleEVMNativeCrossVMNFTFromEVM — type is always the custom cross-VM type so isCadenceNative is always true and _type is never reassigned, meaning isLocked and unlockNFT always use the same type - FlowEVMBridgeConfig.cdc: clarify that isTypePaused never actually returns nil despite its Bool? return type, and explain the correct call-site idiom - FlowEVMBridgeUtils.cdc (mustEscrowERC20): explain that post-state balance assertions are the primary security guarantee and that there is no TOCTOU risk because all EVM calls in a Cadence transaction execute within the same EVM block - FlowEVMBridgeUtils.cdc (ufix64ToUInt256): document that overflow is not a practical concern for standard ERC20 decimals and explain the bound - FlowEVMBridgeHandlers.cdc: explain that UInt(amount) truncation is caught by the immediately following assert and cannot trigger in practice given total FLOW supply constraints - FlowEVMBridgeCustomAssociations.cdc: fix pre-condition error message that force-unwrapped borrowNFTCustomConfig(forType: type) when looking up an already-registered EVM address (wrong key), replacing it with a direct lookup on associationsByEVMAddress Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
joshuahannan
changed the title
m-Peter
approved these changes
Mar 30, 2026
Collaborator
m-Peter
left a comment
m-Peter
left a comment
There was a problem hiding this comment.
LGTM! Very descriptive additions.
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
2 participants
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
During a security audit, several code patterns were flagged as potential issues but were determined to be non-bugs. This PR adds inline comments and fixes a bad error message to make the intent and safety of each pattern clear to future reviewers.
Changes
FlowEVMBridge.cdcas!force-casts toCrossVMNFT.EVMNFTinhandleUpdatedBridgedNFTToEVMare safe — all bridge-defined NFTs are deployed from the bridge's template contract, which unconditionally implementsEVMNFTFlowEVMBridge.cdchandleEVMNativeCrossVMNFTFromEVM— this handler is only reached with a custom cross-VM type, soisCadenceNativeis always true,_typeis never reassigned, andisLocked/unlockNFTalways use the same typeFlowEVMBridgeConfig.cdcisTypePausednever actually returnsnildespite itsBool?return type, and document the correct call-site idiomFlowEVMBridgeUtils.cdcmustEscrowERC20are the primary security guarantee and that there is no TOCTOU risk — all EVM calls within a single Cadence transaction execute in the same EVM blockFlowEVMBridgeUtils.cdcufix64ToUInt256is not a practical concern for standard ERC20 decimals (≤18), and explain the boundFlowEVMBridgeHandlers.cdcUInt(amount)truncation in the WFLOW handler is caught by the immediately following assert, and cannot trigger given total FLOW supply constraintsFlowEVMBridgeCustomAssociations.cdcborrowNFTCustomConfig(forType: type)when looking up an already-registered EVM address — the lookup used the wrong key and would itself panic. Replaced with a direct lookup onassociationsByEVMAddressTest plan
make cdc-test)