Account for callData reverts due to transfers greater than sender balance (#384)

Author: hazim-j

e2e/test/execution.test.ts

@@ -95,6 +95,38 @@ describe("During the execution phase", () => {
     expect(event?.args.success).toBe(true);
   });
 
+  describe("With transfer amounts more than sender balance", () => {
+    test("Sender cannot create a UserOperation by default", async () => {
+      expect.assertions(1);
+      try {
+        const balance = await provider.getBalance(acc.getSender());
+        const response = await client.sendUserOperation(
+          acc.execute(acc.getSender(), balance.mul(2), "0x")
+        );
+        await response.wait();
+      } catch (error: any) {
+        expect(error?.error.code).toBe(errorCodes.executionReverted);
+      }
+    });
+
+    test("Sender can cause a failed UserOperation with state overrides", async () => {
+      const balance = await provider.getBalance(acc.getSender());
+      const response = await client.sendUserOperation(
+        acc.execute(acc.getSender(), balance.mul(2), "0x"),
+        {
+          stateOverrides: {
+            [acc.getSender()]: {
+              balance: balance.mul(3).toHexString(),
+            },
+          },
+        }
+      );
+      const event = await response.wait();
+
+      expect(event?.args.success).toBe(false);
+    });
+  });
+
   test("Sender can transfer max balance of ERC20 token", async () => {
     const balance = await testToken.balanceOf(acc.getSender());
     const response = await client.sendUserOperation(

pkg/client/client.go

@@ -194,17 +194,12 @@ func (i *Client) EstimateUserOperationGas(
 	hash := userOp.GetUserOpHash(epAddr, i.chainID)
 	l = l.WithValues("userop_hash", hash)
 
-	// Parse state override set. If paymaster is not included and sender overrides are not set, default to
-	// overriding sender balance to max uint96. This ensures gas estimation is not blocked by insufficient
-	// funds.
+	// Parse state override set.
 	sos, err := state.ParseOverrideData(os)
 	if err != nil {
 		l.Error(err, "eth_estimateUserOperationGas error")
 		return nil, err
 	}
-	if userOp.GetPaymaster() == common.HexToAddress("0x") {
-		sos = state.WithMaxBalanceOverride(userOp.Sender, sos)
-	}
 
 	// Override op with suggested gas prices if maxFeePerGas is 0. This allows for more reliable gas
 	// estimations upstream. The default balance override also ensures simulations won't revert on

pkg/gas/estimate.go

@@ -89,6 +89,16 @@ func EstimateGas(in *EstimateInput) (verificationGas uint64, callGas uint64, err
 	data["verificationGasLimit"] = hexutil.EncodeBig(big.NewInt(0))
 	data["callGasLimit"] = hexutil.EncodeBig(big.NewInt(0))
 
+	// If paymaster is not included and sender overrides are not set, default to overriding sender balance to
+	// max uint96. This ensures gas estimation is not blocked by insufficient funds.
+	sosCpy, err := state.Copy(in.Sos)
+	if err != nil {
+		return 0, 0, err
+	}
+	if in.Op.GetPaymaster() == common.HexToAddress("0x") {
+		sosCpy = state.WithMaxBalanceOverride(in.Op.Sender, sosCpy)
+	}
+
 	// Find the optimal verificationGasLimit with binary search. A gas price of 0 may result in certain
 	// upstream code paths in the EVM to not be executed which can affect the reliability of gas estimates. In
 	// this case, consider calling the EstimateGas function after setting the gas price on the UserOperation.
@@ -108,7 +118,7 @@ func EstimateGas(in *EstimateInput) (verificationGas uint64, callGas uint64, err
 			Rpc:        in.Rpc,
 			EntryPoint: in.EntryPoint,
 			Op:         simOp,
-			Sos:        in.Sos,
+			Sos:        sosCpy,
 			ChainID:    in.ChainID,
 		})
 		simErr = err
@@ -136,8 +146,9 @@ func EstimateGas(in *EstimateInput) (verificationGas uint64, callGas uint64, err
 	f = (f * (100 + baseVGLBuffer)) / 100
 	data["verificationGasLimit"] = hexutil.EncodeBig(big.NewInt(int64(f)))
 
-	// Find the optimal callGasLimit by setting the gas price to 0 and maxing out the gas limit. We don't run
-	// into the same restrictions here as we do with verificationGasLimit.
+	// Find the optimal callGasLimit by setting the gas price to 0 and maxing out the gas limit. We use the
+	// original state override to account for insufficient balance reverts unless the caller has explicitly
+	// overridden the balance too.
 	data["maxFeePerGas"] = hexutil.EncodeBig(big.NewInt(0))
 	data["maxPriorityFeePerGas"] = hexutil.EncodeBig(big.NewInt(0))
 	data["callGasLimit"] = hexutil.EncodeBig(in.MaxGasLimit)
@@ -165,8 +176,7 @@ func EstimateGas(in *EstimateInput) (verificationGas uint64, callGas uint64, err
 		cgl = in.Ov.NonZeroValueCall()
 	}
 
-	// Run a final simulation to check wether or not value transfers are still okay when factoring in the
-	// expected gas cost.
+	// Run a final simulation using actual gas fees and to confirm one-shot tracing is ok.
 	data["maxFeePerGas"] = hexutil.EncodeBig(in.Op.MaxFeePerGas)
 	data["maxPriorityFeePerGas"] = hexutil.EncodeBig(in.Op.MaxFeePerGas)
 	data["verificationGasLimit"] = hexutil.EncodeBig(vgl)
@@ -179,7 +189,7 @@ func EstimateGas(in *EstimateInput) (verificationGas uint64, callGas uint64, err
 		Rpc:        in.Rpc,
 		EntryPoint: in.EntryPoint,
 		Op:         simOp,
-		Sos:        in.Sos,
+		Sos:        sosCpy,
 		ChainID:    in.ChainID,
 		Tracer:     in.Tracer,
 	})
@@ -204,7 +214,7 @@ func EstimateGas(in *EstimateInput) (verificationGas uint64, callGas uint64, err
 					Rpc:        in.Rpc,
 					EntryPoint: in.EntryPoint,
 					Op:         simOp,
-					Sos:        in.Sos,
+					Sos:        sosCpy,
 					ChainID:    in.ChainID,
 					Tracer:     in.Tracer,
 				})

pkg/state/copy.go

@@ -0,0 +1,26 @@
+package state
+
+import (
+	"encoding/json"
+)
+
+// Copy creates a deep copy of a given OverrideSet.
+func Copy(os OverrideSet) (OverrideSet, error) {
+	cpy := OverrideSet{}
+	for k, v := range os {
+		b, err := json.Marshal(v)
+		if err != nil {
+			return nil, err
+		}
+
+		oa := OverrideAccount{}
+		err = json.Unmarshal(b, &oa)
+		if err != nil {
+			return nil, err
+		}
+
+		cpy[k] = oa
+	}
+
+	return cpy, nil
+}

pkg/state/copy_test.go

@@ -0,0 +1,87 @@
+package state
+
+import (
+	"testing"
+
+	"github.com/ethereum/go-ethereum/common"
+)
+
+func TestCopyNil(t *testing.T) {
+	if os, err := Copy(nil); err != nil {
+		t.Fatalf("got %v, want nil", err)
+	} else if os == nil {
+		t.Fatal("got nil return value")
+	} else if len(os) != 0 {
+		t.Fatalf("got length %v, want 0", len(os))
+	}
+}
+
+func TestCopyNewRef(t *testing.T) {
+	acc := common.HexToAddress("0x0000000000000000000000000000000000000000")
+	stateOvKey := common.HexToHash("0xdead")
+	stateOvVal := common.HexToHash("0xbeef")
+	data := map[string]any{
+		acc.Hex(): map[string]any{
+			"nonce":   "0x1",
+			"code":    "0x01",
+			"balance": "0x1",
+			"state": map[string]any{
+				stateOvKey.String(): stateOvVal.String(),
+			},
+			"stateDiff": map[string]any{
+				stateOvKey.String(): stateOvVal.String(),
+			},
+		},
+	}
+	if os, err := ParseOverrideData(data); err != nil {
+		t.Fatalf("got %v, want nil", err)
+	} else if osCpy, cpyErr := Copy(os); err != nil {
+		t.Fatalf("got %v, want nil", cpyErr)
+	} else if os[acc].Nonce == osCpy[acc].Nonce ||
+		os[acc].Code == osCpy[acc].Code ||
+		os[acc].Balance == osCpy[acc].Balance ||
+		os[acc].State == osCpy[acc].State ||
+		os[acc].StateDiff == osCpy[acc].StateDiff {
+		t.Fatal("OverrideAccount contains identical references")
+	}
+}
+
+func TestCopySameVal(t *testing.T) {
+	acc := common.HexToAddress("0x0000000000000000000000000000000000000000")
+	stateOvKey := common.HexToHash("0xdead")
+	stateOvVal := common.HexToHash("0xbeef")
+	data := map[string]any{
+		acc.Hex(): map[string]any{
+			"nonce":   "0x1",
+			"code":    "0x01",
+			"balance": "0x1",
+			"state": map[string]any{
+				stateOvKey.String(): stateOvVal.String(),
+			},
+			"stateDiff": map[string]any{
+				stateOvKey.String(): stateOvVal.String(),
+			},
+		},
+	}
+	os, err := ParseOverrideData(data)
+	if err != nil {
+		t.Fatalf("got %v, want nil", err)
+	}
+
+	osCpy, cpyErr := Copy(os)
+	if cpyErr != nil {
+		t.Fatalf("got %v, want nil", cpyErr)
+	}
+
+	state := *os[acc].State
+	stateCpy := *osCpy[acc].State
+	stateDiff := *os[acc].StateDiff
+	stateDiffCpy := *osCpy[acc].StateDiff
+	if os[acc].Nonce.String() != osCpy[acc].Nonce.String() ||
+		os[acc].Code.String() != osCpy[acc].Code.String() ||
+		os[acc].Balance.String() != osCpy[acc].Balance.String() ||
+		state[stateOvKey] != stateCpy[stateOvKey] ||
+		stateDiff[stateOvKey] != stateDiffCpy[stateOvKey] {
+		t.Fatal("OverrideAccount contains different values")
+	}
+}