Incorrect usage of try/catch block

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/src/HolographBridge.sol#L261

Vulnerability details

Vulnerability details

Description

There is a function getBridgeOutRequestPayload in HolographBridge contract. It has the following:

/**
 * @dev the revertedBridgeOutRequest function is wrapped into a try/catch function
 */
try this.revertedBridgeOutRequest(msg.sender, toChain, holographableContract, bridgeOutPayload) returns (
  string memory revertReason
) {
  /**
   * @dev a non reverted result is actually a revert
   */
  revert(revertReason);
} catch (bytes memory realResponse) {
  /**
   * @dev a revert is actually success, so the return data is stored as payload
   */
  payload = realResponse;
}

Actually, such a call inside the try block can fail with out of gas error, but the execution can continue.

According to EIP-150 call opcode can consume at most 63/64 gas of parent call, in other words, the child call can consume "all but one 64th" of the gas, and parent calls will have at least 1/64 of gas to continue the execution. So, there is a possibility to continue the execution of the transaction after failing in such a scenario through catch statement. So, such a revert does not mean success, as it is stated in the comment above.

The same applies to all the places in the repo where try + catch construction is used.

Recommended Mitigation Steps

Handle such cases of errors in special catch statements.

[trust1995]

Interesting and correct observation. I would like to know what is the discovered impact of this finding, as when we submit M/H severity we need to argue for actual potential damage. I found a location where this assumption causes operator to be able to fail legit TXs.

437 for reference.

[gzeoneth]

technically correct but without poc, in most case it will revert oog with only 1/64 gas available also duplicate of #176

[ACC01ADE]

Would like to just make a small note that the getBridgeOutRequestPayload function is used with eth_call as a front-end helper function. Running it via eth_send should have no consequence whether it reverts or not.