From 5d7da049ab2bf3909f3dde0b743753fa95a2d34f Mon Sep 17 00:00:00 2001 From: nullun Date: Wed, 30 Jul 2025 10:11:40 +0100 Subject: [PATCH 1/7] move/add asset txn validation into their own wellFormed methods acfg - moved: + AssetName must not be longer than MaxAssetNameBytes. This was moved from `data/transactions/transaction.go:Transaction.WellFormed()`. + UnitName must not be longer than MaxAssetUnitNameBytes. This was moved from `data/transactions/transaction.go:Transaction.WellFormed()`. + URL must not be longer than MaxAssetURLBytes. This was moved from `data/transactions/transaction.go:Transaction.WellFormed()`. + Decimals must not be greater than MaxAssetDecimals. This was moved from `data/transactions/transaction.go:Transaction.WellFormed()`. axfer - new: + XferAsset must not be zero (missing). This was previously caught by `ledger/apply/asset.go` as "asset 0 does not exist or has been deleted". + AssetSender and AssetCloseTo cannot both be set. This was previously caught by `ledger/apply/asset.go` as "cannot close asset by clawback". afrz - new: + FreezeAsset must not be zero (missing). This was previously caught by `ledger/apply/asset.go` as "asset 0 does not exist or has been deleted". + FreezeAccount must not be zero (missing). This was previously caught by `ledger/apply/asset.go` as "asset not found in account". --- data/transactions/asset.go | 58 ++++++++++++++++++++++++++++++++ data/transactions/transaction.go | 39 ++++++++++++++------- 2 files changed, 84 insertions(+), 13 deletions(-) diff --git a/data/transactions/asset.go b/data/transactions/asset.go index 93b6af840e..e334b6a27d 100644 --- a/data/transactions/asset.go +++ b/data/transactions/asset.go @@ -17,6 +17,10 @@ package transactions import ( + "errors" + "fmt" + + "github.com/algorand/go-algorand/config" "github.com/algorand/go-algorand/data/basics" ) @@ -75,3 +79,57 @@ type AssetFreezeTxnFields struct { // AssetFrozen is the new frozen value. AssetFrozen bool `codec:"afrz"` } + +var ( + errAssetIDCannotBeZero = errors.New("asset ID cannot be zero") + errFreezeAccountCannotBeEmpty = errors.New("freeze account cannot be empty") + errConfigAssetNameTooBig = errors.New("transaction asset name too big") + errConfigAssetUnitNameTooBig = errors.New("transaction asset unit name too big") + errConfigAssetURLTooBig = errors.New("transaction asset url too big") + errConfigAssetDecimalsTooHigh = errors.New("transaction asset decimals is too high") + errTransferCannotCloseAssetByClawback = errors.New("cannot close asset by clawback") +) + +func (ac AssetConfigTxnFields) wellFormed(proto config.ConsensusParams) error { + if len(ac.AssetParams.AssetName) > proto.MaxAssetNameBytes { + return fmt.Errorf("%w: %d > %d", errConfigAssetNameTooBig, len(ac.AssetParams.AssetName), proto.MaxAssetNameBytes) + } + + if len(ac.AssetParams.UnitName) > proto.MaxAssetUnitNameBytes { + return fmt.Errorf("%w: %d > %d", errConfigAssetUnitNameTooBig, len(ac.AssetParams.UnitName), proto.MaxAssetUnitNameBytes) + } + + if len(ac.AssetParams.URL) > proto.MaxAssetURLBytes { + return fmt.Errorf("%w: %d > %d", errConfigAssetURLTooBig, len(ac.AssetParams.URL), proto.MaxAssetURLBytes) + } + + if ac.AssetParams.Decimals > proto.MaxAssetDecimals { + return fmt.Errorf("%w (max is %d)", errConfigAssetDecimalsTooHigh, proto.MaxAssetDecimals) + } + + return nil +} + +func (ax AssetTransferTxnFields) wellFormed() error { + if ax.XferAsset == basics.AssetIndex(0) { + return errAssetIDCannotBeZero + } + + if !ax.AssetSender.IsZero() && !ax.AssetCloseTo.IsZero() { + return errTransferCannotCloseAssetByClawback + } + + return nil +} + +func (af AssetFreezeTxnFields) wellFormed() error { + if af.FreezeAsset == basics.AssetIndex(0) { + return errAssetIDCannotBeZero + } + + if af.FreezeAccount.IsZero() { + return errFreezeAccountCannotBeEmpty + } + + return nil +} diff --git a/data/transactions/transaction.go b/data/transactions/transaction.go index 70b2068f3c..9ced317c04 100644 --- a/data/transactions/transaction.go +++ b/data/transactions/transaction.go @@ -337,11 +337,36 @@ func (tx Transaction) WellFormed(spec SpecialAddresses, proto config.ConsensusPa return err } - case protocol.AssetConfigTx, protocol.AssetTransferTx, protocol.AssetFreezeTx: + case protocol.AssetConfigTx: if !proto.Asset { return fmt.Errorf("asset transaction not supported") } + err := tx.AssetConfigTxnFields.wellFormed(proto) + if err != nil { + return err + } + + case protocol.AssetTransferTx: + if !proto.Asset { + return fmt.Errorf("asset transaction not supported") + } + + err := tx.AssetTransferTxnFields.wellFormed() + if err != nil { + return err + } + + case protocol.AssetFreezeTx: + if !proto.Asset { + return fmt.Errorf("asset transaction not supported") + } + + err := tx.AssetFreezeTxnFields.wellFormed() + if err != nil { + return err + } + case protocol.ApplicationCallTx: if !proto.Application { return fmt.Errorf("application transaction not supported") @@ -431,18 +456,6 @@ func (tx Transaction) WellFormed(spec SpecialAddresses, proto config.ConsensusPa if len(tx.Note) > proto.MaxTxnNoteBytes { return fmt.Errorf("transaction note too big: %d > %d", len(tx.Note), proto.MaxTxnNoteBytes) } - if len(tx.AssetConfigTxnFields.AssetParams.AssetName) > proto.MaxAssetNameBytes { - return fmt.Errorf("transaction asset name too big: %d > %d", len(tx.AssetConfigTxnFields.AssetParams.AssetName), proto.MaxAssetNameBytes) - } - if len(tx.AssetConfigTxnFields.AssetParams.UnitName) > proto.MaxAssetUnitNameBytes { - return fmt.Errorf("transaction asset unit name too big: %d > %d", len(tx.AssetConfigTxnFields.AssetParams.UnitName), proto.MaxAssetUnitNameBytes) - } - if len(tx.AssetConfigTxnFields.AssetParams.URL) > proto.MaxAssetURLBytes { - return fmt.Errorf("transaction asset url too big: %d > %d", len(tx.AssetConfigTxnFields.AssetParams.URL), proto.MaxAssetURLBytes) - } - if tx.AssetConfigTxnFields.AssetParams.Decimals > proto.MaxAssetDecimals { - return fmt.Errorf("transaction asset decimals is too high (max is %d)", proto.MaxAssetDecimals) - } if tx.Sender == spec.RewardsPool { // this check is just to be safe, but reaching here seems impossible, since it requires computing a preimage of rwpool return fmt.Errorf("transaction from incentive pool is invalid") From 0526af46b7b035d6fc0fea00d7e32c4c92361e0f Mon Sep 17 00:00:00 2001 From: nullun Date: Thu, 31 Jul 2025 13:18:57 +0100 Subject: [PATCH 2/7] inline error definitions --- data/transactions/asset.go | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/data/transactions/asset.go b/data/transactions/asset.go index e334b6a27d..64e3a31d0f 100644 --- a/data/transactions/asset.go +++ b/data/transactions/asset.go @@ -80,31 +80,21 @@ type AssetFreezeTxnFields struct { AssetFrozen bool `codec:"afrz"` } -var ( - errAssetIDCannotBeZero = errors.New("asset ID cannot be zero") - errFreezeAccountCannotBeEmpty = errors.New("freeze account cannot be empty") - errConfigAssetNameTooBig = errors.New("transaction asset name too big") - errConfigAssetUnitNameTooBig = errors.New("transaction asset unit name too big") - errConfigAssetURLTooBig = errors.New("transaction asset url too big") - errConfigAssetDecimalsTooHigh = errors.New("transaction asset decimals is too high") - errTransferCannotCloseAssetByClawback = errors.New("cannot close asset by clawback") -) - func (ac AssetConfigTxnFields) wellFormed(proto config.ConsensusParams) error { if len(ac.AssetParams.AssetName) > proto.MaxAssetNameBytes { - return fmt.Errorf("%w: %d > %d", errConfigAssetNameTooBig, len(ac.AssetParams.AssetName), proto.MaxAssetNameBytes) + return fmt.Errorf("transaction asset name too big: %d > %d", len(ac.AssetParams.AssetName), proto.MaxAssetNameBytes) } if len(ac.AssetParams.UnitName) > proto.MaxAssetUnitNameBytes { - return fmt.Errorf("%w: %d > %d", errConfigAssetUnitNameTooBig, len(ac.AssetParams.UnitName), proto.MaxAssetUnitNameBytes) + return fmt.Errorf("transaction asset unit name too big: %d > %d", len(ac.AssetParams.UnitName), proto.MaxAssetUnitNameBytes) } if len(ac.AssetParams.URL) > proto.MaxAssetURLBytes { - return fmt.Errorf("%w: %d > %d", errConfigAssetURLTooBig, len(ac.AssetParams.URL), proto.MaxAssetURLBytes) + return fmt.Errorf("transaction asset url too big: %d > %d", len(ac.AssetParams.URL), proto.MaxAssetURLBytes) } if ac.AssetParams.Decimals > proto.MaxAssetDecimals { - return fmt.Errorf("%w (max is %d)", errConfigAssetDecimalsTooHigh, proto.MaxAssetDecimals) + return fmt.Errorf("transaction asset decimals is too high (max is %d)", proto.MaxAssetDecimals) } return nil @@ -112,11 +102,11 @@ func (ac AssetConfigTxnFields) wellFormed(proto config.ConsensusParams) error { func (ax AssetTransferTxnFields) wellFormed() error { if ax.XferAsset == basics.AssetIndex(0) { - return errAssetIDCannotBeZero + return errors.New("asset ID cannot be zero") } if !ax.AssetSender.IsZero() && !ax.AssetCloseTo.IsZero() { - return errTransferCannotCloseAssetByClawback + return errors.New("cannot close asset by clawback") } return nil @@ -124,11 +114,11 @@ func (ax AssetTransferTxnFields) wellFormed() error { func (af AssetFreezeTxnFields) wellFormed() error { if af.FreezeAsset == basics.AssetIndex(0) { - return errAssetIDCannotBeZero + return errors.New("asset ID cannot be zero") } if af.FreezeAccount.IsZero() { - return errFreezeAccountCannotBeEmpty + return errors.New("freeze account cannot be empty") } return nil From 620374470a316cacfd3a860394e82cb8d7b9df62 Mon Sep 17 00:00:00 2001 From: nullun Date: Thu, 31 Jul 2025 14:16:50 +0100 Subject: [PATCH 3/7] allow assetID to be zero for axfer if the amount is zero --- data/transactions/asset.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data/transactions/asset.go b/data/transactions/asset.go index 64e3a31d0f..0f1eb33617 100644 --- a/data/transactions/asset.go +++ b/data/transactions/asset.go @@ -101,7 +101,7 @@ func (ac AssetConfigTxnFields) wellFormed(proto config.ConsensusParams) error { } func (ax AssetTransferTxnFields) wellFormed() error { - if ax.XferAsset == basics.AssetIndex(0) { + if ax.XferAsset == basics.AssetIndex(0) && ax.AssetAmount != 0 { return errors.New("asset ID cannot be zero") } From e1c8e7a987c6da35dfb313e8066bf7dfd462f4c3 Mon Sep 17 00:00:00 2001 From: nullun Date: Thu, 31 Jul 2025 15:46:06 +0100 Subject: [PATCH 4/7] Apply suggestions from code review Co-authored-by: John Jannotti --- data/transactions/asset.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/data/transactions/asset.go b/data/transactions/asset.go index 0f1eb33617..dc76d7bb8c 100644 --- a/data/transactions/asset.go +++ b/data/transactions/asset.go @@ -101,7 +101,7 @@ func (ac AssetConfigTxnFields) wellFormed(proto config.ConsensusParams) error { } func (ax AssetTransferTxnFields) wellFormed() error { - if ax.XferAsset == basics.AssetIndex(0) && ax.AssetAmount != 0 { + if ax.XferAsset == 0 && ax.AssetAmount != 0 { return errors.New("asset ID cannot be zero") } @@ -113,7 +113,7 @@ func (ax AssetTransferTxnFields) wellFormed() error { } func (af AssetFreezeTxnFields) wellFormed() error { - if af.FreezeAsset == basics.AssetIndex(0) { + if af.FreezeAsset == 0 { return errors.New("asset ID cannot be zero") } From d5a542073b6d44dab2540e8e410fd2f0d754c1e5 Mon Sep 17 00:00:00 2001 From: nullun Date: Thu, 31 Jul 2025 15:44:51 +0100 Subject: [PATCH 5/7] test: update expected error --- data/transactions/logic/evalAppTxn_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/data/transactions/logic/evalAppTxn_test.go b/data/transactions/logic/evalAppTxn_test.go index 3008d517d6..742d487d3d 100644 --- a/data/transactions/logic/evalAppTxn_test.go +++ b/data/transactions/logic/evalAppTxn_test.go @@ -494,7 +494,7 @@ func TestAppAxfer(t *testing.T) { // is going to fail anyway, but to keep the behavior consistent, v9 // allows the zero asset (and zero account) in `requireHolding`. test("global CurrentApplicationAddress; txn Accounts 1; int 100"+noid+"int 1", - fmt.Sprintf("Sender (%s) not opted in to 0", appAddr(888))) + "asset ID cannot be zero") test("global CurrentApplicationAddress; txn Accounts 1; int 100" + axfer + "int 1") From cd6be5e7cfe28f4177a2642d953fa25390c0f01e Mon Sep 17 00:00:00 2001 From: nullun Date: Thu, 7 Aug 2025 13:26:29 +0100 Subject: [PATCH 6/7] add a few wellFormed inner transaction tests --- data/transactions/logic/evalAppTxn_test.go | 42 ++++++++++++++++------ 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/data/transactions/logic/evalAppTxn_test.go b/data/transactions/logic/evalAppTxn_test.go index 742d487d3d..20ed339fa4 100644 --- a/data/transactions/logic/evalAppTxn_test.go +++ b/data/transactions/logic/evalAppTxn_test.go @@ -74,9 +74,7 @@ func TestCurrentInnerTypes(t *testing.T) { TestApp(t, "itxn_begin; int axfer; itxn_field TypeEnum; itxn_submit; int 1;", ep, "insufficient balance") TestApp(t, "itxn_begin; byte \"acfg\"; itxn_field Type; itxn_submit; int 1;", ep, "insufficient balance") - TestApp(t, "itxn_begin; byte \"afrz\"; itxn_field Type; itxn_submit; int 1;", ep, "insufficient balance") TestApp(t, "itxn_begin; int acfg; itxn_field TypeEnum; itxn_submit; int 1;", ep, "insufficient balance") - TestApp(t, "itxn_begin; int afrz; itxn_field TypeEnum; itxn_submit; int 1;", ep, "insufficient balance") // allowed since v6 TestApp(t, "itxn_begin; byte \"keyreg\"; itxn_field Type; itxn_submit; int 1;", ep, "insufficient balance") @@ -431,6 +429,13 @@ func TestAppAxfer(t *testing.T) { partitiontest.PartitionTest(t) t.Parallel() + closeWithClawback := ` + itxn_begin + int axfer ; itxn_field TypeEnum + txn Sender ; itxn_field AssetSender + txn Sender ; itxn_field AssetCloseTo + itxn_submit +` axfer := ` itxn_begin int 77 @@ -450,6 +455,8 @@ func TestAppAxfer(t *testing.T) { TestApp(t, source, ep, problem...) } + test(closeWithClawback, "cannot close asset by clawback") + ledger.NewApp(tx.Receiver, 888, basics.AppParams{}) ledger.NewAsset(tx.Receiver, 777, basics.AssetParams{}) // not in foreign-assets of sample ledger.NewAsset(tx.Receiver, 77, basics.AssetParams{}) // in foreign-assets of sample @@ -775,14 +782,7 @@ func TestAssetFreeze(t *testing.T) { int 5000 == ` - // v5 added inners - TestLogicRange(t, 5, 0, func(t *testing.T, ep *EvalParams, tx *transactions.Transaction, ledger *Ledger) { - ledger.NewApp(tx.Receiver, 888, basics.AppParams{}) - // Give it enough for fees. Recall that we don't check min balance at this level. - ledger.NewAccount(appAddr(888), 12*MakeTestProto().MinTxnFee) - TestApp(t, create, ep) - - freeze := ` + freeze := ` itxn_begin int afrz ; itxn_field TypeEnum int 5000 ; itxn_field FreezeAsset @@ -791,6 +791,24 @@ func TestAssetFreeze(t *testing.T) { itxn_submit int 1 ` + missingFreezeAccount := ` + itxn_begin + int afrz ; itxn_field TypeEnum + int 5000 ; itxn_field FreezeAsset + itxn_submit +` + missingAssetID := ` + itxn_begin + int afrz ; itxn_field TypeEnum + itxn_submit +` + // v5 added inners + TestLogicRange(t, 5, 0, func(t *testing.T, ep *EvalParams, tx *transactions.Transaction, ledger *Ledger) { + ledger.NewApp(tx.Receiver, 888, basics.AppParams{}) + // Give it enough for fees. Recall that we don't check min balance at this level. + ledger.NewAccount(appAddr(888), 12*MakeTestProto().MinTxnFee) + TestApp(t, create, ep) + TestApp(t, freeze, ep, "unavailable Asset 5000") tx.ForeignAssets = []basics.AssetIndex{basics.AssetIndex(5000)} tx.ApplicationArgs = [][]byte{{0x01}} @@ -805,6 +823,10 @@ func TestAssetFreeze(t *testing.T) { holding, err = ledger.AssetHolding(tx.Receiver, 5000) require.NoError(t, err) require.Equal(t, false, holding.Frozen) + + // Malformed + TestApp(t, missingFreezeAccount, ep, "freeze account cannot be empty") + TestApp(t, missingAssetID, ep, "asset ID cannot be zero") }) } From d539e9ef1895286c7bb0e3c1de7002365a779266 Mon Sep 17 00:00:00 2001 From: nullun Date: Thu, 7 Aug 2025 13:29:15 +0100 Subject: [PATCH 7/7] tests: allow axfer of 0 amount to anyone I believe this change would also support sending an axfer of 0 amount to yourself (opt in) whilst also including a close-out to yourself (close out) in a single axfer --- data/transactions/logic/evalAppTxn_test.go | 8 ++++++++ data/transactions/logic/ledger_test.go | 19 +++++++++++-------- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/data/transactions/logic/evalAppTxn_test.go b/data/transactions/logic/evalAppTxn_test.go index 20ed339fa4..21dbc52087 100644 --- a/data/transactions/logic/evalAppTxn_test.go +++ b/data/transactions/logic/evalAppTxn_test.go @@ -468,6 +468,14 @@ func TestAppAxfer(t *testing.T) { "assert failed") // app account not opted in ledger.NewAccount(appAddr(888), 10000) // plenty for fees + + // It should be possible to send 0 amount of an asset (existing + // or not) to any account but ourself. Regardless of being opted in + test("global CurrentApplicationAddress; txn Accounts 1; int 0" + axfer + "int 1") + holding, err := ledger.AssetHolding(appAddr(888), 77) + require.ErrorContains(t, err, "no asset 77 for account") + require.Equal(t, uint64(0), holding.Amount) + ledger.NewHolding(appAddr(888), 77, 3000, false) test("global CurrentApplicationAddress; int 77; asset_holding_get AssetBalance; assert; int 3000; ==;") diff --git a/data/transactions/logic/ledger_test.go b/data/transactions/logic/ledger_test.go index eb55406075..fb95d70d73 100644 --- a/data/transactions/logic/ledger_test.go +++ b/data/transactions/logic/ledger_test.go @@ -731,17 +731,20 @@ func (l *Ledger) axfer(from basics.Address, xfer transactions.AssetTransferTxnFi } fholding, ok := fbr.holdings[aid] if !ok { - if from == to && amount == 0 { - // opt in - if params, exists := l.assets[aid]; exists { - fbr.holdings[aid] = basics.AssetHolding{ - Frozen: params.DefaultFrozen, + if amount == 0 { + if from == to { + // opt in + if params, exists := l.assets[aid]; exists { + fbr.holdings[aid] = basics.AssetHolding{ + Frozen: params.DefaultFrozen, + } + } else { + return fmt.Errorf("Asset (%d) does not exist", aid) } - return nil } - return fmt.Errorf("Asset (%d) does not exist", aid) + } else { + return fmt.Errorf("Sender (%s) not opted in to %d", from, aid) } - return fmt.Errorf("Sender (%s) not opted in to %d", from, aid) } if fholding.Frozen { return fmt.Errorf("Sender (%s) is frozen for %d", from, aid)