fix: vm.WithUNSAFECallerAddressProxying under DELEGATECALL (#50)

* fix: `vm.WithUNSAFECallerAddressProxying` under `DELEGATECALL`

* test: `vm.WithUNSAFECallerAddressProxying()` effect on outgoing caller addr

* chore: mark `eth/tracers/js` test flaky

* feat: `vm.PrecompileEnvironment.IncomingCallType()`

* chore: minor documentation edit

* doc: `PrecompileEnvironment` example for determining actual caller

* chore: placate the linter

.github/workflows/go.yml

@@ -18,6 +18,6 @@ jobs:
           go-version: 1.21.4
       - name: Run tests
         run: | # Upstream flakes are race conditions exacerbated by concurrent tests
-          FLAKY_REGEX='go-ethereum/(eth|eth/tracers/logger|accounts/keystore|eth/downloader|miner|ethclient|ethclient/gethclient|eth/catalyst)$';
+          FLAKY_REGEX='go-ethereum/(eth|eth/tracers/js|eth/tracers/logger|accounts/keystore|eth/downloader|miner|ethclient|ethclient/gethclient|eth/catalyst)$';
           go list ./... | grep -P "${FLAKY_REGEX}" | xargs -n 1 go test -short;
           go test -short $(go list ./... | grep -Pv "${FLAKY_REGEX}");

core/vm/contracts.libevm.go

@@ -42,7 +42,7 @@ import (
 //	    args := &evmCallArgs{evm, staticCall, caller, addr, input, gas, nil /*value*/}
 type evmCallArgs struct {
 	evm      *EVM
-	callType callType
+	callType CallType
 
 	// args:start
 	caller ContractRef
@@ -53,15 +53,32 @@ type evmCallArgs struct {
 	// args:end
 }
 
-type callType uint8
+// A CallType refers to a *CALL* [OpCode] / respective method on [EVM].
+type CallType uint8
 
 const (
-	call callType = iota + 1
+	UnknownCallType CallType = iota
-	callCode
+	Call
-	delegateCall
+	CallCode
-	staticCall
+	DelegateCall
+	StaticCall
 )
 
+// String returns a human-readable representation of the CallType.
+func (t CallType) String() string {
+	switch t {
+	case Call:
+		return "Call"
+	case CallCode:
+		return "CallCode"
+	case DelegateCall:
+		return "DelegateCall"
+	case StaticCall:
+		return "StaticCall"
+	}
+	return fmt.Sprintf("Unknown %T(%d)", t, t)
+}
+
 // run runs the [PrecompiledContract], differentiating between stateful and
 // regular types.
 func (args *evmCallArgs) run(p PrecompiledContract, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) {
@@ -115,6 +132,7 @@ type PrecompileEnvironment interface {
 	// ReadOnlyState will always be non-nil.
 	ReadOnlyState() libevm.StateReader
 	Addresses() *libevm.AddressContext
+	IncomingCallType() CallType
 
 	BlockHeader() (types.Header, error)
 	BlockNumber() *big.Int
@@ -132,46 +150,30 @@ func (args *evmCallArgs) env() *environment {
 		value = args.value
 	)
 	switch args.callType {
-	case staticCall:
+	case StaticCall:
 		value = new(uint256.Int)
 		fallthrough
-	case call:
+	case Call:
 		self = args.addr
 
-	case delegateCall:
+	case DelegateCall:
 		value = nil
 		fallthrough
-	case callCode:
+	case CallCode:
 		self = args.caller.Address()
 	}
 
 	// This is equivalent to the `contract` variables created by evm.*Call*()
 	// methods, for non precompiles, to pass to [EVMInterpreter.Run].
 	contract := NewContract(args.caller, AccountRef(self), value, args.gas)
-	if args.callType == delegateCall {
+	if args.callType == DelegateCall {
 		contract = contract.AsDelegate()
 	}
 
 	return &environment{
-		evm:           args.evm,
+		evm:      args.evm,
-		self:          contract,
+		self:     contract,
-		forceReadOnly: args.readOnly(),
+		callType: args.callType,
-	}
-}
-
-func (args *evmCallArgs) readOnly() bool {
-	// A switch statement provides clearer code coverage for difficult-to-test
-	// cases.
-	switch {
-	case args.callType == staticCall:
-		// evm.interpreter.readOnly is only set to true via a call to
-		// EVMInterpreter.Run() so, if a precompile is called directly with
-		// StaticCall(), then readOnly might not be set yet.
-		return true
-	case args.evm.interpreter.readOnly:
-		return true
-	default:
-		return false
 	}
 }
 

core/vm/contracts.libevm_test.go

@@ -16,6 +16,7 @@
 package vm_test
 
 import (
+	"bytes"
 	"fmt"
 	"math/big"
 	"reflect"
@@ -106,6 +107,7 @@ type statefulPrecompileOutput struct {
 	BlockNumber, Difficulty *big.Int
 	BlockTime               uint64
 	Input                   []byte
+	IncomingCallType        vm.CallType
 }
 
 func (o statefulPrecompileOutput) String() string {
@@ -121,6 +123,8 @@ func (o statefulPrecompileOutput) String() string {
 			verb = "%#x"
 		case *libevm.AddressContext:
 			verb = "%+v"
+		case vm.CallType:
+			verb = "%d (%[2]q)"
 		}
 		lines = append(lines, fmt.Sprintf("%s: "+verb, name, fld))
 	}
@@ -149,14 +153,15 @@ func TestNewStatefulPrecompile(t *testing.T) {
 		}
 
 		out := &statefulPrecompileOutput{
-			ChainID:     env.ChainConfig().ChainID,
+			ChainID:          env.ChainConfig().ChainID,
-			Addresses:   env.Addresses(),
+			Addresses:        env.Addresses(),
-			StateValue:  env.ReadOnlyState().GetState(precompile, slot),
+			StateValue:       env.ReadOnlyState().GetState(precompile, slot),
-			ReadOnly:    env.ReadOnly(),
+			ReadOnly:         env.ReadOnly(),
-			BlockNumber: env.BlockNumber(),
+			BlockNumber:      env.BlockNumber(),
-			BlockTime:   env.BlockTime(),
+			BlockTime:        env.BlockTime(),
-			Difficulty:  hdr.Difficulty,
+			Difficulty:       hdr.Difficulty,
-			Input:       input,
+			Input:            input,
+			IncomingCallType: env.IncomingCallType(),
 		}
 		return out.Bytes(), suppliedGas - gasCost, nil
 	}
@@ -199,6 +204,7 @@ func TestNewStatefulPrecompile(t *testing.T) {
 		// Note that this only covers evm.readOnly being true because of the
 		// precompile's call. See TestInheritReadOnly for alternate case.
 		wantReadOnly bool
+		wantCallType vm.CallType
 	}{
 		{
 			name: "EVM.Call()",
@@ -211,6 +217,7 @@ func TestNewStatefulPrecompile(t *testing.T) {
 				Self:   precompile,
 			},
 			wantReadOnly: false,
+			wantCallType: vm.Call,
 		},
 		{
 			name: "EVM.CallCode()",
@@ -223,6 +230,7 @@ func TestNewStatefulPrecompile(t *testing.T) {
 				Self:   caller,
 			},
 			wantReadOnly: false,
+			wantCallType: vm.CallCode,
 		},
 		{
 			name: "EVM.DelegateCall()",
@@ -235,6 +243,7 @@ func TestNewStatefulPrecompile(t *testing.T) {
 				Self:   caller,
 			},
 			wantReadOnly: false,
+			wantCallType: vm.DelegateCall,
 		},
 		{
 			name: "EVM.StaticCall()",
@@ -247,20 +256,22 @@ func TestNewStatefulPrecompile(t *testing.T) {
 				Self:   precompile,
 			},
 			wantReadOnly: true,
+			wantCallType: vm.StaticCall,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			wantOutput := statefulPrecompileOutput{
-				ChainID:     chainID,
+				ChainID:          chainID,
-				Addresses:   tt.wantAddresses,
+				Addresses:        tt.wantAddresses,
-				StateValue:  value,
+				StateValue:       value,
-				ReadOnly:    tt.wantReadOnly,
+				ReadOnly:         tt.wantReadOnly,
-				BlockNumber: header.Number,
+				BlockNumber:      header.Number,
-				BlockTime:   header.Time,
+				BlockTime:        header.Time,
-				Difficulty:  header.Difficulty,
+				Difficulty:       header.Difficulty,
-				Input:       input,
+				Input:            input,
+				IncomingCallType: tt.wantCallType,
 			}
 
 			wantGasLeft := gasLimit - gasCost
@@ -375,10 +386,12 @@ func makeReturnProxy(t *testing.T, dest common.Address, call vm.OpCode) []vm.OpC
 	t.Helper()
 	const p0 = vm.PUSH0
 	contract := []vm.OpCode{
-		vm.PUSH1, 1, // retSize (bytes)
+		vm.CALLDATASIZE, p0, p0, vm.CALLDATACOPY,
-		p0, // retOffset
+
-		p0, // argSize
+		p0,              // retSize
-		p0, // argOffset
+		p0,              // retOffset
+		vm.CALLDATASIZE, // argSize
+		p0,              // argOffset
 	}
 
 	// See CALL signature: https://www.evm.codes/#f1?fork=cancun
@@ -511,13 +524,21 @@ func TestPrecompileMakeCall(t *testing.T) {
 	dest := common.HexToAddress("DE57")
 
 	rng := ethtest.NewPseudoRand(142857)
-	callData := rng.Bytes(8)
+	precompileCallData := rng.Bytes(8)
+
+	// If the SUT precompile receives this as its calldata then it will use the
+	// vm.WithUNSAFECallerAddressProxying() option.
+	unsafeCallerProxyOptSentinel := []byte("override-caller sentinel")
 
 	hooks := &hookstest.Stub{
 		PrecompileOverrides: map[common.Address]libevm.PrecompiledContract{
 			sut: vm.NewStatefulPrecompile(func(env vm.PrecompileEnvironment, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) {
+				var opts []vm.CallOption
+				if bytes.Equal(input, unsafeCallerProxyOptSentinel) {
+					opts = append(opts, vm.WithUNSAFECallerAddressProxying())
+				}
 				// We are ultimately testing env.Call(), hence why this is the SUT.
-				return env.Call(dest, callData, suppliedGas, uint256.NewInt(0))
+				return env.Call(dest, precompileCallData, suppliedGas, uint256.NewInt(0), opts...)
 			}),
 			dest: vm.NewStatefulPrecompile(func(env vm.PrecompileEnvironment, input []byte, suppliedGas uint64) (ret []byte, remainingGas uint64, err error) {
 				out := &statefulPrecompileOutput{
@@ -538,6 +559,7 @@ func TestPrecompileMakeCall(t *testing.T) {
 
 	tests := []struct {
 		incomingCallType vm.OpCode
+		eoaTxCallData    []byte
 		// Unlike TestNewStatefulPrecompile, which tests the AddressContext of
 		// the precompile itself, these test the AddressContext of a contract
 		// called by the precompile.
@@ -551,7 +573,19 @@ func TestPrecompileMakeCall(t *testing.T) {
 					Caller: sut,
 					Self:   dest,
 				},
-				Input: callData,
+				Input: precompileCallData,
+			},
+		},
+		{
+			incomingCallType: vm.CALL,
+			eoaTxCallData:    unsafeCallerProxyOptSentinel,
+			want: statefulPrecompileOutput{
+				Addresses: &libevm.AddressContext{
+					Origin: eoa,
+					Caller: caller, // overridden by CallOption
+					Self:   dest,
+				},
+				Input: precompileCallData,
 			},
 		},
 		{
@@ -562,7 +596,19 @@ func TestPrecompileMakeCall(t *testing.T) {
 					Caller: caller, // SUT runs as its own caller because of CALLCODE
 					Self:   dest,
 				},
-				Input: callData,
+				Input: precompileCallData,
+			},
+		},
+		{
+			incomingCallType: vm.CALLCODE,
+			eoaTxCallData:    unsafeCallerProxyOptSentinel,
+			want: statefulPrecompileOutput{
+				Addresses: &libevm.AddressContext{
+					Origin: eoa,
+					Caller: caller, // CallOption is a NOOP
+					Self:   dest,
+				},
+				Input: precompileCallData,
 			},
 		},
 		{
@@ -573,7 +619,19 @@ func TestPrecompileMakeCall(t *testing.T) {
 					Caller: caller, // as with CALLCODE
 					Self:   dest,
 				},
-				Input: callData,
+				Input: precompileCallData,
+			},
+		},
+		{
+			incomingCallType: vm.DELEGATECALL,
+			eoaTxCallData:    unsafeCallerProxyOptSentinel,
+			want: statefulPrecompileOutput{
+				Addresses: &libevm.AddressContext{
+					Origin: eoa,
+					Caller: caller, // CallOption is a NOOP
+					Self:   dest,
+				},
+				Input: precompileCallData,
 			},
 		},
 		{
@@ -584,7 +642,7 @@ func TestPrecompileMakeCall(t *testing.T) {
 					Caller: sut,
 					Self:   dest,
 				},
-				Input: callData,
+				Input: precompileCallData,
 				// This demonstrates that even though the precompile makes a
 				// (non-static) CALL, the read-only state is inherited. Yes,
 				// this is _another_ way to get a read-only state, different to
@@ -592,19 +650,56 @@ func TestPrecompileMakeCall(t *testing.T) {
 				ReadOnly: true,
 			},
 		},
+		{
+			incomingCallType: vm.STATICCALL,
+			eoaTxCallData:    unsafeCallerProxyOptSentinel,
+			want: statefulPrecompileOutput{
+				Addresses: &libevm.AddressContext{
+					Origin: eoa,
+					Caller: caller, // overridden by CallOption
+					Self:   dest,
+				},
+				Input:    precompileCallData,
+				ReadOnly: true,
+			},
+		},
 	}
 
 	for _, tt := range tests {
-		t.Run(fmt.Sprintf("via %s", tt.incomingCallType), func(t *testing.T) {
+		t.Run(tt.incomingCallType.String(), func(t *testing.T) {
+			t.Logf("calldata = %q", tt.eoaTxCallData)
 			state, evm := ethtest.NewZeroEVM(t)
 			evm.Origin = eoa
 			state.CreateAccount(caller)
 			proxy := makeReturnProxy(t, sut, tt.incomingCallType)
 			state.SetCode(caller, convertBytes[vm.OpCode, byte](proxy))
 
-			got, _, err := evm.Call(vm.AccountRef(eoa), caller, nil, 1e6, uint256.NewInt(0))
+			got, _, err := evm.Call(vm.AccountRef(eoa), caller, tt.eoaTxCallData, 1e6, uint256.NewInt(0))
 			require.NoError(t, err)
 			require.Equal(t, tt.want.String(), string(got))
 		})
 	}
 }
+
+//nolint:testableexamples // Including output would only make the example more complicated and hide the true intent
+func ExamplePrecompileEnvironment() {
+	// To determine the actual caller of a precompile, as against the effective
+	// caller (under EVM rules, as exposed by `Addresses().Caller`):
+	actualCaller := func(env vm.PrecompileEnvironment) common.Address {
+		if env.IncomingCallType() == vm.DelegateCall {
+			// DelegateCall acts as if it were its own caller.
+			return env.Addresses().Self
+		}
+		// CallCode could return either `Self` or `Caller` as it acts as its
+		// caller but doesn't inherit the caller's caller as DelegateCall does.
+		// Having it handled here is arbitrary from a behavioural perspective
+		// and is done only to simplify the code.
+		//
+		// Call and StaticCall don't affect self/caller semantics in any way.
+		return env.Addresses().Caller
+	}
+
+	// actualCaller would typically be a top-level function. It's only a
+	// variable to include it in this example function.
+	_ = actualCaller
+}

core/vm/environment.libevm.go

@@ -31,18 +31,34 @@ import (
 var _ PrecompileEnvironment = (*environment)(nil)
 
 type environment struct {
-	evm           *EVM
+	evm      *EVM
-	self          *Contract
+	self     *Contract
-	forceReadOnly bool
+	callType CallType
 }
 
 func (e *environment) ChainConfig() *params.ChainConfig  { return e.evm.chainConfig }
 func (e *environment) Rules() params.Rules               { return e.evm.chainRules }
-func (e *environment) ReadOnly() bool                    { return e.forceReadOnly || e.evm.interpreter.readOnly }
 func (e *environment) ReadOnlyState() libevm.StateReader { return e.evm.StateDB }
+func (e *environment) IncomingCallType() CallType        { return e.callType }
 func (e *environment) BlockNumber() *big.Int             { return new(big.Int).Set(e.evm.Context.BlockNumber) }
 func (e *environment) BlockTime() uint64                 { return e.evm.Context.Time }
 
+func (e *environment) ReadOnly() bool {
+	// A switch statement provides clearer code coverage for difficult-to-test
+	// cases.
+	switch {
+	case e.callType == StaticCall:
+		// evm.interpreter.readOnly is only set to true via a call to
+		// EVMInterpreter.Run() so, if a precompile is called directly with
+		// StaticCall(), then readOnly might not be set yet.
+		return true
+	case e.evm.interpreter.readOnly:
+		return true
+	default:
+		return false
+	}
+}
+
 func (e *environment) Addresses() *libevm.AddressContext {
 	return &libevm.AddressContext{
 		Origin: e.evm.Origin,
@@ -71,10 +87,10 @@ func (e *environment) BlockHeader() (types.Header, error) {
 }
 
 func (e *environment) Call(addr common.Address, input []byte, gas uint64, value *uint256.Int, opts ...CallOption) ([]byte, uint64, error) {
-	return e.callContract(call, addr, input, gas, value, opts...)
+	return e.callContract(Call, addr, input, gas, value, opts...)
 }
 
-func (e *environment) callContract(typ callType, addr common.Address, input []byte, gas uint64, value *uint256.Int, opts ...CallOption) ([]byte, uint64, error) {
+func (e *environment) callContract(typ CallType, addr common.Address, input []byte, gas uint64, value *uint256.Int, opts ...CallOption) ([]byte, uint64, error) {
 	// Depth and read-only setting are handled by [EVMInterpreter.Run], which
 	// isn't used for precompiles, so we need to do it ourselves to maintain the
 	// expected invariants.
@@ -83,7 +99,7 @@ func (e *environment) callContract(typ callType, addr common.Address, input []by
 	in.evm.depth++
 	defer func() { in.evm.depth-- }()
 
-	if e.forceReadOnly && !in.readOnly { // i.e. the precompile was StaticCall()ed
+	if e.ReadOnly() && !in.readOnly { // i.e. the precompile was StaticCall()ed
 		in.readOnly = true
 		defer func() { in.readOnly = false }()
 	}
@@ -95,6 +111,11 @@ func (e *environment) callContract(typ callType, addr common.Address, input []by
 			// Note that, in addition to being unsafe, this breaks an EVM
 			// assumption that the caller ContractRef is always a *Contract.
 			caller = AccountRef(e.self.CallerAddress)
+			if e.callType == DelegateCall {
+				// self was created with AsDelegate(), which means that
+				// CallerAddress was inherited.
+				caller = AccountRef(e.self.Address())
+			}
 		case nil:
 		default:
 			return nil, gas, fmt.Errorf("unsupported option %T", o)
@@ -102,12 +123,12 @@ func (e *environment) callContract(typ callType, addr common.Address, input []by
 	}
 
 	switch typ {
-	case call:
+	case Call:
 		if in.readOnly && !value.IsZero() {
 			return nil, gas, ErrWriteProtection
 		}
 		return e.evm.Call(caller, addr, input, gas, value)
-	case callCode, delegateCall, staticCall:
+	case CallCode, DelegateCall, StaticCall:
 		// TODO(arr4n): these cases should be very similar to CALL, hence the
 		// early abstraction, to signal to future maintainers. If implementing
 		// them, there's likely no need to honour the

core/vm/evm.go

@@ -230,7 +230,7 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
 	}
 
 	if isPrecompile {
-		args := &evmCallArgs{evm, call, caller, addr, input, gas, value}
+		args := &evmCallArgs{evm, Call, caller, addr, input, gas, value}
 		ret, gas, err = args.RunPrecompiledContract(p, input, gas)
 	} else {
 		// Initialise a new contract and set the code that is to be used by the EVM.
@@ -294,7 +294,7 @@ func (evm *EVM) CallCode(caller ContractRef, addr common.Address, input []byte,
 
 	// It is allowed to call precompiles, even via delegatecall
 	if p, isPrecompile := evm.precompile(addr); isPrecompile {
-		args := &evmCallArgs{evm, callCode, caller, addr, input, gas, value}
+		args := &evmCallArgs{evm, CallCode, caller, addr, input, gas, value}
 		ret, gas, err = args.RunPrecompiledContract(p, input, gas)
 	} else {
 		addrCopy := addr
@@ -340,7 +340,7 @@ func (evm *EVM) DelegateCall(caller ContractRef, addr common.Address, input []by
 
 	// It is allowed to call precompiles, even via delegatecall
 	if p, isPrecompile := evm.precompile(addr); isPrecompile {
-		args := &evmCallArgs{evm, delegateCall, caller, addr, input, gas, nil}
+		args := &evmCallArgs{evm, DelegateCall, caller, addr, input, gas, nil}
 		ret, gas, err = args.RunPrecompiledContract(p, input, gas)
 	} else {
 		addrCopy := addr
@@ -390,7 +390,7 @@ func (evm *EVM) StaticCall(caller ContractRef, addr common.Address, input []byte
 	}
 
 	if p, isPrecompile := evm.precompile(addr); isPrecompile {
-		args := &evmCallArgs{evm, staticCall, caller, addr, input, gas, nil}
+		args := &evmCallArgs{evm, StaticCall, caller, addr, input, gas, nil}
 		ret, gas, err = args.RunPrecompiledContract(p, input, gas)
 	} else {
 		// At this point, we use a copy of address. If we don't, the go compiler will