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

Low level calls to token contracts can result in Potential False Positive

rilwanwild

Summary

The TokenManager's tillIn() function, which handles token transfers, relies on _safe_transfer and _safe_transfer_from. These functions use low-level calls to interact with token contracts but may incorrectly interpret failed transfers as successful for certain tokens, particularly those that don't conform strictly to the ERC20 standard (e.g., USDT).

Vulnerability Details

The _safe_transfer and _safe_transfer_from functions in both TokenManager and Rescuable contracts use low-level call to interact with token contracts.

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

  • They only check if the call was successful (bool success), not if the actual transfer was completed.

  • Some tokens, like USDT, don't return a boolean value from their transfer functions, instead using return without a value.

  • In such scenarios, when a transfer fails, success will still be true as the low-level call was completed successfully without reverting.

Impact

This vulnerability leads to a discrepancy between users' actual token balances and those recorded in userTokenBalanceMap. This mismatch results in incorrect withdrawal amounts, which can affect the solvency of the protocol.

Tools Used

Manual Review

Recommendations

Use Safe Transfer Libraries:

  • One solution would be to use established libraries like OpenZeppelin's SafeERC20 which handle various token implementations safely.

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.