Stratax Contracts
First Flight #57
Beginner FriendlyDeFi
100 EXP
View all submissions
Previous Next
Submission Details
Impact: medium
Likelihood: medium

M02. recoverTokens Uses Unchecked transfer Return Value

Author Revealed upon completion

Root + Impact

Description

  • recoverTokens calls IERC20(_token).transfer(owner, _amount) directly without checking the return value and without using SafeERC20. Some ERC-20 tokens (notably USDT on Ethereum mainnet) return false on failure instead of reverting. The transfer silently fails, the owner believes the recovery succeeded, and the tokens remain in the contract.

// src/Stratax.sol:282-284
​
function recoverTokens(address _token, uint256 _amount) external onlyOwner {
// @> No return value check; no SafeERC20 usage
// For tokens like USDT that return false on failure, this silently does nothing
IERC20(_token).transfer(owner, _amount);
}

Risk

Likelihood:

  • USDT is an extremely common token that returns false on failure rather than reverting — it is a likely candidate for accidental contract deposits

  • Any amount overflow or balance shortfall (e.g., trying to recover more than the balance) produces a silent no-op

Impact:

  • The owner calls recoverTokens believing funds were recovered; the transaction succeeds with a Transfer event from the token that may not fire (USDT does not emit on failed transfers), leaving no on-chain evidence of failure

  • Tokens remain stuck in the contract with no indication of what went wrong

Proof of Concept

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
​
import {Test} from "forge-std/Test.sol";
import {Stratax} from "../../src/Stratax.sol";
import {IERC20} from "forge-std/interfaces/IERC20.sol";
import {ConstantsEtMainnet} from "../Constants.t.sol";
import {UpgradeableBeacon} from "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
import {BeaconProxy} from "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol";
​
// Simulates a token that returns false on failure without reverting (USDT behavior)
contract NonRevertingToken {
mapping(address => uint256) public balanceOf;
​
function transfer(address, uint256 amount) external returns (bool) {
if (balanceOf[msg.sender] < amount) {
return false; // silent failure, no revert
}
balanceOf[msg.sender] -= amount;
return true;
}
​
function decimals() external pure returns (uint8) { return 6; }
}
​
contract UncheckedTransferTest is Test, ConstantsEtMainnet {
Stratax stratax;
NonRevertingToken token;
address owner = address(0x123);
​
function setUp() public {
vm.createSelectFork(vm.envString("ETH_RPC_URL"));
​
Stratax impl = new Stratax();
UpgradeableBeacon beacon = new UpgradeableBeacon(address(impl), address(this));
​
bytes memory initData = abi.encodeWithSelector(
Stratax.initialize.selector,
AAVE_POOL, AAVE_PROTOCOL_DATA_PROVIDER,
address(0xdead), address(0), address(0)
);
BeaconProxy proxy = new BeaconProxy(address(beacon), initData);
stratax = Stratax(address(proxy));
stratax.transferOwnership(owner);
​
token = new NonRevertingToken();
// No tokens in contract — balance is 0
}
​
function test_silent_transfer_failure() public {
vm.prank(owner);
// @> Attempting to recover 100 tokens when contract holds 0
// USDT-style token returns false, but the call does NOT revert
stratax.recoverTokens(address(token), 100e6);
​
// Transaction succeeds, owner thinks recovery worked
// But owner's balance of token is still 0
assertEq(token.balanceOf(owner), 0, "Silent failure: tokens not transferred");
}
}

Recommended Mitigation

Use SafeERC20.safeTransfer which checks the return value and reverts on failure:

// src/Stratax.sol
​
+ import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
// Or use the upgradeable variant if contract needs upgradeable deps
​
function recoverTokens(address _token, uint256 _amount) external onlyOwner {
- IERC20(_token).transfer(owner, _amount);
+ SafeERC20.safeTransfer(IERC20(_token), owner, _amount);
}

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!