LayerZeroModule miscalculates gas, risking loss of assets

[code423n4]

Lines of code

https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/module/LayerZeroModule.sol#L431-L445

Vulnerability details

Description

Holograph gets it's cross chain messaging primitives through Layer Zero. To get pricing estimate, it uses the DstConfig price struct exposed in LZ's RelayerV2

The issue is that the important baseGas and gasPerByte configuration parameters, which are used to calculate a custom amount of gas for the destination LZ message, use the values that come from the source chain. This is in contrast to LZ which handles DstConfigs in a mapping keyed by chainID. The encoded gas amount is described here

Impact

The impact is that when those fields are different between chains, one of two things may happen:

1. Less severe - we waste excess gas, which is refunded to the lzReceive() caller (Layer Zero)
2. More severe - we underprice the delivery cost, causing lzReceive() to revert and the NFT stuck in limbo forever.

The code does not handle a failed lzReceive (differently to a failed executeJob). Therefore, no failure event is emitted and the NFT is screwed.

Tools Used

Manual audit

Recommended Mitigation Steps

Firstly,make sure to use the target gas costs.
Secondly, re-engineer lzReceive to be fault-proof, i.e. save some gas to emit result event.

[gzeoneth]

Might also cause the LZ channel to stuck #244

[ACC01ADE]

I respectfully disagree that this is even a valid issue.
@trust1995 please re-review the affected code. You'll notice that we are in fact extracting destination chain gas data. And if you review the 100s of cross-chain testnet transactions that we have already made with that version of code, you will notice that the math if exact.

[ACC01ADE]

Maybe I am miss-understanding something, so some clarification would be great if you think I'm wrong on this.

[trust1995]

Please take a look at LayerZeroModule.sol 's send function:

function send(
  uint256, /* gasLimit*/
  uint256, /* gasPrice*/
  uint32 toChain,
  address msgSender,
  uint256 msgValue,
  bytes calldata crossChainPayload
) external payable {
  require(msg.sender == address(_operator()), "HOLOGRAPH: operator only call");
  LayerZeroOverrides lZEndpoint;
  assembly {
    lZEndpoint := sload(_lZEndpointSlot)
  }
  // need to recalculate the gas amounts for LZ to deliver message
  lZEndpoint.send{value: msgValue}(
    uint16(_interfaces().getChainId(ChainIdType.HOLOGRAPH, uint256(toChain), ChainIdType.LAYERZERO)),
    abi.encodePacked(address(this), address(this)),
    crossChainPayload,
    payable(msgSender),
    address(this),
    abi.encodePacked(uint16(1), uint256(_baseGas() + (crossChainPayload.length * _gasPerByte())))
  );
}

The function uses _baseGas() and _gasPerByte() as the relayer adapter parameters as described in the submission description's link. These two getters are global for all chains.

I agree that the getMessage() function takes into account the correct fees for the destination chain.

[ACC01ADE]

Please take a look at LayerZeroModule.sol 's send function:

function send(
  uint256, /* gasLimit*/
  uint256, /* gasPrice*/
  uint32 toChain,
  address msgSender,
  uint256 msgValue,
  bytes calldata crossChainPayload
) external payable {
  require(msg.sender == address(_operator()), "HOLOGRAPH: operator only call");
  LayerZeroOverrides lZEndpoint;
  assembly {
    lZEndpoint := sload(_lZEndpointSlot)
  }
  // need to recalculate the gas amounts for LZ to deliver message
  lZEndpoint.send{value: msgValue}(
    uint16(_interfaces().getChainId(ChainIdType.HOLOGRAPH, uint256(toChain), ChainIdType.LAYERZERO)),
    abi.encodePacked(address(this), address(this)),
    crossChainPayload,
    payable(msgSender),
    address(this),
    abi.encodePacked(uint16(1), uint256(_baseGas() + (crossChainPayload.length * _gasPerByte())))
  );
}

The function uses _baseGas() and _gasPerByte() as the relayer adapter parameters as described in the submission description's link. These two getters are global for all chains.

I agree that the getMessage() function takes into account the correct fees for the destination chain.

Ya but these refer to destination gas limits. BaseGas and GasPerByte is the amount of gas that is used by the crossChainMessage function that LayerZero triggers on cross-chain call https://github.com/code-423n4/2022-10-holograph/blob/f8c2eae866280a1acfdc8a8352401ed031be1373/contracts/HolographOperator.sol#L484

[ACC01ADE]

Discussed this in more detail with @trust1995, definitely a critical issue.
Need to add destination chain-specific _baseGas and _gasPerByte to mitigate EVM differences in opcode costs.