Holograph contest - cloudjunky's results

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/main/contracts/HolographOperator.sol#L596

Vulnerability details

Impact

The transfer() function only provides a stipend for the recipient to use of 2300 gas. If the recipient uses more than that, transfers will fail. For longevity, and especially for a project of Holograph’s nature it is better to use call() rather than transfer() to ensure that future gas increases won’t affect ETH transfers.

The transfer() function can fail for a number of reasons;

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.

Additionally, using higher than 2300 gas might be mandatory for some multi-sig wallets.

Recommended mitigation steps

The following git diff demonstrates how call() can be used instead of transfer();

--- a/contracts/HolographOperator.sol
+++ b/contracts/HolographOperator.sol
@@ -593,7 +593,8 @@ contract HolographOperator is Admin, Initializable, HolographOperatorInterface {
     uint256 hlgFee = messagingModule.getHlgFee(toChain, gasLimit, gasPrice);
     address hToken = _registry().getHToken(_holograph().getHolographChainId());
     require(hlgFee < msg.value, "HOLOGRAPH: not enough value");
-    payable(hToken).transfer(hlgFee);
+    (bool success, ) = payable(hToken).call{value: hlgFee}("");
+    require(success, "ETH Transfer failed.");

Note call() introduces the potential for re-entrancy but future proofs ETH transfers into the future in the even that gas usage changes. Functions including call should follow the Check, Effects, Interactions pattern and or Openzeppelin’s ReentrancyGuard.