Summary

It is stated in the readme and by Patrick Collins that the protocol will only accept vetted tokens such as USDC, DAI, WETH, etc..

An issue arise when using tokens that could implement fees in the future (for example, some of the tokens stated that the project will support like USDC and most likely USDT). Those tokens currently have their fees set to 0 but they could enable them at any moment.

In such a case, the deployment of the escrow contracts using those tokens as rewards will always fail.

Vulnerability Details

To deploy an Escrow contract, the EscrowFactory is called and it deploys the new Escrow contract in the generated address. But before deploying the Escrow contract, the token reward funds are transferred to the address where the contract is going to be deployed.

During the deployment of the Escrow contract, the constructor of the Escrow contract has a check that does the following:

Basically it checks the price var (which is the amount of tokens the buyer is paying to the Escrow contract) to see if it is equal to the current token balance in the contract.

This poses a problem if it is used with a token with fees on transfer (as aforementioned above, USDC and USDT could enable in the future since they have that code implement in their contracts). When the token balance was transferred to the contract, a fee was deducted which means *the contract balance is very slightly less than the expected price amount which causes the Escrow deployment to revert.

We have included a POC to showcase how it works in all the cases. You can get the POC file in the following gist: https://gist.github.com/TheNaubit/bff7444affad148a85b6a0d0d1a9e21a

To run it, paste the content of the gist inside a file called FailedDeploymentUsingTokenWithFees.t.sol inside the test folder in the project. Then run them with the following command:

Affected lines of code:

• https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/EscrowFactory.sol#L39

• https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/EscrowFactory.sol#L41

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

For reference, there are other contests with similar findings like:

• https://solodit.xyz/issues/m-05-bribesol-is-not-meant-to-handle-fee-on-transfer-tokens-code4rena-velodrome-finance-velodrome-finance-contest-git

• https://solodit.xyz/issues/m-04-protocol-doesnt-handle-fee-on-transfer-tokens-code4rena-cudos-cudos-contest-git

• https://solodit.xyz/issues/m-08-tokens-with-fees-will-break-the-depositglp-logic-code4rena-redacted-cartel-redacted-cartel-contest-git

Impact

If any of the vetted tokens (like USDC or USDT) enable their fees, no escrow contracts using those popular payment methods can be deployed; and since those stablecoins are a very common payment method, making sure these tokens work always is crucial to ensure the correct experience.

Tools Used

Manual review and Foundry.

Recommendations

A good solution would be setting the price value using the current contract balance and not the user input. So in the EscrowFactory contract: