Bahamut L1 Blockchain Security Review Report

calldata is counted in two different ways: when sending a transaction from an EOA to a contract, and when sending a transaction from a contract to a contract. Function memoryGas() does not take this fact into account. When sending a transaction from contract-to-contract, gas is calculated as follows - for zero bytes, the price for gas is 4, for non-zero bytes, the price for gas is 16.

When sending a transaction from a contract-to-contract, function memoryGas() will calculate the gas according to the standard formula, while the user will receive more activity points than he spent gas.

For example, by sending such a transaction, and filling the block with 30 million gas, we will receive 100 million gas.

func (evm *EVM) memoryGas(data []byte) (uint64, error) {
    var gas uint64
    rules := evm.ChainConfig().Rules(evm.Context.BlockNumber, evm.Context.Random != nil)
    if len(data) > 0 {
        // Zero and non-zero bytes are priced differently
        var nz uint64
        for _, byt := range data {
            if byt != 0 {
                nz++
            }
        }
        // Make sure we don't exceed uint64 for all data combinations
        nonZeroGas := params.TxDataNonZeroGasFrontier
        if rules.IsIstanbul {
            nonZeroGas = params.TxDataNonZeroGasEIP2028
        }
        if (math.MaxUint64-gas)/nonZeroGas < nz {
            return 0, ErrGasUintOverflow
        }
        gas += nz * nonZeroGas
        z := uint64(len(data)) - nz
        if (math.MaxUint64-gas)/params.TxDataZeroGas < z {
            return 0, ErrGasUintOverflow
        }
        gas += z * params.TxDataZeroGas
    }
    return gas, nil
}

We suggest not counting gas either when creating an EOA-to-contract transaction or when creating a contract-to-contract transaction.

In functions CallCode and DelegateCall, activity parameters are incorrectly calculated.

The problem is that when the DelegateCall and CallCode are called, the execution context changes. This can cause problems when counting activity scores.

For example, when using proxy pattern, activity will be credited to the implementation contract. This can cause problems for the validator, since when the implementation changes, all activity points will be lost.

CallCode:

func (evm *EVM) CallCode(caller ContractRef, addr common.Address, input []byte, gas uint64, value *big.Int) (ret []byte, leftOverGas uint64, err error) {
    // Fail if we're trying to execute above the call depth limit
    if evm.depth > int(params.CallCreateDepth) {
        return nil, gas, ErrDepth
    }
    initialGas := gas
    // Fail if we're trying to transfer more than the available balance
    // Note although it's noop to transfer X ether to caller itself. But
    // if caller doesn't have enough balance, it would be an error to allow
    // over-charging itself. So the check here is necessary.
    if !evm.Context.CanTransfer(evm.StateDB, caller.Address(), value) {
        return nil, gas, ErrInsufficientBalance
    }
    snapshot := evm.StateDB.Snapshot()
    // Invoke tracer hooks that signal entering/exiting a call frame
    if evm.Config.Debug {
        evm.Config.Tracer.CaptureEnter(CALLCODE, caller.Address(), addr, input, gas, value)
        defer func(startGas uint64) {
            evm.Config.Tracer.CaptureExit(ret, startGas-gas, err)
        }(gas)
    }
    // It is allowed to call precompiles, even via delegatecall
    if p, isPrecompile := evm.precompile(addr); isPrecompile {
        ret, gas, err = RunPrecompiledContract(p, input, gas)
    } else {
        addrCopy := addr
        // Initialise a new contract and set the code that is to be used by the EVM.
        // The contract is a scoped environment for this execution context only.
        contract := NewContract(caller, AccountRef(caller.Address()), value, gas)
        contract.SetCallCode(&addrCopy, evm.StateDB.GetCodeHash(addrCopy), evm.StateDB.GetCode(addrCopy))
        ret, err = evm.interpreter.Run(contract, input, false)
        gas = contract.Gas
 
        memGas, err := evm.memoryGas(input)
        if err != nil {
            return nil, gas, err
        }
        if caller.Address() != evm.Origin {
            memGas = 0
        }
        evm.StateDB.AddActivity(addrCopy, initialGas-contract.Gas-contract.OthersGas+memGas)
        evm.StateDB.AddActivities(&types.Activity{
            Address:       addrCopy,
            Activity:      evm.StateDB.GetActivity(addrCopy),
            DeltaActivity: initialGas - contract.Gas - contract.OthersGas,
        })
    }
    if err != nil {
        evm.StateDB.RevertToSnapshot(snapshot)
        if err != ErrExecutionReverted {
            gas = 0
        }
    }
    return ret, gas, err
}

DelegateCall:

func (evm *EVM) DelegateCall(caller ContractRef, addr common.Address, input []byte, gas uint64) (ret []byte, leftOverGas uint64, err error) {
    // Fail if we're trying to execute above the call depth limit
    if evm.depth > int(params.CallCreateDepth) {
        return nil, gas, ErrDepth
    }
    initialGas := gas
    snapshot := evm.StateDB.Snapshot()
    // Invoke tracer hooks that signal entering/exiting a call frame
    if evm.Config.Debug {
        evm.Config.Tracer.CaptureEnter(DELEGATECALL, caller.Address(), addr, input, gas, nil)
        defer func(startGas uint64) {
            evm.Config.Tracer.CaptureExit(ret, startGas-gas, err)
        }(gas)
    }
    // It is allowed to call precompiles, even via delegatecall
    if p, isPrecompile := evm.precompile(addr); isPrecompile {
        ret, gas, err = RunPrecompiledContract(p, input, gas)
    } else {
        addrCopy := addr
        // Initialise a new contract and make initialise the delegate values
        contract := NewContract(caller, AccountRef(caller.Address()), nil, gas).AsDelegate()
        contract.SetCallCode(&addrCopy, evm.StateDB.GetCodeHash(addrCopy), evm.StateDB.GetCode(addrCopy))
        ret, err = evm.interpreter.Run(contract, input, false)
        gas = contract.Gas
 
        evm.StateDB.AddActivity(addrCopy, initialGas-contract.Gas-contract.OthersGas)
        evm.StateDB.AddActivities(&types.Activity{
            Address:       addrCopy,
            Activity:      evm.StateDB.GetActivity(addrCopy),
            DeltaActivity: initialGas - contract.Gas - contract.OthersGas,
        })
    }
 
    if err != nil {
        evm.StateDB.RevertToSnapshot(snapshot)
        if err != ErrExecutionReverted {
            gas = 0
        }
    }
    return ret, gas, err
}

Remove activity score calculations from DelegateCall and CallCode.

Function ProcessDeposit() is responsible for adding new validators and their contracts. This function calls function appendValidatorContractsWithVal() which adds a new contract to the given validator. But each validator can only have one contract tied to its address.

ProcessDeposit():

func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verifySignature bool) (state.BeaconState, bool, error) {
    var newValidator bool
    if err := verifyDeposit(beaconState, deposit); err != nil {
        if deposit == nil || deposit.Data == nil {
            return nil, newValidator, err
        }
        return nil, newValidator, errors.Wrapf(err, "could not verify deposit from %#x", bytesutil.Trunc(deposit.Data.PublicKey))
    }
    if err := beaconState.SetEth1DepositIndex(beaconState.Eth1DepositIndex() + 1); err != nil {
        return nil, newValidator, err
    }
    pubKey := deposit.Data.PublicKey
    amount := deposit.Data.Amount
    contract := deposit.Data.DeployedContract
    owner, contractExist := beaconState.ValidatorIndexByContractAddress(bytesutil.ToBytes20(contract))
    if contractExist {
        log.Debugf("Contract %x already regestered by validator %d. Register new validator with 0x0 contract address", contract, owner)
    }
 
    index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey))
    if !ok {
        if verifySignature {
            domain, err := signing.ComputeDomain(params.BeaconConfig().DomainDeposit, nil, nil)
            if err != nil {
                return nil, newValidator, err
            }
            if err := verifyDepositDataSigningRoot(deposit.Data, domain); err != nil {
                // Ignore this error as in the spec pseudo code.
                log.WithError(err).Debug("Skipping deposit: could not verify deposit data signature")
                return beaconState, newValidator, nil
            }
        }
 
        effectiveBalance := amount - (amount % params.BeaconConfig().EffectiveBalanceIncrement)
        if params.BeaconConfig().MaxEffectiveBalance < effectiveBalance {
            effectiveBalance = params.BeaconConfig().MaxEffectiveBalance
        }
 
        if err := beaconState.AppendValidator(&ethpb.Validator{
            PublicKey:                  pubKey,
            WithdrawalCredentials:      deposit.Data.WithdrawalCredentials,
            ActivationEligibilityEpoch: params.BeaconConfig().FarFutureEpoch,
            ActivationEpoch:            params.BeaconConfig().FarFutureEpoch,
            ExitEpoch:                  params.BeaconConfig().FarFutureEpoch,
            WithdrawableEpoch:          params.BeaconConfig().FarFutureEpoch,
            EffectiveBalance:           effectiveBalance,
        }); err != nil {
            return nil, newValidator, err
        }
        newValidator = true
        if err := beaconState.AppendBalance(amount); err != nil {
            return nil, newValidator, err
        }
        if err := beaconState.AppendActivity(0); err != nil {
            return nil, newValidator, err
        }
        contracts := [][]byte{contract}
        if contractExist {
            contracts = [][]byte{make([]byte, 20)}
        }
        if err := beaconState.AppendContracts(&ethpb.ContractsContainer{
            Contracts: contracts,
        }); err != nil {
            return nil, newValidator, err
        }
    } else {
        if err := helpers.IncreaseBalance(beaconState, index, amount); err != nil {
            return nil, newValidator, err
        }
        if !contractExist && !isZeroContract(contract) {
            if err := helpers.AppendValidatorContracts(beaconState, index, contract); err != nil {
                return nil, newValidator, err
            }
        }
    }
 
    return beaconState, newValidator, nil
}

appendValidatorContractsWithVal()

func appendValidatorContractsWithVal(cc *ethpb.ContractsContainer, contract []byte) *ethpb.ContractsContainer {
    contracts := cc.Contracts
    contracts = append(contracts, contract)
    cc.Contracts = contracts
    return cc
}

Delete the old contract before adding a new one.

When adding a validator's contract, if the contract exists, a new contract object with a zero address is added.

The addition of a null contract is due to the fact that the check for the existence of a contract and its addition occur in different if blocks.

func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verifySignature bool) (state.BeaconState, bool, error) {
    [...]
    index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey))
    if !ok {
        [...]
        if contractExist {
            contracts = [][]byte{make([]byte, 20)}
        }
        if err := beaconState.AppendContracts(&ethpb.ContractsContainer{
            Contracts: contracts,
        }); err != nil {
            return nil, newValidator, err
        }
    } else {
        [...]
    }
 
    return beaconState, newValidator, nil
}

For example:

func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verifySignature bool) (state.BeaconState, bool, error) {
    [...]
    index, ok := beaconState.ValidatorIndexByPubkey(bytesutil.ToBytes48(pubKey))
    if !ok {
        [...]
        if contractExist {
            contracts = [][]byte{make([]byte, 20)}
        } else {
            err := beaconState.AppendContracts(&ethpb.ContractsContainer{Contracts: contracts,})
            if err != nil {
                return nil, newValidator, err
            }
        }
    } else {
        [...]
    }
 
    return beaconState, newValidator, nil
}

Function ProcessTransactionsGasPerPeriodUpdate() calculates the gas update transaction per period. In the calculation of gas for the period, it is multiplied by the constant cost of the transaction. This value is hardcoded, which can lead to problems when changing the constant cost per transaction.

