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

[M-3] Missing Revert Error on Transfer Failure in `TokenDivider::transferErcTokens`

0xsnoweth

Summary

The transferErcTokens function calls IERC20(tokenInfo.erc20Address).transferFrom to transfer ERC20 tokens but does not provide a descriptive revert error if the transfer fails. This can lead to silent failures, making it difficult to diagnose issues and potentially leaving the contract in an inconsistent state.

Vulnerability Details

The transferErcTokens function updates the sender's and recipient's balances before calling transferFrom. If the transferFrom call fails (e.g., due to insufficient allowance or a non-compliant ERC20 token), the function reverts with a generic error message, making it difficult to diagnose the issue. This can result in silent failures, where the contract state (e.g., balances) is updated even though the token transfer was unsuccessful.

function transferErcTokens(address nftAddress, address to, uint256 amount) external {
// ... (checks omitted for brevity)
balances[msg.sender][tokenInfo.erc20Address] -= amount;
balances[to][tokenInfo.erc20Address] += amount;
emit TokensTransfered(amount, tokenInfo.erc20Address);
​
// Unchecked transferFrom call
IERC20(tokenInfo.erc20Address).transferFrom(msg.sender, to, amount); // No descriptive revert error
}

Exploit Scenario:

  1. A user calls transferErcTokens to transfer ERC20 tokens. 2. The transferFrom call fails (e.g., due to insufficient allowance or a non-compliant token).

  2. The function reverts with a generic error message, making it difficult to diagnose the issue.

  3. The balances may already be updated, leading to inconsistencies and potential fund loss.

Impact

Medium Impact: Funds are indirectly at risk if the transfer fails silently. The contract's state (balances) may become inconsistent if the transfer fails but the balances are already updated.

  1. State Inconsistency: Users may lose funds if the transfer fails without a clear error message, and the contract state may not reflect the actual token balances.

  2. Diagnostic Challenges: Silent failures make it difficult to diagnose issues, leading to poor user experience and potential financial losses.

Tools Used

Manual Review, Foundry

Recommendations

  1. Medium Impact: Funds are indirectly at risk if the transfer fails silently. The contract's state (balances) may become inconsistent if the transfer fails but the balances are already updated.

  2. State Inconsistency: Users may lose funds if the transfer fails without a clear error message, and the contract state may not reflect the actual token balances.

  3. Diagnostic Challenges: Silent failures make it difficult to diagnose issues, leading to poor user experience and potential financial losses.

bool success = IERC20(tokenInfo.erc20Address).transferFrom(msg.sender, to, amount);
- require(success, "TokenDivider: Transfer failed");
+ require(success, "TokenDivider: ERC20 transfer failed. Check allowance and balance.");
}

Additional Considerations:

Use OpenZeppelin's SafeERC20 library to handle non-compliant ERC20 tokens and ensure safe transfers.

Emit an event for failed transfers to improve transparency and debugging:

event TransferFailed(address indexed from, address indexed to, uint256 amount);
if (!success) {
emit TransferFailed(msg.sender, to, amount);
revert("TokenDivider: ERC20 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.