Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: low
Invalid

Use `safeTransferFrom()` and `safeTransfer()` instead of `transferFrom()` and `transfer()`

0xnbvc

Vulnerability Details

The internal functions _safe_transfer() and _safe_transfer_from() in the Rescuable contract use transfer() and transferFrom() instead of safeTransfer() and safeTransferFrom() and it is misleading.

TokenManager::tillIn() calls _safe_transfer() which uses transfer() instead of safeTransfer():

Rescuable.sol#L84-L96

function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
>> abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}

TokenManager::withdraw() calls _transfer() and _safe_transfer_from() which use transferFrom() instead of safeTransferFrom():

Rescuable.sol#L104-L117

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

Inconsistent naming: The function names _safe_transfer() and _safe_transfer_from() imply safe operations, but they don't actually use the safe versions of the transfer functions, which is misleading to developers and auditors.
Moreover, if non-standard ERC20 tokens such as USDT are used in the future, the contract will not be able to handle them correctly.

Recommendations

Use safeTransfer() and safeTransferFrom() instead of transfer() and transferFrom().

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 year 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.