From 86e96e5cb70532a28c1a4777bc2e881b7c11c0f5 Mon Sep 17 00:00:00 2001 From: Afshin Arani Date: Tue, 12 Sep 2023 17:27:12 +0300 Subject: [PATCH 1/3] Backend(UtxoCoin): make sure fee is less than out This prevents any future bug to cause unreasonable fees. --- src/GWallet.Backend/Exceptions.fs | 1 + .../UtxoCoin/UtxoCoinAccount.fs | 37 ++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/src/GWallet.Backend/Exceptions.fs b/src/GWallet.Backend/Exceptions.fs index f2f9a1bce..aa5b2fde7 100644 --- a/src/GWallet.Backend/Exceptions.fs +++ b/src/GWallet.Backend/Exceptions.fs @@ -28,3 +28,4 @@ exception InvalidJson exception TransactionAlreadySigned exception TransactionNotSignedYet +exception MinerFeeHigherThanOutputs diff --git a/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs b/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs index 373876325..37f32ca3a 100644 --- a/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs +++ b/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs @@ -397,9 +397,42 @@ module Account = let internal CheckValidPassword (account: NormalAccount) (password: string) = GetPrivateKey account password |> ignore + let private ValidateMinerFee currency (rawTransaction: string) = + async { + let network = GetNetwork currency + + let txToValidate = Transaction.Parse (rawTransaction, network) + + let totalOutputsAmount = txToValidate.TotalOut + + let getInputAmount (input: TxIn) = + async { + let job = ElectrumClient.GetBlockchainTransaction (input.PrevOut.Hash.ToString()) + let! inputOriginTxString = Server.Query currency (QuerySettings.Default ServerSelectionMode.Fast) job None + let inputOriginTx = Transaction.Parse (inputOriginTxString, network) + return inputOriginTx.Outputs.[input.PrevOut.N].Value + } + + let! amounts = + txToValidate.Inputs + |> Seq.map getInputAmount + |> Async.Parallel + + let totalInputsAmount = Seq.sum amounts + + let minerFee = totalInputsAmount - totalOutputsAmount + if minerFee > totalOutputsAmount then + return raise MinerFeeHigherThanOutputs + + return () + } + let private BroadcastRawTransaction currency (rawTx: string): Async = - let job = ElectrumClient.BroadcastTransaction rawTx - Server.Query currency QuerySettings.Broadcast job None + async { + do! ValidateMinerFee currency rawTx + let job = ElectrumClient.BroadcastTransaction rawTx + return! Server.Query currency QuerySettings.Broadcast job None + } let internal BroadcastTransaction currency (transaction: SignedTransaction<_>) = // FIXME: stop embedding TransactionInfo element in SignedTransaction From 19795ba9adca4f3285760c6490e2b85eb46fa11d Mon Sep 17 00:00:00 2001 From: Afshin Arani Date: Wed, 13 Sep 2023 14:55:39 +0300 Subject: [PATCH 2/3] Backend(Ether): make sure fee is less than output --- src/GWallet.Backend/Ether/EtherAccount.fs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/GWallet.Backend/Ether/EtherAccount.fs b/src/GWallet.Backend/Ether/EtherAccount.fs index d3b63fb2f..2b00420c0 100644 --- a/src/GWallet.Backend/Ether/EtherAccount.fs +++ b/src/GWallet.Backend/Ether/EtherAccount.fs @@ -218,7 +218,24 @@ module internal Account = return failwith <| SPrintF1 "Assertion failed: currency %A should be Ether or Ether token" account.Currency } + let private ValidateMinerFee (trans: string) = + let intDecoder = IntTypeDecoder() + + let tx = TransactionFactory.CreateTransaction trans + + let amountInWei = intDecoder.DecodeBigInteger tx.Value + + // TODO: handle validating miner fee in token transfer (where amount is zero) + if amountInWei <> BigInteger.Zero then + let gasLimitInWei = intDecoder.DecodeBigInteger tx.GasLimit + let gasPriceInWei = intDecoder.DecodeBigInteger tx.GasPrice + let minerFeeInWei = gasLimitInWei * gasPriceInWei + + if minerFeeInWei > amountInWei then + raise MinerFeeHigherThanOutputs + let private BroadcastRawTransaction (currency: Currency) trans = + ValidateMinerFee trans Ether.Server.BroadcastTransaction currency ("0x" + trans) let BroadcastTransaction (trans: SignedTransaction<_>) = From aaa8c2e56fec71874a86870d40f5edb535c817f6 Mon Sep 17 00:00:00 2001 From: Afshin Arani Date: Wed, 13 Sep 2023 15:22:54 +0300 Subject: [PATCH 3/3] Backend,Frontend.Console: ask when unreasonable fee --- src/GWallet.Backend/Account.fs | 18 +++---- src/GWallet.Backend/Ether/EtherAccount.fs | 15 +++--- .../UtxoCoin/UtxoCoinAccount.fs | 11 +++-- src/GWallet.Frontend.Console/Program.fs | 47 ++++++++++++++----- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/src/GWallet.Backend/Account.fs b/src/GWallet.Backend/Account.fs index 042b097e6..da7b3d440 100644 --- a/src/GWallet.Backend/Account.fs +++ b/src/GWallet.Backend/Account.fs @@ -198,15 +198,15 @@ module Account = // FIXME: broadcasting shouldn't just get N consistent replies from FaultTolerantClient, // but send it to as many as possible, otherwise it could happen that some server doesn't // broadcast it even if you sent it - let BroadcastTransaction (trans: SignedTransaction<_>): Async = + let BroadcastTransaction (trans: SignedTransaction<_>) (ignoreHigherMinerFeeThanAmount: bool): Async = async { let currency = trans.TransactionInfo.Proposal.Amount.Currency let! txId = if currency.IsEtherBased() then - Ether.Account.BroadcastTransaction trans + Ether.Account.BroadcastTransaction trans ignoreHigherMinerFeeThanAmount elif currency.IsUtxo() then - UtxoCoin.Account.BroadcastTransaction currency trans + UtxoCoin.Account.BroadcastTransaction currency trans ignoreHigherMinerFeeThanAmount else failwith <| SPrintF1 "Unknown currency %A" currency @@ -313,14 +313,15 @@ module Account = let SweepArchivedFunds (account: ArchivedAccount) (balance: decimal) (destination: IAccount) - (txMetadata: IBlockchainFeeInfo) = + (txMetadata: IBlockchainFeeInfo) + (ignoreHigherMinerFeeThanAmount: bool) = match txMetadata with | :? Ether.TransactionMetadata as etherTxMetadata -> - Ether.Account.SweepArchivedFunds account balance destination etherTxMetadata + Ether.Account.SweepArchivedFunds account balance destination etherTxMetadata ignoreHigherMinerFeeThanAmount | :? UtxoCoin.TransactionMetadata as utxoTxMetadata -> match account with | :? UtxoCoin.ArchivedUtxoAccount as utxoAccount -> - UtxoCoin.Account.SweepArchivedFunds utxoAccount balance destination utxoTxMetadata + UtxoCoin.Account.SweepArchivedFunds utxoAccount balance destination utxoTxMetadata ignoreHigherMinerFeeThanAmount | _ -> failwith "If tx metadata is UTXO type, archived account should be too" | _ -> failwith "tx metadata type unknown" @@ -330,6 +331,7 @@ module Account = (destination: string) (amount: TransferAmount) (password: string) + (ignoreHigherMinerFeeThanAmount: bool) : Async = let baseAccount = account :> IAccount if (baseAccount.PublicAddress.Equals(destination, StringComparison.InvariantCultureIgnoreCase)) then @@ -348,14 +350,14 @@ module Account = currency match account with | :? UtxoCoin.NormalUtxoAccount as utxoAccount -> - UtxoCoin.Account.SendPayment utxoAccount btcTxMetadata destination amount password + UtxoCoin.Account.SendPayment utxoAccount btcTxMetadata destination amount password ignoreHigherMinerFeeThanAmount | _ -> failwith "Account not Utxo-type but tx metadata is? report this bug (sendpayment)" | :? Ether.TransactionMetadata as etherTxMetadata -> if not (currency.IsEtherBased()) then failwith "Account not ether-type but tx metadata is? report this bug (sendpayment)" - Ether.Account.SendPayment account etherTxMetadata destination amount password + Ether.Account.SendPayment account etherTxMetadata destination amount password ignoreHigherMinerFeeThanAmount | _ -> failwith "Unknown tx metadata type" diff --git a/src/GWallet.Backend/Ether/EtherAccount.fs b/src/GWallet.Backend/Ether/EtherAccount.fs index 2b00420c0..fc42e3f5d 100644 --- a/src/GWallet.Backend/Ether/EtherAccount.fs +++ b/src/GWallet.Backend/Ether/EtherAccount.fs @@ -234,8 +234,9 @@ module internal Account = if minerFeeInWei > amountInWei then raise MinerFeeHigherThanOutputs - let private BroadcastRawTransaction (currency: Currency) trans = - ValidateMinerFee trans + let private BroadcastRawTransaction (currency: Currency) trans (ignoreHigherMinerFeeThanAmount: bool) = + if not ignoreHigherMinerFeeThanAmount then + ValidateMinerFee trans Ether.Server.BroadcastTransaction currency ("0x" + trans) let BroadcastTransaction (trans: SignedTransaction<_>) = @@ -370,19 +371,21 @@ module internal Account = let SweepArchivedFunds (account: ArchivedAccount) (balance: decimal) (destination: IAccount) - (txMetadata: TransactionMetadata) = + (txMetadata: TransactionMetadata) + (ignoreHigherMinerFeeThanAmount: bool) = let accountFrom = (account:>IAccount) let amount = TransferAmount(balance, balance, accountFrom.Currency) let ecPrivKey = EthECKey(account.GetUnencryptedPrivateKey()) let signedTrans = SignTransactionWithPrivateKey account txMetadata destination.PublicAddress amount ecPrivKey - BroadcastRawTransaction accountFrom.Currency signedTrans + BroadcastRawTransaction accountFrom.Currency signedTrans ignoreHigherMinerFeeThanAmount let SendPayment (account: NormalAccount) (txMetadata: TransactionMetadata) (destination: string) (amount: TransferAmount) - (password: string) = + (password: string) + (ignoreHigherMinerFeeThanAmount: bool) = let baseAccount = account :> IAccount if (baseAccount.PublicAddress.Equals(destination, StringComparison.InvariantCultureIgnoreCase)) then raise DestinationEqualToOrigin @@ -391,7 +394,7 @@ module internal Account = let trans = SignTransaction account txMetadata destination amount password - BroadcastRawTransaction currency trans + BroadcastRawTransaction currency trans ignoreHigherMinerFeeThanAmount let private CreateInternal (password: string) (seed: array): FileRepresentation = let privateKey = EthECKey(seed, true) diff --git a/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs b/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs index 37f32ca3a..8a5036e27 100644 --- a/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs +++ b/src/GWallet.Backend/UtxoCoin/UtxoCoinAccount.fs @@ -427,9 +427,10 @@ module Account = return () } - let private BroadcastRawTransaction currency (rawTx: string): Async = + let private BroadcastRawTransaction currency (rawTx: string) (ignoreHigherMinerFeeThanAmount: bool): Async = async { - do! ValidateMinerFee currency rawTx + if not ignoreHigherMinerFeeThanAmount then + do! ValidateMinerFee currency rawTx let job = ElectrumClient.BroadcastTransaction rawTx return! Server.Query currency QuerySettings.Broadcast job None } @@ -444,13 +445,14 @@ module Account = (destination: string) (amount: TransferAmount) (password: string) + (ignoreHigherMinerFeeThanAmount: bool) = let baseAccount = account :> IAccount if (baseAccount.PublicAddress.Equals(destination, StringComparison.InvariantCultureIgnoreCase)) then raise DestinationEqualToOrigin let finalTransaction = SignTransaction account txMetadata destination amount password - BroadcastRawTransaction baseAccount.Currency finalTransaction + BroadcastRawTransaction baseAccount.Currency finalTransaction ignoreHigherMinerFeeThanAmount // TODO: maybe move this func to Backend.Account module, or simply inline it (simple enough) let public ExportUnsignedTransactionToJson trans = @@ -473,6 +475,7 @@ module Account = (balance: decimal) (destination: IAccount) (txMetadata: TransactionMetadata) + (ignoreHigherMinerFeeThanAmount: bool) = let currency = (account:>IAccount).Currency let network = GetNetwork currency @@ -480,7 +483,7 @@ module Account = let privateKey = Key.Parse(account.GetUnencryptedPrivateKey(), network) let signedTrans = SignTransactionWithPrivateKey account txMetadata destination.PublicAddress amount privateKey - BroadcastRawTransaction currency (signedTrans.ToHex()) + BroadcastRawTransaction currency (signedTrans.ToHex()) ignoreHigherMinerFeeThanAmount let internal Create currency (password: string) (seed: array): Async = async { diff --git a/src/GWallet.Frontend.Console/Program.fs b/src/GWallet.Frontend.Console/Program.fs index 2e9351468..2b59a7a64 100644 --- a/src/GWallet.Frontend.Console/Program.fs +++ b/src/GWallet.Frontend.Console/Program.fs @@ -6,14 +6,31 @@ open System.Text.RegularExpressions open GWallet.Backend open GWallet.Frontend.Console +let SendWithMinerFeeValidation (sendPaymentFunc: bool -> unit) = + try + sendPaymentFunc false + + UserInteraction.PressAnyKeyToContinue () + with + | :? MinerFeeHigherThanOutputs -> + let userWantsToContinue = + UserInteraction.AskYesNo "Miner fee is higher than amount transferred, are you ABSOLUTELY sure you want to broadcast?" + if userWantsToContinue then + sendPaymentFunc true + + UserInteraction.PressAnyKeyToContinue () + let rec TrySendAmount (account: NormalAccount) transactionMetadata destination amount = let password = UserInteraction.AskPassword false - try + + let sendPayment ignoreHigherMinerFeeThanAmount = let txIdUri = - Account.SendPayment account transactionMetadata destination amount password + Account.SendPayment account transactionMetadata destination amount password ignoreHigherMinerFeeThanAmount |> Async.RunSynchronously Console.WriteLine(sprintf "Transaction successful:%s%s" Environment.NewLine (txIdUri.ToString())) - UserInteraction.PressAnyKeyToContinue () + + try + SendWithMinerFeeValidation sendPayment with | :? DestinationEqualToOrigin -> Presentation.Error "Transaction's origin cannot be the same as the destination." @@ -69,11 +86,13 @@ let BroadcastPayment() = if UserInteraction.AskYesNo "Do you accept?" then try - let txIdUri = - Account.BroadcastTransaction signedTransaction - |> Async.RunSynchronously - Console.WriteLine(sprintf "Transaction successful:%s%s" Environment.NewLine (txIdUri.ToString())) - UserInteraction.PressAnyKeyToContinue () + let broadcastTx ignoreHigherMinerFeeThanAmount = + let txIdUri = + Account.BroadcastTransaction signedTransaction ignoreHigherMinerFeeThanAmount + |> Async.RunSynchronously + Console.WriteLine(sprintf "Transaction successful:%s%s" Environment.NewLine (txIdUri.ToString())) + + SendWithMinerFeeValidation broadcastTx with | :? DestinationEqualToOrigin -> Presentation.Error "Transaction's origin cannot be the same as the destination." @@ -413,11 +432,13 @@ let rec CheckArchivedAccountsAreEmpty(): bool = match maybeFee with | None -> () | Some(feeInfo) -> - let txId = - Account.SweepArchivedFunds archivedAccount balance account feeInfo - |> Async.RunSynchronously - Console.WriteLine(sprintf "Transaction successful, its ID is:%s%s" Environment.NewLine txId) - UserInteraction.PressAnyKeyToContinue () + let sweepFunds ignoreHigherMinerFeeThanAmount = + let txId = + Account.SweepArchivedFunds archivedAccount balance account feeInfo ignoreHigherMinerFeeThanAmount + |> Async.RunSynchronously + Console.WriteLine(sprintf "Transaction successful, its ID is:%s%s" Environment.NewLine txId) + + SendWithMinerFeeValidation sweepFunds not (archivedAccountsInNeedOfAction.Any())