CodeHawks Escrow Contract - Competition Details
Cyfrin
Foundry
40,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Fee-on-transfer tokens aren't supported

nican0r

Summary

Fee-on-transfer tokens aren't supported by the current escrow implementation.

Vulnerability Details

In EscrowFactory::newEscrow L39 the price variable is used to transfer tokens from the buyer to the address at which the Escrow will be deployed:

tokenContract.safeTransferFrom(msg.sender, computedAddress, price);
Escrow escrow = new Escrow{salt: salt}(
price,
tokenContract,
msg.sender,
seller,
arbiter,
arbiterFee
);

for fee-on-transfer tokens the actual amount transferred to the Escrow address will be less than the price. This will cause the call to create a new instance of Escrow on L40 to trigger the following revert in the Escrow constructor L44:

if (tokenContract.balanceOf(address(this)) < price) revert Escrow__MustDeployWithTokenBalance();

buyers will therefore be unable to create an Escrow using fee-on-transfer tokens such as USDT.

Impact

The protocol prevents the use of fee-on-transfer tokens without explicitly defining these conditions. The inability to do so doesn't require malicious action by either party and given the sponsor comment with regard to compatible tokens this doesn't appear to be addressed:

"For the moment assume the following:

WETH, USDC, LINK, DAI

But, the buyer and seller could do whatever they want - just we would recommend against that."

Tools Used

Manual Review

Recommendations

Change the value passed in the instantiation of a new Escrow in EscrowFactory::newEscrow L41 to the balance of the Escrow address:

Escrow escrow = new Escrow{salt: salt}(
IERC20,
tokenContract.balanceOf(computedAddress),
msg.sender,
seller,
arbiter,
arbiterFee
);

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.