Tadle

Summary

tokenManager.withdraw() function uses transfer() to send ether, this can fail in some scenarios as gas amount is fixed to 2300 as the user can be a smart wallet or smart contract (maybe integrations by another project)

Vulnerability Details

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/core/TokenManager.sol#L169

In the tokenManager.withdraw(), transfer() is used for native ETH send to the user, when the token address to be withdrawn is a wrapped native token. The transfer() and send() functions forward a fixed amount of 2300 gas. Historically, it has often been recommended to use these functions for value transfers to guard against reentrancy attacks. However, the gas cost of EVM instructions may change significantly during hard forks which may break already deployed contract systems that make fixed assumptions about gas costs. For example. EIP 1884 broke several existing smart contracts due to a cost increase of the SLOAD instruction.

Impact

The use of the transfer() function for sending ether to an address will inevitably make the transaction fail when:

• The claimer is a smart wallet with a complex fallback.

• The claimer smart contract does not implement a payable function.

• The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.

• The claimer smart contract implements a payable fallback function that needs less than 2300 gas units but is called through proxy, raising the call's gas usage above 2300.
  Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Tools Used

manual review

Recommended Mitigation Steps

Use call() instead of transfer().

Keep in mind that call() introduces the risk of reentrancy. But, let the implementation follows the checks-effects interactions pattern or use openzeppelin's reentrancyGuardUpgradeable library.