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/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

• env FOUNDRY_OFFLINE=true forge test --match-contract FlowYieldVaultsRequestsTest --threads 1
• flow test cadence/tests/evm_bridge_lifecycle_test.cdc (fails in current environment due unresolved Cadence dependency/import setup, e.g. missing DeFiActions declaration in imported FlowYieldVaults)

[claude]

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

This fix correctly addresses the semantic gap between EVM-level call success and ERC20 boolean return values. The overall approach is sound. Below are observations from most to least significant.

---

Issues

1. Requests can get stuck in PROCESSING state when approve returns false

Severity: Medium (pre-existing behavior extended to a new path)

When approve returns false at line 1108–1119, completeProcessing() returns false without ever calling the Solidity completeProcessing(). But startProcessing() was already called earlier in the flow, leaving the request in PROCESSING state permanently on the Solidity side.

// If this returns false, the request is stuck in PROCESSING on the Solidity side
if !FlowYieldVaultsEVM.isERC20BoolReturnSuccess(approveResult.data) {
    emit RequestFailed(...)
    return false  // Solidity completeProcessing() never called
}

The existing EVM-level approveResult.status check at line 1094 has the same behavior, so this PR doesn't introduce a regression — but it adds a second code path with the same silent-stuck-request risk. Consider whether completeProcessing() should call the Solidity contract with success: false even when the refund mechanism fails, so requests never get stranded.

---

2. Approved allowance leaks on downstream failure

Severity: Low (pre-existing, unrelated to this PR)

If approve passes (both EVM status and bool check), but the subsequent Solidity completeProcessing() call fails at line 1130, the token allowance for flowYieldVaultsRequestsAddress remains set. An attacker who can craft a transaction that passes the approve check but fails completeProcessing() gains an unspent allowance. Worth noting for future hardening (e.g., approve(..., 0) cleanup on failure).

---

3. Error-handling asymmetry between completeProcessing and bridgeERC20ToEVM

Severity: Informational

completeProcessing uses graceful return false when approve returns false; bridgeERC20ToEVM uses panic when transfer returns false. This is internally consistent (panic already existed for EVM-level transferResult.status failures), but it's worth documenting why panic is correct here.

Since depositTokens (line 1246) and coaRef.call (line 1256) are both EVM operations within the same Cadence transaction, a panic atomically reverts both — so the tokens aren't lost. The doc comment could note this explicitly to avoid future confusion.

---

Correctness of isERC20BoolReturnSuccess

The implementation is correct:

access(self) fun isERC20BoolReturnSuccess(_ data: [UInt8]): Bool {
    if data.length == 0 { return true }   // ✓ non-standard ERC20 (e.g., USDT) compat
    if data.length != 32 { return false } // ✓ rejects malformed data
    var i = 0
    while i < 31 {
        if data[i] != 0 { return false }  // ✓ checks padding bytes
        i = i + 1
    }
    return data[31] == 1                  // ✓ ABI bool: 0x00...01 = true
}

ABI-encoded bool true is 0x0000...0001 (32 bytes, big-endian), and the loop correctly validates bytes 0–30 are zero and byte 31 is exactly 1. The access(self) contract-level placement is appropriate for a pure utility function.

Minor style note: the while loop could use for i in 0..<31 in newer Cadence, but the current form is readable and correct.

---

Test Coverage Gap

This is the most actionable concern. Neither evm_bridge_lifecycle_test.cdc nor error_handling_test.cdc contains any test for:

• isERC20BoolReturnSuccess unit tests (empty data, 32-byte true, 32-byte false, wrong length, non-zero padding)
• approve returning false triggering RequestFailed
• transfer returning false triggering panic/revert

The PR notes the Cadence test environment is broken. Before merge, at minimum isERC20BoolReturnSuccess should have unit tests since it's pure logic with no external dependencies. Suggested cases:

// isERC20BoolReturnSuccess unit tests
assert(isERC20BoolReturnSuccess([]))                      // empty → true
assert(isERC20BoolReturnSuccess([0,0,...,0,1]))           // 32-byte true → true
assert(!isERC20BoolReturnSuccess([0,0,...,0,0]))          // 32-byte false → false
assert(!isERC20BoolReturnSuccess([0,0,...,0,2]))          // 32-byte 2 → false
assert(!isERC20BoolReturnSuccess([1,0,...,0,1]))          // non-zero padding → false
assert(!isERC20BoolReturnSuccess([0,1]))                  // 2 bytes → false
assert(!isERC20BoolReturnSuccess([0,0,...,0,1,0]))        // 33 bytes → false

---

Summary

Area	Assessment
Core logic correctness	✅ Correct
ABI bool decoding	✅ Correct (empty data, 32-byte true/false)
Non-standard ERC20 compatibility	✅ Handled
Error message clarity	✅ Good
Stuck PROCESSING state risk	⚠️ Pre-existing, new path added
Allowance leak on downstream failure	⚠️ Pre-existing
Cadence test coverage	❌ Missing — blocks confidence in the fix

The fix is directionally correct and the validation logic is sound. Main ask: add unit tests for isERC20BoolReturnSuccess and integration tests for the new failure paths before merging.