fix: delegated signatures: check every field of txs and roundtrip eth…

… <-> FIL

chain/signatures.go

@@ -22,15 +22,24 @@ func AuthenticateMessage(msg *types.SignedMessage, signer address.Address) error
 	typ := msg.Signature.Type
 	switch typ {
 	case crypto.SigTypeDelegated:
-		txArgs, err := ethtypes.NewEthTxArgsFromMessage(&msg.Message)
+		txArgs, err := ethtypes.EthTxArgsFromMessage(&msg.Message)
 		if err != nil {
 			return xerrors.Errorf("failed to reconstruct eth transaction: %w", err)
 		}
-		msg, err := txArgs.ToRlpUnsignedMsg()
+		roundTripMsg, err := ethtypes.MessageFromEthTxArgsAndSender(txArgs, msg.Message.From)
+		if err != nil {
+			return xerrors.Errorf("failed to reconstruct filecoin msg: %w", err)
+		}
+
+		if !msg.Message.Equals(roundTripMsg) {
+			return xerrors.New("ethereum tx failed to roundtrip")
+		}
+
+		rlpEncodedMsg, err := txArgs.ToRlpUnsignedMsg()
 		if err != nil {
 			return xerrors.Errorf("failed to repack eth rlp message: %w", err)
 		}
-		digest = msg
+		digest = rlpEncodedMsg
 	default:
 		digest = msg.Message.Cid().Bytes()
 	}

chain/types/ethtypes/eth_transactions.go

