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

[M-1] `TokenManager::withdraw` may lock users funds due to incompatibility with tokens missing return values

philbugcatcher

Summary

The TokenManager::withdraw function relies on Rescuable::_safe_transfer_from to transfer tokens using the transferFrom method. However, this function reverts for tokens like USDT, which don’t return a boolean success value, making it impossible for users to withdraw these tokens.

Vulnerability Details

The function TokenManager::withdraw uses the method transferFrom of the underlying function Rescuable::_safe_transfer_from to transfer tokens to the users. As currently designed, this underlying function reverts if the token the user attempts to withdraw does not return a boolean success value, as shown below:

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

This behavior is incompatible with certain tokens, such as USDT, which do not return a boolean on transferFrom, resulting in users being unable to withdraw these tokens from the protocol.

Impact

This issue results in a complete inability for users to withdraw affected tokens, leading to the loss of access to their funds. Tokens remain stuck in the contract, rendering them unusable.

Tools Used

Manual code review.

Recommendations

To avoid this issue, consider using OpenZeppelin’s SafeERC20::safeTransferFrom function, which accounts for tokens that do not return a boolean success value. Link to OpenZeppelin’s implementation: SafeERC20.sol

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.