diff --git a/cadence/contracts/FlowYieldVaultsEVM.cdc b/cadence/contracts/FlowYieldVaultsEVM.cdc index 5611b57..64cf72f 100644 --- a/cadence/contracts/FlowYieldVaultsEVM.cdc +++ b/cadence/contracts/FlowYieldVaultsEVM.cdc @@ -1007,7 +1007,8 @@ access(all) contract FlowYieldVaultsEVM { /// @notice Marks a request as COMPLETED or FAILED, returning escrowed funds on failure /// @dev For failed CREATE/DEPOSIT: returns funds from COA to EVM contract via msg.value (native) - /// or approve+transferFrom (ERC20). For WITHDRAW/CLOSE or success: no refund sent. + /// or approve+transferFrom (ERC20). ERC20 approve return data is validated when present. + /// For WITHDRAW/CLOSE or success: no refund sent. /// @param requestId The request ID to complete /// @param success Whether the operation succeeded /// @param yieldVaultId The associated YieldVault Id @@ -1076,6 +1077,19 @@ access(all) contract FlowYieldVaultsEVM { ) return false } + + if !FlowYieldVaultsEVM.isERC20BoolReturnSuccess(approveResult.data) { + emit RequestFailed( + requestId: requestId, + userAddress: "", + requestType: requestType, + tokenAddress: tokenAddress.toString(), + amount: refundAmount, + yieldVaultId: yieldVaultId, + reason: "ERC20 approve for refund returned false or invalid bool data" + ) + return false + } } } @@ -1186,7 +1200,8 @@ access(all) contract FlowYieldVaultsEVM { /// @notice Bridges a Cadence vault to ERC20 tokens on EVM and transfers to recipient /// @dev Uses COA.depositTokens() to bridge tokens to EVM, then calls ERC20.transfer() - /// to send the tokens to the recipient address. + /// to send the tokens to the recipient address. ERC20 transfer return data is validated + /// when present to ensure semantic success. /// @param vault The Cadence vault containing tokens to bridge (will be consumed) /// @param recipient The EVM address to receive the tokens /// @param tokenAddress The ERC20 token address on EVM @@ -1222,6 +1237,10 @@ access(all) contract FlowYieldVaultsEVM { let errorMsg = FlowYieldVaultsEVM.decodeEVMError(transferResult.data) panic("ERC20 transfer to recipient failed: \(errorMsg)") } + + if !FlowYieldVaultsEVM.isERC20BoolReturnSuccess(transferResult.data) { + panic("ERC20 transfer to recipient returned false or invalid bool data") + } } // ============================================ @@ -1990,6 +2009,29 @@ access(all) contract FlowYieldVaultsEVM { return "EVM revert data: 0x\(String.encodeHex(data))" } + /// @notice Validates ERC20 bool return data semantics + /// @dev ERC20 methods may return no data (accepted for compatibility) or ABI-encoded bool. + /// When return data is present, it must be exactly 32 bytes with value `1` (true). + access(all) view fun isERC20BoolReturnSuccess(_ data: [UInt8]): Bool { + if data.length == 0 { + return true + } + + if data.length != 32 { + return false + } + + var i = 0 + while i < 31 { + if data[i] != 0 { + return false + } + i = i + 1 + } + + return data[31] == 1 + } + /// @notice Emits the RequestFailed event and returns a ProcessResult with success=false /// @dev This is a helper function to emit the RequestFailed event and return a ProcessResult with success=false /// @param request The EVM request that failed diff --git a/cadence/tests/error_handling_test.cdc b/cadence/tests/error_handling_test.cdc index 5ac3ad1..bf00615 100644 --- a/cadence/tests/error_handling_test.cdc +++ b/cadence/tests/error_handling_test.cdc @@ -200,3 +200,43 @@ fun testRequestStatusFailedStructure() { Test.assertEqual(FlowYieldVaultsEVM.RequestStatus.FAILED.rawValue, failedRequest.status) Test.assertEqual("Insufficient balance", failedRequest.message) } + +access(all) +fun testMarkRequestAsFailedRejectsFalseApproveRefunds() { + // completeProcessing only needs the configured requests contract address as the + // spender for approve(address,uint256). The call should fail before any request + // contract interaction, so a dummy address is enough for this regression path. + let requestsResult = updateRequestsAddress(admin, mockRequestsAddr.toString()) + Test.expect(requestsResult, Test.beSucceeded()) + + let requestFailedCountBefore = Test.eventsOfType(Type()).length + // Deploy a minimal ERC20-shaped contract that returns false from approve(...) + // without reverting, matching the bug class fixed by this PR. + let falseApproveTokenAddress = deployFalseApproveToken(admin) + + // The transaction itself asserts that markRequestAsFailed(...) returns false after + // the refund approval check, rather than silently succeeding. + let markFailedResult = _executeTransaction( + "transactions/mark_request_as_failed_false_approve.cdc", + [falseApproveTokenAddress], + admin + ) + Test.expect(markFailedResult, Test.beSucceeded()) + + let requestFailedEvents = Test.eventsOfType(Type()) + Test.assert( + requestFailedEvents.length > requestFailedCountBefore, + message: "Expected RequestFailed events for the approve(false) refund path" + ) + + // markRequestAsFailed emits one RequestFailed immediately. The last event should + // be the follow-up failure emitted by completeProcessing after approve(false). + let lastEvent = requestFailedEvents[requestFailedEvents.length - 1] as! FlowYieldVaultsEVM.RequestFailed + let expectedTokenAddress = EVM.addressFromString(falseApproveTokenAddress).toString() + Test.assertEqual(4242 as UInt256, lastEvent.requestId) + Test.assertEqual(expectedTokenAddress, lastEvent.tokenAddress) + Test.assertEqual( + "ERC20 approve for refund returned false or invalid bool data", + lastEvent.reason + ) +} diff --git a/cadence/tests/test_helpers.cdc b/cadence/tests/test_helpers.cdc index ff17511..7966c14 100644 --- a/cadence/tests/test_helpers.cdc +++ b/cadence/tests/test_helpers.cdc @@ -14,6 +14,9 @@ access(all) let admin = Test.getAccount(0x0000000000000007) // testing alias access(all) let mockRequestsAddr = EVM.addressFromString("0x0000000000000000000000000000000000000002") access(all) let nativeFlowAddr = EVM.addressFromString("0xFFfFfFffFFfffFFfFFfFFFFFffFFFffffFfFFFfF") +// Creation bytecode for solidity/src/test/FalseApproveToken.sol. It is embedded so +// the Cadence test suite can deploy the mock without depending on a Forge build step. +access(all) let falseApproveTokenBytecode = "60808060405234601457608690816100198239f35b5f80fdfe60808060405260043610156011575f80fd5b5f90813560e01c63095ea7b3146025575f80fd5b34604c576040366003190112604c576004356001600160a01b03811603604c576020918152f35b5080fdfea2646970667358221220c128b17595d998b699a78b40cad92d7f4abc61b14795de2abd078778dd30397164736f6c63430008140033" /* --- Mock Vault and Strategy Identifiers --- */ @@ -263,6 +266,35 @@ fun setupCOA(_ signer: Test.TestAccount): Test.TransactionResult { ) } +access(all) +fun deployEVMContract(_ signer: Test.TestAccount, _ bytecode: String, _ gasLimit: UInt64): String { + let deployResult = _executeTransaction( + "../transactions/deploy_evm_contract.cdc", + [bytecode, gasLimit], + signer + ) + Test.expect(deployResult, Test.beSucceeded()) + + // The deployment transaction itself has no return value, so the deployed address + // is recovered from the latest EVM.TransactionExecuted event. + let txnEvents = Test.eventsOfType(Type()) + Test.assert(txnEvents.length > 0, message: "Expected an EVM.TransactionExecuted event after deployment") + + let evt = txnEvents[txnEvents.length - 1] as? EVM.TransactionExecuted + ?? panic("Latest event is not EVM.TransactionExecuted") + let contractAddress = evt.contractAddress + Test.assert(contractAddress.length > 0, message: "Deployed contract address should not be empty") + + return contractAddress +} + +access(all) +fun deployFalseApproveToken(_ signer: Test.TestAccount): String { + // Reuse the generic deploy helper so the regression test exercises the same COA + // deployment path we use for other EVM fixtures. + return deployEVMContract(signer, falseApproveTokenBytecode, UInt64(15_000_000)) +} + /* --- FlowYieldVaultsEVM specific script helpers --- */ access(all) diff --git a/cadence/tests/transactions/mark_request_as_failed_false_approve.cdc b/cadence/tests/transactions/mark_request_as_failed_false_approve.cdc new file mode 100644 index 0000000..ef7d4cf --- /dev/null +++ b/cadence/tests/transactions/mark_request_as_failed_false_approve.cdc @@ -0,0 +1,35 @@ +import "EVM" +import "FlowYieldVaultsEVM" + +transaction(tokenAddress: String) { + prepare(signer: auth(BorrowValue) &Account) { + let worker = signer.storage.borrow<&FlowYieldVaultsEVM.Worker>( + from: FlowYieldVaultsEVM.WorkerStoragePath + ) ?? panic("Could not borrow FlowYieldVaultsEVM worker") + + // Any failed CREATE or DEPOSIT request with a non-native token and a positive + // amount enters the refund approval path inside completeProcessing(...). + let request = FlowYieldVaultsEVM.EVMRequest( + id: 4242, + user: EVM.addressFromString("0x0000000000000000000000000000000000000099"), + requestType: FlowYieldVaultsEVM.RequestType.CREATE_YIELDVAULT.rawValue, + status: FlowYieldVaultsEVM.RequestStatus.PROCESSING.rawValue, + tokenAddress: EVM.addressFromString(tokenAddress), + amount: 1, + yieldVaultId: nil, + timestamp: 0, + message: "", + vaultIdentifier: "", + strategyIdentifier: "" + ) + + // The regression is fixed only if this returns false after approve(false) + // instead of letting the request look successfully completed. + let result = worker.markRequestAsFailed( + request, + message: "Synthetic failure before refund approval" + ) + + assert(!result, message: "Expected approve(false) refund path to return false") + } +} diff --git a/solidity/src/test/FalseApproveToken.sol b/solidity/src/test/FalseApproveToken.sol new file mode 100644 index 0000000..15e741b --- /dev/null +++ b/solidity/src/test/FalseApproveToken.sol @@ -0,0 +1,10 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.20; + +contract FalseApproveToken { + // Minimal weird-token fixture: the call succeeds at the EVM level but returns + // `false`, which is the exact behavior the Cadence regression test needs. + function approve(address, uint256) external pure returns (bool) { + return false; + } +}