Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Malformed BASIC_TRANSFER transactions not rejected #645

Open
StarGeezerPhil opened this issue Jun 20, 2024 · 4 comments
Open

Malformed BASIC_TRANSFER transactions not rejected #645

StarGeezerPhil opened this issue Jun 20, 2024 · 4 comments

Comments

@StarGeezerPhil
Copy link

StarGeezerPhil commented Jun 20, 2024

It appears that malformed BASIC_TRANSFER transactions that do not include AmountNanos are not rejected by backend and can be signed and submitted into the mempool.

However, these transactions do not appear to be then accepted and mined into the chain.

@lazynina @diamondhands0 my concern is that this leaves us open to a vulnerability where the mempool could become hijacked in a DOS-type attack/clog the mempool entirely - that could overwhelm nodes and validators.

Here are the details - transactions created by user DiamondThumb:

This is his example payload provided:
image

Example transactions crash the blockchain explorer:
https://explorer.deso.com/txn/3JuEUQSXue97tHWVrRidZC1GUU1Qdau78YskxHGukxByz1g6xxNCCo
https://explorer.deso.com/txn/3JuETYrJjmN8LV1tvCfcZdsvMpNzB8pjzhQzAJ2BsojgcQJic4QkFu
https://explorer.deso.com/txn/3JuETB3wvCm1UQ8bFNU7uRmoWZ9hE621e4waJDYzNDeYs5a9kfb41x
https://explorer.deso.com/txn/3JuETt6hMbiYtrkAFUb1pVqWhwLj1xroXdmWbYmZzpY1SN7WKsFZua

Here is the verbose transaction details for the first example from my front-end:

{
  "TransactionIDBase58Check": "3JuEUQSXue97tHWVrRidZC1GUU1Qdau78YskxHGukxByz1g6xxNCCo",
  "TransactionHashHex": "db7fceaebbcedecbd933dfcfcc7d65d104ebc7d2578479313edeed5176716096",
  "RawTransactionHex": "00010360d99b827779f9e34fefbad4f23927fdf9bed8c90f820b0a374f7cdf03c98c5b00020021035a8155c505cc022a7bafc29d6a46ec6430cfeb68fe8cc16e613d04c6a1169d1f000001ab01ba8e15f5eb90a8aad2a0a652",
  "Outputs": [
    {
      "PublicKeyBase58Check": "BC1YLiMGYzDWEo73gJpqFZ32txELPQti7w13ccm9GTMyePHWtPNfxES",
      "AmountNanos": 0
    }
  ],
  "TransactionType": "BASIC_TRANSFER",
  "TransactionMetadata": {
    "BlockHashHex": "",
    "TxnIndexInBlock": 0,
    "TxnType": "BASIC_TRANSFER",
    "TransactorPublicKeyBase58Check": "BC1YLiJUUwY9Q5cbT1XCSVTp53piLWM3o1uCWvU8wAvG1wUVvNbupij",
    "AffectedPublicKeys": [
      {
        "PublicKeyBase58Check": "BC1YLiMGYzDWEo73gJpqFZ32txELPQti7w13ccm9GTMyePHWtPNfxES",
        "Metadata": "BasicTransferOutput"
      },
      {
        "PublicKeyBase58Check": "BC1YLiJUUwY9Q5cbT1XCSVTp53piLWM3o1uCWvU8wAvG1wUVvNbupij",
        "Metadata": "TransactorPublicKeyBase58Check"
      }
    ],
    "TxnOutputs": [
      {
        "PublicKey": "A2DZm4J3efnjT++61PI5J/35vtjJD4ILCjdPfN8DyYxb",
        "AmountNanos": 0
      }
    ],
    "BasicTransferTxindexMetadata": {
      "TotalInputNanos": 171,
      "TotalOutputNanos": 0,
      "FeeNanos": 171,
      "UtxoOpsDump": "",
      "UtxoOps": null,
      "DiamondLevel": 0,
      "PostHashHex": ""
    }
  },
  "TxnNonce": {
    "ExpirationBlockHeight": 345914,
    "PartialID": 5930258375685453000
  },
  "TxnFeeNanos": 171,
  "TxnVersion": 1,
  "RawTxnMetadata": {}
}

DiamondThumb's thread regarding his issue here: https://desocialworld.com/posts/20891c713fe091a02b4bd556b4f2cfeef4a758ec6fdf5a7f3522317d6557ee9e

@StarGeezerPhil
Copy link
Author

StarGeezerPhil commented Jun 20, 2024

Suggested change to basic_transer.go
https://github.com/deso-protocol/backend/blob/e6eadebc934e18f6c3e6c595b03fa8bef6a40c7d/scripts/tools/toolslib/basic_transfer.go

func _generateUnsignedSendDeSo(senderPubKey *btcec.PublicKey, recipientPubKey *btcec.PublicKey, amountNanos int64,
	params *lib.DeSoParams, node string) (*routes.SendDeSoResponse, error) {
	**if !amountNanos || amountNanos <= 0 {
		return nil, errors.New("_generateUnsignedSendDeSo() invalid AmountNanos: must be a positive integer")
	}**
	endpoint := node + routes.RoutePathSendDeSo

@diamondhands0
Copy link
Member

Very interesting. We're taking a look at this now.

@StarGeezerPhil
Copy link
Author

StarGeezerPhil commented Jun 20, 2024

Pull request with validation: @diamondhands0

#646

@diamondhands0
Copy link
Member

@StarGeezerPhil this should be fixed now. It looks like the mempool was failing to reject txns with an invalid signature (the amount being zero was actually a red herring). Can you try submitting the txns again to node.deso.org and lmk your results?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants