fix(FLOW-13): validate ERC20 call return semantics

[liobrasil]

Summary

Fixes #28

Validates ERC20 call return semantics in the Cadence worker so EVM-level call success is not treated as token-operation success when ERC20 methods explicitly return false.

Key points:

• completeProcessing() now validates ERC20 approve(address,uint256) return data when present
• bridgeERC20ToEVM() now validates ERC20 transfer(address,uint256) return data when present
• Empty return data is still accepted for compatibility with non-standard ERC20 tokens
• Malformed or non-boolean return payloads are treated as failure

Changes

• Added isERC20BoolReturnSuccess(_ data: [UInt8]): Bool
• On failed refund path, approve that returns false (or invalid bool data) now emits RequestFailed and aborts completion
• ERC20 transfer to recipient now panics if return data indicates semantic failure
• Updated function docs to clarify return-data validation behavior

Test plan

• ./local/run_solidity_tests.sh
• ./local/run_cadence_tests.sh

[claude]

Review: fix(FLOW-13) — validate ERC20 call return semantics

The core logic is correct. isERC20BoolReturnSuccess properly implements ABI-encoded bool validation (empty → accepted for non-standard ERC20 compatibility; exactly 32 bytes of 0x000...001 → true; anything else → false). Placement of the new checks is correct in both completeProcessing (approve-returns-false on the refund path) and bridgeERC20ToEVM (transfer-returns-false on the withdrawal path). Atomicity is preserved — the panic in bridgeERC20ToEVM reverts all EVM state including the preceding depositTokens call.

Issues

1. testERC20TransferFalseReturnIsRejected does not exercise bridgeERC20ToEVM (inline, error_handling_test.cdc:316)

The test only confirms isERC20BoolReturnSuccess correctly classifies the raw return bytes from a direct EVM call. The actual new code path — if !FlowYieldVaultsEVM.isERC20BoolReturnSuccess(transferResult.data) { panic(...) } inside bridgeERC20ToEVM — is never invoked by any test. A future refactor could silently drop the panic check with no failing test.

2. Hardcoded bytecode can silently drift from FalseApproveToken.sol (inline, test_helpers.cdc:22)

The comment asks for manual regeneration but there is no CI enforcement. An update to the Solidity source without regenerating the hex will deploy the old contract in tests without any compile-time or test-time signal.

Non-issues noted

• The empty-userAddress in the new RequestFailed emission is consistent with the existing pattern directly above it — completeProcessing does not receive the user address.
• The "request stuck in PROCESSING" concern for the approve(false) path is pre-existing: before this PR the same outcome occurred via safeTransferFrom failing in Solidity and being caught in the same completeProcessing return-false branch. No regression introduced.
• processRequest correctly panics when completeProcessing returns false (line 640), triggering crash recovery. markRequestAsFailed intentionally does not re-panic.

[claude]

No Cadence tests cover the two new failure paths added in this PR:

1. completeProcessing() — ERC20 approve returns false (semantic failure) on the refund path
2. bridgeERC20ToEVM() — ERC20 transfer returns false (semantic failure)

Both paths are reachable in production and the fix is security-relevant, so they deserve explicit test coverage. Suggested additions to error_handling_test.cdc (or a new erc20_return_semantics_test.cdc):

• A mock ERC20 whose approve returns ABI-encoded false → assert completeProcessing returns false and emits RequestFailed
• A mock ERC20 whose transfer returns ABI-encoded false → assert the WorkerHandler transaction panics/reverts
• Unit cases for isERC20BoolReturnSuccess directly: empty data, 32-byte true, 32-byte false, <32 non-empty bytes, >32 bytes, last byte = 2

The function itself (isERC20BoolReturnSuccess) is access(self), so unit tests must go through the integration path or the function visibility should be temporarily widened for testing (e.g., access(contract) + test helper, or a test-only view script).

[claude]

Double RequestFailed emission in the crash-recovery path.

completeProcessing is called from two places:

1. processRequest – if this block fires and returns false, processRequest immediately panics (line ~640), which rolls back the entire transaction including this RequestFailed event. The emission is effectively dead code in that path (same pre-existing pattern on the approveResult.status branch above).

