ERC20 transfers does not work on non-standard compliant tokens like USDT

[code423n4]

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/main/contracts/PermissionlessBasicPoolFactory.sol#L198

Vulnerability details

Impact

PermissionlessBasicPoolFactory.sol Consider this function:

function deposit(uint poolId, uint amount) external {
    Pool storage pool = pools[poolId];
    require(pool.id == poolId, 'Uninitialized pool');
    require(block.timestamp > pool.startTime, 'Cannot deposit before pool start');
    require(block.timestamp < pool.endTime, 'Cannot deposit after pool ends');
    require(pool.totalDepositsWei < pool.maximumDepositWei, 'Maximum deposit already reached');
    if (pool.totalDepositsWei + amount > pool.maximumDepositWei) {
        amount = pool.maximumDepositWei - pool.totalDepositsWei;
    }
    pool.totalDepositsWei += amount;
    pool.numReceipts++;

    Receipt storage receipt = pool.receipts[pool.numReceipts];
    receipt.id = pool.numReceipts;
    receipt.amountDepositedWei = amount;
    receipt.timeDeposited = block.timestamp;
    receipt.owner = msg.sender;

    bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
    require(success, 'Token transfer failed');

    emit DepositOccurred(poolId, pool.numReceipts, msg.sender);
}

if pool.depositToken === <USDT_ADDRESS> this function will revert because transferFrom of usdt does'nt return anything. and IERC20.transferFrom want a boolean.

other places with same problem:

MerkleDropFactory.sol
  77,48:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
  107,42:         require(IERC20(tree.tokenAddress).transfer(destination, value), "ERC20 transfer failed");

MerkleResistor.sol
  121,48:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
  204,42:         require(IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal), 'Token transfer failed');

MerkleVesting.sol
  89,48:         require(IERC20(merkleTree.tokenAddress).transferFrom(msg.sender, address(this), value), "ERC20 transfer failed");
  173,34:         IERC20(tree.tokenAddress).transfer(destination, currentWithdrawal);

PermissionlessBasicPoolFactory.sol
  144,62:             success = success && IERC20(pool.rewardTokens[i]).transferFrom(msg.sender, address(this), amount); ///
  198,49:         bool success = IERC20(pool.depositToken).transferFrom(msg.sender, address(this), amount);
  230,62:             success = success && IERC20(pool.rewardTokens[i]).transfer(receipt.owner, transferAmount);
  233,55:         success = success && IERC20(pool.depositToken).transfer(receipt.owner, receipt.amountDepositedWei);
  252,62:             success = success && IERC20(pool.rewardTokens[i]).transfer(pool.excessBeneficiary, rewards);
  269,62:             success = success && IERC20(pool.rewardTokens[i]).transfer(globalBeneficiary, tax);

Recommended Mitigation Steps

Use OpenZeppelin’s SafeERC20 safeTransfer/safeTransferFrom instead of transfer/transferFrom that handles the return value check as well as non-standard-compliant tokens.

[illuzen]

Duplicate #27