recoverTokens() Uses Unsafe transfer() — Silent Failure on Non-Reverting Tokens

Description

• The ERC20 standard specifies that transfer() returns a bool indicating success or failure. However, many widely-used tokens (USDT, BNB, OMG, etc.) return false on failure instead of reverting. If the return value is not checked, a failed transfer appears as a successful transaction.

• recoverTokens() is the only mechanism to rescue stuck or surplus tokens from the Stratax contract, and it does not check the return value:

Risk

Likelihood:

• USDT is the most widely used stablecoin and is explicitly listed as a standard ERC20 token compatible with Aave V3

• Any scenario where USDT or similar tokens end up in the contract (surplus from unwind, accidental transfer, dust from swaps) will trigger this issue when recovery is attempted

• The owner has no on-chain indication that the recovery failed

Impact:

• Tokens that return false instead of reverting will remain stuck in the contract permanently

• The owner sees a successful transaction on-chain and believes the tokens were recovered — they have no reason to retry

• recoverTokens() is the only way to rescue tokens from the contract (there is no withdrawFromAave() or similar function), making this the single point of failure for token recovery

Real-World Precedent:

• USDT's transfer() returns false on failure (e.g., when the sender is blacklisted)

• This is one of the most common ERC20 integration bugs — OpenZeppelin created SafeERC20 specifically to address it

• Multiple protocols have lost funds due to unchecked transfer() return values

Proof of Concept

How the issue manifests:

1. Surplus USDT accumulates in the Stratax contract (e.g., from an unwind operation that supplied excess debt tokens to Aave, then the aTokens were somehow transferred to the contract)

2. Owner calls recoverTokens(USDT, amount) to rescue the tokens

3. USDT's transfer() encounters an internal failure condition (e.g., the Stratax contract is blacklisted on USDT, or the amount exceeds the balance) and returns false

4. Since the return value is not checked, the transaction completes successfully on-chain

5. No tokens are transferred, but the transaction shows as successful

6. Owner believes the recovery worked and does not retry

7. The USDT remains permanently stuck in the contract

PoC code:

Expected outcome: The recoverTokens() call completes without revert even though no tokens were transferred. The owner has no indication of failure.

Recommended Mitigation

The root cause is that IERC20.transfer() returns a bool that is not checked. Solidity does not automatically revert on a false return value — it must be explicitly handled. OpenZeppelin's SafeERC20 library wraps all ERC20 calls to handle both non-reverting tokens (check return value) and non-returning tokens (check call success).

Primary fix — Use SafeERC20:

Why this works:

• safeTransfer() checks the return value of transfer() and reverts if it returns false

• It also handles tokens that don't return a value at all (non-standard implementations like early USDT)

• If the transfer fails for any reason, the transaction reverts, clearly signaling to the owner that recovery was unsuccessful

• The gas overhead is minimal (~200 gas for the return value check)

Note: The same SafeERC20 pattern should also be applied to all other IERC20.transfer() and IERC20.transferFrom() calls in the contract (e.g., createLeveragedPosition line 325 uses transferFrom without safety checks). However, those calls are within flash loan callbacks where a failure would cause the entire atomic operation to revert anyway, making them lower priority.