2. markRequestAsFailed (crash-recovery) – here there is no downstream panic. markRequestAsFailed already emits RequestFailed before calling completeProcessing. If approve returns false and this block fires, a second RequestFailed event for the same requestId is written on-chain and persists. Combined with the event already emitted by markRequestAsFailed, observers see two failure events for one request.

The pre-existing approveResult.status != EVM.Status.successful branch has the identical problem; this PR extends the pattern. The cleanest fix is to suppress the emit inside completeProcessing and let the single emission from markRequestAsFailed stand, or to add a guard that skips the emit when the refund-approval path is entered from crash-recovery.

[claude]

access(all) on an internal helper

isERC20BoolReturnSuccess is only called internally by this contract. Exposing it as access(all) unnecessarily widens the public surface. Consider access(contract) (or at most access(account)).

Suggested change

	access(all) view fun isERC20BoolReturnSuccess(_ data: [UInt8]): Bool {
	access(contract) view fun isERC20BoolReturnSuccess(_ data: [UInt8]): Bool {

[claude]

Compatibility: any return data not exactly 32 bytes is treated as failure

The guard correctly handles the common non-standard-token pattern (empty return = success) and the standard ABI-encoded bool (32 bytes). However, a small class of tokens returns extra data beyond the canonical 32 bytes (e.g. some proxy wrappers or multi-return-value variants). Those would hit this branch and be treated as a semantic failure, causing either a stuck request (approve path) or a panic + full revert (transfer path).

If the intent is to be strict, this is fine as documented. But it should at least be called out in the doc comment so operators know which tokens are compatible. Consider mentioning it in the @dev block above:

Tokens that return non-empty data other than a 32-byte ABI-encoded bool are unsupported and treated as failure.

[claude]

No unit tests for this pure function

isERC20BoolReturnSuccess is pure and has several discrete edge cases, all of which can be exercised without an EVM or full stack:

input	expected
[] (empty)	true
32 bytes, last = 1, rest = 0	true
32 bytes, last = 0, rest = 0	false (approve returned false)
32 bytes, a middle byte is non-zero	false (malformed)
31 bytes	false
33 bytes	false

None of these are covered in error_handling_test.cdc or validation_test.cdc. A dedicated unit test (or additions to an existing test file) would make regressions here immediately visible. The function being access(all) (see other comment) makes direct invocation from tests straightforward.

[claude]

Approve false-return leaves request in PROCESSING (same as pre-existing status-failure path)

When this branch returns false, completeProcessing() on the EVM contract is never called, so the request remains in PROCESSING status indefinitely. This behaviour already existed for the approveResult.status != successful path above, so this is not a regression introduced here.

Worth noting: the SchedulerHandler crash-recovery path (which marks panicked transactions as FAILED) does not trigger on a clean return false. If the approve consistently returns false for a given token, the request will be permanently stuck unless manually recovered. A follow-up to propagate a failure status update to the EVM contract even in this path may be worth tracking.

[claude]

Duplicate RequestFailed event when called from markRequestAsFailed

markRequestAsFailed calls emitRequestFailed(request, message) on line 663 before calling completeProcessing. When the false-approve branch is hit, this block emits a second RequestFailed for the same requestId — with userAddress: "" instead of the real user address.

Off-chain indexers listening for RequestFailed will see two events for the same request; the second one clobbers the correct user address with an empty string.

In the processRequest path this doesn't matter because the subsequent panic on line 640 causes the transaction to revert, discarding all emitted events. But in the markRequestAsFailed path the transaction succeeds, so both events are persisted.

The existing test (testMarkRequestAsFailedRejectsFalseApproveRefunds) validates only requestFailedEvents.length > requestFailedCountBefore and checks the last event, so it actually asserts against the duplicate — masking this bug.

Fix: guard the emit (and the early return false) with a flag indicating that the caller already emitted the event, or restructure so completeProcessing never emits RequestFailed directly and always delegates event emission to its callers.

[claude]

access(all) should be access(contract) (or access(self))

Every other contract-level helper in this file (decodeEVMError, uint256FromUFix64, balanceFromUFix64, ufix64FromUInt256, emitRequestFailed, …) is access(self). Marking this one access(all) unnecessarily widens the public API surface and is inconsistent.

Suggested change

	access(all) view fun isERC20BoolReturnSuccess(_ data: [UInt8]): Bool {
	access(contract) view fun isERC20BoolReturnSuccess(_ data: [UInt8]): Bool {

[claude]

No test coverage for the transfer-false panic path

The approve-false refund path gets a dedicated test (testMarkRequestAsFailedRejectsFalseApproveRefunds), but this new branch — which panics and reverts the entire withdrawal transaction — has no test. It is the higher-severity failure mode: it leaves funds in the COA (deposited by depositTokens above) until the transaction is rolled back, and it would trigger crash recovery in SchedulerHandler.

A minimal test would deploy an analogous FalseTransferToken (same pattern as FalseApproveToken but with a transfer that returns false) and verify that a WITHDRAW_FROM_YIELDVAULT request using that token causes the worker transaction to panic.

[claude]

Missing test for the transfer false-return path.

The PR adds this panic branch for false-returning transfer(), but the test suite only exercises the approve false-return path (via markRequestAsFailed → completeProcessing). FalseApproveToken.sol only implements approve, so there is currently no coverage for this new code path.

A false-returning transfer() occurs in bridgeFundsToEVMUser → bridgeERC20ToEVM, which is the withdrawal path (WITHDRAW_FROM_YIELDVAULT and CLOSE_YIELDVAULT). The panic here causes the whole WorkerHandler transaction to revert, leaving the request in PROCESSING state until crash recovery marks it FAILED. That behaviour is intentional, but a regression test with a FalseTransferToken fixture would confirm the check fires and doesn't silently succeed.

[claude]

Hardcoded bytecode is not validated against the Solidity source.

falseApproveTokenBytecode was compiled from solidity/src/test/FalseApproveToken.sol at a specific point in time, but there is no CI step that re-compiles the contract and checks the two stay in sync. If FalseApproveToken.sol is changed (e.g. compiler version bump, optimizer setting, or adding a transfer stub for the missing-test case above), this hex string will silently use stale bytecode and the test will continue to pass against the wrong contract.

Consider adding a note in FalseApproveToken.sol linking to this constant and/or a CI assertion that the compiled creation bytecode matches the embedded string.

[claude]

No regression test covers this new branch. The approve-false path gets a dedicated testMarkRequestAsFailedRejectsFalseApproveRefunds test, but the symmetric case here — a token whose transfer() returns false — has no corresponding test. Because bridgeERC20ToEVM is reached on the happy path of WITHDRAW requests (not the refund path), a FalseTransferToken fixture and a matching test that triggers a WITHDRAW against it would be needed to give this guard the same confidence as the approve check.

[claude]

The test name implies the transfer-returns-false case is rejected during a withdrawal, but the test only verifies isERC20BoolReturnSuccess rejects the raw return data via a direct EVM call — it never invokes bridgeERC20ToEVM. The actual code path added by this PR (if !FlowYieldVaultsEVM.isERC20BoolReturnSuccess(transferResult.data) { panic(...) } in bridgeERC20ToEVM) is exercised nowhere in the test suite.

Consider adding a test that drives a full WITHDRAW_FROM_YIELDVAULT request (or at minimum calls bridgeFundsToEVMUser / bridgeERC20ToEVM directly) with falseApproveToken as the ERC20 and asserts the transaction fails. Without it, a future refactor could accidentally drop the panic check and no test would catch the regression.

[claude]

The comment says to regenerate this manually from solidity/foundry.toml defaults, but there is no CI check enforcing that. If FalseApproveToken.sol is updated (e.g., to add a transferFrom stub for a future test path) without regenerating the bytecode, the tests will silently deploy the old contract. The mismatch won't cause a compile error — it will just quietly test the wrong behavior.

One lightweight guard: add a comment with the compiler version and a short content hash of FalseApproveToken.sol used to produce this bytecode, so a reviewer (or CI script) can spot staleness at a glance.