Current implementation cant work with fee-on-transfer tokens

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L44

Summary

There is an issue in the provided Escrow and EscrowFactory Solidity contracts that prevents proper functioning with fee-on-transfer tokens. Due to the contract design, the price value specified during escrow creation might not match the actual number of tokens received because of the fees subtracted on transfer.

Vulnerability Details

The vulnerability resides in the contract design and assumptions. The newEscrow function in the EscrowFactory contract and the constructor of the Escrow contract assume that the total amount transferred is equal to the price. With fee-on-transfer tokens, this is not true because a fee is deducted from each transfer, reducing the actual amount received by the contract. This leads to a scenario where the balance of tokens in the contract is less than the price, causing the transaction to fail due to the check if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();.

Impact

This issue could severely limit the use of the escrow contract with a broad class of tokens, specifically those that charge a fee on transfer. If a user attempts to set up an escrow with such a token, the transaction would fail, causing inconvenience and limiting functionality.

Tools Used

Manual code review.

Recommendations

To correct this issue, the constructor of the Escrow contract should be modified to set the price value to the actual balance of tokens in the contract after the transfer. This can be achieved by making the following change:

diff --git a/src/Escrow.sol b/src/Escrow.sol
index 5b5fd1f..f4c5bdf 100644
--- a/src/Escrow.sol
+++ b/src/Escrow.sol
@@ -30,7 +30,6 @@ contract Escrow is IEscrow, ReentrancyGuard {
     /// @dev Funds should be sent to this address prior to its deployment, via create2. The constructor checks that the tokens have
     /// been sent to this address.
     constructor(
-        uint256 price,
         IERC20 tokenContract,
         address buyer,
         address seller,
@@ -40,9 +39,8 @@ contract Escrow is IEscrow, ReentrancyGuard {
         if (address(tokenContract) == address(0)) revert Escrow__TokenZeroAddress();
         if (buyer == address(0)) revert Escrow__BuyerZeroAddress();
         if (seller == address(0)) revert Escrow__SellerZeroAddress();
-        if (arbiterFee >= price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
-        if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();
-        i_price = price;
+        i_price = tokenContract.balanceOf(address(this));
+        if (arbiterFee >= i_price) revert Escrow__FeeExceedsPrice(price, arbiterFee);
         i_tokenContract = tokenContract;
         i_buyer = buyer;
         i_seller = seller;

Cyfrin / 2023-07-escrow