40,000 USDC
View results
Submission Details
Severity: medium

confirmReceipt and resolveDispute() - unhandled safeTransfer return value can lead to unexpected transfers

Vulnerability Details

ERC20 implementations are not always consistent. Some implementations of safeTransfer could return ‘false’ on failure instead of reverting. It is better to wrap such calls into require() statements.
The safeTransfer function is used in 4 instances in the Escrow.sol contract - once in confirmReceipt() and thrice in resolveDispute()

https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L98
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L120
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L123
https://github.com/Cyfrin/2023-07-escrow/blob/main/src/Escrow.sol#L127 \

Impact

  1. In confirmReceipt() if the safeTransfer has returned False but if it is not checked then the seller might not get the funds, and it could be locked in the contract, as there is no other way in the contract to get those funds back even to buyer account.

  2. In resolveDispute(), if the transfer has returned False during buyer or arbiter transaction, then the seller might get unexpected funds transferred to his account. Else if it False is during the seller transaction, then fund gets locked as like in the confirmReceipt() case.

Tools Used

Manual Review

Recommendations

Check the return value of the safeTransfer function, and check with a require() statement

Support

FAQs

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