Execution may be stuck in destination chain as operators estimate gas consumption

[code423n4]

Lines of code

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

Vulnerability details

Description

The Operator CLI uses jobEstimator() static function to calculate the required gas cost to perform executeJob(). The problem is that it overestimates used gas. The gas() opcode is called at the end of the assembly block, but needs to be calculated right after the call and correct for the memory storage of "result" variable:

assembly {
  calldatacopy(0, bridgeInRequestPayload.offset, sub(bridgeInRequestPayload.length, 0x40))
  /**
   * @dev bridgeInRequest doNotRevert is purposefully set to false so a rever would happen
   */
  mstore8(0xE3, 0x00)
  let result := call(gas(), sload(_bridgeSlot), callvalue(), 0, sub(bridgeInRequestPayload.length, 0x40), 0, 0)
  /**
   * @dev if for some reason the call does not revert, it is force reverted
   */
  if eq(result, 1) {
    returndatacopy(0, 0, returndatasize())
    revert(0, returndatasize())
  }
  /**
   * @dev remaining gas is set as the return value
   */
  mstore(0x00, gas())
  return(0x00, 0x20)
}

The impact of this overestimation of gas is that honest operators may reject a job to be executed, as they calculate higher gas consumption which means it will not be executed successfully. Therefore, executions may be stuck on the destination chain indefinitely.

Impact

Executions may be stuck in destination chain.

Tools Used

Manual audit

Recommended Mitigation Steps

Gas needs to be calculated right after the call and correct for the memory storage of "result" variable:

[gzeoneth]

Minor difference, execution won't be stuck as operation only need to supply gas as specified regardless of the estimation result.

[ACC01ADE]

The estimation is accurate, caller of function knows gasIn, function returns gasLeft, caller then knows that gasLimit = gasIn - gasLeft. Whether or not an operator trusts the formula, follows it, or refuses to operate is entirely out of scope.

[ACC01ADE]

To clarify, the estimation is accurate in the sense that it will have more gas limit available than is actually used. Meaning that there will be a bit of dust wei leftover still. An operator should never be in the negative if executing a job with this gas limit being set.

[trust1995]

The issue highlights that the gas which sender pays for, which is just the bridgeIn request, is less than what the operator estimates it will cost, because of the surrounding overhead. Therefore, it may be the case that operator decides the gas cost is too expensive and refuse to execute it, although user was honest and specified gas correctly. Does that make sense?

[gzeoneth]

Agree there would be an edge case that if the user submit the exact amount of gas will be rejected by operator. The risk seems to be low because at worst this should only cause some delay.

[trust1995]

I think it would cause more than delay, because if operator would refuse to executeJob this operation, the NFT would not be available in either chain.
The current code doesn't support recovery of the NFT in this scenario

[gzeoneth]

I think it would cause more than delay, because if operator would refuse to executeJob this operation, the NFT would not be available in either chain. The current code doesn't support recovery of the NFT in this scenario

I think executeJob is permissionless after certain delay?

[trust1995]

Sure, but why would any executor take up an execution that is calculated to be too expensive?

[gzeoneth]

Sure, but why would any executor take up an execution that is calculated to be too expensive?

The user himself can execute the job, my point being the nft would not be lost. And I believe in most of the case the flow should prompt the user to include sufficient buffer to avoid this situation.

[trust1995]

Agree that user can always self execute, if it wasn't the case it would be HIGH risk as there is actually no way to rescue the NFT. However we are still talking about:

1. temporary freeze of asset until user self rescues. That itself is M/H level impact in most bug bounties I've come across (Latest immunefi bug bounty here)
2. user is forced to become an operator, stake funds, run CLI client etc. Definitely not what they signed up for. M is applicable when assets are not at direct risk, but some basic functionality of the protocol is impaired, like in our case.

[gzeoneth]

The fact that this require the user to intentionally submit the exact gas limit manually (which can be reasonably assumed to be prevented by UI) and an attacker cannot force a user into such situation (delay attack is not possible) make this issue low risk.

[trust1995]

Don't agree that this could only happen when user manually submits. We have no information ahead of time about how the UI works and whether it passes a spare gas limit over the actual gas amount. If it passes gas limit without buffers, the overestimation would lead gas limit to be too high and therefore not attractive for honest operators.
I've made my case. If judge's opinion is still Low I accept it.

[gzeoneth]

Judging this as low risk and as a QA report