call() should be used instead of transfer()

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x056fd0f433a0330da5622493d13587233e16bc9e71481eb32659b79458c1257f Severity: medium

Description: Description

The transfer() function 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.

Attack Scenario

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. 4) Additionally, using higher than 2300 gas might be mandatory for some multisig wallets.

Issue code location:

https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L218

Proof of Concept (PoC) File

In IncentivizedMessageEscrow.sol at L-218, the transaction would fail as described for the above mentioned four different scenarios.

218                payable(incentive.refundGasTo).transfer(gasRefund);

Revised Code File (Optional)

Use call instead of transfer


    function submitMessage(
        bytes32 destinationIdentifier,
        bytes calldata destinationAddress,
        bytes calldata message,
        IncentiveDescription calldata incentive
    ) checkBytes65Address(destinationAddress) external payable returns(uint256 gasRefund, bytes32 messageIdentifier) {

 . . . some code

       unchecked {
            if (msg.value > sum) {
                // We know: msg.value >= sum, thus msg.value - sum >= 0.
                gasRefund = msg.value - sum;
-                payable(incentive.refundGasTo).transfer(gasRefund);
+              (bool success, ) = payable(incentive.refundGasTo).call{gasRefund}("");
+              require(success, "gas refund failed");
                return (gasRefund, messageIdentifier);
            }
        }
        return (0, messageIdentifier);
    }

References

Check the consensys article on Stop Using Solidity's transfer() Now

hats-finance / Catalyst-Exchange-0x3026c1ea29bf1280f99b41934b2cb65d053c9db4

call() should be used instead of transfer() #2