Tadle

Tadle

Tadle

DeFi
30,000 USDC
Submission Details
Severity: high
Valid

[M-2] `TokenManger::withdraw` doesn't check for before and after balances of ERC20 tokens that return false instead of reverting, causing user balance to be lost.

Updates

Lead Judging Commences

0xnevi Lead Judge about 1 month 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

Appeal created

karanel Submitter
about 1 month ago
0xnevi Lead Judge
about 1 month ago
karanel Submitter
about 1 month ago
0xnevi Lead Judge
about 1 month ago
karanel Submitter
about 1 month ago
0xnevi Lead Judge
about 1 month ago
karanel Submitter
about 1 month ago
0xnevi Lead Judge
about 1 month ago
karanel Submitter
about 1 month ago
0xnevi Lead Judge 20 days ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-TokenManager-safeTransferFrom-withdraw-missing-approve

This issue's severity has similar reasonings to #252, whereby If we consider the correct permissioned implementation for the `approve()` function within `CapitalPool.sol`, this would be a critical severity issue, because the withdrawal of funds will be permanently blocked and must be rescued by the admin via the `Rescuable.sol` contract, given it will always revert [here](https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/CapitalPool.sol#L36-L38) when attempting to call a non-existent function selector `approve` within the TokenManager contract. Similarly, the argument here is the approval function `approve` was made permisionless, so if somebody beforehand calls approval for the TokenManager for the required token, the transfer will infact not revert when a withdrawal is invoked. I will leave open for escalation discussions, but based on my first point, I believe high severity is appropriate. It also has a slightly different root cause and fix whereby an explicit approval needs to be provided before a call to `_safe_transfer_from()`, if not, the alternative `_transfer()` function should be used to provide an approval, assuming a fix was implemented for issue #252

Support

FAQs

Can’t find an answer? Join our Discord or follow us on Twitter.