Tadle
Tadle
DeFiFoundry
27,750 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Insufficient Handling of Non-Standard ERC20 Tokens Leading to Accounting Discrepancies

dimi6oni

Summary

The Tadle protocol utilizes a token whitelist mechanism managed by a trusted protocol owner to mitigate risks associated with non-standard ERC20 tokens. While this approach provides a significant layer of security, some residual risks remain due to the complex nature of various token implementations and the potential for human error in the vetting process.

Vulnerability Detail

The primary safeguard against problematic tokens in the Tadle protocol is the whitelist mechanism implemented in the TokenManager contract:

modifier onlyInTokenWhiteList(bool _isPointToken, address _tokenAddress) {
if (!_isPointToken && !tokenWhiteListed[_tokenAddress]) {
revert TokenIsNotWhiteListed(_tokenAddress);
}
_;
}
​

This whitelist, managed by a trusted protocol owner, provides a strong first line of defense. However, the protocol's token handling, particularly in the _transfer function, still assumes standard ERC20 behavior:

function _transfer(
address _token,
address _from,
address _to,
uint256 _amount,
address _capitalPoolAddr
) internal {
// ... (previous code)
​
if (fromBalanceAft != fromBalanceBef - _amount) {
revert TransferFailed();
}
​
if (toBalanceAft != toBalanceBef + _amount) {
revert TransferFailed();
}
}
​

While the whitelist significantly reduces the risk, it doesn't eliminate the potential for issues with non-standard tokens that may have been whitelisted.

Impact

The impacts, while mitigated by the whitelist, still include:

  • Accounting Discrepancies: If a non-standard token (e.g., fee-on-transfer) is whitelisted, it could lead to minor accounting issues.

  • Operational Disruptions: Unexpected token behavior could potentially disrupt specific protocol operations.

  • Delayed Response to Token Changes: If a whitelisted token changes its behavior post-vetting, it might take time to detect and respond to the change.

Tool used

Manual Review

Recommendation

Given the existing safeguards and the trusted nature of the protocol owner, a complete overhaul of the token handling system is unnecessary. Instead:

  1. Enhance the Whitelisting Process:

  • Develop a comprehensive checklist for token vetting, including checks for non-standard behaviors.

  • Implement a time-lock or multi-signature requirement for adding new tokens to the whitelist.

  1. Develop wrapper contracts for non-standard tokens: Create standardized interfaces for different token types (e.g., fee-on-transfer, rebasing) and implement wrappers that handle their unique behaviors internally.

  2. Implement a "Force of Transfer" (FoT) approach: Replace the current transfer checks with balance-based verifications:

function safeTransfer(IERC20 token, address to, uint256 amount) internal {
uint256 balanceBefore = token.balanceOf(address(this));
token.transfer(to, amount);
uint256 balanceAfter = token.balanceOf(address(this));
uint256 actualAmount = balanceBefore - balanceAfter;
require(actualAmount <= amount, "Transfer amount exceeded expected");
if (actualAmount < amount) {
// Handle fee-on-transfer case
// Update internal accounting with actualAmount
}
}
​
Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-FOT-Rebasing

Valid medium, there are disruptions to the ability to take market actions. The following functions will be disrupted without the possibiliy of reaching settlement, since the respective offers cannot be created/listed regardless of mode when transferring collateral token required to the CapitalPool contract or when refunding token from user to capital pool during relisting. So withdrawal is not an issue - `createOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L96-L102) - `listOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L355-L362) - `relistOffer()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L515-L521) - `createTaker()` - reverts [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/PreMarkets.sol#L831-L836) I believe medium severity is appropriate although the likelihood is high and impact is medium (only some level of disruption i.e. FOT tokens not supported and no funds at risk)

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.