func ProcessTransactionsGasPerPeriodUpdate(state state.BeaconState) (state.BeaconState, error) {
    period := uint64(params.BeaconConfig().EpochsPerActivityPeriod)
    txsPerPeriod := ((state.TransactionsGasPerPeriod()+state.TransactionsPerLatestEpoch()*21000)*period - state.TransactionsGasPerPeriod()) / periodc
    if err := state.SetTransactionsGasPerPeriod(txsPerPeriod); err != nil {
        return nil, err
    }
    if err := state.SetTransactionsPerLatestEpoch(0); err != nil {
        return nil, err
    }
 
    return state, nil
}

Use constant variables instead of hardcoded values.

Function TransitionDb() will transition the state by applying the current message and returning the evm execution result. During the execution of the TransitionDb(), activity scores are logged before and after refunds. Since function TransitionDb() will be called during the execution of any transaction, logging will also occur during any transaction, which can significantly slow down the blockchain as a whole.

func (st *StateTransition) TransitionDb() (*ExecutionResult, error) {
    [...]
 
    for _, act := range st.state.GetCurrentActivities() {
        log.Debug("Activities before refund", "address", act.Address, "activity", act.Activity, "delta", act.DeltaActivity) // @note logs in for loops
    }
 
    [...]
 
    for _, act := range st.state.GetCurrentActivities() {
        log.Debug("Activities after refund", "address", act.Address, "activity", act.Activity, "delta", act.DeltaActivity)
    }
 
    [...]
}

We would suggest implementing the logging logic, in which only those activity parameters that were changed during the transaction will be included in the logs.

At runtime, function CallCode() checks if the address of the caller is the address that created the transaction. But due to the way opcode CallCode() works, the address of caller never changes.

func (evm *EVM) CallCode(caller ContractRef, addr common.Address, input []byte, gas uint64, value *big.Int) (ret []byte, leftOverGas uint64, err error) {
    [...]
    if p, isPrecompile := evm.precompile(addr); isPrecompile {
        ret, gas, err = RunPrecompiledContract(p, input, gas)
    } else {
        [...]
        if caller.Address() != evm.Origin {
            memGas = 0
        }
        [...]
    }
    [...]
}

Remove redundant checks to optimize the code.

The documentation for function ProcessNonStakersGasPerPeriodUpdate() uses the name of function ProcessTransactionsGasPerPeriodUpdate().

// ProcessTransactionsGasPerPeriodUpdate processes the updates to non stakers gas per period.
func ProcessNonStakersGasPerPeriodUpdate(state state.BeaconState) (state.BeaconState, error) {
    period := uint64(params.BeaconConfig().EpochsPerActivityPeriod)
    nonStakersGasPerPeriod := ((state.NonStakersGasPerPeriod()+state.NonStakersGasPerEpoch())*period - state.NonStakersGasPerPeriod()) / period
    if err := state.SetNonStakersGasPerPeriod(nonStakersGasPerPeriod); err != nil {
        return nil, err
    }
    if err := state.SetNonStakersGasPerEpoch(0); err != nil {
        return nil, err
    }
 
    return state, nil
}

Function memoryGas() calculates the price of calldata during the execution of the Call() function. During the execution of the function memoryGas(), checks are made to prevent variable gas from overflowing. These checks are redundant since the blocksize will not allow variable gas to overflow.

func (evm *EVM) memoryGas(data []byte) (uint64, error) {
    [...]
    if len(data) > 0 {
        [...]
        nonZeroGas := params.TxDataNonZeroGasFrontier
        if rules.IsIstanbul {
            nonZeroGas = params.TxDataNonZeroGasEIP2028
        }
        if (math.MaxUint64)/nonZeroGas < nz { // @note redundant check (block limit)
            return 0, ErrGasUintOverflow
        }
        gas += nz * nonZeroGas
 
        z := uint64(len(data)) - nz
        if (math.MaxUint64-gas)/params.TxDataZeroGas < z { // @note redundant check
            return 0, ErrGasUintOverflow
        }
        gas += z * params.TxDataZeroGas
    }
    return gas, nil
}

Remove redundant checks to optimize code performance.