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

Unchecked ERC20 Transfers Across the Codebase

udogodwin2k22

summary

The contract contains unchecked ERC20 token transfers (transfer and transferFrom) across multiple functions, violating the ERC20 standard by not verifying the return values. This oversight can result in silent transfer failures for certain tokens, leading to fund loss, and inconsistent states.

Vulnerability Details

The contract contains multiple instances of unchecked ERC20 token transfers (IERC20.transfer and IERC20.transferFrom) where the return value is not verified. This violates the ERC20 standard, which specifies that these functions should return a boolean value indicating the success or failure of the transfer.

Ignoring these return values can result in undetected failures when certain tokens do not revert on failure but instead return false. This behavior is common in some non-standard ERC20 tokens, such as USDT or EURS. If a transfer fails but the contract continues execution as if the operation succeeded, it can lead to severe issues, such as inconsistent contract state or potential fund loss.

Using buyOrder function to explain,

function buyOrder(uint256 orderIndex, address seller) external payable {
if(seller == address(0)) {
revert TokenDivider__InvalidSeller();
}
​
SellOrder memory order = s_userToSellOrders[seller][orderIndex];
​
if(msg.value < order.price) {
revert TokenDivider__IncorrectEtherAmount();
}
​
uint256 fee = order.price / 100;
uint256 sellerFee = fee / 2;
​
​
if(msg.value < order.price + sellerFee) {
revert TokenDivider__InsuficientEtherForFees();
}
​
​
balances[msg.sender][order.erc20Address] += order.amount;
​
s_userToSellOrders[seller][orderIndex] = s_userToSellOrders[seller][s_userToSellOrders[seller].length - 1];
s_userToSellOrders[seller].pop();
emit OrderSelled(msg.sender, order.price);
// 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); ------------> Affected line
​
}

The transfer function of the IERC20 interface is called, but the return value (a bool indicating success or failure) is ignored. If the transfer fails (returns false), the function does not handle it. This means: The contract assumes the transfer succeeded even though it didn't. Tokens like USDT, BNB and others are missing a return value on transfer and transferFrom, which would break integration with the application. There are also tokens that do not revert on failure in transfer but return a false boolean value like EURS. The current implementation would continue execution even in these failure cases, treating them as successful transfers.

sellErc20 Function
The IERC20.transferFrom function is called without verifying the return value:

IERC20(tokenInfo.erc20Address).transferFrom(msg.sender, address(this), amount);

A seller attempts to sell tokens using the sellErc20 function. If transferFrom fails silently, the tokens are never deposited into the contract, but the user is falsely credited with a successful transaction.

transferErcTokens Function
Here, the return value of IERC20.transferFrom is also ignored:

IERC20(tokenInfo.erc20Address).transferFrom(msg.sender, to, amount);

In transferErcTokens, a user tries to transfer tokens to another address. The transferFrom call fails silently, causing the contract to operate under the assumption that the transfer succeeded.

Impact

  1. Users could lose funds if transfers fail silently

  2. Contract state would become out of sync with actual token balances

  3. Subsequent operations might be executed based on the assumption of a successful transfer

Tools used

Manual Review

Slither

Recommendation

Add proper return value checks for all ERC20 transfers and transferFrom calls:
These changes ensure that the contract only proceeds with the assumption of success if the transfer or transferFrom call explicitly returns true.

Example, in buyOrder

bool success = IERC20(order.erc20Address).transfer(msg.sender, order.amount);
require(success, "Token transfer failed");
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.