The TokenManager::_transfer
function performs unnecessary balance checks before and after the transfer. Additionally, it uses exact equality checks for balance differences, which can cause issues with tokens that have transfer fees or unusual rounding behaviors.
The _transfer
function performs balance checks that are typically handled by the ERC20 token itself:
These checks can cause issues with tokens that have transfer fees or non-standard implementations. The test case demonstrates this issue: (to be inserted in PreMarkets.t.sol
)
Potential for failed transfers when using tokens with transfer fees or non-standard implementations.
Users may be unable to withdraw their funds if using tokens with transfer fees.
Users may be unable to send their funds if using tokens with transfer fees.
Manual Review - Testing
Remove the unnecessary balance checks in the _transfer
function.
Instead of using exact equality checks, implement a tolerance threshold for balance differences to account for potential transfer fees or rounding issues.
Consider using OpenZeppelin's SafeERC20 library for safer token transfers.
Here's an example of how the _transfer
function could be improved:
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)
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.