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

`TokenManager` transfer functions do not correctly verify transfer success

0xbrivan2

Summary

The TokenManager transfer functions only verify the success of the call operation, which can lead to undetected transfer failures for tokens that return false instead of reverting

Vulnerability Details

The transfer functions in the TokenTransfer contract(inherited from Rescuable) perform low-level calls to token contracts and only verify the success of the call operation. For example, in _safe_transfer_from function:

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();
}
}

As shown above, the function ONLY asserts the success of the call operation. However, some tokens do not revert on transfer failure; instead, they return false. This flaw could result in a situation where a failed token transfer does not cause the transaction to revert

The same issue is present in the _safe_transfer function.

Impact

Tokens that return false on transfer failure will not trigger a transaction revert, leading to incorrect token balances tracking

Tool Used

Manual Review

Recommendations

Consider updating the transfer success check on the two mentioned functions above as follows:

// _safe_transfer_from:
​
(bool success, bytes memory data) = token.call(abi.encodeWithSelector(TRANSFER_FROM_SELECTOR, from, to, amount));
require(success && (data.length == 0 || abi.decode(data, (bool))), 'TRANSFER_FAILED');
Updates

Lead Judging Commences

0xnevi Lead Judge 9 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.