The use of the deprecated transfer() function for an address will inevitably make the transaction fail when:
1 The claimer smart contract does not implement a payable function.
2 The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit.
3 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.
jefflau
commented
2 years ago
Lines of code
https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L183 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L204 https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L211
Vulnerability details
Impact
The use of the deprecated transfer() function for an address will inevitably make the transaction fail when: 1 The claimer smart contract does not implement a payable function. 2 The claimer smart contract does implement a payable fallback which uses more than 2300 gas unit. 3 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.
Proof of Concept
https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L183
https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L204
https://github.com/code-423n4/2022-07-ens/blob/main/contracts/ethregistrar/ETHRegistrarController.sol#L211
Tools Used
Manual audit
Recommended Mitigation Steps
I recommend using call() instead of transfer() :
(bool success, ) = msg.sender.call{amount}(""); require(success, "Transfer failed.");