sherlock-audit / 2024-08-tokamak-network-judging

1 stars 0 forks source link

GGONE - Gas Overestimation May Lead to Transaction Rejection #49

Open sherlock-admin2 opened 1 month ago

sherlock-admin2 commented 1 month ago

GGONE

Medium

Gas Overestimation May Lead to Transaction Rejection

Summary

Reference Link

https://github.com/sherlock-audit/2023-01-optimism-judging/issues/235

In the contract’s gas limit calculation, there is no differentiation between zero and non-zero byte gas costs, leading to an overestimation of gas limits. Zero bytes require only 4 gas, but the contract calculates using a uniform 16 gas/byte. This overestimation may cause the transaction gas limit to exceed actual requirements, resulting in rejection by OptimismPortal or miners.

Vulnerability Detail

This is MigrateWithdrawal() code: https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/op-chain-ops/crossdomain/migrate.go#L27-L73

func MigrateWithdrawal(
  withdrawal *LegacyWithdrawal,
  l1CrossDomainMessenger *common.Address,
  chainID *big.Int,
) (*Withdrawal, error) {
  // Attempt to parse the value
  value, err := withdrawal.Value()
  if err != nil {
      return nil, fmt.Errorf("cannot migrate withdrawal: %w", err)
  }

  abi, err := bindings.L1CrossDomainMessengerMetaData.GetAbi()
  if err != nil {
      return nil, err
  }

  // Migrated withdrawals are specified as version 0. Both the
  // L2ToL1MessagePasser and the CrossDomainMessenger use the same
  // versioning scheme. Both should be set to version 0
  versionedNonce := EncodeVersionedNonce(withdrawal.XDomainNonce, new(big.Int))
  // Encode the call to `relayMessage` on the `CrossDomainMessenger`.
  // The minGasLimit can safely be 0 here.
  data, err := abi.Pack(
      "relayMessage",
      versionedNonce,
      withdrawal.XDomainSender,
      withdrawal.XDomainTarget,
      value,
      new(big.Int),
      []byte(withdrawal.XDomainData),
  )
  if err != nil {
      return nil, fmt.Errorf("cannot abi encode relayMessage: %w", err)
  }

  gasLimit := MigrateWithdrawalGasLimit(data, chainID)

  w := NewWithdrawal(
      versionedNonce,
      &predeploys.L2CrossDomainMessengerAddr,
      l1CrossDomainMessenger,
      value,
      new(big.Int).SetUint64(gasLimit),
      data,
  )
  return w, nil
}

RelayPerByteDataCost : https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/op-chain-ops/crossdomain/migrate.go#L15-23

var (
  RelayConstantOverhead            uint64 = 200_000
  RelayPerByteDataCost             uint64 = params.TxDataNonZeroGasEIP2028
  MinGasDynamicOverheadNumerator   uint64 = 64
  MinGasDynamicOverheadDenominator uint64 = 63
  RelayCallOverhead                uint64 = 40_000
  RelayReservedGas                 uint64 = 40_000
  RelayGasCheckBuffer              uint64 = 5_000
)

params.TxDataNonZeroGasEIP2028: https://github.com/ethereum/go-ethereum/blob/master/params/protocol_params.go#L94

  TxDataNonZeroGasEIP2028   uint64 = 16   // Per byte of non zero data attached to a transaction after EIP 2028 (part in Istanbul)

https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/op-chain-ops/crossdomain/migrate.go#L77-L81

func MigrateWithdrawalGasLimit(data []byte, chainID *big.Int) uint64 {
  // Compute the upper bound on the gas limit. This could be more
  // accurate if individual 0 bytes and non zero bytes were accounted
  // for.
  dataCost := uint64(len(data)) * RelayPerByteDataCost

https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol#L346-L351

    function baseGas(bytes calldata _message, uint32 _minGasLimit) public pure returns (uint64) {
        return
        // Constant overhead
        RELAY_CONSTANT_OVERHEAD
        // Calldata overhead
        + (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD)
        ....

https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol#L105C2-L105C59

    uint64 public constant MIN_GAS_CALLDATA_OVERHEAD = 16;

In the MigrateWithdrawal() and baseGas() functions, the gas limit is calculated assuming all bytes in the message consume 16 gas/byte, without accounting for the fact that zero bytes only consume 4 gas. This overestimation can lead to excessively high gas limits for transactions with large amounts of data, potentially causing execution issues.

The specific code snippets are as follows:


// Gas estimation in baseGas
(uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD)

// Gas estimation in MigrateWithdrawal
dataCost := uint64(len(data)) * RelayPerByteDataCost

Both functions apply a uniform gas cost of 16 gas/byte, failing to differentiate between zero and non-zero bytes, resulting in gas overestimation.

Impact

Overestimated gas limits can cause the transaction to be rejected by OptimismPortal or miners due to the mismatch between the estimated gas and the actual requirements.

If the gas limit exceeds the Ethereum L1 block gas limit (typically 30M), the transaction may not be relayed to L1, affecting cross-chain message delivery.

Code Snippet

Tool used

Manual Review

Recommendation

It is recommended to adjust the gas limit calculation by distinguishing between zero and non-zero bytes. Zero bytes should be charged at 4 gas/byte, while non-zero bytes should remain at 16 gas/byte. This will make the gas limit more accurate and reduce the risk of transaction rejection.