Cyfrin / 2023-07-escrow

17 stars 12 forks source link

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

Open codehawks-bot opened 11 months ago

codehawks-bot commented 11 months ago

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

Severity

Low Risk

Relevant GitHub Links

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

Summary

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

In the extremely unlikely scenario where the total funds that arrived in the Escrow contract's address when transferred by the buyer, is LESS than what the buyer sent, i.e. tokenContract.balanceOf(address(this)) < price, for whatever reason, then the above if statement will revert and no Escrow contract will be created, as intended.

All good, but see my recommendation.

Vulnerability Details

n/a

Impact

Frustration.

Tools Used

VSC, manual.

Recommendations

However, maybe just my noobness but it's my suggestion to have some solution implemented that would eliminate this highly unlikely potential risk of reverting and temporarily DoS-ing buyer's attempt to create escrow contract, by ensuring that the price amount of tokens the buyer transfers to the Escrow contract's address is exactly what will arrive there, always.

Additionally, which tokens/token contracts may contribute to this potential "risk" of causing less tokens to arrive than what was sent?

serial-coder commented 10 months ago

Escalate

How can this be a valid issue?

The impact is "Frustration". That shouldn't be an issue. The author's recommendations also do not provide any contributions.

alymurtazamemon commented 10 months ago

Escalate

This is not a valid issue and we are giving almost ~100 USDC for it to finders.

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

The supported tokens do not take fees while transferring and it is not possible for the user to send tokens and the contract to receive less.

PatrickAlphaC commented 10 months ago

The impact would be the users would send a failed tx.

Agreed. This is info.