Summary

The current logic for the newEscrow function of the EscrowFactory will always revert when the specified tokenContract is a fee-on-transfer token such as USDT (if/when fees are turned on). This breaks the specs for this contract, which state that only malicious/ERC777 tokens shouldn't be allowed. The issue arises because newEscrow only allows transferring price amount of tokens to the new escrow contract. However, after the fee is applied, the balance of the escrow contract will be less than price, which will cause a check in the constructor to always revert. This makes it impossible to create escrows using these tokens.

Vulnerability Details

The newEscrow function is defined as follows:

The safeTransferFrom call should be sending price amount of tokens from msg.sender to the address of the escrow contract which is about to be created. For fee-on-transfer tokens, the actual amount sent will be price - fee, or in other words, some amount less than price.

In the Escrow constructor there is the following check:

Since the amount sent will be some amount less than price, this check will always revert, meaning it's impossible to call newEscrow with these tokens.

Impact

The newEscrow function of the EscrowFactory contract does not support all the tokens that it should based on specs, breaking core functionality.

Tools Used

Manual review

Recommendations

When creating new escrows using the newEscrow function, there should be another variable priceToPay which is the actual amount used in the tokenContract.safeTransferFrom(..) call, rather than using the price variable.