fix(accounts): lockup check sender in context not in message

tests/systemtests/account_test.go

@@ -155,3 +155,54 @@ func TestAccountsMigration(t *testing.T) {
 		"--fees=1stake")
 	systest.RequireTxSuccess(t, rsp)
 }
+
+func TestVestingAccountHacker(t *testing.T) {
+	// Reset the chain and create a CLI wrapper.
+	systest.Sut.ResetChain(t)
+	cli := systest.NewCLIWrapper(t, systest.Sut, systest.Verbose)
+
+	// Create a new key for the vesting account.
+	vestingAccName := "vesting-account"
+	vestingAccAddr := cli.AddKey(vestingAccName)
+	require.NotEmpty(t, vestingAccAddr)
+
+	// Modify the genesis to fund the vesting account with 1,000,000 stake.
+	// (Here we use a CLI command "add-genesis-vesting-account" that accepts vesting parameters.)
+	systest.Sut.ModifyGenesisCLI(t, []string{
+		"genesis", "add-genesis-vesting-account", vestingAccAddr, "1000000stake",
+		"--vesting-start=1", "--vesting-end=10",
+	})
+
+	// Start the chain.
+	systest.Sut.StartChain(t)
+
+	// Unlock tokens by submitting a vesting unlock transaction.
+	// (We assume that the "tx vesting unlock" command will compute and release any unlockable funds.)
+	unlockRsp := cli.RunAndWait("tx", "vesting", "unlock", vestingAccAddr,
+		fmt.Sprintf("--from=%s", vestingAccAddr),
+		"--fees=1stake")
+	systest.RequireTxSuccess(t, unlockRsp)
+
+	// Query the vesting account balance to verify that some tokens have been unlocked.
+	balance := cli.QueryBalance(vestingAccAddr, "stake")
+	require.True(t, balance > 0, "expected some unlocked funds in vesting account, got %d", balance)
+
+	// Create a separate hacker account.
+	hackerAccName := "hacker"
+	hackerAddr := cli.AddKey(hackerAccName)
+	require.NotEmpty(t, hackerAddr)
+
+	// Now, have the hacker attempt to send funds from the vesting account to their own account.
+	// Since the hacker does not hold the vesting account's key, the transaction should fail.
+	assertUnauthorized := func(t assert.TestingT, err error, msgAndArgs ...interface{}) bool {
+		errMsg := err.Error()
+		return strings.Contains(errMsg, "unauthorized") ||
+			strings.Contains(errMsg, "signature verification failed")
+	}
+
+	hackTxRsp := cli.WithRunErrorMatcher(assertUnauthorized).RunAndWait("tx", "bank", "send",
+		vestingAccAddr, hackerAddr, "500stake",
+		fmt.Sprintf("--from=%s", hackerAddr),
+		"--fees=1stake")
+	systest.RequireTxFailure(t, hackTxRsp)
+}

x/accounts/defaults/lockup/continuous_locking_account_test.go

@@ -43,7 +43,6 @@ func TestContinuousAccountDelegate(t *testing.T) {
 
 	acc := setupContinuousAccount(t, sdkCtx, ss)
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -62,7 +61,6 @@ func TestContinuousAccountDelegate(t *testing.T) {
 	})
 
 	_, err = acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(5)),
 	})
@@ -86,7 +84,6 @@ func TestContinuousAccountUndelegate(t *testing.T) {
 	acc := setupContinuousAccount(t, sdkCtx, ss)
 	// Delegate first
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -98,7 +95,6 @@ func TestContinuousAccountUndelegate(t *testing.T) {
 
 	// Undelegate
 	_, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -129,7 +125,6 @@ func TestContinuousAccountSendCoins(t *testing.T) {
 
 	acc := setupContinuousAccount(t, sdkCtx, ss)
 	_, err := acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})
@@ -144,7 +139,6 @@ func TestContinuousAccountSendCoins(t *testing.T) {
 	})
 
 	_, err = acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})

x/accounts/defaults/lockup/delayed_locking_account_test.go

@@ -40,7 +40,6 @@ func TestDelayedAccountDelegate(t *testing.T) {
 
 	acc := setupDelayedAccount(t, sdkCtx, ss)
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -59,7 +58,6 @@ func TestDelayedAccountDelegate(t *testing.T) {
 	})
 
 	_, err = acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(5)),
 	})
@@ -83,7 +81,6 @@ func TestDelayedAccountUndelegate(t *testing.T) {
 	acc := setupDelayedAccount(t, sdkCtx, ss)
 	// Delegate first
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -95,7 +92,6 @@ func TestDelayedAccountUndelegate(t *testing.T) {
 
 	// Undelegate
 	_, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -126,7 +122,6 @@ func TestDelayedAccountSendCoins(t *testing.T) {
 
 	acc := setupDelayedAccount(t, sdkCtx, ss)
 	_, err := acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})
@@ -141,7 +136,6 @@ func TestDelayedAccountSendCoins(t *testing.T) {
 	})
 
 	_, err = acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})

