Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: medium
Invalid

A selling can go through with the ERC20 token transfer failing and yet the transaction won't revert

0xziin

Summary

A selling can go through with the ERC20 token transfer failing and yet the transaction won't revert.
The ether amount will be paid but the user could still not receive his ERC20 tokens.

The TokenDivider.sol::buyOrder() function is the one having a flaw:

https://github.com/Cyfrin/2025-01-pieces-protocol/blob/main/src/TokenDivider.sol#L261-L307

Vulnerability Details

https://github.com/Cyfrin/2025-01-pieces-protocol/blob/main/src/TokenDivider.sol#L305

function buyOrder(uint256 orderIndex, address seller) external payable {
...
// Transfer The Ether
​
(bool success, ) = payable(order.seller).call{value: (order.price - sellerFee)}("");
​
if(!success) {
revert TokenDivider__TransferFailed();
}
​
(bool taxSuccess, ) = payable(owner()).call{value: fee}("");
​
​
if(!taxSuccess) {
revert TokenDivider__TransferFailed();
}
​
IERC20(order.erc20Address).transfer(msg.sender, order.amount); // <- Here is the flaw
​
}
IERC20(order.erc20Address).transfer(msg.sender, order.amount); // This won't revert if the transfer doesn't go through

If the transfer() fails, the transaction won't revert. Usually for the transaction to revert on a failing transfer, you need to check for success or just use the safeTransfer() function instead of transfer().

Checking for success:

bool success = IERC20(order.erc20Address).transfer(msg.sender, order.amount);
require(success, "Token transfer failed");


The problem is that you can never know with certainty that the transfer() function of the ERC20 contract is well implemented and will return 0 for sure if something went wrong.

When using safeTransfer() instead of transfer():

=> If safeTransfer() fails, the transaction will revert.

A practical example for a transaction of an ERC20 token not going through could be : the user being blacklisted by a token contract with
a blacklist like USDC.


The sell order will be considered done, the transfer of Ethers will go through but not the ERC20 transfer.

Impact

Loss of funds, the user spends ethers but never gets his tokens in exchange.

Tools Used

Github, manual review.

Recommendations

Use the safeTransfer() function from the safeERC20's openzeppelin library.

Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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