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

contracts that call ERC20's approve(), transferFrom() and transfer() dont handle false return value

adeolu

Summary

contracts that call ERC20's approve(), transferFrom() and transfer() dont logic that handles when a token returns false value

Vulnerability Details

According to the EIP-20 specification it mandates that all contracts calling an erc20 token's approve(), transferFrom() and transfer() function should have logic to handle when these functions return false.

https://eips.ethereum.org/EIPS/eip-20#methods

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

But in the tadle contracts where erc20 token's approve(), transferFrom() and transfer() is called there is no sufficient handling for these false return cases which may mean that a call to these token functions does not actually transfer value from one address to the other.

Example of erc20 tokens that return false instead of reverting on failed transfers are Basic Attention Token (BAT), Huobi Token (HT), Compound USD Coin (cUSDC), 0x Protocol Token (ZRX)

function approve(address tokenAddr) external {
address tokenManager = tadleFactory.relatedContracts(
RelatedContractLibraries.TOKEN_MANAGER
);
(bool success, ) = tokenAddr.call(
abi.encodeWithSelector(
APPROVE_SELECTOR,
tokenManager,
type(uint256).max
)
);
​
if (!success) {
revert ApproveFailed();
}
}

  • in rescuable.sol which is inherited by TokenManager.sol and CapitalPool.sol, the functions safeTransferFrom() and _safeTransfer() do not handle return of false values too as they dont check the bytes data retured by the low level call().

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

function _safe_transfer(
address token,
address to,
uint256 amount
) internal {
(bool success, ) = token.call(
abi.encodeWithSelector(TRANSFER_SELECTOR, to, amount)
);
​
if (!success) {
revert TransferFailed();
}
}
​
/**
* @dev Safe transfer.
* @param token The token to transfer. If 0, it is ether.
* @param to The address of the account to transfer to.
* @param amount The amount to transfer.
*/
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();
}
}
}

Note that call()returns two arguments which are booland bytes. The bool indicates whether the low level call was sucessfully executed with no reverts while the bytes is the encoded return data for the function, should incase the function does return a value/values. In this case, if erc20token.transfer() has no reverts and does return false, this will be in the encoded bytesand it should be decoded and handled better.

Impact

tadle contracts assume the erc20 functions approve(), transferFrom() and transfer() do not return false and this is wrong as some tokens can return false. It also breaks the EIP20 recommendations for callers of ERC20 tokens.

Tools Used

manual review

Recommendations

follow the EIP-20 specifications. Better to use openzeppelin's SafeERC20 library for this.

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.