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

user's might not get their funds back, even though withdraw() tx is successful

dinkras

Code snippets:

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L175-L180

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L111

Summary

In case users want to withdraw their assets from the protocol, they should call TokenManager::withdraw().

However when it comes to the actual transfer logic of the ERC20 tokens, the returned value of the low level calls is never checked. Which means that transfers might actually fail, but the protocol will show that they have passed.

EIP20(https://eips.ethereum.org/EIPS/eip-20) states that:

  • Callers MUST handle false from returns (bool success). Callers MUST NOT assume that false is never returned!

For any non WETH ERC20 token the TokenManager.withdraw() function calls _safe_transfer_from()

function _safe_transfer_from(
address token,
address from,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(//@audit does not handle false return values
);
​
if (!success) {
revert TransferFailed();
}
}

However in case the low level call returns false, the tx won't revert and the whole withdraw() operation will show as success, even though the actual transfer of tokens failed.

Vulnerability Details

Described in the Summary section

Impact

Failed TokenManager.withdraw() transfers of ERC20 tokens will show as success. Users will be confused, protocol reputation will be harmed.

Tools Used

Manual review

Recommendations

Use OpenZeppelin's SafeERC20.sol lib for transferring tokens.

Instances to fix: Rescuable._safe_transfer_from() and _safe_transfer()

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

Appeal created

dinkras Submitter
about 1 year ago
0xnevi Lead Judge
12 months ago
0xnevi Lead Judge 12 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.