Merge pull request #10007 from filecoin-project/asr/delegated-siggy

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 := txArgs.ToUnsignedMessage(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,10 +8,10 @@ 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"
-	"github.com/filecoin-project/go-state-types/abi"
 	"github.com/filecoin-project/go-state-types/big"
 	builtintypes "github.com/filecoin-project/go-state-types/builtin"
 	"github.com/filecoin-project/go-state-types/builtin/v10/eam"
@@ -58,27 +58,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
+		to           *EthAddress
-		decodedParams []byte
+		params       []byte
-		paramsReader  = bytes.NewReader(msg.Params)
+		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
+			params = create.Initcode
-		case builtintypes.MethodsEAM.Create2:
-			var create2 eam.Create2Params
-			if err := create2.UnmarshalCBOR(paramsReader); err != nil {
-				return EthTxArgs{}, err
-			}
-			decodedParams = create2.Initcode
 		default:
 			return EthTxArgs{}, fmt.Errorf("unsupported EAM method")
 		}
@@ -89,12 +91,30 @@ func NewEthTxArgsFromMessage(msg *types.Message) (EthTxArgs, error) {
 		}
 		to = &addr
 
-		if len(msg.Params) > 0 {
+		if len(msg.Params) == 0 {
-			params, err := cbg.ReadByteArray(paramsReader, uint64(len(msg.Params)))
+			if msg.Method != builtintypes.MethodSend {
-			if err != nil {
+				return EthTxArgs{}, xerrors.Errorf("cannot invoke method %d on non-EAM actor without params", msg.Method)
-				return EthTxArgs{}, err
+			}
+		} 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{}, xerrors.Errorf("failed to read params byte array: %w", err)
+			}
+
+			if paramsReader.Len() != 0 {
+				return EthTxArgs{}, xerrors.Errorf("extra data found in params")
+			}
+			if len(params) == 0 {
+				// Otherwise, we don't get a guaranteed round-trip.
+				return EthTxArgs{}, xerrors.Errorf("cannot invoke contracts with empty parameters from an eth-account")
 			}
-			decodedParams = params
 		}
 	}
 
@@ -103,84 +123,95 @@ 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 (tx *EthTxArgs) ToSignedMessage() (*types.SignedMessage, error) {
+func (tx *EthTxArgs) ToUnsignedMessage(from address.Address) (*types.Message, error) {
-	from, err := tx.Sender()
+	if tx.ChainID != build.Eip155ChainId {
-	if err != nil {
+		return nil, xerrors.Errorf("unsupported chain id: %d", tx.ChainID)
-		return nil, err
 	}
 
-	var to address.Address
+	var err error
+	method := builtintypes.MethodSend
 	var params []byte
-
+	var to address.Address
-	if len(tx.To) == 0 && len(tx.Input) == 0 {
+	// nil indicates the EAM, only CreateExternal is allowed
-		return nil, fmt.Errorf("to and input cannot both be empty")
-	}
-
-	var method abi.MethodNum
 	if tx.To == nil {
-		// TODO unify with applyEvmMsg
-
-		// this is a contract creation
 		to = builtintypes.EthereumAddressManagerActorAddr
-
+		// TODO: Uncomment
-		params2, err := actors.SerializeParams(&eam.CreateParams{
+		//method = builtintypes.MethodsEAM.CreateExternal
+		method = builtintypes.MethodsEAM.Create
+		if len(tx.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: tx.Input,
 			Nonce:    uint64(tx.Nonce),
 		})
 		if err != nil {
 			return nil, fmt.Errorf("failed to serialize Create params: %w", err)
 		}
-		params = params2
-		method = builtintypes.MethodsEAM.Create
-	} else {
-		addr, err := tx.To.ToFilecoinAddress()
-		if err != nil {
-			return nil, err
-		}
-		to = addr
 
-		if len(tx.Input) > 0 {
+	} else {
-			var buf bytes.Buffer
+		to, err = tx.To.ToFilecoinAddress()
-			if err := cbg.WriteByteArray(&buf, tx.Input); err != nil {
+		if err != nil {
-				return nil, fmt.Errorf("failed to encode tx input into a cbor byte-string")
+			return nil, xerrors.Errorf("failed to convert To into filecoin addr: %w", err)
-			}
+		}
-			params = buf.Bytes()
+		if len(tx.Input) == 0 {
-			method = builtintypes.MethodsEVM.InvokeContract
+			// Yes, this is redundant, but let's be sure what we're doing
-		} else {
 			method = builtintypes.MethodSend
+			params = make([]byte, 0)
+		} else {
+			// must be InvokeContract
+			method = builtintypes.MethodsEVM.InvokeContract
+			buf := new(bytes.Buffer)
+			if err = cbg.WriteByteArray(buf, tx.Input); err != nil {
+				return nil, xerrors.Errorf("failed to write input args: %w", err)
+			}
+
+			params = buf.Bytes()
 		}
 	}
 
-	msg := &types.Message{
+	return &types.Message{
-		Nonce:      uint64(tx.Nonce),
+		Version:    0,
-		From:       from,
 		To:         to,
+		From:       from,
+		Nonce:      uint64(tx.Nonce),
 		Value:      tx.Value,
-		Method:     method,
-		Params:     params,
 		GasLimit:   int64(tx.GasLimit),
 		GasFeeCap:  tx.MaxFeePerGas,
 		GasPremium: tx.MaxPriorityFeePerGas,
-	}
+		Method:     method,
+		Params:     params,
+	}, nil
+}
 
-	sig, err := tx.Signature()
+func (tx *EthTxArgs) ToSignedMessage() (*types.SignedMessage, error) {
+	from, err := tx.Sender()
 	if err != nil {
-		return nil, err
+		return nil, xerrors.Errorf("failed to calculate sender: %w", err)
 	}
 
-	signedMsg := types.SignedMessage{
+	unsignedMsg, err := tx.ToUnsignedMessage(from)
-		Message:   *msg,
+	if err != nil {
-		Signature: *sig,
+		return nil, xerrors.Errorf("failed to convert to unsigned msg: %w", err)
 	}
-	return &signedMsg, nil
 
+	siggy, err := tx.Signature()
+	if err != nil {
+		return nil, xerrors.Errorf("failed to calculate signature: %w", err)
+	}
+
+	return &types.SignedMessage{
+		Message:   *unsignedMsg,
+		Signature: *siggy,
+	}, nil
 }
 
 func (tx *EthTxArgs) HashedOriginalRlpMsg() ([]byte, error) {

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()