#37206 [SC-Medium] Overflow due to lack of checks leading to incorrect price calculation

Submitted on Nov 28th 2024 at 20:08:33 UTC by @okmxuse for Audit Comp | Celo

• Report ID: #37206

• Report Type: Smart Contract

• Report severity: Medium

• Target: https://github.com/celo-org/optimism/blob/celo10/op-chain-ops/cmd/check-derivation/main.go

• Impacts:

  • Smart contract unable to operate due to lack of token funds

Description

Inside the check-derivation/main.go function, the getRandomSignedTransaction function is invoked (note that this function traces all the way back to checkConsolidation which is then called in the main.go function ).

getRandomSignedTransaction calls IntrinsicGas at three places. We will focus on the one that includes the accessList, which is case types.AccessListTxType:

=> case types.AccessListTxType:
	accessList := types.AccessList{types.AccessTuple{
		Address:     randomAddress,
		StorageKeys: []common.Hash{common.HexToHash("0x1234")},
	}}
	gasLimit, err := core.IntrinsicGas(data, accessList, false, true, true, false, nil, nil)
	if err != nil {
		return nil, fmt.Errorf("failed to get intrinsicGas: %w", err)
	}

Next, let's look at the IntrinsicGas function:

func IntrinsicGas(data []byte, accessList types.AccessList, isContractCreation, isHomestead, isEIP2028, isEIP3860 bool, feeCurrency *common.Address, feeIntrinsicGas common.IntrinsicGasCosts) (uint64, error) {
	// Set the starting gas for the raw transaction
	var gas uint64
	if isContractCreation && isHomestead {
		gas = params.TxGasContractCreation
	} else {
		gas = params.TxGas
	}
	dataLen := uint64(len(data))
	// Bump the required gas by the amount of transactional data
	if dataLen > 0 {
		// Zero and non-zero bytes are priced differently
		var nz uint64
		for _, byt := range data {
			if byt != 0 {
				nz++
			}
		}
		// Ensure we do not exceed uint64 limits for all data combinations
		nonZeroGas := params.TxDataNonZeroGasFrontier
		if isEIP2028 {
			nonZeroGas = params.TxDataNonZeroGasEIP2028
		}
=>		if (math.MaxUint64-gas)/nonZeroGas < nz {
			return 0, ErrGasUintOverflow
		}
		gas += nz * nonZeroGas

		z := dataLen - nz
=>		if (math.MaxUint64-gas)/params.TxDataZeroGas < z {
			return 0, ErrGasUintOverflow
		}
		gas += z * params.TxDataZeroGas

		if isContractCreation && isEIP3860 {
			lenWords := toWordSize(dataLen)
=>			if (math.MaxUint64-gas)/params.InitCodeWordGas < lenWords {
				return 0, ErrGasUintOverflow
			}
			gas += lenWords * params.InitCodeWordGas
		}
	}

	if feeCurrency != nil {
		intrinsicGasForFeeCurrency, ok := common.CurrencyIntrinsicGasCost(feeIntrinsicGas, feeCurrency)
		if !ok {
			return 0, fmt.Errorf("%w: %x", exchange.ErrUnregisteredFeeCurrency, feeCurrency)
		}
=>		if (math.MaxUint64 - gas) < intrinsicGasForFeeCurrency {
			return 0, ErrGasUintOverflow
		}
		gas += intrinsicGasForFeeCurrency
	}

	if accessList != nil {
=>		gas += uint64(len(accessList)) * params.TxAccessListAddressGas
=>		gas += uint64(accessList.StorageKeys()) * params.TxAccessListStorageKeyGas
	}
	return gas, nil
}

Looking at the arrowed code-lines everywhere in the function, when gas is incremented, overflow checks are performed using conditions like:

if (math.MaxUint64 - gas)/value < multiplier {
	return 0, ErrGasUintOverflow
}

This ensures that the gas does not become bigger than uint64.

However, no overflow check is performed in the final block involving the accessList, specifically at:

	if accessList != nil {
=>		gas += uint64(len(accessList)) * params.TxAccessListAddressGas
=>		gas += uint64(accessList.StorageKeys()) * params.TxAccessListStorageKeyGas
	}

As you can see it simply adds the result to gas without any overflow check.

Impact

This bug can lead to silent overflows, causing:

• Incorrect computation of gas limits for transactions which will cause the complete flow to fail.

Recommendation

Add an overflow check for the gas additions within the accessList block:

	if accessList != nil {
		addressGas := uint64(len(accessList)) * params.TxAccessListAddressGas
		if (math.MaxUint64 - gas) < addressGas {
			return 0, ErrGasUintOverflow
		}
		gas += addressGas

		storageKeyGas := uint64(accessList.StorageKeys()) * params.TxAccessListStorageKeyGas
		if (math.MaxUint64 - gas) < storageKeyGas {
			return 0, ErrGasUintOverflow
		}
		gas += storageKeyGas
	}

This ensures that both address and storage key gas costs are properly validated, preventing silent overflows and ensuring accurate gas calculations.

Proof of Concept

POC

First I modified the main IntrinsicGasfunction slightly to ensure that an overflow is guaranteed:

+	if accessList != nil {
+		// set the gas to uint64 - 2400 to guarantee it to trigger (note this is just for testing simplicity)
+		gas := uint64(18446744073709551615)
+		gas += uint64(len(accessList)) * params.TxAccessListAddressGas
+		gas += uint64(accessList.StorageKeys()) * params.TxAccessListStorageKeyGas
+ // return the gas to log in the test file 
	}
+	return gas, nil
+ }

then create a main_test.go file and past the following:

package main_test

import (
	"math/rand"
	"testing"

	"github.com/ethereum-optimism/optimism/op-service/testutils"
	"github.com/ethereum/go-ethereum/common"
	"github.com/ethereum/go-ethereum/core"
	"github.com/ethereum/go-ethereum/core/types"
)

func Test_Overflow(t *testing.T) {
	rng := rand.New(rand.NewSource(1337))
	data := testutils.RandomData(rng, 10)

	accessList := make(types.AccessList, 5) 
	for i := range accessList {
		accessList[i] = types.AccessTuple{
			Address:     randomAddress(rng),
			StorageKeys: []common.Hash{common.HexToHash("0x1234")},
		}
	}

	// Call IntrinsicGas with the near-overflow AccessList
	gas, err := core.IntrinsicGas(data, accessList, false, true, true, false, nil, nil)

	// Log both the gas value and the error to understand the result
	t.Logf("Returned gas: %d", gas) // Correctly print the gas value
	t.Logf("Caught expected error: %v", err)
}

// Helper function to generate random Ethereum addresses
func randomAddress(rng *rand.Rand) common.Address {
	var addr [20]byte
	rng.Read(addr[:])
	return common.Address(addr)
}

• run go test -v -run Test_Overflow

• Logs:

=== RUN   Test_Overflow
    main_test.go:30: Returned gas: 42660
    main_test.go:31: Caught expected error: <nil>
--- PASS: Test_Overflow (0.00s)
PASS

As we can see the gas has wrapped and now returns 42660.