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 Summary

The fix is correct and necessary — approve() returning false was silently ignored before, and the new isERC20BoolReturnSuccess helper properly validates ABI-encoded bool return data while preserving compatibility with non-returning tokens (USDT-style). Three issues worth addressing:

1. Missing test for the bridgeERC20ToEVM false-return transfer path (medium)

The approve-false path in completeProcessing gets a dedicated regression test (testMarkRequestAsFailedRejectsFalseApproveRefunds). The symmetric guard added to bridgeERC20ToEVM — covering a token whose transfer() returns false on the WITHDRAW happy path — has no corresponding test. A FalseTransferToken fixture and a test that processes a WITHDRAW request against it would close this gap.

2. isERC20BoolReturnSuccess visibility (access(all)) (minor)

The helper is a pure validation function with no side effects, so access(all) carries no security risk. However, it widens the contract's public API where access(self) would suffice. Compare with decodeEVMError which is access(self). If it needs to remain script-callable for test assertions, that's fine — just worth documenting as intentional.

3. Embedded bytecode drift (minor)

falseApproveTokenBytecode is a hand-compiled constant in test_helpers.cdc. If FalseApproveToken.sol is modified (e.g. to add a transfer() that returns false for the missing test above), the embedded bytes will silently diverge from the source. A comment pinning the solc version and a checksum, or a CI verification step, would catch this.

Not flagged

• The double RequestFailed emission from markRequestAsFailed + completeProcessing is pre-existing behavior and the test comment explicitly acknowledges it — no change needed.
• The needsRefund && approve returns false → request stays PROCESSING scenario is identical to the pre-existing behavior when the Solidity completeProcessing call itself reverts; not a regression.
• isERC20BoolReturnSuccess correctly checks only when status == EVM.Status.successful, so revert data never reaches it.

[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]

access(all) makes this callable by any script or external transaction. It's a pure validation helper with no side effects, so there's no security risk, but it unnecessarily widens the contract's public API. Compare with decodeEVMError which is access(self). Consider access(self) or access(contract) here; if it needs to remain callable from scripts for test assertions, access(all) view is acceptable but worth documenting as intentional.

[claude]

The creation bytecode is hand-embedded and will silently drift if FalseApproveToken.sol is ever edited (e.g. to add transfer for the symmetric test case). Consider adding a comment that pins the expected solc version and a checksum, or a CI step that rebuilds and compares. As-is, a source change won't surface until a test is run against the live emulator.