Core Contracts

Summary

The withdraw function in the Treasury contract currently uses the transfer method to send tokens to a recipient. However, this can cause issues with certain ERC-20 tokens like USDC and USDT, which do not revert on failure but instead return a boolean value. If the function does not check the return value of transfer, it may incorrectly assume that the transfer was successful even if it failed. This could lead to incorrect token accounting, leaving the contract in an inconsistent state where the recorded balances do not match the actual token balances.

Vulnerability Details

• Issue:

  • The function currently uses IERC20(token).transfer(recipient, amount); for token transfers.

  • Some ERC-20 tokens, such as USDC and USDT, do not revert on failure. Instead, they return false when a transfer fails.

  • The contract does not check the return value of transfer, so it assumes that the transfer always succeeds.

  Problematic Code:

  IERC20(token).transfer(recipient, amount);

• Root Cause:

  • The ERC-20 standard is loosely defined, and some implementations (such as USDC and USDT) return a boolean rather than reverting on failure.

  • The contract does not handle tokens that return a boolean, leading to possible silent failures in transfers.

• Expected Behavior:

  • Instead of assuming the transfer always succeeds, the contract should explicitly check the return value and revert if the transfer fails.

  • Alternatively, it can use OpenZeppelin’s SafeERC20 library, which correctly handles different ERC-20 implementations.

Impact

• Incorrect Token Accounting:
  If a transfer fails and the contract does not detect it, the _balances state variable will be reduced even though the actual tokens remain in the contract. This creates an inconsistency between the contract’s internal state and the actual token balance.

Tools Used

• Manual Code Review

• Analysis of Non-Standard ERC-20 Implementations (USDC, USDT)

Recommendations

1. Use SafeERC20 for Transfers:
   The best approach is to use OpenZeppelin’s SafeERC20 library, which ensures compatibility with tokens that return a boolean instead of reverting.

   Fixed Code:

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

   ​

   using SafeERC20 for IERC20;

   ​

   function withdraw(

   address token,

   uint256 amount,

   address recipient

   ) external override nonReentrant onlyRole(MANAGER_ROLE) {

   if (token == address(0)) revert InvalidAddress();

   if (recipient == address(0)) revert InvalidRecipient();

   if (_balances[token] < amount) revert InsufficientBalance();

   _balances[token] -= amount;

   _totalValue -= amount;

   IERC20(token).safeTransfer(recipient, amount);

   emit Withdrawn(token, amount, recipient);

   }

2. Manually Check the Return Value (Alternative Fix):
   If avoiding external dependencies, explicitly check the return value of transfer and revert if it fails.

   Alternative Code Fix:

   bool success = IERC20(token).transfer(recipient, amount);

   if (!success) revert TransferFailed();