x/accounts/defaults/lockup/lockup.go

@@ -128,7 +128,8 @@ func (bva *BaseLockup) Delegate(
 ) (
 	*lockuptypes.MsgExecuteMessagesResponse, error,
 ) {
-	err := bva.checkSender(ctx, msg.Sender)
+	sender := accountstd.Sender(ctx)
+	err := bva.checkSender(ctx, sender)
 	if err != nil {
 		return nil, err
 	}
@@ -183,7 +184,8 @@ func (bva *BaseLockup) Undelegate(
 ) (
 	*lockuptypes.MsgExecuteMessagesResponse, error,
 ) {
-	err := bva.checkSender(ctx, msg.Sender)
+	sender := accountstd.Sender(ctx)
+	err := bva.checkSender(ctx, sender)
 	if err != nil {
 		return nil, err
 	}
@@ -255,7 +257,8 @@ func (bva *BaseLockup) WithdrawReward(
 ) (
 	*lockuptypes.MsgExecuteMessagesResponse, error,
 ) {
-	err := bva.checkSender(ctx, msg.Sender)
+	sender := accountstd.Sender(ctx)
+	err := bva.checkSender(ctx, sender)
 	if err != nil {
 		return nil, err
 	}
@@ -282,7 +285,8 @@ func (bva *BaseLockup) SendCoins(
 ) (
 	*lockuptypes.MsgExecuteMessagesResponse, error,
 ) {
-	err := bva.checkSender(ctx, msg.Sender)
+	sender := accountstd.Sender(ctx)
+	err := bva.checkSender(ctx, sender)
 	if err != nil {
 		return nil, err
 	}
@@ -321,16 +325,12 @@ func (bva *BaseLockup) SendCoins(
 	return &lockuptypes.MsgExecuteMessagesResponse{Responses: resp}, nil
 }
 
-func (bva *BaseLockup) checkSender(ctx context.Context, sender string) error {
+func (bva *BaseLockup) checkSender(ctx context.Context, sender []byte) error {
 	owner, err := bva.Owner.Get(ctx)
 	if err != nil {
 		return sdkerrors.ErrInvalidAddress.Wrapf("invalid owner address: %s", err.Error())
 	}
-	senderBytes, err := bva.addressCodec.StringToBytes(sender)
-	if err != nil {
-		return sdkerrors.ErrInvalidAddress.Wrapf("invalid sender address: %s", err.Error())
-	}
-	if !bytes.Equal(owner, senderBytes) {
+	if !bytes.Equal(owner, sender) {
 		return errors.New("sender is not the owner of this vesting account")
 	}
 

x/accounts/defaults/lockup/periodic_locking_account_test.go

@@ -2,16 +2,20 @@ package lockup
 
 import (
 	"context"
+	"errors"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
 
 	"cosmossdk.io/core/header"
 	"cosmossdk.io/core/store"
+	"cosmossdk.io/core/transaction"
 	"cosmossdk.io/log"
 	"cosmossdk.io/math"
+	"cosmossdk.io/x/accounts/accountstd"
 	lockuptypes "cosmossdk.io/x/accounts/defaults/lockup/v1"
+	banktypes "cosmossdk.io/x/bank/types"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )
@@ -54,7 +58,6 @@ func TestPeriodicAccountDelegate(t *testing.T) {
 
 	acc := setupPeriodicAccount(t, sdkCtx, ss)
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -73,7 +76,6 @@ func TestPeriodicAccountDelegate(t *testing.T) {
 	})
 
 	_, err = acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(5)),
 	})
@@ -93,7 +95,6 @@ func TestPeriodicAccountDelegate(t *testing.T) {
 	})
 
 	_, err = acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(4)),
 	})
@@ -117,7 +118,6 @@ func TestPeriodicAccountUndelegate(t *testing.T) {
 	acc := setupPeriodicAccount(t, sdkCtx, ss)
 	// Delegate first
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -129,7 +129,6 @@ func TestPeriodicAccountUndelegate(t *testing.T) {
 
 	// Undelegate
 	_, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -161,7 +160,6 @@ func TestPeriodicAccountSendCoins(t *testing.T) {
 
 	acc := setupPeriodicAccount(t, sdkCtx, ss)
 	_, err := acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})
@@ -176,7 +174,6 @@ func TestPeriodicAccountSendCoins(t *testing.T) {
 	})
 
 	_, err = acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})
@@ -217,3 +214,62 @@ func TestPeriodicAccountGetLockCoinInfo(t *testing.T) {
 	require.True(t, unlocked.AmountOf("test").Equal(math.NewInt(10)))
 	require.True(t, locked.AmountOf("test").Equal(math.ZeroInt()))
 }
