Txn: Based on consensus param 'EnforceAuthAddrSenderDiff', check that auth… (#6504)

Co-authored-by: John Jannotti <jannotti@gmail.com>

Author: gmalouf

cmd/goal/clerk.go

@@ -274,6 +274,9 @@ func writeTxnToFile(client libgoal.Client, signTx bool, dataDir string, walletNa
 		if err != nil {
 			reportErrorf("Signer invalid (%s): %v", signerAddress, err)
 		}
+		if authAddr == tx.Sender {
+			reportErrorf("AuthAddr cannot be the same as the transaction sender")
+		}
 	}
 
 	stxn, err := createSignedTransaction(client, signTx, dataDir, walletName, tx, authAddr)
@@ -449,6 +452,9 @@ var sendCmd = &cobra.Command{
 			if err != nil {
 				reportErrorf("Signer invalid (%s): %v", signerAddress, err)
 			}
+			if authAddr == payment.Sender {
+				reportErrorf("AuthAddr cannot be the same as the transaction sender")
+			}
 		}
 
 		var stx transactions.SignedTxn
@@ -531,7 +537,7 @@ var sendCmd = &cobra.Command{
 
 			// Append the signer since it's a rekey txn
 			if basics.Address(addr) == stx.Txn.Sender {
-				reportWarnln(rekeySenderTargetSameError)
+				reportErrorf("AuthAddr cannot be the same as the transaction sender")
 			}
 			stx.AuthAddr = basics.Address(addr)
 		}
@@ -862,6 +868,9 @@ var signCmd = &cobra.Command{
 				if lsig.Logic != nil {
 					txn.Lsig = lsig
 					if signerAddress != "" {
+						if authAddr == txn.Txn.Sender {
+							reportErrorf("AuthAddr cannot be the same as the transaction sender")
+						}
 						txn.AuthAddr = authAddr
 					}
 				}

config/consensus.go

@@ -247,6 +247,9 @@ type ConsensusParams struct {
 	// SupportRekeying indicates support for account rekeying (the RekeyTo and AuthAddr fields)
 	SupportRekeying bool
 
+	// EnforceAuthAddrSenderDiff requires that AuthAddr must be empty or different from Sender
+	EnforceAuthAddrSenderDiff bool
+
 	// application support
 	Application bool
 
@@ -1461,6 +1464,7 @@ func initConsensusProtocols() {
 
 	vFuture.AppSizeUpdates = true
 	vFuture.AllowZeroLocalAppRef = true
+	vFuture.EnforceAuthAddrSenderDiff = true
 
 	Consensus[protocol.ConsensusFuture] = vFuture
 

data/transactions/verify/txn.go

@@ -84,6 +84,7 @@ var errTxGroupInvalidFee = errors.New("txgroup fee requirement overflow")
 var errTxnSigHasNoSig = errors.New("signedtxn has no sig")
 var errTxnSigNotWellFormed = errors.New("signedtxn should only have one of Sig or Msig or LogicSig")
 var errRekeyingNotSupported = errors.New("nonempty AuthAddr but rekeying is not supported")
+var errAuthAddrEqualsSender = errors.New("AuthAddr must be different from Sender")
 var errUnknownSignature = errors.New("has one mystery sig. WAT?")
 
 // TxGroupErrorReason is reason code for ErrTxGroupError
@@ -168,6 +169,10 @@ func txnBatchPrep(gi int, groupCtx *GroupContext, verifier crypto.BatchVerifier)
 		return &TxGroupError{err: errRekeyingNotSupported, GroupIndex: gi, Reason: TxGroupErrorReasonGeneric}
 	}
 
+	if groupCtx.consensusParams.EnforceAuthAddrSenderDiff && !s.AuthAddr.IsZero() && s.AuthAddr == s.Txn.Sender {
+		return &TxGroupError{err: errAuthAddrEqualsSender, GroupIndex: gi, Reason: TxGroupErrorReasonGeneric}
+	}
+
 	if err := s.Txn.WellFormed(groupCtx.specAddrs, groupCtx.consensusParams); err != nil {
 		return &TxGroupError{err: err, GroupIndex: gi, Reason: TxGroupErrorReasonNotWellFormed}
 	}

data/transactions/verify/txn_test.go

@@ -1531,3 +1531,70 @@ func TestLogicSigMsigBothFlags(t *testing.T) {
 	err = verifyLogicSig()
 	require.ErrorContains(t, err, "LogicSig should only have one of Sig, Msig, or LMsig but has more than one")
 }
+
+func TestAuthAddrSenderDiff(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	t.Run("v41-disabled", func(t *testing.T) { testAuthAddrSenderDiff(t, protocol.ConsensusV41, false) })
+	t.Run("future-enabled", func(t *testing.T) { testAuthAddrSenderDiff(t, protocol.ConsensusFuture, true) })
+}
+
+func testAuthAddrSenderDiff(t *testing.T, consensusVer protocol.ConsensusVersion, enforceEnabled bool) {
+	secrets, addrs, _ := generateAccounts(3)
+	sender := addrs[0]
+	otherAddr := addrs[1]
+
+	blockHeader := createDummyBlockHeader(consensusVer)
+	proto := config.Consensus[consensusVer]
+
+	makeTxn := func() transactions.Transaction {
+		return transactions.Transaction{
+			Type: protocol.PaymentTx,
+			Header: transactions.Header{
+				Sender:      sender,
+				Fee:         basics.MicroAlgos{Raw: proto.MinTxnFee},
+				FirstValid:  1,
+				LastValid:   100,
+				GenesisHash: blockHeader.GenesisHash,
+			},
+			PaymentTxnFields: transactions.PaymentTxnFields{
+				Receiver: addrs[2],
+				Amount:   basics.MicroAlgos{Raw: 1000},
+			},
+		}
+	}
+
+	// Test 1: AuthAddr == Sender
+	tx1 := makeTxn()
+	stxn := tx1.Sign(secrets[0])
+	stxn.AuthAddr = sender
+	groupCtx, err := PrepareGroupContext([]transactions.SignedTxn{stxn}, &blockHeader, nil, nil)
+	require.NoError(t, err)
+	err = verifyTxn(0, groupCtx)
+	if enforceEnabled {
+		require.ErrorContains(t, err, "AuthAddr must be different from Sender",
+			"AuthAddr == Sender should be rejected when enforcement is enabled")
+	} else {
+		require.NoError(t, err,
+			"AuthAddr == Sender should be allowed when enforcement is disabled")
+	}
+
+	// Test 2: Empty AuthAddr should always be allowed
+	tx2 := makeTxn()
+	stxn = tx2.Sign(secrets[0])
+	stxn.AuthAddr = basics.Address{}
+	groupCtx, err = PrepareGroupContext([]transactions.SignedTxn{stxn}, &blockHeader, nil, nil)
+	require.NoError(t, err)
+	err = verifyTxn(0, groupCtx)
+	require.NoError(t, err, "empty AuthAddr should always be allowed")
+
+	// Test 3: AuthAddr != Sender should pass the check (legitimate rekeying case)
+	// Sign with otherAddr's key to pass signature verification
+	tx3 := makeTxn()
+	stxn = tx3.Sign(secrets[1]) // Sign with secrets[1] which corresponds to otherAddr
+	stxn.AuthAddr = otherAddr
+	groupCtx, err = PrepareGroupContext([]transactions.SignedTxn{stxn}, &blockHeader, nil, nil)
+	require.NoError(t, err)
+	err = verifyTxn(0, groupCtx)
+	require.NoError(t, err, "AuthAddr != Sender should pass verification (legitimate rekeying)")
+}

protocol/consensus.go

@@ -47,7 +47,7 @@ const DEPRECATEDConsensusV3 = ConsensusVersion("v3")
 // closes out an account.
 const DEPRECATEDConsensusV4 = ConsensusVersion("v4")
 
-// DEPRECATEDConsensusV5 sets MinTxnFee to 1000 and fixes a blance lookback bug
+// DEPRECATEDConsensusV5 sets MinTxnFee to 1000 and fixes a balance lookback bug
 const DEPRECATEDConsensusV5 = ConsensusVersion("v5")
 
 // DEPRECATEDConsensusV6 adds support for explicit ephemeral-key parameters