F-2025-0006·unchecked-erc20-transfer

Unsafe token transfers in VaultV3 can lead to state inconsistencies

Fixedvaultetfstrategyd2-contracts

TL;DR

VaultV3::custodyFunds and VaultV3::returnFunds update vault state and emit events regardless of whether the underlying ERC-20 transfer actually succeeds, leaving the vault stuck in custodied or non-custodied state when a transfer silently fails.

Severity

MEDIUM

Impact

MEDIUM

Likelihood

MEDIUM

Method

MManual review

CAT.

Complexity

LOW

Exploitability

MEDIUM

02Section · Description

Description

VaultV3 orchestrates the handoff of assets between the vault and the trader at the start and end of each epoch:

solidity

function custodyFunds() external onlyTrader notCustodied duringEpoch returns (uint256) {
    uint256 amount = totalAssets();
    require(amount > 0, "!amount");
    custodied = true;
    custodiedAmount = amount;
    IERC20(asset()).transfer(trader, amount); // No success validation
    emit FundsCustodied(epochId, amount);
    return amount;
}

function returnFunds(uint256 _amount) external onlyTrader {
    require(custodied, "!custody");
    require(_amount > 0, "!amount");
    epoch.epochEnd = uint80(block.timestamp);
    custodied = false;
    IERC20(asset()).transferFrom(trader, address(this), _amount); // No success validation
    emit FundsReturned(currentEpoch, _amount);
}

Both functions use IERC20.transfer / IERC20.transferFrom and ignore the return value. Many ERC-20 tokens (notably USDT, and others) return false on failure instead of reverting. Additionally, state changes happen before the transfer rather than after, so even reverts would leave a fee/event trail.

Even with a trusted trader, several non-malicious scenarios can trigger this:

Token transfers being temporarily paused (e.g. USDC during the SVB crisis).
Missing or insufficient allowance for transferFrom.
Token blacklisting (USDC, USDT) or transfer limits.
Tokens returning false instead of reverting.

03Section · Impact

Impact

Vault becomes stuck in custodied = true state while no funds were actually transferred to the trader.
Or, in the reverse direction, custodied flips to false and the epoch is closed while no funds actually returned.
Incorrect epoch transitions desynchronize accounting.
Mismatched accounting of assets between totalAssets() and the vault's real balance.
Temporary or permanent locking of user funds.
Requires manual governance intervention to recover.

04Section · Recommendation

Recommendation

Use OpenZeppelin's SafeERC20 library and follow Checks-Effects-Interactions:

solidity

import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";

contract VaultV3 {
    using SafeERC20 for IERC20;

    function custodyFunds() external onlyTrader notCustodied duringEpoch returns (uint256) {
        uint256 amount = totalAssets();
        require(amount > 0, "!amount");
        custodied = true;
        custodiedAmount = amount;
        IERC20(asset()).safeTransfer(trader, amount);
        emit FundsCustodied(epochId, amount);
        return amount;
    }

    function returnFunds(uint256 _amount) external onlyTrader {
        require(custodied, "!custody");
        require(_amount > 0, "!amount");
        epoch.epochEnd = uint80(block.timestamp);
        custodied = false;
        IERC20(asset()).safeTransferFrom(trader, address(this), _amount);
        emit FundsReturned(currentEpoch, _amount);
    }
}

05Section · Resolution

Resolution

D2: Fixed in e54eb5b.

Cyfrin: Verified.

Status

Fixed

Fix commit

e54eb5b