Tadle
Tadle
DeFi
30,000 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Token transfer may fail silently causing recipient to lose funds - some tokens return false rather than revert on failure

honour

Summary

Some tokens don't revert on transfer failure but return false instead and although the protocol only allows whitrlisted tokens for trading points ,the points token itself does not need to be whitelisted and can have this behaviour.

Vulnerability Details

Rescuable::_safe_transfer_from and Rescuable::_safe_transfer perform low level call without verifying the returned value(if any) to be true.
In the case where where the token returns false on transfer failure and the call fails ,the transaction silently fails(does not revert) without actually transfering funds to the recipient.

function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}
​
/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}

Impact

MEDIUM - in case of token transfer failure the recipient will lose funds

Tools Used

Manual Review

Recommendations

function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
- (bool success, ) = token.call(
- abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
- );
​
- if (!success) {
- revert TransferFailed();
- }
+ (bool success,bytes returnData ) = token.call(
+ abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
+ );
​
+ if (!success || (returnData.length != 0 && !abi.decode(returnData, (bool)))) {
+ revert TransferFailed();
+ }
}
​
/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
- (bool success, ) = token.call(
- abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
- );
​
- if (!success) {
- revert TransferFailed();
- }
​
+ (bool success,bytes returnData ) = token.call(
+ abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount)
+ );
​
+ if (!success || (returnData.length != 0 && !abi.decode(returnData, (bool)))) {
+ revert TransferFailed();
+ }
}
Updates

Lead Judging Commences

0xnevi Lead Judge 10 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

[invalid] finding-weird-erc-20-return-boolean-Rescuable

I believe the issues and duplicates do not warrant low severity severity as even if the call to transfers returns false instead of reverting, there is no impact as it is arguably correct given there will be insufficient funds to perform a rescue/withdrawal. This will not affect `tillIn()` as there are explicit balance [checks that revert accordingly](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L255-L260) to prevent allowing creation of offers without posting the necessary collateral

Support

FAQs

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