Fix audit findings in EVM Cadence contract#8518
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdated the EVM Cadence stdlib contract (docs, ABI helpers, COA validation, pause guards, and a small API purity change) and replaced hardcoded genesis/state-commitment hexes used by bootstrap tests and unit-test utilities. (26 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
joshuahannan
force-pushed
the
josh/evm-contract-security-audit
branch
from
bb521c5 to
e29a7fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/evm/stdlib/contract.cdc`:
- Around line 1088-1110: The error message concatenation in the COA ownership
mismatch branch concatenates "EVM.validateCOAOwnershipProof(): Cannot validate
COA ownership." with "The provided evm address..." resulting in "ownership.The"
without a space; update the string used in the first ValidationResult (inside
the if let coaRef borrow branch) so there is a trailing space (or include a
leading space in the concatenated string) before concatenation—locate the
ValidationResult construction that returns isValid: false in the COA address
bytes loop (using coaRef, coaAddressBytes, evmAddress) and add the missing
whitespace to the concatenated message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: abff165d-e6d8-4024-99f2-db1f22ed1124
📒 Files selected for processing (1)
fvm/evm/stdlib/contract.cdc
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
fvm/evm/stdlib/contract.cdc (1)
1095-1096:⚠️ Potential issue | 🟡 MinorAdd the missing space in the COA mismatch message.
These literals still concatenate as
ownership.The, so the returnedproblemstring reads awkwardly.✏️ Suggested fix
- problem: "EVM.validateCOAOwnershipProof(): Cannot validate COA ownership." - .concat("The provided evm address does not match the account's COA address.") + problem: "EVM.validateCOAOwnershipProof(): Cannot validate COA ownership. " + .concat("The provided evm address does not match the account's COA address.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.cdc` around lines 1095 - 1096, The problem string concatenation in EVM.validateCOAOwnershipProof() produces "ownership.The" because there's no space between the two literals; update the concatenation around the variable/problem construction (the `problem` assignment in EVM.validateCOAOwnershipProof) to include a space either at the end of the first literal ("EVM.validateCOAOwnershipProof(): Cannot validate COA ownership. ") or at the start of the second literal (" The provided evm address...") so the final message reads correctly.
🧹 Nitpick comments (1)
fvm/evm/stdlib/contract.cdc (1)
928-930: Clarify thebytesmapping example.This contract already defines
EVMBytes/EVMBytes4/EVMBytes32as the Cadence-side byte wrappers. Showing[UInt8] → byteshere is confusing unless raw[UInt8]is intentionally supported too.✏️ One possible doc fix
- /// corresponding Solidity types (e.g. UInt256 → uint256, [UInt8] → bytes). + /// corresponding Solidity types (e.g. UInt256 → uint256, EVMBytes → bytes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/evm/stdlib/contract.cdc` around lines 928 - 930, The documentation comment that maps Cadence types to Solidity types is ambiguous about bytes; update the example to reference the Cadence EVM byte wrapper types (EVMBytes, EVMBytes4, EVMBytes32) instead of showing [UInt8] → bytes, or explicitly state if raw [UInt8] arrays are also supported; edit the comment near the ABI encoding routine so it reads something like: Cadence EVMBytes / EVMBytes4 / EVMBytes32 map to Solidity bytes/bytes4/bytes32 (or, if raw [UInt8] is supported, add a sentence clarifying that [UInt8] will be accepted and converted to EVMBytes).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fvm/evm/stdlib/contract.cdc`:
- Around line 1095-1096: The problem string concatenation in
EVM.validateCOAOwnershipProof() produces "ownership.The" because there's no
space between the two literals; update the concatenation around the
variable/problem construction (the `problem` assignment in
EVM.validateCOAOwnershipProof) to include a space either at the end of the first
literal ("EVM.validateCOAOwnershipProof(): Cannot validate COA ownership. ") or
at the start of the second literal (" The provided evm address...") so the final
message reads correctly.
---
Nitpick comments:
In `@fvm/evm/stdlib/contract.cdc`:
- Around line 928-930: The documentation comment that maps Cadence types to
Solidity types is ambiguous about bytes; update the example to reference the
Cadence EVM byte wrapper types (EVMBytes, EVMBytes4, EVMBytes32) instead of
showing [UInt8] → bytes, or explicitly state if raw [UInt8] arrays are also
supported; edit the comment near the ABI encoding routine so it reads something
like: Cadence EVMBytes / EVMBytes4 / EVMBytes32 map to Solidity
bytes/bytes4/bytes32 (or, if raw [UInt8] is supported, add a sentence clarifying
that [UInt8] will be accepted and converted to EVMBytes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 517fb23f-5b51-4710-9d6c-935fd32362ad
📒 Files selected for processing (1)
fvm/evm/stdlib/contract.cdc
| /// @return the token decimals of the ERC20 | ||
| access(all) | ||
| fun deposit(from: @FlowToken.Vault) { | ||
| pre { |
There was a problem hiding this comment.
How come the pre check here is removed? This will allow EVM state-change, allowing funds to enter Flow EVM, but the withdraw function below still has the pre check. Note that COA.deposit & COA.withdraw do create EVM.TransactionExecuted events, which means that an EVM state-change occurred.
There was a problem hiding this comment.
My bad, just missed the comment in the PR description:
Remove redundant isPaused() pre-condition from CadenceOwnedAccount.deposit: the delegated call to EVMAddress.deposit already enforces the pause guard.
| /// the from address as the signer. | ||
| /// The transaction state changes are not persisted. | ||
| /// This is useful for gas estimation or calling view contract functions. | ||
| /// Note: dry-run functions are intentionally exempt from the isPaused() guard — |
| return ValidationResult( | ||
| isValid: false, | ||
| problem: "EVM.validateCOAOwnershipProof(): Cannot validate COA ownership." | ||
| .concat("The provided evm address does not match the account's COA address.") |
There was a problem hiding this comment.
Do we still need the String.concat here? Not related to the PR, but it's a good opportunity to update it.
There was a problem hiding this comment.
Good catch! I'll change that
| problem: nil | ||
| isValid: false, | ||
| problem: "EVM.validateCOAOwnershipProof(): Cannot validate COA ownership. " | ||
| .concat("Could not borrow the COA resource for account \(address).") |
There was a problem hiding this comment.
Do we still need the String.concat here? Not related to the PR, but it's a good opportunity to update it.
joshuahannan
force-pushed
the
josh/evm-contract-security-audit
branch
from
ef5c8f1 to
24877dc
Compare
joshuahannan
force-pushed
the
josh/evm-contract-security-audit
branch
from
24877dc to
d62ef3e
Compare
4 participants
Summary
Security and quality audit fixes for
fvm/evm/stdlib/contract.cdc:validateCOAOwnershipProof: replaced nil-check + force-unwrap pattern withif let, and capturedcoaRef.address().bytesinto a single binding rather than callingaddress()twice.Balance.isZero()missingviewannotation: allows use in view contexts and pre-conditions.isPaused()pre-condition fromCadenceOwnedAccount.deposit: the delegated call toEVMAddress.depositalready enforces the pause guard.CadenceOwnedAccount.balance(): callInternalEVM.balancedirectly instead of constructing an intermediateEVMAddressstruct.@returndocs oninitAddressandCadenceOwnedAccount.deposit(both said "returns the token decimals of the ERC20").encodeABIanddecodeABI.setupHeartbeataccess(all) rationale: explains why public access is safe (idempotency enforced by storage path collision).dryRun: clarifies that dry-run functions intentionally bypass the pause guard since they perform no state mutations./evm/about→/build/evm/how-it-works.Summary by CodeRabbit
Bug Fixes
Documentation
Tests