Tadle

Summary

#L70 uses .transfer() to send ether to other addresses. There are a number of issues with using .transfer(), as it can fail for a number of reasons (specified in the Proof of Concept).

1. The destination is a smart contract that doesn’t implement a payable function or it implements a payable function but that function uses more than 2300 gas units.

2. The destination is a smart contract that doesn’t implement a payable fallback function or it implements a payable fallback function but that function uses more than 2300 gas units.

3. The destination is a smart contract but that smart contract is called via an intermediate proxy contract increasing the case requirements to more than 2300 gas units. A further example of unknown destination complexity is that of a multisig wallet that as part of its operation uses more than 2300 gas units.

4. Future changes or forks in Ethereum result in higher gas fees than transfer provides. The .transfer() creates a hard dependency on 2300 gas units being appropriate now and into the future.

Vulnerability Details

https://github.com/Cyfrin/2024-08-tadle/blob/04fd8634701697184a3f3a5558b41c109866e5f8/src/utils/Rescuable.sol#L70

other instances:

#L169

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

Impact

If a user falls into one of the above categories, they'll be unable to receive funds

Tools Used

Recommendations

Instead use the .call() function to transfer ether and avoid some of the limitations of .transfer(). This would be accomplished by changing payEther() to something like;

Gas units can also be passed to the .call() function as a variable to accomodate any uses edge cases. Gas could be a mutable state variable that can be set by the contract owner.