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

Protocol will be incompatible with some ERC20 tokens, like XAUt (Tether Gold)

dimah7

Summary

As per protocol's README, tokens that will be whitelisted to interact with the protocol will be ETH, WETH and any EIP-20 following standard tokens. However based on the project's current design, not all tokens will be suitable to the codebase. One example is XAUt, which is EIP-20 compilant. According to EIP-20 standard a transfer/transferFrom function should return a bool value. This token returns false even when a successul transfer is conducted.

Vulnerability Details

Following the transfer functionalities in the protocol (Rescuable.sol):

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();
}
}

It will not handle this specific edge case properly, thus will make the transfer transactions revert with transfer failed error.

PoC:

Add the following mock token contract in the PreMarkets.t.sol test file and run forge test --mt testFailTransfer -vvvv:

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
​
contract MockFailedTransfer is ERC20 {
​
constructor() ERC20("MockTransferFailed", "MTF") {}
​
function mint(address account, uint256 amount) public {
_mint(account, amount);
}
​
function transferFrom(address, /*sender*/ address, /*recipient*/ uint256 /*amount*/ )
public
pure
override
returns (bool)
{
return false;
}
​
function transfer(address to, uint256 value) public override returns (bool) {
address owner = _msgSender();
_transfer(owner, to, value);
return false;
}
}

Make the setup in PreMarketsTest:

+ import {MockFailedTransfer} from "./MockTransferFailed.sol";
​
contract PreMarketsTest is Test {
...
+ MockFailedTransfer mockToken;
...
​
function setUp() public {
...
+ mockToken = new MockFailedTransfer();
...
​
- address[] memory tokenAddressList = new address[](2);
+ address[] memory tokenAddressList = new address[](3);
tokenAddressList[0] = address(mockUSDCToken);
tokenAddressList[1] = address(weth9);
+ tokenAddressList[2] = address(mockToken);
}
function testFailTransfer() public {
address alice = makeAddr("Alice");
mockToken.mint(alice, 1e18);
​
vm.startPrank(alice);
mockToken.approve(address(tokenManager), type(uint256).max);
preMarktes.createOffer(CreateOfferParams(
marketPlace,
address(mockToken),
1000,
0.01 * 1e18,
12000,
300,
OfferType.Ask,
OfferSettleType.Turbo
)
);
vm.stopPrank();
}

  • Result from the traces output:

MockFailedTransfer::transferFrom(Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea], UpgradeableProxy: [0x5Fa39CD9DD20a3A77BA0CaD164bD5CF0d7bb3303], 12000000000000000 [1.2e16])
│ │ │ │ │ └─ ← [Return] false
│ │ │ │ ├─ [562] MockFailedTransfer::balanceOf(Alice: [0xBf0b5A4099F0bf6c8bC4252eBeC548Bae95602Ea]) [staticcall]
│ │ │ │ │ └─ ← [Return] 1000000000000000000 [1e18]
│ │ │ │ ├─ [562] MockFailedTransfer::balanceOf(UpgradeableProxy: [0x5Fa39CD9DD20a3A77BA0CaD164bD5CF0d7bb3303]) [staticcall]
│ │ │ │ │ └─ ← [Return] 0
│ │ │ │ └─ ← [Revert] TransferFailed()
│ │ │ └─ ← [Revert] TransferFailed()
│ │ └─ ← [Revert] TransferFailed()
│ └─ ← [Revert] TransferFailed()
└─ ← [Revert] TransferFailed()

Impact

  • Impact: Medium, no loss of assets, but breaks functionality, the token will simply not work on this codebase

  • Likelihood: Low, as it requires XAUt (Tether Gold) token to be whitelisted

Tools Used

Manual Review

Recommendations

Restrict using this specific token, or add a check if that particular is used for transfer, to handle the returned different bool value accordingly

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

Support

FAQs

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