@@ -8,6 +8,7 @@ import (
 
 	cbg "github.com/whyrusleeping/cbor-gen"
 	"golang.org/x/crypto/sha3"
+	"golang.org/x/xerrors"
 
 	"github.com/filecoin-project/go-address"
 	gocrypto "github.com/filecoin-project/go-crypto"
@@ -58,27 +59,29 @@ type EthTxArgs struct {
 	S                    big.Int     `json:"s"`
 }
 
-func NewEthTxArgsFromMessage(msg *types.Message) (EthTxArgs, error) {
+func EthTxArgsFromMessage(msg *types.Message) (EthTxArgs, error) {
 	var (
-		to            *EthAddress
-		decodedParams []byte
-		paramsReader  = bytes.NewReader(msg.Params)
+		to           *EthAddress
+		params       []byte
+		paramsReader = bytes.NewReader(msg.Params)
 	)
 
+	if msg.Version != 0 {
+		return EthTxArgs{}, xerrors.Errorf("unsupported msg version: %d", msg.Version)
+	}
+
 	if msg.To == builtintypes.EthereumAddressManagerActorAddr {
 		switch msg.Method {
+		// TODO: Uncomment
+		//case builtintypes.MethodsEAM.CreateExternal:
 		case builtintypes.MethodsEAM.Create:
+			// TODO: Uncomment
+			// var create eam.CreateExternalParams
 			var create eam.CreateParams
 			if err := create.UnmarshalCBOR(paramsReader); err != nil {
 				return EthTxArgs{}, err
 			}
-			decodedParams = create.Initcode
-		case builtintypes.MethodsEAM.Create2:
-			var create2 eam.Create2Params
-			if err := create2.UnmarshalCBOR(paramsReader); err != nil {
-				return EthTxArgs{}, err
-			}
-			decodedParams = create2.Initcode
+			params = create.Initcode
 		default:
 			return EthTxArgs{}, fmt.Errorf("unsupported EAM method")
 		}
@@ -89,12 +92,26 @@ func NewEthTxArgsFromMessage(msg *types.Message) (EthTxArgs, error) {
 		}
 		to = &addr
 
-		if len(msg.Params) > 0 {
-			params, err := cbg.ReadByteArray(paramsReader, uint64(len(msg.Params)))
+		if len(msg.Params) == 0 {
+			if msg.Method != builtintypes.MethodSend {
+				return EthTxArgs{}, xerrors.Errorf("cannot invoke method %d on non-EAM actor without params", msg.Method)
+			}
+		} else {
+			if msg.Method != builtintypes.MethodsEVM.InvokeContract {
+				return EthTxArgs{},
+					xerrors.Errorf("invalid methodnum %d: only allowed non-send method is InvokeContract(%d)",
+						msg.Method,
+						builtintypes.MethodsEVM.InvokeContract)
+			}
+
+			params, err = cbg.ReadByteArray(paramsReader, uint64(len(msg.Params)))
 			if err != nil {
-				return EthTxArgs{}, err
+				return EthTxArgs{}, xerrors.Errorf("failed to read params byte array: %w", err)
+			}
+
+			if paramsReader.Len() != 0 {
+				return EthTxArgs{}, xerrors.Errorf("extra data found in params")
 			}
-			decodedParams = params
 		}
 	}
 
@@ -103,13 +120,79 @@ func NewEthTxArgsFromMessage(msg *types.Message) (EthTxArgs, error) {
 		Nonce:                int(msg.Nonce),
 		To:                   to,
 		Value:                msg.Value,
-		Input:                decodedParams,
+		Input:                params,
 		MaxFeePerGas:         msg.GasFeeCap,
 		MaxPriorityFeePerGas: msg.GasPremium,
 		GasLimit:             int(msg.GasLimit),
 	}, nil
 }
 
+func MessageFromEthTxArgsAndSender(args EthTxArgs, from address.Address) (*types.Message, error) {
+	if args.ChainID != build.Eip155ChainId {
+		return nil, xerrors.Errorf("unsupported chain id: %d", args.ChainID)
+	}
+
+	if !args.V.NilOrZero() || !args.R.NilOrZero() || !args.S.NilOrZero() {
+		return nil, xerrors.New("tx signature fields should be 0")
+	}
+
+	var err error
+	method := builtintypes.MethodSend
+	var params []byte
+	var to address.Address
+	// nil indicates the EAM, only CreateExternal is allowed
+	if args.To == nil {
+		to = builtintypes.EthereumAddressManagerActorAddr
+		// TODO: Uncomment
+		//method = builtintypes.MethodsEAM.CreateExternal
+		method = builtintypes.MethodsEAM.Create
+		if len(args.Input) == 0 {
+			return nil, xerrors.New("cannot call CreateExternal without params")
+		}
+		// TODO: CreateExternalParams, it doesn't have a nonce
+		params, err = actors.SerializeParams(&eam.CreateParams{
+			Initcode: args.Input,
+			Nonce:    uint64(args.Nonce),
+		})
+		if err != nil {
+			return nil, fmt.Errorf("failed to serialize Create params: %w", err)
+		}
+
+	} else {
+		to, err = args.To.ToFilecoinAddress()
+		if err != nil {
+			return nil, xerrors.Errorf("failed to convert To into filecoin addr: %w", err)
+		}
+		if len(args.Input) == 0 {
+			// Yes, this is redundant, but let's be sure what we're doing
+			method = builtintypes.MethodSend
+			params = make([]byte, 0)
+		} else {
+			// must be InvokeContract
+			method = builtintypes.MethodsEVM.InvokeContract
+			buf := new(bytes.Buffer)
+			if err = cbg.WriteByteArray(buf, args.Input); err != nil {
+				return nil, xerrors.Errorf("failed to write input args: %w", err)
+			}
+
+			params = buf.Bytes()
+		}
+	}
+
+	return &types.Message{
+		Version:    0,
+		To:         to,
+		From:       from,
+		Nonce:      uint64(args.Nonce),
+		Value:      args.Value,
+		GasLimit:   int64(args.GasLimit),
+		GasFeeCap:  args.MaxFeePerGas,
+		GasPremium: args.MaxPriorityFeePerGas,
+		Method:     method,
+		Params:     params,
+	}, nil
+}
+
 func (tx *EthTxArgs) ToSignedMessage() (*types.SignedMessage, error) {
 	from, err := tx.Sender()
 	if err != nil {
@@ -139,6 +222,7 @@ func (tx *EthTxArgs) ToSignedMessage() (*types.SignedMessage, error) {
 		}
 		params = params2
 		method = builtintypes.MethodsEAM.Create
+		//method = builtintypes.MethodsEAM.CreateExternal
 	} else {
 		addr, err := tx.To.ToFilecoinAddress()
 		if err != nil {

itests/eth_account_abstraction_test.go

@@ -70,7 +70,7 @@ func TestEthAccountAbstraction(t *testing.T) {
 	msgFromPlaceholder, err = client.GasEstimateMessageGas(ctx, msgFromPlaceholder, nil, types.EmptyTSK)
 	require.NoError(t, err)
 
-	txArgs, err := ethtypes.NewEthTxArgsFromMessage(msgFromPlaceholder)
+	txArgs, err := ethtypes.EthTxArgsFromMessage(msgFromPlaceholder)
 	require.NoError(t, err)
 
 	digest, err := txArgs.ToRlpUnsignedMsg()
@@ -106,7 +106,7 @@ func TestEthAccountAbstraction(t *testing.T) {
 	msgFromPlaceholder, err = client.GasEstimateMessageGas(ctx, msgFromPlaceholder, nil, types.EmptyTSK)
 	require.NoError(t, err)
 
-	txArgs, err = ethtypes.NewEthTxArgsFromMessage(msgFromPlaceholder)
+	txArgs, err = ethtypes.EthTxArgsFromMessage(msgFromPlaceholder)
 	require.NoError(t, err)
 
 	digest, err = txArgs.ToRlpUnsignedMsg()
@@ -178,7 +178,7 @@ func TestEthAccountAbstractionFailure(t *testing.T) {
 	require.NoError(t, err)
 
 	msgFromPlaceholder.Value = abi.TokenAmount(types.MustParseFIL("1000"))
-	txArgs, err := ethtypes.NewEthTxArgsFromMessage(msgFromPlaceholder)
+	txArgs, err := ethtypes.EthTxArgsFromMessage(msgFromPlaceholder)
 	require.NoError(t, err)
 
 	digest, err := txArgs.ToRlpUnsignedMsg()
@@ -216,7 +216,7 @@ func TestEthAccountAbstractionFailure(t *testing.T) {
 	msgFromPlaceholder, err = client.GasEstimateMessageGas(ctx, msgFromPlaceholder, nil, types.EmptyTSK)
 	require.NoError(t, err)
 
-	txArgs, err = ethtypes.NewEthTxArgsFromMessage(msgFromPlaceholder)
+	txArgs, err = ethtypes.EthTxArgsFromMessage(msgFromPlaceholder)
 	require.NoError(t, err)
 
 	digest, err = txArgs.ToRlpUnsignedMsg()