+
+func TestPeriodicAccountSendCoinsUnauthorized(t *testing.T) {
+	ctx, ss := newMockContext(t)
+	// Initialize context with current time.
+	sdkCtx := sdk.NewContext(nil, true, log.NewNopLogger()).WithContext(ctx).WithHeaderInfo(header.Info{
+		Time: time.Now(),
+	})
+
+	// Create a periodic locking account for the "owner".
+	acc := setupPeriodicAccount(t, sdkCtx, ss)
+
+	// Fast-forward block time so that all tokens are unlocked.
+	startTime, err := acc.StartTime.Get(sdkCtx)
+	require.NoError(t, err)
+	// In our setup, the total locking periods add up to 3 minutes.
+	sdkCtx = sdkCtx.WithHeaderInfo(header.Info{
+		Time: startTime.Add(3 * time.Minute),
+	})
+
+	// Verify that the tokens are fully unlocked.
+	unlocked, locked, err := acc.GetLockCoinsInfo(sdkCtx, sdkCtx.HeaderInfo().Time)
+	require.NoError(t, err)
+	require.True(t, unlocked.AmountOf("test").Equal(math.NewInt(10)), "expected all tokens to be unlocked")
+	require.True(t, locked.AmountOf("test").Equal(math.ZeroInt()), "expected no locked tokens")
+
+	ctx2, _ := newMockContext2(t)
+	sdkCtx2 := sdk.NewContext(nil, true, log.NewNopLogger()).WithContext(ctx2).WithHeaderInfo(header.Info{
+		Time: startTime.Add(3 * time.Minute),
+	})
+	// Attempt to send coins using an unauthorized sender "hacker" instead of "owner".
+	_, err = acc.SendCoins(sdkCtx2, &lockuptypes.MsgSend{
+		ToAddress: "receiver",
+		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
+	})
+	require.Error(t, err, "non-owner should not be able to send coins")
+}
+
+// Create new mock context with different sender
+func newMockContext2(t *testing.T) (context.Context, store.KVStoreService) {
+	t.Helper()
+	return accountstd.NewMockContext(
+		0, []byte("lockup_account"), []byte("hacker"), TestFunds,
+		func(ctx context.Context, sender []byte, msg transaction.Msg) (transaction.Msg, error) {
+			typeUrl := sdk.MsgTypeURL(msg)
+			switch typeUrl {
+			case "/cosmos.bank.v1beta1.MsgSend":
+				return &banktypes.MsgSendResponse{}, nil
+			default:
+				return nil, errors.New("unrecognized request type")
+			}
+		}, func(ctx context.Context, req transaction.Msg) (transaction.Msg, error) {
+			typeUrl := sdk.MsgTypeURL(req)
+			switch typeUrl {
+			default:
+				return nil, errors.New("unrecognized request type")
+			}
+		},
+	)
+}

x/accounts/defaults/lockup/permanent_locking_account_test.go

@@ -39,7 +39,6 @@ func TestPermanentAccountDelegate(t *testing.T) {
 
 	acc := setupPermanentAccount(t, sdkCtx, ss)
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -59,7 +58,6 @@ func TestPermanentAccountUndelegate(t *testing.T) {
 	acc := setupPermanentAccount(t, sdkCtx, ss)
 	// Delegate first
 	_, err := acc.Delegate(sdkCtx, &lockuptypes.MsgDelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -71,7 +69,6 @@ func TestPermanentAccountUndelegate(t *testing.T) {
 
 	// Undelegate
 	_, err = acc.Undelegate(sdkCtx, &lockuptypes.MsgUndelegate{
-		Sender:           "owner",
 		ValidatorAddress: valAddress,
 		Amount:           sdk.NewCoin("test", math.NewInt(1)),
 	})
@@ -103,7 +100,6 @@ func TestPermanentAccountSendCoins(t *testing.T) {
 
 	acc := setupPermanentAccount(t, sdkCtx, ss)
 	_, err := acc.SendCoins(sdkCtx, &lockuptypes.MsgSend{
-		Sender:    "owner",
 		ToAddress: "receiver",
 		Amount:    sdk.NewCoins(sdk.NewCoin("test", math.NewInt(5))),
 	})

x/accounts/defaults/lockup/utils_test.go

@@ -70,7 +70,7 @@ func (h headerService) HeaderInfo(ctx context.Context) header.Info {
 func newMockContext(t *testing.T) (context.Context, store.KVStoreService) {
 	t.Helper()
 	return accountstd.NewMockContext(
-		0, []byte("lockup_account"), []byte("sender"), TestFunds,
+		0, []byte("lockup_account"), []byte("owner"), TestFunds,
 		func(ctx context.Context, sender []byte, msg transaction.Msg) (transaction.Msg, error) {
 			typeUrl := sdk.MsgTypeURL(msg)
 			switch typeUrl {