feat: api: sanity check the "to" address of outgoing messages (#12135)

* feat: api: sanity check the "to" address of outgoing messages

If the "to" address of an outgoing message is a _delegated_ address,
verify that it maps to a valid Ethereum address. This isn't a consensus
critical change, but it'll help prevent client-side address conversion
libraries from directing messages into oblivion (e.g., by
mis-translating `0xff0000....` addresses into `f410f...` addresses
instead of `f0...` addresses.

* tests for invalid delegated addresses

* fix lint

---------

Co-authored-by: aarshkshah1992 <aarshkshah1992@gmail.com>

itests/mempool_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/filecoin-project/go-address"
 	"github.com/filecoin-project/go-state-types/big"
 
 	"github.com/filecoin-project/lotus/api"
@@ -18,6 +19,33 @@ import (
 const mPoolThrottle = time.Millisecond * 100
 const mPoolTimeout = time.Second * 10
 
+func TestMemPoolPushOutgoingInvalidDelegated(t *testing.T) {
+	//stm: @CHAIN_MEMPOOL_PENDING_001, @CHAIN_STATE_WAIT_MSG_001, @CHAIN_MEMPOOL_CAP_GAS_FEE_001
+	//stm: @CHAIN_MEMPOOL_PUSH_002
+	ctx := context.Background()
+	firstNode, _, _, ens := kit.EnsembleTwoOne(t, kit.MockProofs())
+	ens.InterconnectAll()
+	kit.QuietMiningLogs()
+
+	sender := firstNode.DefaultKey.Address
+	badTo, err := address.NewFromString("f410f74aaaaaaaaaaaaaaaaaaaaaaaaac5sh2bf3lgta")
+	require.NoError(t, err)
+
+	bal, err := firstNode.WalletBalance(ctx, sender)
+	require.NoError(t, err)
+	toSend := big.Div(bal, big.NewInt(10))
+
+	msg := &types.Message{
+		From:  sender,
+		Value: toSend,
+		To:    badTo,
+	}
+
+	_, err = firstNode.MpoolPushMessage(ctx, msg, nil)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "is a delegated address but not a valid Eth Address")
+}
+
 func TestMemPoolPushSingleNode(t *testing.T) {
 	//stm: @CHAIN_MEMPOOL_CREATE_MSG_CHAINS_001, @CHAIN_MEMPOOL_SELECT_001
 	//stm: @CHAIN_MEMPOOL_PENDING_001, @CHAIN_STATE_WAIT_MSG_001, @CHAIN_MEMPOOL_CAP_GAS_FEE_001

node/impl/full/mpool.go

@@ -16,6 +16,7 @@ import (
 	"github.com/filecoin-project/lotus/chain/messagepool"
 	"github.com/filecoin-project/lotus/chain/messagesigner"
 	"github.com/filecoin-project/lotus/chain/types"
+	"github.com/filecoin-project/lotus/chain/types/ethtypes"
 	"github.com/filecoin-project/lotus/node/modules/dtypes"
 )
 
@@ -131,14 +132,24 @@ func (a *MpoolAPI) MpoolClear(ctx context.Context, local bool) error {
 }
 
 func (m *MpoolModule) MpoolPush(ctx context.Context, smsg *types.SignedMessage) (cid.Cid, error) {
+	if err := sanityCheckOutgoingMessage(&smsg.Message); err != nil {
+		return cid.Undef, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", smsg.Cid(), smsg.Message.From, smsg.Message.Nonce, err)
+	}
 	return m.Mpool.Push(ctx, smsg, true)
 }
 
 func (a *MpoolAPI) MpoolPushUntrusted(ctx context.Context, smsg *types.SignedMessage) (cid.Cid, error) {
+	if err := sanityCheckOutgoingMessage(&smsg.Message); err != nil {
+		return cid.Undef, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", smsg.Cid(), smsg.Message.From, smsg.Message.Nonce, err)
+	}
 	return a.Mpool.PushUntrusted(ctx, smsg)
 }
 
 func (a *MpoolAPI) MpoolPushMessage(ctx context.Context, msg *types.Message, spec *api.MessageSendSpec) (*types.SignedMessage, error) {
+	if err := sanityCheckOutgoingMessage(msg); err != nil {
+		return nil, xerrors.Errorf("message from %s failed sanity check: %w", msg.From, err)
+	}
+
 	cp := *msg
 	msg = &cp
 	inMsg := *msg
@@ -223,6 +234,11 @@ func (a *MpoolAPI) MpoolPushMessage(ctx context.Context, msg *types.Message, spe
 }
 
 func (a *MpoolAPI) MpoolBatchPush(ctx context.Context, smsgs []*types.SignedMessage) ([]cid.Cid, error) {
+	for _, msg := range smsgs {
+		if err := sanityCheckOutgoingMessage(&msg.Message); err != nil {
+			return nil, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", msg.Cid(), msg.Message.From, msg.Message.Nonce, err)
+		}
+	}
 	var messageCids []cid.Cid
 	for _, smsg := range smsgs {
 		smsgCid, err := a.Mpool.Push(ctx, smsg, true)
@@ -235,6 +251,11 @@ func (a *MpoolAPI) MpoolBatchPush(ctx context.Context, smsgs []*types.SignedMess
 }
 
 func (a *MpoolAPI) MpoolBatchPushUntrusted(ctx context.Context, smsgs []*types.SignedMessage) ([]cid.Cid, error) {
+	for _, msg := range smsgs {
+		if err := sanityCheckOutgoingMessage(&msg.Message); err != nil {
+			return nil, xerrors.Errorf("message %s from %s with nonce %d failed sanity check: %w", msg.Cid(), msg.Message.From, msg.Message.Nonce, err)
+		}
+	}
 	var messageCids []cid.Cid
 	for _, smsg := range smsgs {
 		smsgCid, err := a.Mpool.PushUntrusted(ctx, smsg)
@@ -247,6 +268,11 @@ func (a *MpoolAPI) MpoolBatchPushUntrusted(ctx context.Context, smsgs []*types.S
 }
 
 func (a *MpoolAPI) MpoolBatchPushMessage(ctx context.Context, msgs []*types.Message, spec *api.MessageSendSpec) ([]*types.SignedMessage, error) {
+	for i, msg := range msgs {
+		if err := sanityCheckOutgoingMessage(msg); err != nil {
+			return nil, xerrors.Errorf("message #%d from %s with failed sanity check: %w", i, msg.From, err)
+		}
+	}
 	var smsgs []*types.SignedMessage
 	for _, msg := range msgs {
 		smsg, err := a.MpoolPushMessage(ctx, msg, spec)
@@ -277,3 +303,19 @@ func (a *MpoolAPI) MpoolGetNonce(ctx context.Context, addr address.Address) (uin
 func (a *MpoolAPI) MpoolSub(ctx context.Context) (<-chan api.MpoolUpdate, error) {
 	return a.Mpool.Updates(ctx)
 }
+
+func sanityCheckOutgoingMessage(msg *types.Message) error {
+	// Check that the message's TO address is a _valid_ Eth address if it's a delegated address.
+	//
+	// It's legal (from a consensus perspective) to send funds to any 0xf410f address as long as
+	// the payload is at most 54 bytes, but the vast majority of this address space is
+	// essentially a black-hole. Unfortunately, the conversion from 0x addresses to Filecoin
+	// native addresses has a few pitfalls (especially with respect to masked ID addresses), so
+	// we've added this check to the API to avoid accidentally (and avoidably) sending messages
+	// to these black-hole addresses.
+	if msg.To.Protocol() == address.Delegated && !ethtypes.IsEthAddress(msg.To) {
+		return xerrors.Errorf("message recipient %s is a delegated address but not a valid Eth Address", msg.To)
+	}
+
+	return nil
+}

node/impl/full/mpool_test.go

@@ -0,0 +1,67 @@
+package full
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/filecoin-project/go-address"
+	"github.com/filecoin-project/go-state-types/crypto"
+
+	"github.com/filecoin-project/lotus/chain/types"
+)
+
+func TestSanityCheckOutgoingMessage(t *testing.T) {
+	// fails for invalid delegated address
+	badTo, err := address.NewFromString("f410f74aaaaaaaaaaaaaaaaaaaaaaaaac5sh2bf3lgta")
+	require.NoError(t, err)
+	msg := &types.Message{
+		To: badTo,
+	}
+
+	err = sanityCheckOutgoingMessage(msg)
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "is a delegated address but not a valid Eth Address")
+
+	// works for valid delegated address
+	goodTo, err := address.NewFromString("f410faxfebiima2gp4lduo2k3vt2iuqapuk3logeftky")
+	require.NoError(t, err)
+	msg = &types.Message{
+		To: goodTo,
+	}
+	err = sanityCheckOutgoingMessage(msg)
+	require.NoError(t, err)
+
+	// works for valid non-delegated address
+	goodTo, err = address.NewFromString("f1z762skeib2v6zlkvhywmjxbv3dxoiv4hmb6gs4y")
+	require.NoError(t, err)
+	msg = &types.Message{
+		To: goodTo,
+	}
+	err = sanityCheckOutgoingMessage(msg)
+	require.NoError(t, err)
+}
+
+func TestMpoolPushInvalidDelegatedAddressFails(t *testing.T) {
+	badTo, err := address.NewFromString("f410f74aaaaaaaaaaaaaaaaaaaaaaaaac5sh2bf3lgta")
+	require.NoError(t, err)
+	module := &MpoolModule{}
+	m := &MpoolAPI{
+		MpoolModuleAPI: module,
+	}
+	smsg := &types.SignedMessage{
+		Message: types.Message{
+			From: badTo,
+			To:   badTo,
+		},
+		Signature: crypto.Signature{
+			Type: crypto.SigTypeSecp256k1,
+			Data: []byte("signature"),
+		},
+	}
+	_, err = m.MpoolPush(context.Background(), smsg)
+	require.Error(t, err)
+
+	require.Contains(t, err.Error(), "is a delegated address but not a valid Eth Address")
+}