Incorrect Allowance Check in `_transfer()` Function.
Summary
The _transfer() function in the tokenManager.sol contract contains a condition that checks if the token allowance is exactly zero before attempting to approve the transfer. This condition can cause issues, particularly when the allowance is non-zero but less than the transfer amount, leading to a potential failure in processing the transfer.
Vulnerability Details
In the _transfer() function, there is a check to see if the capital pool's address's token allowance for the contract is equal to zero IERC20(_token).allowance(_from, address(this)) == 0x0. If true, the contract calls approve to set the allowance. However, this condition fails to account for situations where the allowance is less than the required transfer amount but not zero, leading to potential transfer failures.
Impact
If the allowance is non-zero but insufficient to cover the _amount, the transfer will fail because the contract will not increase the allowance as needed. This could disrupt normal operations and lead to failed transactions.
Tools Used
Manual Review
Recommendations
Change the Condition: Modify the condition to check if the allowance is less than the _amount rather than exactly zero. This will ensure that the contract only attempts to increase the allowance when necessary.
Lead Judging Commences
finding-TokenManager-approve-wrong-address-input
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. The argument up in the air is since the approval function `approve` was made permisionless, the `if` block within the internal `_transfer()` function will never be invoked if somebody beforehand calls approval for the TokenManager for the required token, so 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.
Prize pool breakdown
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.