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

Limited support to a specific subset of ERC20 tokens

air

Summary

The ReadME states that "any token that follows the ERC20 standard", therefore it should take into account all the different ways of signalling success and failure.

This is not the case, as all ERC20's transfer(), transferFrom(), and approve() functions are either not verified at all or verified for returning true. As a result, depending on the ERC20 token, some transfer errors may result in passing unnoticed, and/or some successfull transfer may be treated as failed.

Currently the only supported ERC20 tokens are the ones that fulfill both the following requirements:

  • always revert on failure;

  • always returns boolean true on success.

An example of a very well known token that is not supported is Tether USD (USDT).

Vulnerability Details

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

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L15

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L24

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

Tokens have different ways of signalling success and failure, and this affect mostly transfer(), transferFrom() and approve() in ERC20 tokens. While some tokens revert upon failure, others consistently return boolean flags to indicate success or failure, and many others have mixed behaviours

See below a snippet of the USDT Token contract compared to the 0x's ZRX Token contract where the USDT Token transfer function does not even return a boolean value, while the ZRX token consistently returns boolean value hence returning false on failure instead of reverting.

USDT Token snippet (no return value) from Etherscan

function transferFrom(address _from, address _to, uint _value) public onlyPayloadSize(3 * 32) {
var _allowance = allowed[_from][msg.sender];
​
// Check is not needed because sub(_allowance, _value) will already throw if this condition is not met
// if (_value > _allowance) throw;
​
uint fee = (_value.mul(basisPointsRate)).div(10000);
if (fee > maximumFee) {
fee = maximumFee;
}
if (_allowance < MAX_UINT) {
allowed[_from][msg.sender] = _allowance.sub(_value);
}
uint sendAmount = _value.sub(fee);
balances[_from] = balances[_from].sub(_value);
balances[_to] = balances[_to].add(sendAmount);
if (fee > 0) {
balances[owner] = balances[owner].add(fee);
Transfer(_from, owner, fee);
}
Transfer(_from, _to, sendAmount);
}
​

ZRX Token snippet (consistently true or false boolean result) from Etherscan

function transferFrom(address _from, address _to, uint _value) returns (bool) {
if (balances[_from] >= _value && allowed[_from][msg.sender] >= _value && balances[_to] + _value >= balances[_to]) {
balances[_to] += _value;
balances[_from] -= _value;
allowed[_from][msg.sender] -= _value;
Transfer(_from, _to, _value);
return true;
} else { return false; }
}
​

}

Impact

  1. If the ERC20 token being traded is one that consistently returns a boolean result in the case of success and failure like for example 0x's ZRX Token contract. Where the return value is currently not verified to be true the transfer may fail (e.g.: no tokens transferred due to insufficient balance) but the error would not be detected by the TokenManager contract.

  2. If ERC20 token being traded is one that do not return a boolean value like for example the well known Tether USD Token contract. Successful transfers would cause a revert in the TokenManager contracts where the return value is verified to be true due to the token not returing boolean results.

    Same is true for appove calls.

Tools Used

Manual Review

Recommendations

To handle most of these inconsistent behaviors across multiple tokens, either use OpenZeppelin's SafeERC20 library,

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.