Partial accounts cannot use the ETHRegistrarController #434
Description
Intro
.transfer only forwards 2300 gas. If the destination is a contract, e.g. a multisig wallet, the gas is not enough to execute the receive or fallback function, making the tx revert.
Use addr.call{value: x}("") instead of addr.transfer(x)
Vulnerability Details
ETHRegistrarController provides change for users' register and renew calls. Refund the excess amount paid by the user. Similarly, both the BulkRenewal and StaticBulkRenewal contracts will refund any overpaid amount to the user.
/* contracts/ethregistrar/ETHRegistrarController.sol */
201 | if (msg.value > (price.base + price.premium)) {
202 | payable(msg.sender).transfer(
203 | msg.value - (price.base + price.premium)
204 | );
205 | }
/* contracts/ethregistrar/ETHRegistrarController.sol */
220 | if (msg.value > price.base) {
221 | payable(msg.sender).transfer(msg.value - price.base);
222 | }
/* contracts/ethregistrar/StaticBulkRenewal.sol */
052 | // Send any excess funds back
053 | payable(msg.sender).transfer(address(this).balance);
/* contracts/ethregistrar/BulkRenewal.sol */
071 | // Send any excess funds back
072 | payable(msg.sender).transfer(address(this).balance);The transfer function is used here to transfer funds. The transfer function itself has a default gaslimit of 2300 wei.
There are two scenarios that can cause the transaction to fail:
If the target msg.sender of the transfer function is a contract and does not implement a receive or fallback function.
If the target msg.sender of the transfer function is a contract, and its receive or fallback function is too complex, consuming more than 2300 gas.
Since the return result of the controller.rentPrice function in the renewAll function of StaticBulkRenewal matches the internal rentPrice result of ETHRegistrarController, the totalPrice exactly equals the actual amount used, with no excess funds to be refunded. As a result, the ETHRegistrarController contract on the current Mainnet and Sepolia environments has not encountered this issue by chance.
Other contracts interacting with ETHRegistrarController could potentially trigger this denial-of-service vulnerability.
On the other hand, since the msg.value in both BulkRenewal and StaticBulkRenewal contracts is controlled by users, excess funds may be refunded, which could trigger this vulnerability (in fact, users on the mainnet have already experienced denial-of-service).
When fixing this issue, besides modifying the .transfer in ETHRegistrarController, the .transfer in both BulkRenewal and StaticBulkRenewal contracts also needs to be updated.
Please use addr.call{value: x}("") instead of addr.transfer(x).
Impact Details
Complex contract accounts (such as MultiSig Account, AA, etc.) or contracts that do not implement the receive or fallback functions cannot use the register and renew functions of ETHRegistrarController, nor the renewAll functions of the BulkRenewal and StaticBulkRenewal contracts.
Proof of Concept
Taking StaticBulkRenewal https://etherscan.io/address/0xa12159e5131b1eEf6B4857EEE3e1954744b5033A on the Mainnet as an example, some users have already encountered renewAll failures due to the limit of .transfer.
The failed transactions are as follows:
https://etherscan.io/tx/0xf3ac8a77093a0460b39f11c3bcd1a627251dacb61c4303ee81898f8bfd6b60ab
https://etherscan.io/tx/0x29ed86cbe07ad7ddf89801042a3a417fa6a9ada083a6e7d9d1b9e9bd6059e807
https://etherscan.io/tx/0x4dbf25f33451c31b08117b7c946c83830a38323d43d8ae812f403ae2895da1e1