FulfillInterpreter - Wrong order of actions in fallback handling
When a transaction with a `callTo` that is not `address(0)` is fulfilled, the funds to be withdrawn on the user's behalf are first transferred to the `FulfillInterpreter` instance that is associated with this `TransactionManager` instance. After that, `execute` is called on that interpreter instance, which, in turn, tries to make a call to `callTo`. If that call reverts or isn't made in the first place because `callTo` is not a contract address, the funds are transferred directly to the `receivingAddress` in the transaction (which becomes `fallbackAddress` in execute); otherwise, it's the called contract's task to transfer the previously approved funds from the interpreter.\\n```\\nbool isNative = LibAsset.isNativeAsset(assetId);\\nif (!isNative) {\\n  LibAsset.increaseERC20Allowance(assetId, callTo, amount);\\n}\\n\\n// Check if the callTo is a contract\\nbool success;\\nbytes memory returnData;\\nif (Address.isContract(callTo)) {\\n  // Try to execute the callData\\n  // the low level call will return `false` if its execution reverts\\n  (success, returnData) = callTo.call{value: isNative ? amount : 0}(callData);\\n}\\n\\n// Handle failure cases\\nif (!success) {\\n  // If it fails, transfer to fallback\\n  LibAsset.transferAsset(assetId, fallbackAddress, amount);\\n  // Decrease allowance\\n  if (!isNative) {\\n    LibAsset.decreaseERC20Allowance(assetId, callTo, amount);\\n  }\\n}\\n```\\n\\nFor the fallback scenario, i.e., the call isn't executed or fails, the funds are first transferred to `fallbackAddress`, and the previously increased allowance is decreased after that. If the token supports it, the recipient of the direct transfer could try to exploit that the approval hasn't been revoked yet, so the logically correct order is to decrease the allowance first and transfer the funds later. However, it should be noted that the `FulfillInterpreter` should, at any point in time, only hold the funds that are supposed to be transferred as part of the current transaction; if there are any excess funds, these are leftovers from a previous failure to withdraw everything that could have been withdrawn, so these can be considered up for grabs. Hence, this is only a minor issue.
We recommend reversing the order of actions for the fallback case: Decrease the allowance first, and transfer later. Moreover, it would be better to increase the allowance only in case a call will actually be made, i.e., if `Address.isContract(callTo)` is `true`.\\nRemark\\nThis issue was already present in the original version of the code but was missed initially and only found during the re-audit.
```\\nbool isNative = LibAsset.isNativeAsset(assetId);\\nif (!isNative) {\\n  LibAsset.increaseERC20Allowance(assetId, callTo, amount);\\n}\\n\\n// Check if the callTo is a contract\\nbool success;\\nbytes memory returnData;\\nif (Address.isContract(callTo)) {\\n  // Try to execute the callData\\n  // the low level call will return `false` if its execution reverts\\n  (success, returnData) = callTo.call{value: isNative ? amount : 0}(callData);\\n}\\n\\n// Handle failure cases\\nif (!success) {\\n  // If it fails, transfer to fallback\\n  LibAsset.transferAsset(assetId, fallbackAddress, amount);\\n  // Decrease allowance\\n  if (!isNative) {\\n    LibAsset.decreaseERC20Allowance(assetId, callTo, amount);\\n  }\\n}\\n```\\n