fix(FLOW-16): reconcile cross-VM refund precision and harden worker recovery#37
Conversation
EVM uses uint256 with 18 decimals while Cadence uses UFix64 with 8 decimals. Converting between formats truncates precision beyond 8 decimal places. This is not exploitable (truncation favors protocol) and minimum deposit mitigates dust loss. Added precision warnings to: - Cadence contract header - Cadence ufix64FromUInt256() function docstring - Solidity contract header Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
PR #37 Review — Precision Residual RefundThis PR introduces a well-motivated fix: the old The design intent is sound. Three concrete issues are worth addressing before merging. 1. No escape hatch for a stuck
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /// EVM uses uint256 with 18 decimals (wei), while Cadence uses UFix64 with 8 decimals. | ||
| /// Converting between these formats truncates precision beyond 8 decimal places. | ||
| /// For example: 1.123456789012345678 FLOW (EVM) becomes 1.12345678 FLOW (Cadence). | ||
| /// This is not exploitable (users receive slightly less, not more) and the 1 FLOW |
There was a problem hiding this comment.
Incomplete analysis — precision truncation IS a liveness bug in the refund path
The note correctly describes forward-direction truncation (deposit → Cadence), but misses the critical reverse case: when a native FLOW CREATE/DEPOSIT request fails, completeProcessing (lines 1056–1060) converts refundAmount (UInt256, 18 dec) → UFix64 (8 dec, truncated) → EVM.Balance via setFLOW. The resulting msg.value is smaller than the original request.amount.
Solidity then reverts with MsgValueMustEqualAmount (msg.value != request.amount, exact match). This causes completeProcessing to return false → processRequest panics → the WorkerHandler transaction reverts. Because the PROCESSING status was committed in a prior startProcessingBatch transaction, it is not reverted. When the SchedulerHandler detects the panic and calls markRequestAsFailed, it hits the same truncation, the same Solidity revert, and the same false return — after which it removes the request from scheduledRequests with no retry (line 674 of WorkerOps, comment: "errors are not considered transient"). The request is permanently stuck in PROCESSING with user funds in the COA.
The statement "users receive slightly less, not more" is true for withdrawals/close, but for the refund-on-failure path the protocol currently cannot return even the truncated amount because Solidity demands the exact original amount.
Affected amounts: any wei value where amount % 10^10 != 0 (e.g. any amount with non-zero attoflow below 10 gwei). Amounts that are exact multiples of 10^10 wei (1 FLOW, 1.5 FLOW, etc.) are unaffected.
| * PRECISION NOTE: EVM uses uint256 with 18 decimals, while Cadence uses UFix64 with 8 decimals. | ||
| * Amounts are truncated beyond 8 decimal places during cross-VM conversion. | ||
| * Example: 1.123456789012345678 FLOW → 1.12345678 FLOW (loss of ~9e-9 FLOW). | ||
| * This is not exploitable (truncation favors the protocol) and the minimum deposit mitigates dust. |
There was a problem hiding this comment.
The "not exploitable" claim is incomplete — see the refund path
The forward direction (deposit → Cadence) is indeed safe: users put in more than they get, so the protocol is not at risk. But completeProcessing in Solidity (line 1001) requires an exact match: if (msg.value != request.amount) revert MsgValueMustEqualAmount().
When a native FLOW request fails, the Cadence worker reconstructs the refund amount via UFix64, losing the same sub-8-decimal precision. The resulting msg.value is less than request.amount, so Solidity reverts. The Cadence transaction panics, the request stays permanently in PROCESSING on EVM, and there is no admin escape hatch (dropRequests only operates on PENDING). User funds in the COA have no path back.
Fix: Change line 1001 to if (msg.value < request.amount) and return the excess to the COA, or have the Cadence worker bypass the UFix64 conversion when building the refund balance and use the raw attoflow value directly.
| refundValue = FlowYieldVaultsEVM.balanceFromUFix64(refundUFix64, tokenAddress: tokenAddress) | ||
| // Native FLOW: send the original attoflow amount back to EVM without | ||
| // round-tripping through UFix64, which would truncate sub-8-decimal precision. | ||
| refundValue = EVM.Balance(attoflow: UInt(refundAmount)) |
There was a problem hiding this comment.
The fix is correct. The old code round-tripped through UFix64 truncation:
// OLD — BROKEN for sub-8-decimal amounts
let refundUFix64 = ufix64FromUInt256(refundAmount, ...) // truncates to 8 decimals
refundValue = balanceFromUFix64(refundUFix64, ...) // encodes truncated amountBecause FlowYieldVaultsRequests.sol enforces msg.value == request.amount exactly:
if (msg.value != request.amount) revert MsgValueMustEqualAmount();…any request.amount with sub-8-decimal precision would cause the EVM call to revert. completeProcessing returning false triggers a panic in processRequest, and the request would be permanently stuck in PROCESSING.
The new EVM.Balance(attoflow: UInt(refundAmount)) sends the original wei amount without precision loss, which is the right fix. UInt in Cadence is arbitrary-precision, so the cast is safe for any deposit size.
One gap: the Solidity tests added in this PR call completeProcessing directly as the COA and verify the Solidity-side accounting. They don't exercise the Cadence code path (completeProcessing → EVM.Balance(attoflow: UInt(refundAmount))). Consider adding a Cadence error_handling_test scenario with a sub-8-decimal amount (e.g. 1_000_000_000_123_456_789 attoflow) that triggers a failure, and asserting the claimable refund matches the exact input amount.
| assertEq(c.getUserPendingBalance(user, NATIVE_FLOW), 0); | ||
| assertEq(c.getClaimableRefund(user, NATIVE_FLOW), amount); | ||
| } | ||
|
|
There was a problem hiding this comment.
This test correctly verifies that the Solidity contract accepts and credits the full sub-8-decimal amount (1 ether + 123456789).
Note the coverage gap: these tests simulate the COA calling completeProcessing{value: amount} directly in Foundry. They validate the Solidity accounting but don't test the changed Cadence code — specifically that EVM.Balance(attoflow: UInt(refundAmount)) produces the right value and that the resulting EVM call carries the correct msg.value. The bug fix lives entirely in the Cadence contract, so a Cadence-level integration test (in error_handling_test.cdc) is the only way to close that gap.
| let calldata = EVM.encodeABIWithSignature( | ||
| "completeProcessing(uint256,bool,uint64,string)", | ||
| [requestId, success, evmYieldVaultId, message] | ||
| "completeProcessing(uint256,bool,uint64,string,uint256)", |
There was a problem hiding this comment.
Bug (regression): markRequestAsFailed sends the wrong refundAmount for WITHDRAW requests
Switching from the 4-arg to the 5-arg ABI here is correct for processRequest, but markRequestAsFailed (line 679) still passes refundAmount: request.amount unconditionally and is not updated in this PR.
Before this PR, the 4-arg Solidity overload silently ignored refundAmount for non-escrowed types. Now the 5-arg Solidity overload calls _completeProcessing which strictly validates:
uint256 expectedFailedRefundAmount = isEscrowedRequest ? request.amount : 0;
if (refundAmount != expectedFailedRefundAmount) revert InvalidRefundAmount(...);For a WITHDRAW_FROM_YIELDVAULT request with a non-zero amount:
isEscrowedRequest = false→ expected = 0- Cadence passes
request.amount→ revert markRequestAsFailedreturnsfalse→ request permanently stuck inPROCESSING
This breaks both crash-recovery paths in FlowYieldVaultsEVMWorkerOps.cdc (lines 326 and 667).
The fix is to use the helper introduced in this PR in markRequestAsFailed:
refundAmount: FlowYieldVaultsEVM.refundAmountForCompletion(request: request, success: false),
| /// @return The EVM-side amount that can be processed in Cadence without losing precision | ||
| access(self) fun exactCadenceRepresentableAmount(_ value: UInt256, tokenAddress: EVM.EVMAddress): UInt256 { | ||
| let cadenceAmount = FlowYieldVaultsEVM.ufix64FromUInt256(value, tokenAddress: tokenAddress) | ||
| return FlowYieldVaultsEVM.uint256FromUFix64(cadenceAmount, tokenAddress: tokenAddress) |
There was a problem hiding this comment.
Risk: divergence between Cadence and Solidity precision residual computations
exactCadenceRepresentableAmount uses FlowEVMBridgeUtils (Cadence on-chain library) to round-trip the value. The Solidity mirror, _expectedPrecisionResidual, uses hardcoded arithmetic:
// native FLOW
return amount % 1e10;
// ERC20
uint8 decimals = IERC20Metadata(tokenAddress).decimals();
uint256 quantum = 10 ** (decimals - 8);
return amount % quantum;If these two code paths ever return different values for the same input — e.g. because FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount uses a different decimal source than IERC20Metadata.decimals() for some token — then completeProcessing will always revert with InvalidRefundAmount, permanently bricking every request for that token.
Recommendations:
- Add a test that cross-checks the two computations for at least one ERC20 with 18 decimals and one with 6 decimals.
- Consider adding a view function to the Solidity contract that exposes the expected residual so the Cadence worker can read it on-chain rather than recomputing it independently.
| let needsRefund = refundAmount > 0 | ||
| && (requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue | ||
| || requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue) |
There was a problem hiding this comment.
Redundant requestType guard — refundAmountForCompletion already returns 0 for WITHDRAW/CLOSE
refundAmountForCompletion returns 0 for any non-escrowed request type (WITHDRAW/CLOSE), so refundAmount > 0 can only ever be true for CREATE/DEPOSIT requests. The requestType == CREATE_YIELDVAULT || ... check here is therefore dead code and can cause a reader to believe there is a code path where refundAmount > 0 for WITHDRAW/CLOSE.
Simplify to:
| let needsRefund = refundAmount > 0 | |
| && (requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue | |
| || requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue) | |
| let needsRefund = refundAmount > 0 |
| if (precisionLossDecimals > 77) return amount; | ||
|
|
||
| uint256 quantum = 10 ** precisionLossDecimals; | ||
| return amount % quantum; |
There was a problem hiding this comment.
IERC20Metadata.decimals() called without revert guard — breaks success-path finalization for tokens without decimals()
decimals() is in the optional IERC20Metadata extension, not the base ERC-20 spec. If a token passes the allowlist but doesn't implement it, completeProcessing(success=true) will always revert for CREATE/DEPOSIT on that token. The failure is not permanent (crash recovery marks the request FAILED and issues a full refund), but users can never successfully complete a create/deposit for that token. _setTokenConfig has no guard for this.
Suggested fixes:
- Call
IERC20Metadata(tokenAddress).decimals()in_setTokenConfigto reject tokens at registration time. - Or wrap line 1698 in a
try/catchand fall back to 18 decimals (the widely-used default).
There was a problem hiding this comment.
It's very rare for a token not to conform to IERC20Metadata and not returning decimals() @claude
There was a problem hiding this comment.
Claude finished @liobrasil's task in 55s —— View job
- Read relevant code and analyze the
decimals()concern - Respond to pushback on rarity of tokens without
decimals()
Fair point — tokens that implement ERC-20 but not decimals() are vanishingly rare in practice (virtually every token, including all OZ-based ones, includes it). The concern stands as a theoretical correctness issue rather than a practical one.
The reason I flagged it is that the failure mode is silent-until-too-late: if a non-conforming token ever were allowlisted via setTokenConfig, users could submit CREATE/DEPOSIT requests successfully, but those requests would be permanently uncompletable (because _expectedPrecisionResidual would revert on every completeProcessing call). The crash-recovery path via markRequestAsFailed would still work since it takes the success=false branch which skips _expectedPrecisionResidual.
So practically speaking the impact is bounded — requests would end up in a failed state and users would get refunds — but the user experience would be bad and the root cause non-obvious.
The minimal mitigation is a one-liner in setTokenConfig:
function setTokenConfig(address tokenAddress, bool isSupported, ...) external onlyOwner {
if (isSupported && !isNative) {
IERC20Metadata(tokenAddress).decimals(); // revert early if token doesn't implement it
}
...
}This adds a trivial read at registration time (owner-only, one-time) and makes the invariant explicit: every allowlisted ERC20 must implement decimals(). Given how rare the failure case is I understand if you consider this not worth the noise — just flagging the failure mode so it's a conscious tradeoff rather than a blind spot.
|
|
||
| /** | ||
| * @dev Registers a new YieldVault with comprehensive ownership tracking. | ||
| * Updates multiple mappings to enable O(1) lookups for: |
There was a problem hiding this comment.
decimals() revert risk — requests could get permanently stuck in PROCESSING
IERC20Metadata(tokenAddress).decimals() (a few lines below, at the ERC20 branch) is a new external call added by this PR. If any configured ERC20 token does not implement decimals() — it is optional in the ERC20 spec — every completeProcessing() call for that token will revert here.
Since completeProcessing() is called from the Cadence worker, a revert causes the worker transaction to panic (keeping the request in PROCESSING state). Crash-recovery then calls markRequestAsFailed() → completeProcessing() — same revert — effectively a permanently stuck request.
The practical risk is low because FlowEVMBridge-onboarded tokens almost certainly implement decimals(). The correct defence is to validate (or cache) the decimals at setTokenConfig time so a misconfigured token is rejected immediately rather than discovered mid-flight:
// In _setTokenConfig, for non-native tokens:
uint8 tokenDecimals = IERC20Metadata(tokenAddress).decimals(); // reverts early if missing
// store in TokenConfig for later useAlternatively, add a try/catch (not available for arbitrary external calls in Solidity, so a static call wrapper is needed) or document that tokens without decimals() are unsupported and are rejected at the bridge onboarding step.
| let needsRefund = !success | ||
| && refundAmount > 0 | ||
| // Determine if refund is needed for CREATE/DEPOSIT lifecycle accounting | ||
| let needsRefund = refundAmount > 0 |
There was a problem hiding this comment.
Redundant requestType guard obscures invariant ownership
refundAmountForCompletion() already guarantees refundAmount == 0 for WITHDRAW/CLOSE requests, so the requestType check here is unreachable dead code. This is fine for defence-in-depth, but if the check is ever wrong it masks the real bug: Solidity would still reject the call via InvalidRefundAmount, so a caller cannot use this guard to bypass the accounting check.
Worth documenting explicitly:
// refundAmountForCompletion() already guarantees refundAmount == 0 for WITHDRAW/CLOSE,
// so the requestType guard below is a secondary safety net only.
let needsRefund = refundAmount > 0
&& (requestType == FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue
|| requestType == FlowYieldVaultsEVM.RequestType.DEPOSIT_TO_YIELDVAULT.rawValue)Not a bug, but the comment should clarify that refundAmountForCompletion is the primary invariant so future readers don't assume the guard here is load-bearing.
| return request.amount | ||
| } | ||
|
|
||
| let exactProcessableAmount = FlowYieldVaultsEVM.exactCadenceRepresentableAmount( |
There was a problem hiding this comment.
ERC20 residual: round-trip through FlowEVMBridgeUtils must match Solidity's amount % 10^(decimals-8)
exactCadenceRepresentableAmount performs convertERC20AmountToCadenceAmount → convertCadenceAmountToERC20Amount, which is the Cadence-side equivalent of amount - (amount % 10^(decimals-8)). The Solidity side independently reimplements the same formula in _expectedPrecisionResidual using a live call to IERC20Metadata.decimals().
These two code paths must produce identical residuals or completeProcessing will revert with InvalidRefundAmount for every ERC20 CREATE/DEPOSIT success completion. The test suite verifies the Solidity half in isolation (test_CompleteProcessing_SuccessCreditsERC20ResidualRefund) but there is no Cadence-level or worker-level integration test for ERC20 dust amounts. An integration test (Cadence or shell worker test) that sends amount = N * 10^(decimals-8) + dust through a full lifecycle would pin this cross-system invariant.
| return amount % 1e10; | ||
| } | ||
|
|
||
| uint8 decimals = IERC20Metadata(tokenAddress).decimals(); |
There was a problem hiding this comment.
ERC20 residual: two independent implementations must stay in sync
_expectedPrecisionResidual computes the residual using a live IERC20Metadata(tokenAddress).decimals() call and amount % 10^(decimals-8).
The Cadence side independently produces the same value via FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount → convertCadenceAmountToERC20Amount round-trip (exactCadenceRepresentableAmount). If those utilities use a different decimal source or rounding mode, the InvalidRefundAmount check will always revert on ERC20 CREATE/DEPOSIT success completions, permanently stranding those requests in PROCESSING.
The new worker-test SCENARIO 4 covers only native FLOW dust; there is no end-to-end test that exercises an ERC20 token with sub-8-decimal-precision amounts through the full Cadence → EVM completion path to confirm the two residual calculations agree.
| requestId: scheduledRequestId, | ||
| workerTransactionId: request.workerTransactionId, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Good fix — continue was genuinely missing here
Before this PR, the StopAllMarkFailedSkipped path fell through and unconditionally executed scheduledRequests.remove(key: scheduledRequestId) on line 339. That meant a request could be dropped from tracking even when markRequestAsFailed returned false — leaving it stuck in PROCESSING with no recovery. The continue correctly keeps the request in scheduledRequests so the next scheduler run can retry finalisation.
| FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: requestId) | ||
| // Keep the request tracked if EVM finalization fails so crash recovery can retry later. | ||
| if success { | ||
| FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: requestId) |
There was a problem hiding this comment.
No retry limit — could leave requests stuck indefinitely
If markRequestAsFailed returns false (e.g., COA has insufficient native FLOW to send the refund, or the EVM call reverts for any persistent reason), the request stays in scheduledRequests and will be retried on every subsequent scheduler run with no back-off, counter, or escalation path beyond the WorkerHandlerPanicDetected event.
In practice the most plausible trigger is an underfunded COA: if the COA has zero balance it can never satisfy the msg.value == refundAmount check on the Solidity side for a native-FLOW failed CREATE/DEPOSIT, so the request is permanently unfinalisable.
Worth either: (a) documenting the invariant that the COA must maintain a reserve sufficient to cover refunds, or (b) adding a max-retry counter after which the request is dropped from scheduledRequests and an operator alert is emitted.
| let calldata = EVM.encodeABIWithSignature( | ||
| "completeProcessing(uint256,bool,uint64,string)", | ||
| [requestId, success, evmYieldVaultId, message] | ||
| "completeProcessing(uint256,bool,uint64,string,uint256)", |
There was a problem hiding this comment.
Breaking ABI change — needs coordinated testnet re-deployment
The Solidity function signature changed from completeProcessing(uint256,bool,uint64,string) to completeProcessing(uint256,bool,uint64,string,uint256). The Cadence calldata here is consistent with the new Solidity signature and the artifact JSON is updated, so the code is internally coherent.
However, any previously deployed testnet instance of FlowYieldVaultsRequests (address 0xF633C9... in CLAUDE.md) must be re-deployed before this Cadence contract is upgraded, otherwise every call to completeProcessing will revert because the 4-arg selector no longer exists. A phased or atomic upgrade is needed.
| /// @param request The request being completed | ||
| /// @param success Whether the Cadence operation succeeded | ||
| /// @return Amount to credit to claimable refunds on the Solidity side | ||
| access(self) fun refundAmountForCompletion(request: EVMRequest, success: Bool): UInt256 { |
There was a problem hiding this comment.
No Cadence unit tests for refundAmountForCompletion
This function is the critical path that determines how much is sent to completeProcessing for every request completion. It has three distinct branches (non-escrowed, failed escrowed, successful escrowed with residual) and the ERC20 residual branch adds a round-trip through exactCadenceRepresentableAmount.
The new behaviour is covered by integration tests (run_worker_tests.sh) and Solidity unit tests, but there are no Cadence-level unit tests in cadence/tests/. Given that a wrong return value here either strands funds in the COA (residual not refunded) or bricks requests (wrong refund causes InvalidRefundAmount revert on the Solidity side), at minimum error_handling_test.cdc and evm_bridge_lifecycle_test.cdc should exercise the residual paths.
| return amount % 1e10; | ||
| } | ||
|
|
||
| uint8 decimals = IERC20Metadata(tokenAddress).decimals(); |
There was a problem hiding this comment.
The strict InvalidRefundAmount validation creates a hard coupling between this live decimals() call and whatever decimal count Cadence's FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount used when computing the residual. If the two sides ever disagree by even 1 token-unit — e.g., because the EVM bridge was configured with a decimal count different from what the token contract reports today — every successful completeProcessing call for that token will revert with InvalidRefundAmount.
The failure recovery path (success: false, refundAmount: request.amount) bypasses the precision computation entirely, so users still get a full refund; there is no permanent fund lock. However, every deposit/create attempt for the mismatched token would auto-fail, which would be a silent systematic outage for that token.
Worth adding a note in setTokenConfig or in admin docs that the token's on-chain decimals() must match the decimal count used in the Cadence bridge config for that token, or residual validation will always reject successful completions.
| if (decimals <= 8) return 0; | ||
|
|
||
| uint8 precisionLossDecimals = decimals - 8; | ||
| if (precisionLossDecimals > 77) return amount; |
There was a problem hiding this comment.
Guard against overflow in 10 ** precisionLossDecimals is correct (10^77 < 2^256 < 10^78), but the behavior when precisionLossDecimals > 77 (i.e., decimals > 85) is to treat the entire amount as un-representable and return amount as the residual. That means the Cadence side must also compute exactProcessableAmount = 0 (bridging nothing), otherwise refundAmountForCompletion on the Cadence side returns amount and _expectedPrecisionResidual on the Solidity side returns amount — they agree, so the call proceeds.
This is an edge-case path that no real-world token would hit (no token has 85+ decimals), but it's worth a short comment explaining the invariant: "for decimals > 85, the entire amount is residual because no value is representable in UFix64."
| FlowYieldVaultsRequests.InvalidRefundAmount.selector, | ||
| uint256(0), | ||
| refundAmount | ||
| ) |
There was a problem hiding this comment.
Two coverage gaps for the new InvalidRefundAmount validation that are worth adding here:
-
Failed WITHDRAW/CLOSE with
refundAmount != 0—_completeProcessingrequiresrefundAmount == 0for non-escrowed failed requests; currently there is no test confirmingInvalidRefundAmountis thrown when the COA accidentally passes a non-zero value. -
Successful CREATE/DEPOSIT with a ≤8-decimal token (e.g., 6-decimal USDC) —
_expectedPrecisionResidualreturns 0 for such tokens, socompleteProcessingmust be called withrefundAmount == 0. A test usingTestERC20withtokenDecimals_ = 6would confirm the contract acceptsrefundAmount = 0for a clean amount and rejects any non-zerorefundAmount.
| vm.prank(user); | ||
| c.claimRefund(address(token)); | ||
| assertEq(token.balanceOf(user), balanceBefore + residual); | ||
| assertEq(c.totalAccountedBalance(address(token)), 0); |
There was a problem hiding this comment.
Missing test: ERC20 failed CREATE/DEPOSIT with a dusty amount.
test_CompleteProcessing_FailureRefundsExactCreateAmountWithDust and the DEPOSIT equivalent cover native FLOW, and test_CompleteProcessing_RefundERC20Tokens covers ERC20 failure but uses a clean 5 ether (no sub-8-decimal remainder). There is no test that exercises the branch where !success && isEscrowedRequest with an ERC20 token whose amount contains a low-precision residual — i.e., the COA must return the full amount + dust, not just the _expectedPrecisionResidual.
Suggested addition:
function test_CompleteProcessing_FailureRefundsExactERC20AmountWithDust() public {
TestERC20 token = new TestERC20("Mock Token", "MOCK", 18);
uint256 residual = 123456789;
uint256 amount = 1 ether + residual;
vm.prank(c.owner());
c.setTokenConfig(address(token), true, 1 ether, false);
token.mint(user, amount);
vm.startPrank(user);
token.approve(address(c), amount);
uint256 reqId = c.createYieldVault(address(token), amount, VAULT_ID, STRATEGY_ID);
vm.stopPrank();
vm.startPrank(coa);
_startProcessingBatch(reqId);
token.approve(address(c), amount);
c.completeProcessing(reqId, false, c.NO_YIELDVAULT_ID(), "Cadence error", amount);
vm.stopPrank();
assertEq(c.getClaimableRefund(user, address(token)), amount);
assertEq(c.totalAccountedBalance(address(token)), amount);
}
|
|
||
| /// @notice Converts a UFix64 amount from Cadence to UInt256 for EVM | ||
| /// @dev For native FLOW: Uses 18 decimals (FLOW to attoflow conversion) | ||
| /// For ERC20: Uses FlowEVMBridgeUtils to look up token decimals |
There was a problem hiding this comment.
No Cadence unit tests exist for exactCadenceRepresentableAmount or refundAmountForCompletion. The integration path in run_worker_tests.sh covers the happy path with a 18-decimal native FLOW and a MOET ERC20 token, but the following cases are not tested at the unit level:
- Token with ≤ 8 decimals (e.g. USDC at 6 decimal) — should return 0 residual.
- Amount that is already exactly quantum-aligned — should return 0 residual, and a success-path
completeProcessingwithrefundAmount = 0must not revert. - Token with
decimals > 85(theprecisionLossDecimals > 77guard on the Solidity side, where the entire amount is residual). WITHDRAW_FROM_YIELDVAULT/CLOSE_YIELDVAULTrequests —refundAmountForCompletionmust always return 0 regardless of success/failure.
Adding a Cadence test file (e.g. cadence/tests/precision_residual_test.cdc) that covers these cases would protect against silent regressions when FlowEVMBridgeUtils conversion behaviour changes.
| requestId: scheduledRequestId, | ||
| workerTransactionId: request.workerTransactionId, | ||
| ) | ||
| continue |
There was a problem hiding this comment.
The continue fix is correct, but the failure path it protects is not regression-tested. Scenario 7 only exercises the path where markRequestAsFailed returns true (EVM completeProcessing succeeds). There is no test that forces completeProcessing to revert inside stopAll() and then verifies:
- The
StopAllMarkFailedSkippedevent is emitted. - The request remains in
scheduledRequests(not dropped). - A subsequent scheduler run retries finalization and succeeds.
Without this test, a future refactor that accidentally removes the continue would not be caught. An integration-level test that stubs or temporarily breaks the EVM contract during stopAll() and then restores it would be the most direct regression coverage.
| assert_tx_success "$MOET_CONFIG_OUTPUT" "MOET token config applied" | ||
|
|
||
| log_test "Approve MOET residual refunds from COA" | ||
| MOET_COA_APPROVAL_OUTPUT=$(approve_erc20_from_coa "$MOET_EVM_ADDRESS" "$FLOW_VAULTS_REQUESTS_CONTRACT" "123456789") |
There was a problem hiding this comment.
This pre-approval is superfluous. When the Cadence worker processes a MOET request with an ERC20 residual, completeProcessing (Cadence) issues its own approve(evmContractAddress, refundAmount) call right before the EVM completeProcessing transaction — which replaces this allowance. The 123456789 set here has no effect on the actual test outcome.
Keeping it creates two problems:
- It misleads future contributors into thinking a global pre-approval is required for ERC20 residual refunds to work.
- When
DEPOSIT_DUST_RESIDUAL = 987654321is later used, the mismatch (987654321 > 123456789) looks like a bug even though it is harmless.
Remove this block, or add a comment explaining that it is intentionally a no-op (and why it is kept).
| ); | ||
| c.completeProcessing{value: refundAmount}(reqId, true, 100, "YieldVault created", refundAmount); | ||
| vm.stopPrank(); | ||
| } |
There was a problem hiding this comment.
The two InvalidRefundAmount revert tests only exercise CREATE requests. There's no test verifying that a non-zero refundAmount is rejected for WITHDRAW or CLOSE completions.
Both request types go through the same validation path (isEscrowedRequest = false → expectedRefund = 0), but the branch is never exercised in the test suite. If the isEscrowedRequest check were accidentally widened, nothing would catch it.
Suggested additions:
function test_CompleteProcessing_RevertNonZeroRefundForWithdraw() public {
vm.prank(user);
uint256 reqId = c.withdrawFromYieldVault(42, 1 ether);
vm.startPrank(coa);
_startProcessingBatch(reqId);
vm.expectRevert(
abi.encodeWithSelector(
FlowYieldVaultsRequests.InvalidRefundAmount.selector,
uint256(0),
uint256(1)
)
);
c.completeProcessing(reqId, true, 42, "Withdrawn", 1);
vm.stopPrank();
}
function test_CompleteProcessing_RevertNonZeroRefundForClose() public {
vm.prank(user);
uint256 reqId = c.closeYieldVault(42);
vm.startPrank(coa);
_startProcessingBatch(reqId);
vm.expectRevert(
abi.encodeWithSelector(
FlowYieldVaultsRequests.InvalidRefundAmount.selector,
uint256(0),
uint256(1)
)
);
c.completeProcessing(reqId, true, 42, "Closed", 1);
vm.stopPrank();
}
| prepare(signer: auth(IssueStorageCapabilityController) &Account) { | ||
| self.schedulerHandlerCap = signer.capabilities.storage | ||
| .issue<auth(FlowTransactionScheduler.Execute) &{FlowTransactionScheduler.TransactionHandler}>( | ||
| FlowYieldVaultsEVMWorkerOps.SchedulerHandlerStoragePath |
There was a problem hiding this comment.
issue creates a new unrevoked capability controller on every invocation. Running this transaction repeatedly (as happens across test scenarios) accumulates orphaned controllers on the account.
Consider borrowing the existing controller if one exists, or revoking it before issuing a new one. A common pattern is:
let existing = signer.capabilities.storage.getControllers(forPath: FlowYieldVaultsEVMWorkerOps.SchedulerHandlerStoragePath)
if existing.length > 0 {
self.schedulerHandlerCap = existing[0].capability as! Capability<auth(FlowTransactionScheduler.Execute) &{FlowTransactionScheduler.TransactionHandler}>
} else {
self.schedulerHandlerCap = signer.capabilities.storage.issue<...>(...)
}For a test-only helper this won't cause test failures, but it's an antipattern that will make debugging account state harder over time.
| assert_tx_success "$MOET_CONFIG_OUTPUT" "MOET token config applied" | ||
|
|
||
| log_test "Approve MOET residual refunds from COA" | ||
| MOET_COA_APPROVAL_OUTPUT=$(approve_erc20_from_coa "$MOET_EVM_ADDRESS" "$FLOW_VAULTS_REQUESTS_CONTRACT" "123456789") |
There was a problem hiding this comment.
This static pre-approval ("123456789") is redundant. The Cadence worker already calls approve(refundAmount) inline inside completeProcessing (see FlowYieldVaultsEVM.cdc ~line 1075), so the COA approves the exact precision residual as part of the same atomic transaction.
The pre-approval happens to match MOET_CREATE_DUST_RESIDUAL, so the test passes, but it could silently mask a broken inline approval path and would fail for any residual amount other than 123456789.
Consider removing this pre-approval step so the test exercises the inline approval path end-to-end.
| return amount % 1e10; | ||
| } | ||
|
|
||
| uint8 decimals = IERC20Metadata(tokenAddress).decimals(); |
There was a problem hiding this comment.
decimals() revert will permanently trap successful ERC20 completions
IERC20Metadata(tokenAddress).decimals() is called inside completeProcessing. If a configured ERC20 token doesn't implement IERC20Metadata (the method is optional per EIP-20), this call reverts and every successful CREATE/DEPOSIT completion for that token will fail permanently, trapping those requests in PROCESSING state.
Notably the failure path (!success branch) never calls decimals(), so failures can be finalized but successes cannot — a deeply asymmetric stuck state with no clear recovery path.
Fix: validate decimals() is callable in setTokenConfig for non-native tokens:
| uint8 decimals = IERC20Metadata(tokenAddress).decimals(); | |
| uint8 decimals; | |
| try IERC20Metadata(tokenAddress).decimals() returns (uint8 d) { | |
| decimals = d; | |
| } catch { | |
| return 0; // treat as 0-residual (≤8 decimal behaviour) | |
| } |
Or, more robustly, gate setTokenConfig on a successful decimals() call so the contract never holds funds it cannot finalize.
| request.amount | ||
| ) | ||
| : 0; | ||
| if (refundAmount != expectedSuccessRefundAmount) { |
There was a problem hiding this comment.
Strict equality validation creates an unrecoverable stuck state if Cadence/Solidity residuals diverge
Both sides independently compute the residual: Solidity uses amount % 10^(decimals-8) (via _expectedPrecisionResidual), while Cadence uses FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount + inverse round-trip.
For native FLOW the math is provably identical. For ERC20 tokens, the Cadence bridge utility looks up decimals from the ERC20 on-chain at bridge time; if that lookup ever returns a value differing from IERC20Metadata(tokenAddress).decimals() called here (e.g., a proxy token whose decimals() returns different values depending on caller context, or a future upgrade), the two sides compute different residuals.
Because the check is != (exact match), any single-wei discrepancy causes completeProcessing to revert on every call, leaving the request permanently stuck in PROCESSING. There is currently no admin override for a PROCESSING request that can never complete.
Consider documenting the invariant explicitly and adding an admin forceFailProcessing escape hatch for the stuck-PROCESSING recovery scenario.
| uint256(1) | ||
| ) | ||
| ); | ||
| c.completeProcessing(reqId, true, 42, "Withdrawn", 1); |
There was a problem hiding this comment.
Test gap: success: false, refundAmount > 0 on WITHDRAW/CLOSE is untested
This test only covers success: true. The !success branch for a non-escrowed request type follows different code (expectedFailedRefundAmount = 0) and is not covered by any test. Add a variant:
// should also revert: failed WITHDRAW with a non-zero refundAmount
c.completeProcessing(reqId, false, 42, "Failed", 1);
| // Keep the request tracked if EVM finalization fails so crash recovery can retry later. | ||
| // Retries are currently unbounded until the underlying issue is resolved. | ||
| if success { | ||
| FlowYieldVaultsEVMWorkerOps.scheduledRequests.remove(key: requestId) |
There was a problem hiding this comment.
Unbounded retry burns scheduler capacity on every run
When markRequestFailed returns false (EVM call fails), the request stays in scheduledRequests indefinitely. On every subsequent scheduler run the same entry is re-checked, the EVM call fails again, and the slot is consumed but nothing progresses. With multiple such entries the effective scheduler capacity shrinks permanently.
The comment acknowledges this is unbounded. Consider adding a retry counter to ScheduledRequest and after N failures emitting a distinct MarkFailedExhausted event (or switching to a separate "needs-admin-review" set), so monitoring can surface the stuck entry without indefinitely degrading throughput.
| request.amount | ||
| ) | ||
| : 0; | ||
| if (refundAmount != expectedSuccessRefundAmount) { |
There was a problem hiding this comment.
Strict equality on precision residual can permanently lock funds
If the Cadence refundAmountForCompletion and the Solidity _expectedPrecisionResidual ever diverge by even 1 wei for a given request, this revert InvalidRefundAmount fires and there is no recovery path — the request remains stuck in PROCESSING forever (cannot be cancelled, cannot be retried by the COA, cannot be dropped by admin because dropRequests only touches PENDING). The user's escrowed funds would be permanently unrecoverable.
Divergence scenarios:
FlowEVMBridgeUtils.convertERC20AmountToCadenceAmounton the Cadence side uses the bridge's internal decimal registry;IERC20Metadata(token).decimals()on the Solidity side reads directly from the token. If the bridge registry and the token's owndecimals()differ (e.g., after a token upgrade or mis-registration), the two residuals will not agree.- A bug introduced in either computation path during a contract upgrade.
Suggested mitigations:
- Add an admin
rescueProcessingRequest(uint256 requestId)that can forcibly move a stuckPROCESSINGrequest toFAILEDwith a full refund — a last-resort escape hatch. - Or relax the success-path check to
refundAmount <= expectedSuccessRefundAmount(the COA is already trusted; over-refunding by a few wei is safer than locking funds).
The failure-path strict check (refundAmount == request.amount) does not have the same lock-in risk because dropRequests + a re-deposited COA balance can handle failures, but the success-path check has no such escape.
| return amount % 1e10; | ||
| } | ||
|
|
||
| uint8 decimals = IERC20Metadata(tokenAddress).decimals(); |
There was a problem hiding this comment.
IERC20Metadata.decimals() call on the hot path: reverts if token lacks it
_expectedPrecisionResidual is called inside the completeProcessing hot path (both success and failure branches for CREATE/DEPOSIT). For ERC20 tokens, it calls IERC20Metadata(tokenAddress).decimals() — a new external call not present before this PR.
If a supported token does not implement decimals() (e.g., certain non-standard ERC20s), this call reverts. The result: every completeProcessing for that token will revert, permanently locking the request in PROCESSING state (same no-escape scenario as the strict-equality issue above).
The fix is straightforward — cache the decimal value in TokenConfig when the token is registered (setTokenConfig):
struct TokenConfig {
bool isSupported;
uint256 minimumBalance;
bool isNative;
uint8 decimals; // cached at registration time
}Then read from storage here instead of making an external call. This also removes the gas cost of an external call on every completeProcessing invocation.
| return request.amount | ||
| } | ||
|
|
||
| let exactProcessableAmount = FlowYieldVaultsEVM.exactCadenceRepresentableAmount( |
There was a problem hiding this comment.
exactCadenceRepresentableAmount double-converts through FlowEVMBridgeUtils — decimal mismatch risk
The round-trip ufix64FromUInt256 → uint256FromUFix64 relies on FlowEVMBridgeUtils.convertERC20AmountToCadenceAmount / convertCadenceAmountToERC20Amount to determine how many decimal places the token has. The Solidity side uses IERC20Metadata(tokenAddress).decimals() for the same purpose.
These two sources of truth can diverge:
- If the FlowEVMBridge registry stores a decimal count that differs from what the ERC20 token's
decimals()returns (e.g., after a token upgrade or initial mis-registration), the residuals computed byrefundAmountForCompletion(Cadence) and_expectedPrecisionResidual(Solidity) will differ, causingcompleteProcessingto revert on Solidity.
Consider adding an assertion here (or in tests) that the bridge-registry decimal count equals the ERC20 decimals() result for every configured token, and document that this invariant must hold for the precision accounting to work correctly.
3 participants
Summary
Fixes #31.
This PR fixes cross-VM refund precision for CREATE/DEPOSIT completion and hardens worker recovery for failed requests.
It preserves exact native FLOW refunds on failed completion, credits any CREATE/DEPOSIT precision residual that Cadence cannot represent back to
claimableRefunds, and fixes failed non-escrowed recovery so requests are not orphaned inPROCESSING.What Changed
refundAmountduring Solidity completion.completeProcessingpath so completion uses a single explicit ABI.Scope
Test Plan
cd solidity && forge test./local/run_worker_tests.sh